[webkit-reviews] review denied: [Bug 47813] [HTML5] "form" attribute support for form control elements : [Attachment 73059] Patch V1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 47813: [HTML5] "form" attribute support for form control elements
https://bugs.webkit.org/show_bug.cgi?id=47813

Attachment 73059: Patch V1
https://bugs.webkit.org/attachment.cgi?id=73059&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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-cont
rols-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.