Bug 116334 – Expose a way to know when forms are added to a page or when form controls are added to a form in the injected bundle

• Component: WebKit2
• Status: RESOLVED FIXED

[Jessie Berlin]

We should use the didAssociateFormControls and shouldNotifyOnFormChanges callbacks on ChromeClient.

<rdar://problem/13905067>

[Jessie Berlin]

Created attachment 202124 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 202124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202124&action=review

> Source/WebKit2/ChangeLog:26
> +        At this time, there is no known use for the elements themselves, so don't bother piping
> +        that through to the API.

Hmm. I wonder if checks in injected bundle will be efficient enough without knowing the elements. These calls might be made frequently on certain pages.

[Jessie Berlin]

(In reply to comment #2)
> (From update of attachment 202124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202124&action=review
> 
> > Source/WebKit2/ChangeLog:26
> > +        At this time, there is no known use for the elements themselves, so don't bother piping
> > +        that through to the API.
> 
> Hmm. I wonder if checks in injected bundle will be efficient enough without knowing the elements. These calls might be made frequently on certain pages.

Thanks for the review! I discussed this further with you and others, and decided to add the elements. New patch coming momentarily.

[Jessie Berlin]

Created attachment 202328 [details]
Patch (version 2)

[Alexey Proskuryakov]

Comment on attachment 202328 [details]
Patch (version 2)

View in context: https://bugs.webkit.org/attachment.cgi?id=202328&action=review

> Source/WebKit2/ChangeLog:20
> +        (InjectedBundlePageFormClient):

It's best to remove such unhelpful lines from ChangeLogs (it's a bug in the tool that it generates them).

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:133
> +    unsigned size = elements.size();

The type should be size_t.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:135
> +    Vector<RefPtr<APIObject> > elementHandles;

We are now using C++11 ">>" in WebKit2 code (as well as in all Mac code).

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:138
> +    for (unsigned i = 0; i < size; ++i)

size_t

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.h:59
> +    void didAssociateFormControls(WebPage*, const Vector<RefPtr<WebCore::Element> >&);

Same comment about ">>".

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:719
> +void WebChromeClient::didAssociateFormControls(const Vector<RefPtr<WebCore::Element> >& elements)

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h:173
> +    virtual void didAssociateFormControls(const Vector<RefPtr<WebCore::Element> >&) OVERRIDE;

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2/DidAssociateFormControls_Bundle.cpp:39
> +    virtual void didCreatePage(WKBundleRef, WKBundlePageRef);

OVERRIDE?

[Jessie Berlin]

(In reply to comment #5)
> (From update of attachment 202328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202328&action=review
> 
> > Source/WebKit2/ChangeLog:20
> > +        (InjectedBundlePageFormClient):
> 
> It's best to remove such unhelpful lines from ChangeLogs (it's a bug in the tool that it generates them).

Done.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:133
> > +    unsigned size = elements.size();
> 
> The type should be size_t.

Done.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:135
> > +    Vector<RefPtr<APIObject> > elementHandles;
> 
> We are now using C++11 ">>" in WebKit2 code (as well as in all Mac code).

I initially had that, but check-webkit-style complained. I fixed this and Filed https://bugs.webkit.org/show_bug.cgi?id=116474

> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:138
> > +    for (unsigned i = 0; i < size; ++i)
> 
> size_t

Done.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.h:59
> > +    void didAssociateFormControls(WebPage*, const Vector<RefPtr<WebCore::Element> >&);
> 
> Same comment about ">>”.

Done.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:719
> > +void WebChromeClient::didAssociateFormControls(const Vector<RefPtr<WebCore::Element> >& elements)
> 
> Ditto.

Done.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h:173
> > +    virtual void didAssociateFormControls(const Vector<RefPtr<WebCore::Element> >&) OVERRIDE;
> 
> Ditto.

Done.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/DidAssociateFormControls_Bundle.cpp:39
> > +    virtual void didCreatePage(WKBundleRef, WKBundlePageRef);
> 
> OVERRIDE?

Done.

Thanks for the review!

[Jessie Berlin]

Comment on attachment 202328 [details]
Patch (version 2)

Committed with the changes mentioned above in http://trac.webkit.org/changeset/150398