[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