WebKitWebPage should emit a signal when the page is loaded.
Created attachment 189782 [details] Patch
How is this different than http://webkitgtk.org/reference/webkit2gtk/unstable/WebKitWebView.html#WebKitWebView-load-changed ? I also worry about the use of "document" here. The DOM Document being ready for use is different than the load finishing.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Agree with Martin, would be good to see how this API would be used in, say, Epiphany or any other client.
(In reply to comment #2) > How is this different than http://webkitgtk.org/reference/webkit2gtk/unstable/WebKitWebView.html#WebKitWebView-load-changed ? I also worry about the use of "document" here. The DOM Document being ready for use is different than the load finishing. This is part of the extensions API. The extensions API provides access to the DOM, the idea is to add a signal that allow applications to know when it's safe to get the DOM document. The patch is obviously wrong because it implements didFinishLoadForFrame callback instead of didFinishDocumentLoadForFrame
(In reply to comment #4) > Agree with Martin, would be good to see how this API would be used in, say, Epiphany or any other client. It will be used by ephy, for example, to get the dom document when it's available and search for login input fields to pre-fill the saved auth data, avoiding this way a message from the ui to tell the extension when to get the DOM document.
Comment on attachment 189782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189782&action=review > Source/WebKit2/ChangeLog:7 > + Please explain briefly here what this signal is for. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:51 > + WebViewTest* test = (WebViewTest*) user_data; Use C++ style cast here. You can avoid the cast and use directly WebViewTest* test instead of gpointer user_data if you cast the function pointer in g_dbus_connection_signal_subscribe > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:70 > + GRefPtr<GVariant> result = adoptGRef(g_dbus_proxy_call_sync( > + proxy.get(), > + "ConnectDocumentLoadedSignal", > + g_variant_new("(t)", webkit_web_view_get_page_id(test->m_webView)), > + G_DBUS_CALL_FLAGS_NONE, > + -1, > + 0, > + 0)); > + g_assert(result); We don't need to send a message to the extension to connect to the signal, simply connect to the signal from the extension as soon as you have a page, connecting to the page-created signal of WebKitWebExtension. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:87 > + g_assert(documentLoadedSignalReceived); I'm not sure we need the global variable, since the callback finishes the loop, if the test doesn't timeout and you are here is because the callback was called. > Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp:40 > +static void documentLoadedCallback(WebKitWebPage*, gpointer user_data) user_data -> userData > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:50 > +static void didFinishLoadForFrame(WKBundlePageRef page, WKBundleFrameRef frame, WKTypeRef* userData, const void *clientInfo) This is not the callback we want to implement, it's didFinishDocumentLoadForFrame, the eb page load is already tracked in the UI process. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:120 > + * This signal is emitted when a #WebKitWebPage load is finished. This is wrong, this signal is equivalent to WebKitWebView::document-load-finished in WebKit1, emitted when the DOM document has been loaded. You should explain here that this can be used to get the DOM document with webkit_web_page_get_dom_document.
Created attachment 189919 [details] Patch
The patch looks good to me if Martin and Gustavo don't have any objection. Adding WebKit2 owners to the CC.
Comment on attachment 189919 [details] Patch None from me =)
Comment on attachment 189919 [details] Patch Typically it's safe to start using the DOM after didClearWindowObject for frame instead of didFinishDocumentLoadForFrame and the signal would pass the actual Window object as a parameter.
Comment on attachment 189919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189919&action=review Sorry, I forgot to mention that otherwise the patch looks good. Thanks for explaining it to me. It makes a lot more sense. Another important thing is that if you are exposing these signal for WebExentions you should really also expose an isolated worlds API. It would be good to understand how that fits in properly to the API. For instance you likely want to pass the DOMWrapperWorld (maybe instead of the Window object) when you fire the window-object-cleared signal. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:56 > + "/org/webkit/gtk/WebExtensionTest" , "org.webkit.gtk.WebExtensionTest", test->m_mainLoop)); Super minor nit: You have an extra space before the comma.
(In reply to comment #11) > (From update of attachment 189919 [details]) > Typically it's safe to start using the DOM after didClearWindowObject for frame instead of didFinishDocumentLoadForFrame and the signal would pass the actual Window object as a parameter. Isn't that for injecting javascript? what is didFinishDocumentLoadFormFrame for then? is it still useful in the API?, I think I've seen apps using document-loaded in wk1 to get the DOM document, is that wrong?
(In reply to comment #12) > (From update of attachment 189919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189919&action=review > > Sorry, I forgot to mention that otherwise the patch looks good. Thanks for explaining it to me. It makes a lot more sense. Another important thing is that if you are exposing these signal for WebExentions you should really also expose an isolated worlds API. It would be good to understand how that fits in properly to the API. For instance you likely want to pass the DOMWrapperWorld (maybe instead of the Window object) when you fire the window-object-cleared signal. Yes, I thought that was a different signal in the API to allow injecting javascript. I'm adding dape to the CC because he has worked with that in webkit1. I have no idea about those APIs.
(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 189919 [details] [details]) > > Typically it's safe to start using the DOM after didClearWindowObject for frame instead of didFinishDocumentLoadForFrame and the signal would pass the actual Window object as a parameter. > > Isn't that for injecting javascript? what is didFinishDocumentLoadFormFrame for then? is it still useful in the API?, I think I've seen apps using document-loaded in wk1 to get the DOM document, is that wrong? Injecting JavaScript and touching the DOM are activities that are deeply interconnected. It looks like dispatchDidClearWindowObjectInWorld happens as soon as the Window object is ready while dispatchDidFinishDocumentLoad happens after parsing of the main resource is finished. Exposing both of these are really useful, because depending on your usecase you might want to modify the DOM as soon as possible. I think this patch should expose both signals for completeness.
(In reply to comment #15) > I think this patch should expose both signals for completeness. Though, on second thought, if window-object-cleared needs an IsolatedWorlds API (and it probably does) then it might be a good candidate for splitting off.
Comment on attachment 189919 [details] Patch Restoring the review flag. :)
Created attachment 189969 [details] Patch Fix extra space before the comma.
Comment on attachment 189969 [details] Patch Looks good to me. Sorry for the confusion earlier!
Created attachment 190888 [details] Patch Minor fix: some method names in the commets were truncated.
Comment on attachment 190888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190888&action=review Found a problem in the unit tests > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:70 > + g_main_loop_run(test->m_mainLoop); You should unsubscribe the signal here, or it will affect other tests finishing the main loop when document loaded is emitted. See g_dbus_connection_signal_unsubscribe().
Created attachment 191698 [details] Patch Fixed issue unsubscribing the signal.
Comment on attachment 191698 [details] Patch Perfect, thanks!
Comment on attachment 191698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191698&action=review Found a small issue, see below. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:52 > + g_signal_emit(WEBKIT_WEB_PAGE(clientInfo), signals[DOCUMENT_LOADED], 0); Add an early return here if this it not the main frame, we only want to emit this signal for the main frame.
Created attachment 192433 [details] Patch Add early return if it's not the main frame.
Ok, I think this is ready to land.
Pinging owners as this is ready since a while ago and it's blocking some other patches for WebKit2GTK+.
ping owners, this is blocking some other patches.
Comment on attachment 192433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192433&action=review Ok for WebKit2. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:48 > +enum { > + DOCUMENT_LOADED, > + > + LAST_SIGNAL > +}; > + > struct _WebKitWebPagePrivate { > WebPage* webPage; > }; > > +static guint signals[LAST_SIGNAL] = { 0, }; > + This looks like shooting a fly with a canon. Any reason not to make this simpler? > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:177 > + 0 // didLayout alignment?
(In reply to comment #29) > (From update of attachment 192433 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192433&action=review > > Ok for WebKit2. > > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:48 > > +enum { > > + DOCUMENT_LOADED, > > + > > + LAST_SIGNAL > > +}; > > + > > struct _WebKitWebPagePrivate { > > WebPage* webPage; > > }; > > > > +static guint signals[LAST_SIGNAL] = { 0, }; > > + > > This looks like shooting a fly with a canon. > Any reason not to make this simpler? This is the "standard" way of implementing signals in GTK+/GNOME. > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:177 > > + 0 // didLayout > > alignment?
(In reply to comment #29) > (From update of attachment 192433 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192433&action=review > > Ok for WebKit2. Thanks for the review. > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:48 > > +enum { > > + DOCUMENT_LOADED, > > + > > + LAST_SIGNAL > > +}; > > + > > struct _WebKitWebPagePrivate { > > WebPage* webPage; > > }; > > > > +static guint signals[LAST_SIGNAL] = { 0, }; > > + > > This looks like shooting a fly with a canon. > Any reason not to make this simpler? As Carlos explained this is the standard way in GNOME. > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:177 > > + 0 // didLayout > > alignment? What's the problem with alignment here? It's using 4-spaces and style checker didn't complain. Are you talking about the alignment of the comment regarding the previous lines? Maybe I can add an extra space between "0" and "//".
> > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:177 > > > + 0 // didLayout > > > > alignment? > > What's the problem with alignment here? It's using 4-spaces and style checker didn't complain. > Are you talking about the alignment of the comment regarding the previous lines? Maybe I can add an extra space between "0" and "//". It was for the comment. I would just a comma for the last null. I really don't mind if you you leave it as it is. I was just a nitpick on the way.
Created attachment 197737 [details] Patch
Comment on attachment 197737 [details] Patch Clearing flags on attachment: 197737 Committed r148281: <http://trac.webkit.org/changeset/148281>
All reviewed patches have been landed. Closing bug.