[webkit-reviews] review denied: [Bug 110614] [GTK][WK2] Add document-loaded signal to WebKitWebPage : [Attachment 189782] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 110614: [GTK][WK2] Add document-loaded signal to WebKitWebPage
https://bugs.webkit.org/show_bug.cgi?id=110614

Attachment 189782: Patch
https://bugs.webkit.org/attachment.cgi?id=189782&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.