Bug 111288 – [GTK][WK2] Add webkit_web_page_get_uri to WebKit2 GTK+ API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

This is required to implement the adblock extension in Epiphnay.

[Manuel Rego Casasnovas]

Created attachment 191169 [details]
Patch

Patch to be applied after bug #110614.

[Carlos Garcia Campos]

Comment on attachment 191169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191169&action=review

Patch looks good to me, thanks!

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:291
> +    const char* uri;
> +    g_variant_get(result, "(&s)", &uri);
> +    g_assert_cmpstr(uri, ==, webkit_web_view_get_uri(test->m_webView));

Instead of testing only that the uris match, it would be interesting to test also that the uri is first /redirect and then /normal. You could add a class deriving from WebViewTest, because I think we don't need LoadTrackingTest for this particular test. The class could have a Vector of Strings where you add the current URI every time it changes. Then in the test you can use that vector to check that at 0 it's /redirect and at 1 it's /normal.

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:377
> +

Remove this empty line.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:193
> +    g_object_class_install_property(gObjectClass,

Move gObjectClass to the next line so that all parameters are lined up.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:195
> +        g_param_spec_string("uri",

Ditto, move "uri"

[Manuel Rego Casasnovas]

Created attachment 191209 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 191209 [details]
Patch

Excellent!

[Carlos Garcia Campos]

Comment on attachment 191209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191209&action=review

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:297
> +        const char* uri;
> +        g_variant_get(result, "(&s)", &uri);
> +        g_assert_cmpstr(uri, ==, webkit_web_view_get_uri(test->m_webView));
> +        test->m_URIs.append(uri);

I've noticed that this test is flaky, it depends on whether the dbus message is received after or before the wk message. I think you could connect to WebKitWebView::notify::uri and fill another Vector. And in the test compare the vectors removing the comparison here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:329
> +    g_assert_cmpstr(test->m_URIs[0].data(), ==, kServer->getURIForPath("/redirect").data());
> +    g_assert_cmpstr(test->m_URIs[1].data(), ==, kServer->getURIForPath("/normal").data());

Also noticed a problem here, kServer->getURIForPath returns a temporary CString, that can't be used with g_assert_cmpstr because it's macro, not a function. See bug https://bugs.webkit.org/show_bug.cgi?id=111346. Use ASSERT_CMP_CSTRING when the patch in that bug lands.

[Manuel Rego Casasnovas]

Created attachment 191281 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 191281 [details]
Patch

Perfect.

[Manuel Rego Casasnovas]

Adding WebKit2 owners to the CC.

[Carlos Garcia Campos]

Comment on attachment 191281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191281&action=review

> Source/WebKit2/ChangeLog:9
> +

You have removed the Reviewed by line.

[Manuel Rego Casasnovas]

Created attachment 191666 [details]
Patch

Fixed the issue in ChangeLog file. Sorry :-(

[Carlos Garcia Campos]

Comment on attachment 191666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191666&action=review

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:320
> +        g_signal_connect(m_webView, "notify::uri", G_CALLBACK(webViewURIChanged), this);
> +    }

You should add a destructor to disconnect the signal and unsubscribe the dbus signal too.

[Manuel Rego Casasnovas]

Created attachment 192741 [details]
Patch

New patch unsubscribing signals when the test finishes.

[Carlos Garcia Campos]

Comment on attachment 192741 [details]
Patch

Thanks!

[Manuel Rego Casasnovas]

Comment on attachment 192741 [details]
Patch

Remove commit‑queue flag until bug #110614 lands.

[Manuel Rego Casasnovas]

Created attachment 198047 [details]
Patch

Upload rebased patch as bug #110614 has already landed.

[WebKit Commit 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

[Manuel Rego Casasnovas]

Pinging owners as this patch is quite simple, it only adds the URI property to WebKitWebPage.

[WebKit Commit Bot]

Comment on attachment 198047 [details]
Patch

Rejecting attachment 198047 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 198047, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
eeded at 233 (offset 3 lines).
Hunk #5 succeeded at 353 with fuzz 2 (offset 52 lines).
1 out of 5 hunks FAILED -- saving rejects to file Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp.rej
patching file Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h
Hunk #1 succeeded at 60 with fuzz 2 (offset 3 lines).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Anders Carlsson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/31660

[Manuel Rego Casasnovas]

Created attachment 198687 [details]
Patch

Rebased patch against current trunk r148658.

[WebKit Commit Bot]

Comment on attachment 198687 [details]
Patch

Clearing flags on attachment: 198687

Committed r148666: <http://trac.webkit.org/changeset/148666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.