[webkit-reviews] review denied: [Bug 110375] Add client callbacks to notify of changes of associated from controls : [Attachment 191546] Patch

[bugzilla-daemon at webkit.org]

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

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

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


> Source/WebCore/dom/Document.cpp:6057
> +    m_didAssociateFormControlsTimer.startOneShot(0.1);

Why not 0?

> Source/WebCore/html/FormAssociatedElement.cpp:166
> +    if (m_form && m_form != currentForm && m_form->inDocument())
> +	   element->document()->didAssociateFormControl(element);

What if we associated a form and then subsequently "unassociated" it?
Why is it okay to still notify the client.

It might be okay to do that for your specific use case but that doesn't sound
like a sound API to me.

> Source/WebCore/html/FormAssociatedElement.h:27
> +#include "Timer.h"

Why do we need Timer here?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1178
> +    WebVector<WebNode*> elementVector((size_t) elements.size());

Please use static_cast.

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

I don't think this is the way to get WebNode. Simply call WebNode. r- because
of this.