[GTK][WK2] Add document-loaded signal to WebKitWebPage

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

WebKitWebPage should emit a signal when the page is loaded.

[Manuel Rego Casasnovas]

Created attachment 189782 [details]
Patch

[Martin Robinson]

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.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Agree with Martin, would be good to see how this API would be used in, say, Epiphany or any other client.

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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.

[Manuel Rego Casasnovas]

Created attachment 189919 [details]
Patch

[Carlos Garcia Campos]

The patch looks good to me if Martin and Gustavo don't have any objection. Adding WebKit2 owners to the CC.

[Gustavo Noronha (kov)]

Comment on attachment 189919 [details]
Patch

None from me =)

[Martin Robinson]

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.

[Martin Robinson]

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.

[Carlos Garcia Campos]

(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?

[Carlos Garcia Campos]

(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.

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Martin Robinson]

Comment on attachment 189919 [details]
Patch

Restoring the review flag. :)

[Manuel Rego Casasnovas]

Created attachment 189969 [details]
Patch

Fix extra space before the comma.

[Martin Robinson]

Comment on attachment 189969 [details]
Patch

Looks good to me. Sorry for the confusion earlier!

[Manuel Rego Casasnovas]

Created attachment 190888 [details]
Patch

Minor fix: some method names in the commets were truncated.

[Carlos Garcia Campos]

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().

[Manuel Rego Casasnovas]

Created attachment 191698 [details]
Patch

Fixed issue unsubscribing the signal.

[Carlos Garcia Campos]

Comment on attachment 191698 [details]
Patch

Perfect, thanks!

[Carlos Garcia Campos]

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.

[Manuel Rego Casasnovas]

Created attachment 192433 [details]
Patch

Add early return if it's not the main frame.

[Carlos Garcia Campos]

Ok, I think this is ready to land.

[Manuel Rego Casasnovas]

Pinging owners as this is ready since a while ago and it's blocking some other patches for WebKit2GTK+.

[Carlos Garcia Campos]

ping owners, this is blocking some other patches.

[Benjamin Poulain]

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?

[Carlos Garcia Campos]

(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?

[Manuel Rego Casasnovas]

(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 "//".

[Benjamin Poulain]

> > > 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.

[Manuel Rego Casasnovas]

Created attachment 197737 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 197737 [details]
Patch

Clearing flags on attachment: 197737

Committed r148281: <http://trac.webkit.org/changeset/148281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.