Hi,
This patch will cover most of form-associated elements, but <label> and <object> elements are not. For <object>, we need to change its class hierarchy, as https://bugs.webkit.org/show_bug.cgi?id=48821 described and would be addressed on the issue. For <label>, we also might need to change its class inheritance because it isn't a subclass of HTMLFormControlElement. I think it would be better that making another issue to work on it so the patch doesn't include "form" attribute support for <label>. I'll create new issue for <label> if this patch seems to be reasonable.

Comment on attachment 73042[details]
Patch V0
We need to support
- HTMLFormElement::elements contains elements of which form attribute value point the form even if elements are outside of the form
- Form submission contains name&value pairs for such elements.

(In reply to comment #3)
> (From update of attachment 73042[details])
> We need to support
> - HTMLFormElement::elements contains elements of which form attribute value point the form even if elements are outside of the form
> - Form submission contains name&value pairs for such elements.
Kent-san,
Thank you for your review. I didn't realize the requirement. I've revised the patch. It might be somewhat naive, but I'd like ask you to some advice whether I'm not going to wrong way. It would be great if you could give me some advice for this patch.
Thanks,

Comment on attachment 73059[details]
Patch V1
View in context: https://bugs.webkit.org/attachment.cgi?id=73059&action=review
Yes, we need to update HTMLFormElement::m_associatedElements. However, HTMLFormElement::formElementIndex() doesn't work well with elements outside of the form. We need to update formElementIndex() implementation, and should add test cases for form.elements[] order.
We need to handle cases that
- A "form" attribute of a control is added or changed to another form, or removed.
- An "id" attribute of a form element is added, changed, or removed.
You had better to take a look at the specification again. It mentions many cases of changing form owner.
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#attr-fae-form
Also, we need to take care of multiple form elements with identical ID values. Document::getElementById() returns the first element in document-order in such case. So <foo form=id1> should be associated to the first form with id=id1 in document-order.
> WebCore/html/HTMLFormElement.cpp:150
> + for (Node* node = document()->traverseNextNode(); node; node = node->traverseNextNode()) {
This is not acceptable at all. This code will introduce significant performance regression for existing pages.
I recommend to store a list (hash?) of form controls with id attribute to Document, like existing Document::registerFormElementWithState() and unregisterFormElementWithState(), and search the list for controls pointing this form element.

(In reply to comment #6)
Kent-san,
Thank you for review and detailed comments. I've revised the patch. The patch still might be not good enough, but I'd like you to glance over the current implementation.
Following your suggestion, I've added a list of form-associated elements with form attribute. I also added two variables into HTMLFormElement for avoiding performance regression for existing pages. They are indices of m_associatedElements and denotes the range of elements which are children of the form element. These variables are used when form-associated elements which don't have form attribute are inserted. I think it would have less impact on existing pages.
On the other hand, for elements with form attribute, I couldn't come up with efficient ways to handle them. So the current implementation walks through the entire tree of the document to find the right index to be inserted.

(In reply to comment #9)
> On the other hand, for elements with form attribute, I couldn't come up with efficient ways to handle them. So the current implementation walks through the entire tree of the document to find the right index to be inserted.
Why don't you keep a centralized table of form associations?

(In reply to comment #9)
> On the other hand, for elements with form attribute, I couldn't come up with efficient ways to handle them. So the current implementation walks through the entire tree of the document to find the right index to be inserted.
We discussed this offline.
- Controls with form attribute may be rare. The traversal cost might be acceptable.
- However, we have an idea of reducing the traversal cost. Ishibashi-san will try it.
(In reply to comment #10)
> Why don't you keep a centralized table of form associations?
It won't help so much. We need to detect control order anyway.

Comment on attachment 73367[details]
Patch V2
View in context: https://bugs.webkit.org/attachment.cgi?id=73367&action=review
Kent-san,
Thank you for the review and suggestions. I've updated the patch and I'll post it soon.
I've implemented some algorithms for elements which have form attribute, that are binary search algorithm and comparing the position of two elements, to find the right place to be inserted into m_associatedElements of HTMLFormElement. I hope that the patch would be more efficient than the old one.
Regards,
>> LayoutTests/fast/forms/script-tests/form-attribute-elements-order.js:5
>> +container.innerHTML = '<input name="victim" id="before1" form="owner" />' +
>
> nit: You don't need double-quotes surrounding attribute values in many cases.
Thank you for letting me know that. I've removed these double-quotes.
>> LayoutTests/fast/forms/script-tests/form-attribute.js:34
>> + '</form>';
>
> How about the following similar case?
>
> <form id=owner>
> <form id=shouldNotBeOwner>
> <input id=inputElement name=victime form=owner>
> </form>
> </form>
I've added the case. BTW, it seems that Webkit ignores nested inner form elements so I just added test which ensures each form attribute points the out-most form element. I'm not sure that is correct, so please let me know if I was done something wrong.
>> WebCore/html/HTMLFormElement.cpp:406
>> +unsigned HTMLFormElement::formElementIndexWithFormAttribute(HTMLFormControlElement* element)
>
> Does this work if the element is inside of the form?
>
> <form id=owner>
> <input>
> <input form=owner>
> <input>
> </form>
>
> or
>
>
> <form id=owner>
> <form>
> <input>
> <input form=owner>
> <input>
> </form>
> </form>
Added some test cases like that.

Comment on attachment 73705[details]
Patch V3
View in context: https://bugs.webkit.org/attachment.cgi?id=73705&action=review> WebCore/html/HTMLFormElement.cpp:403
> +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)
- 'n1' 'n2" are not good names. They should be node1 and node2.
- This function should belong to Document class.
- The "int" return value is not good. We should introduce new enum.
> WebCore/html/HTMLFormElement.cpp:415
> + // Keeping the nearest sucessors of each nodes for later use.
typo and grammer: "successor of each node"
> WebCore/html/HTMLFormElement.cpp:420
> + while (ancestor1 && ancestor2) {
> + if (ancestor1 == ancestor2)
> + break;
You can write "while (ancestor1 && ancestor2 && ancestor1 != ancestor2) {"
> WebCore/html/HTMLFormElement.cpp:441
> + ASSERT(successor1 && successor2);
successor1 or successor2 can be 0 if n1 is an ancestor of n2, or vice versa. It never happens in the form attribute case, but we should support it because we'd like to move this function to Document.
BTW, you shouldn't use && in ASSERT(). You should have written
ASSERT(successor1);
ASSERT(successor2);

(In reply to comment #15)
Kent-san,
Thank you for your quick review.
> (From update of attachment 73705[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73705&action=review
>
> > WebCore/html/HTMLFormElement.cpp:403
> > +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)
>
> - 'n1' 'n2" are not good names. They should be node1 and node2.
> - This function should belong to Document class.
> - The "int" return value is not good. We should introduce new enum.
I see. I'll move the function to Document class. So, should we also make nodeDepth() belong to other class, say, Node class?
> > WebCore/html/HTMLFormElement.cpp:415
> > + // Keeping the nearest sucessors of each nodes for later use.
>
> typo and grammer: "successor of each node"
Thanks, I'll fix it.
>
> > WebCore/html/HTMLFormElement.cpp:420
> > + while (ancestor1 && ancestor2) {
> > + if (ancestor1 == ancestor2)
> > + break;
>
> You can write "while (ancestor1 && ancestor2 && ancestor1 != ancestor2) {"
I'll fix it.
>
> > WebCore/html/HTMLFormElement.cpp:441
> > + ASSERT(successor1 && successor2);
>
> successor1 or successor2 can be 0 if n1 is an ancestor of n2, or vice versa. It never happens in the form attribute case, but we should support it because we'd like to move this function to Document.
I'll remove it when I moves the function to the Document class.
> BTW, you shouldn't use && in ASSERT(). You should have written
> ASSERT(successor1);
> ASSERT(successor2);
I see. Thank you for suggestion:-)

(In reply to comment #16)
> > > WebCore/html/HTMLFormElement.cpp:403
> > > +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)
> >
> > - 'n1' 'n2" are not good names. They should be node1 and node2.
> > - This function should belong to Document class.
> > - The "int" return value is not good. We should introduce new enum.
>
> I see. I'll move the function to Document class. So, should we also make nodeDepth() belong to other class, say, Node class?
Oops, I have found Node::compareDocumentPosition() now. So we can use it and compareTreeOrder() is not needed. I'm sorry for wasting your time.
compareDocumentPosition() looks slower than your compareTreeOrder(). But it would be acceptable. We don't need nodeDepth() too.

(In reply to comment #17)
Kent-san,
> Oops, I have found Node::compareDocumentPosition() now. So we can use it and compareTreeOrder() is not needed. I'm sorry for wasting your time.
> compareDocumentPosition() looks slower than your compareTreeOrder(). But it would be acceptable. We don't need nodeDepth() too.
Thank you for letting me know the function. Before starting implementation, I should investigate existing code more carefully. It's definitely my fault. Sorry for wasting your time. I'll revise the patch.
Regards,

The commit-queue encountered the following flaky tests while processing attachment 73870[details]:
fast/workers/storage/interrupt-database-sync.html
webarchive/test-link-rel-icon.html
Please file bugs against the tests. These tests were authored by ddkilzer@webkit.org and dumi@chromium.org. The commit-queue is continuing to process your patch.