[GTK] New API to notify about dynamically added forms
Created attachment 293001 [details] Patch
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?
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
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.
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.
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
(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.
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.
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).
Created attachment 294147 [details] Patch
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.
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
Created attachment 294165 [details] Patch for landing
Tree is open again, ready to land it?
Committed r208532: <http://trac.webkit.org/changeset/208532>
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'
If we can't figure this out (I don't know what to do), I think we should roll it out. :(
(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.
I'm fixing the warning in another bug, there is no reason to revert this.