Bug 164050 – [GTK] New API to notify about dynamically added forms

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[GTK] New API to notify about dynamically added forms

[Sergio Villar Senin]

Created attachment 293001 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 293001 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:169
> +static void didAssociateFormControls(WKBundlePageRef page, WKArrayRef elementHandles, const void* clientInfo)

Omit names of unused params.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:174
> +static bool shouldNotifyOnFormChanges(WKBundlePageRef page, const void* clientInfo)

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:176
> +    return true;

I guess the previous callback is never emitted if this is not implemented or returns false?

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:203
> +    WKBundlePageFormClientV2 wkBundlePageFormClient = {
> +        {
> +            2, // version
> +            clientInfo,
> +        },
> +        0, // textFieldDidBeginEditing,
> +        0, // textFieldDidEndEditing
> +        0, // textDidChangeInTextField
> +        0, // textDidChangeInTextArea
> +        0, // shouldPerformActionInTextField
> +        0, // willSubmitForm,
> +
> +        // Version 1.
> +        0, // willSendSubmitEvent
> +
> +        // version 2.
> +        0, // didFocusTextField
> +        shouldNotifyOnFormChanges,
> +        didAssociateFormControls,
> +    };
> +    WKBundlePageSetFormClient(page, &wkBundlePageFormClient.base);

This doesn't belong here, you should do this in WebKitWebPage, see webkitWebPageCreate. That way you can use the page as client info and use that directly from the callback.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:479
> +     * WebKitWebPage::did-associate-form-controls:

did-foo is WebKit internals style, but in GTK+ API we don't usually use that style. I would name this something like form-controls-added or form-controls-associated

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:481
> +     *

Would it be possible to include information about the form controls added?

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:492
> +        0, 0, 0,

0, 0, nullptr

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPagePrivate.h:29
> +void webkitWebPageDidAssociateFormControls(WebKitWebPage*);

You don't need this either if the page form client is handled directly in WebKitWebPage.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:252
> +    test->runJavaScriptAndWaitUntilFinished(addFormScript, 0);

0 -> nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:271
> +    WebViewTest::add("WebKitWebExtension", "document-loaded-signal", testWebExtensionDidAssociateFormControls);

document-loaded-signal?

[Sergio Villar Senin]

Comment on attachment 293001 [details]
Patch

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

>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:176
>> +    return true;
> 
> I guess the previous callback is never emitted if this is not implemented or returns false?

Right.

>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:203
>> +    WKBundlePageSetFormClient(page, &wkBundlePageFormClient.base);
> 
> This doesn't belong here, you should do this in WebKitWebPage, see webkitWebPageCreate. That way you can use the page as client info and use that directly from the callback.

Right. I have messed it somehow because I have done precisely what you are mentioning. Not sure how I could end up with this...

>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:479
>> +     * WebKitWebPage::did-associate-form-controls:
> 
> did-foo is WebKit internals style, but in GTK+ API we don't usually use that style. I would name this something like form-controls-added or form-controls-associated

Ack.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:271
>> +    WebViewTest::add("WebKitWebExtension", "document-loaded-signal", testWebExtensionDidAssociateFormControls);
> 
> document-loaded-signal?

It's obviously wrong. Copy-paste issues

[Sergio Villar Senin]

Comment on attachment 293001 [details]
Patch

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

>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:481
>> +     *
> 
> Would it be possible to include information about the form controls added?

Yep. The notification carries an array of associated elements.

[Michael Catanzaro]

Comment on attachment 293001 [details]
Patch

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

Awesome!

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:484
> +     * Emmited after form elements (or form associated elements) are associated to a particular web
> +     * page. Useful to implement form autofilling with web pages where form fields are dynamically
> +     * added (as many JS frameworks) do.

Emmited -> Emitted

So this signal can occur multiple times per load, right? That should be mentioned.

[Sergio Villar Senin]

Created attachment 293022 [details]
Patch

New approach using an API::InjectedBundle::FormClient subclass to avoid the C API churn. Also renamed the signal name to make it more glib-ish. Last but not least, the signal now carries an array of WebKitDOMElement associated to forms

[Carlos Garcia Campos]

(In reply to comment #6)
> Created attachment 293022 [details]
> Patch
> 
> New approach using an API::InjectedBundle::FormClient subclass to avoid the
> C API churn. Also renamed the signal name to make it more glib-ish. Last but
> not least, the signal now carries an array of WebKitDOMElement associated to
> forms

I looked for API::InjectedBundle::FormClient before to propose you to use it instead and didn't find it, now I've seen that it's not in API dir.

[Carlos Garcia Campos]

Comment on attachment 293022 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:509
> +     * @elements: a #GPtrArray of #WebKitDOMElement with the associated DOM elements in forms

We should clarify here the ownership of both the container (the array, and the contents)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:272
> +    gpointer data = g_object_get_data(G_OBJECT(extension), "dbus-connection");
> +    if (data)
> +        emitFormControlsAssociated(G_DBUS_CONNECTION(data), formElements->len);
> +    else
> +        delayedSignalsQueue.append(DelayedSignal(FormControlsAssociatedSignal, formElements->len));

We should check the contents of the array, that the elements are really a WebKitDOMElement, I wonder if they should be WebKitDOMHTMLElement, btw. Instead of passing the len, maybe we could use an id in the UI process and pass the element id, and then check it's the one we added.

[Michael Catanzaro]

Comment on attachment 293022 [details]
Patch

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

Thanks a bunch. It looks almost there, but Carlos's comments above still need to be addressed.

>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:509
>> +     * @elements: a #GPtrArray of #WebKitDOMElement with the associated DOM elements in forms
> 
> We should clarify here the ownership of both the container (the array, and the contents)

I guess the right annotation would be:

@elements: (element-type WebKit.DOMElement) (transfer none): a #GPtrArray...

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:513
> +     * Emmited after form elements (or form associated elements) are associated to a particular web
> +     * page. Useful to implement form autofilling with web pages where form fields are dynamically
> +     * added (as many JS frameworks) do.

I guess you missed my earlier comment. Please fix Emmited -> Emitted. Please also mention here if it can be emitted more than once (I guess it can).

[Sergio Villar Senin]

Created attachment 294147 [details]
Patch

[Michael Catanzaro]

Comment on attachment 294147 [details]
Patch

LGTM. I think Carlos will want to do final review. Please remember that we don't want to commit until Apple gets their internal CI working again.

[Carlos Garcia Campos]

Comment on attachment 294147 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:357
> +    void didAssociateFormControls(WebPage*, const Vector<RefPtr<WebCore::Element>>& elements) override

No need for WebCore:: here.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:363
> +        webkitWebPageFormControlsAssociated(m_webPage, formElements.get());

I think you can emit the signal here directly, it's just one line.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:510
> +     * #WebKitDOMElement with the list of forms in the page

Indent this line, just adding 4 spaces or use a single line.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:514
> +     * added (as many JS frameworks) do. This might be emitted multiple times for the same web page.

(as many JS frameworks) do -> (as many JS frameworks do)

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:517
> +     * those cases the @elements array carries the list of those elements which mave moved.

mave moved -> have moved

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:518
> +     *

Even when we use the transfer none, we should document here that the elements array will be alive during signal emission, and user should take a ref to keep it alive.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:225
> +    g_main_loop_quit(test->m_mainLoop);

test->quitMainLoop();

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:235
> +        0,

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:239
> +        0,

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:243
> +        0);

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:261
> +    test->runJavaScriptAndWaitUntilFinished(addFormScript, 0);

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:263
> +    g_main_loop_run(test->m_mainLoop);

Why do you run the main loop again right after it has quit? Where is it quit after this? Ah I see, didAssociateFormControlsCallback. This is racy I think. You shouldn't use runJavaScriptAndWaitUntilFinished in this case, but simply webkit_web_view_run_javascript (without ready handler) and run the loop manually. What we are interested in, is the didAssociateFormControlsCallback to be emitted

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:271
> +    test->runJavaScriptAndWaitUntilFinished(moveFormElementScript, 0);

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:272
> +

And here you need to run the loop again to ensure the callback has been called, this only ensures that the script has run, but we don't know if the callback was emitted or not.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:250
> +        0,

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:255
> +        0);

nullptr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:267
> +    GUniquePtr<char> formIds(g_string_free(formIdsBuilder, false));

FALSE

[Sergio Villar Senin]

Created attachment 294165 [details]
Patch for landing

[Michael Catanzaro]

Tree is open again, ready to land it?

[Sergio Villar Senin]

Committed r208532: <http://trac.webkit.org/changeset/208532>

[Michael Catanzaro]

So we need to figure out the warning it introduced:

[5372/5556] Generating ../../WebKit2WebExtension-4.0.gir
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement
<unknown>:: Warning: WebKit2WebExtension: Deprecated reference to identifier prefix WebKit in GIName WebKit.DOMElement

I tried changing "WebKit.DOMElement" to simply "DOMElement", but:

[38/60] Generating ../../WebKit2WebExtension-4.0.gir
<unknown>:: Warning: WebKit2WebExtension: (Signal)form-controls-associated: DOMElement: Unknown type: 'DOMElement'

I also tried "WebKit2.DOMElement":

[36/60] Generating ../../WebKit2WebExtension-4.0.gir
<unknown>:: Warning: WebKit2WebExtension: (Signal)form-controls-associated: WebKit2.DOMElement: Unknown type: 'WebKit2.DOMElement'

[Michael Catanzaro]

If we can't figure this out (I don't know what to do), I think we should roll it out. :(

[Carlos Garcia Campos]

(In reply to comment #17)
> If we can't figure this out (I don't know what to do), I think we should
> roll it out. :(

Because of a warning compiling the gtk-doc? I don't think so. Rolling this out would affect epiphany, this is not causing any crash, not build failure and we have plenty of time to fix it before the stable release.

[Sergio Villar Senin]

I'm fixing the warning in another bug, there is no reason to revert this.