[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 #40 from Elliott Sprehn <esprehn at chromium.org>  2013-02-21 18:17:03 PST ---
(From update of attachment 189650)
View in context: https://bugs.webkit.org/attachment.cgi?id=189650&action=review

This is going to be super spammy, lets batch together the changes so we don't make hundreds of these calls during page load. rniwa wanted this in ChromeClient so lets put it there too.

> Source/WebCore/html/FormAssociatedElement.cpp:165
> +    if (m_form && m_form != currentForm && m_form->inDocument()

We probably want the Client to have a method like bool shouldNotifyOnFormChanges() that lets you control if these are batched and if they're fired.

>> Source/WebCore/html/FormAssociatedElement.cpp:281
>> +      element->document()->frame()->editor()->client()->didAssociateInput(element);
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

This is going to spam you with notifications and also means you have tons of timers. This isn't the approach you want. Instead you want to collect in a HashSet (or ListHashSet) all the Elements that have changed and then give a batch notification to the client.

didAssociateFormControls(HashSet<Element>) or some such.

>> Source/WebCore/html/FormAssociatedElement.cpp:282
>> +    }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

webkit-patch should be warning you about these before you upload. It's nice if you fix them before you upload. :)

> Source/WebCore/html/FormAssociatedElement.h:122
> +    Timer<FormAssociatedElement> m_didAssociateElementTimer;

This makes every form element bigger for just this feature which is bad. You want a shared timer per document probably.

> Source/WebCore/html/HTMLFormElement.cpp:759
> +    this->document()->frame()->editor()->client()->didAddForm(this);

Same.

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