Bug 81420 - Break the association between form controls and their owner when the owner leaves the tree
Summary: Break the association between form controls and their owner when the owner le...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 15:24 PDT by Adam Klein
Modified: 2012-04-10 19:58 PDT (History)
4 users (show)

See Also:

Attachments
Patch (9.53 KB, patch)
2012-03-16 15:34 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Added expectation (10.50 KB, patch)
2012-03-16 15:51 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Patch (7.23 KB, patch)
2012-04-10 12:20 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-03-16 15:24:53 PDT 
Break the association between form controls and their owner when the owner leaves the tree
Comment 1 Adam Klein 2012-03-16 15:34:26 PDT 
Created attachment 132402 [details]
Patch
Comment 2 Adam Klein 2012-03-16 15:51:23 PDT 
Created attachment 132409 [details]
Added expectation
Comment 3 Kent Tamura 2012-03-20 21:34:51 PDT 
Comment on attachment 132409 [details]
Added expectation

View in context: https://bugs.webkit.org/attachment.cgi?id=132409&action=review

> Source/WebCore/html/HTMLFormElement.cpp:161
> +    for (unsigned i = 0; i < m_associatedElements.size(); ++i)
> +        m_associatedElements[i]->formRemovedFromTree(root);

This is a bad iteration pattern.  formRemovedFromTree() updates m_associatedElements.

Form-association code is complex, and introducing FormAssociatedElement::m_formNeedsSameRootCheck makes it more complex.  I'd like to avoid m_formNeedsRootCheck as possible.
I'd like to change formRemovedFromTree() so that it unregister a control only if the control has form= attribute or its root is different from the root of this form.
Comment 4 Kent Tamura 2012-03-20 21:47:10 PDT 
(In reply to comment #3)
> I'd like to change formRemovedFromTree() so that it unregister a control only if the control has form= attribute or its root is different from the root of this form.

and it does nothing otherwise.
I don't think findRoot cost is significant.
Comment 5 Adam Klein 2012-04-10 12:20:18 PDT 
Created attachment 136504 [details]
Patch
Comment 6 Adam Klein 2012-04-10 12:21:43 PDT 
Comment on attachment 132409 [details]
Added expectation

View in context: https://bugs.webkit.org/attachment.cgi?id=132409&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:161
>> +        m_associatedElements[i]->formRemovedFromTree(root);
> 
> This is a bad iteration pattern.  formRemovedFromTree() updates m_associatedElements.
> 
> Form-association code is complex, and introducing FormAssociatedElement::m_formNeedsSameRootCheck makes it more complex.  I'd like to avoid m_formNeedsRootCheck as possible.
> I'd like to change formRemovedFromTree() so that it unregister a control only if the control has form= attribute or its root is different from the root of this form.

Changed to make a copy of the vector before iterating.

I've removed m_formNeedsSameRootCheck, greatly simplifying this patch. Note that a form with an ID going away is already handled in HTMLFormElement::removedFromDocument.
Comment 7 Kent Tamura 2012-04-10 18:36:39 PDT 
Comment on attachment 136504 [details]
Patch

ok
Comment 8 WebKit Review Bot 2012-04-10 19:58:06 PDT 
Comment on attachment 136504 [details]
Patch

Clearing flags on attachment: 136504

Committed r113817: <http://trac.webkit.org/changeset/113817>
Comment 9 WebKit Review Bot 2012-04-10 19:58:11 PDT 
All reviewed patches have been landed.  Closing bug.