[webkit-reviews] review granted: [Bug 110375] Add client callbacks to notify of changes of associated form controls : [Attachment 194095] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 20 18:30:30 PDT 2013


Ryosuke Niwa <rniwa at webkit.org> has granted Dane Walllinga
<dgwallinga at chromium.org>'s request for review:
Bug 110375: Add client callbacks to notify of changes of associated form
controls
https://bugs.webkit.org/show_bug.cgi?id=110375

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194095&action=review


> Source/WebCore/dom/Document.cpp:6078
> +    if (!frame() || !frame()->page()
> +	   ||
!frame()->page()->chrome()->client()->shouldNotifyOnFormChanges())

Why are you wrapping the line here? We can fit both lines in one line.

> Source/WebCore/dom/Document.cpp:6090
> +    HashSet<Element*> associatedFormControls;

Adding a blank line here will improve the readability.

> Source/WebCore/dom/Document.cpp:6094
> +    HashSet<Element*>::iterator controlsEnd =
m_associatedFormControls.end();
> +    for (HashSet<Element*>::iterator it = m_associatedFormControls.begin();
it != controlsEnd; ++it)
> +	   associatedFormControls.add(*it);
> +   
frame()->page()->chrome()->client()->didAssociateFormControls(associatedFormCon
trols);

Now I'm reading this code for the second time, I feel like we should just pass
a Vector to didAssociateFormControls because
then we can use copyToVector in HashSet.h as opposed to manually iterating over
them.


More information about the webkit-reviews mailing list