WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
110614
[GTK][WK2] Add document-loaded signal to WebKitWebPage
https://bugs.webkit.org/show_bug.cgi?id=110614
Summary
[GTK][WK2] Add document-loaded signal to WebKitWebPage
Manuel Rego Casasnovas
Reported
2013-02-22 08:46:53 PST
WebKitWebPage should emit a signal when the page is loaded.
Attachments
Patch
(9.96 KB, patch)
2013-02-22 09:32 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(9.04 KB, patch)
2013-02-23 03:24 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(9.04 KB, patch)
2013-02-24 04:06 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(9.13 KB, patch)
2013-02-28 22:46 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(9.14 KB, patch)
2013-03-06 02:50 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(9.44 KB, patch)
2013-03-11 03:22 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(9.58 KB, patch)
2013-04-12 02:52 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1
2013-02-22 09:32:59 PST
Created
attachment 189782
[details]
Patch
Martin Robinson
Comment 2
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.
WebKit Review Bot
Comment 3
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
Gustavo Noronha (kov)
Comment 4
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.
Carlos Garcia Campos
Comment 5
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
Carlos Garcia Campos
Comment 6
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.
Carlos Garcia Campos
Comment 7
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.
Manuel Rego Casasnovas
Comment 8
2013-02-23 03:24:51 PST
Created
attachment 189919
[details]
Patch
Carlos Garcia Campos
Comment 9
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.
Gustavo Noronha (kov)
Comment 10
2013-02-23 07:14:41 PST
Comment on
attachment 189919
[details]
Patch None from me =)
Martin Robinson
Comment 11
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.
Martin Robinson
Comment 12
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.
Carlos Garcia Campos
Comment 13
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?
Carlos Garcia Campos
Comment 14
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.
Martin Robinson
Comment 15
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.
Martin Robinson
Comment 16
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.
Martin Robinson
Comment 17
2013-02-23 12:34:27 PST
Comment on
attachment 189919
[details]
Patch Restoring the review flag. :)
Manuel Rego Casasnovas
Comment 18
2013-02-24 04:06:30 PST
Created
attachment 189969
[details]
Patch Fix extra space before the comma.
Martin Robinson
Comment 19
2013-02-24 07:40:34 PST
Comment on
attachment 189969
[details]
Patch Looks good to me. Sorry for the confusion earlier!
Manuel Rego Casasnovas
Comment 20
2013-02-28 22:46:00 PST
Created
attachment 190888
[details]
Patch Minor fix: some method names in the commets were truncated.
Carlos Garcia Campos
Comment 21
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().
Manuel Rego Casasnovas
Comment 22
2013-03-06 02:50:25 PST
Created
attachment 191698
[details]
Patch Fixed issue unsubscribing the signal.
Carlos Garcia Campos
Comment 23
2013-03-06 02:53:57 PST
Comment on
attachment 191698
[details]
Patch Perfect, thanks!
Carlos Garcia Campos
Comment 24
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.
Manuel Rego Casasnovas
Comment 25
2013-03-11 03:22:19 PDT
Created
attachment 192433
[details]
Patch Add early return if it's not the main frame.
Carlos Garcia Campos
Comment 26
2013-03-11 03:36:04 PDT
Ok, I think this is ready to land.
Manuel Rego Casasnovas
Comment 27
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+.
Carlos Garcia Campos
Comment 28
2013-04-11 23:38:12 PDT
ping owners, this is blocking some other patches.
Benjamin Poulain
Comment 29
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?
Carlos Garcia Campos
Comment 30
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?
Manuel Rego Casasnovas
Comment 31
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 "//".
Benjamin Poulain
Comment 32
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.
Manuel Rego Casasnovas
Comment 33
2013-04-12 02:52:16 PDT
Created
attachment 197737
[details]
Patch
WebKit Commit Bot
Comment 34
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
>
WebKit Commit Bot
Comment 35
2013-04-12 09:32:28 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug