[Webkit-unassigned] [Bug 110375] Add client callbacks to notify of changes of associated from controls

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=110375





--- Comment #78 from Dane Walllinga <dgwallinga at chromium.org>  2013-03-06 12:33:08 PST ---
(From update of attachment 191787)
View in context: https://bugs.webkit.org/attachment.cgi?id=191787&action=review

>> Source/WebCore/dom/Document.cpp:6078
>> +        return;
> 
> I don't like the fact we have to call this virtual function every time a form control association changes. Can this be a page setting instead?

That depends entirely upon what you mean by that. A field on the page returned by frame()->page() which is presumably initialized by a call to the same virtual function on the ChromeClient?

>> Source/WebCore/dom/Document.cpp:6080
>> +    m_didAssociateFormControlsTimer.startOneShot(0);
> 
> You should check m_didAssociateFormControlsTimer is already active or not and call startOneShot only if the timer was inactive.

done

>> Source/WebCore/html/FormAssociatedElement.cpp:162
>> +    HTMLFormElement* currentForm = m_form;
> 
> It's strange to call this "current" form since the "current" value has been changed at where this variable is used again.
> I would call it originalForm instead.

done

>> Source/WebCore/html/FormAssociatedElement.cpp:166
>> +        element->document()->didAssociateFormControl(element);
> 
> Given that we're notifying the client even when we "unassociated" a form, we should probably rename all these functions to something like didChangeFormControlAssociation.

At the moment, we shouldn't be notifying when we unassociate a form - hence m_form being the first part of the condition.

>> Source/WebCore/html/FormAssociatedElement.cpp:174
>> +        HTMLFormElement* currentForm = m_form;
> 
> Ditto about the variable name.

done

>> Source/WebKit/chromium/public/WebAutofillClient.h:98
>> +    // These methods are called when the form structure of a page changes
> 
> I don't think this comment adds any value to the code if we had renamed the function name.
> In fact, the comment is more ambiguous than the function name even as is.
> Please remove it unless we have better things to say.

done

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1156
>> +        elementVector[i] = WebNode((*it));
> 
> Nit: WebNode(*it)

done

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1157
>> +        ++i;
> 
> Nit: Increment this in the for loop as in ++it, ++i.
> 
> Once you did that, please remove the curly brackets as this will become a single line statement.

done

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.