[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 #77 from Ryosuke Niwa <rniwa at webkit.org>  2013-03-06 10:45:58 PST ---
(From update of attachment 191787)
View in context: https://bugs.webkit.org/attachment.cgi?id=191787&action=review

> Source/WebCore/ChangeLog:5
> +        Hook FormAssociatedElement, HTMLFormElement to notify EditorClient of form changes after a page has loaded.
> +        Will be used to add autofill support for ajax-y webpages. e.g if while filling out a form, new fields
> +        are dynamically created, autofill can know to re-query the autofill server and keep going.

This should be the bug title. Please put a long change description after "Reviewed by" line followed by a blank line.

> Source/WebCore/dom/Document.cpp:6078
> +    if (!frame() || !frame()->page()->chrome()->client()->shouldNotifyOnFormChanges())
> +        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?

> 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.

> 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.

> Source/WebCore/html/FormAssociatedElement.cpp:166
> +    HTMLElement* element = toHTMLElement(this);     
> +    if (m_form && m_form != currentForm && m_form->inDocument())
> +        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.

> Source/WebCore/html/FormAssociatedElement.cpp:174
> +        HTMLFormElement* currentForm = m_form;

Ditto about the variable name.

> 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.

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

Nit: WebNode(*it)

> 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.

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