Bug 110614

Summary: [GTK][WK2] Add document-loaded signal to WebKitWebPage
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cgarcia, commit-queue, gns, jdapena, mrobinson, sam, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 83681, 111288    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Manuel Rego Casasnovas 2013-02-22 08:46:53 PST
WebKitWebPage should emit a signal when the page is loaded.
Comment 1 Manuel Rego Casasnovas 2013-02-22 09:32:59 PST
Created attachment 189782 [details]
Patch
Comment 2 Martin Robinson 2013-02-22 09:46:43 PST
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.
Comment 3 WebKit Review Bot 2013-02-22 12:36:29 PST
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
Comment 4 Gustavo Noronha (kov) 2013-02-22 13:44:30 PST
Agree with Martin, would be good to see how this API would be used in, say, Epiphany or any other client.
Comment 5 Carlos Garcia Campos 2013-02-23 01:18:47 PST
(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
Comment 6 Carlos Garcia Campos 2013-02-23 01:20:15 PST
(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 7 Carlos Garcia Campos 2013-02-23 01:33:39 PST
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.
Comment 8 Manuel Rego Casasnovas 2013-02-23 03:24:51 PST
Created attachment 189919 [details]
Patch
Comment 9 Carlos Garcia Campos 2013-02-23 04:20:22 PST
The patch looks good to me if Martin and Gustavo don't have any objection. Adding WebKit2 owners to the CC.
Comment 10 Gustavo Noronha (kov) 2013-02-23 07:14:41 PST
Comment on attachment 189919 [details]
Patch

None from me =)
Comment 11 Martin Robinson 2013-02-23 07:43:05 PST
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 12 Martin Robinson 2013-02-23 07:47:22 PST
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.
Comment 13 Carlos Garcia Campos 2013-02-23 12:18:22 PST
(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?
Comment 14 Carlos Garcia Campos 2013-02-23 12:21:55 PST
(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.
Comment 15 Martin Robinson 2013-02-23 12:28:43 PST
(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.
Comment 16 Martin Robinson 2013-02-23 12:29:33 PST
(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 17 Martin Robinson 2013-02-23 12:34:27 PST
Comment on attachment 189919 [details]
Patch

Restoring the review flag. :)
Comment 18 Manuel Rego Casasnovas 2013-02-24 04:06:30 PST
Created attachment 189969 [details]
Patch

Fix extra space before the comma.
Comment 19 Martin Robinson 2013-02-24 07:40:34 PST
Comment on attachment 189969 [details]
Patch

Looks good to me. Sorry for the confusion earlier!
Comment 20 Manuel Rego Casasnovas 2013-02-28 22:46:00 PST
Created attachment 190888 [details]
Patch

Minor fix: some method names in the commets were truncated.
Comment 21 Carlos Garcia Campos 2013-03-06 02:27:23 PST
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().
Comment 22 Manuel Rego Casasnovas 2013-03-06 02:50:25 PST
Created attachment 191698 [details]
Patch

Fixed issue unsubscribing the signal.
Comment 23 Carlos Garcia Campos 2013-03-06 02:53:57 PST
Comment on attachment 191698 [details]
Patch

Perfect, thanks!
Comment 24 Carlos Garcia Campos 2013-03-10 09:27:43 PDT
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.
Comment 25 Manuel Rego Casasnovas 2013-03-11 03:22:19 PDT
Created attachment 192433 [details]
Patch

Add early return if it's not the main frame.
Comment 26 Carlos Garcia Campos 2013-03-11 03:36:04 PDT
Ok, I think this is ready to land.
Comment 27 Manuel Rego Casasnovas 2013-03-21 10:27:59 PDT
Pinging owners as this is ready since a while ago and it's blocking some other patches for WebKit2GTK+.
Comment 28 Carlos Garcia Campos 2013-04-11 23:38:12 PDT
ping owners, this is blocking some other patches.
Comment 29 Benjamin Poulain 2013-04-12 00:03:44 PDT
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?
Comment 30 Carlos Garcia Campos 2013-04-12 00:10:37 PDT
(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?
Comment 31 Manuel Rego Casasnovas 2013-04-12 00:21:49 PDT
(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 "//".
Comment 32 Benjamin Poulain 2013-04-12 00:58:18 PDT
> > > 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.
Comment 33 Manuel Rego Casasnovas 2013-04-12 02:52:16 PDT
Created attachment 197737 [details]
Patch
Comment 34 WebKit Commit Bot 2013-04-12 09:32:25 PDT
Comment on attachment 197737 [details]
Patch

Clearing flags on attachment: 197737

Committed r148281: <http://trac.webkit.org/changeset/148281>
Comment 35 WebKit Commit Bot 2013-04-12 09:32:28 PDT
All reviewed patches have been landed.  Closing bug.