Bug 111288

Summary: [GTK][WK2] Add webkit_web_page_get_uri to WebKit2 GTK+ API
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cgarcia, commit-queue, gns, gyuyoung.kim, mrobinson, rakuco, sam, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110614    
Bug Blocks: 112160    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Manuel Rego Casasnovas 2013-03-04 00:09:45 PST
This is required to implement the adblock extension in Epiphnay.
Comment 1 Manuel Rego Casasnovas 2013-03-04 01:10:39 PST
Created attachment 191169 [details]
Patch

Patch to be applied after bug #110614.
Comment 2 Carlos Garcia Campos 2013-03-04 02:09:01 PST
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"
Comment 3 Manuel Rego Casasnovas 2013-03-04 05:40:13 PST
Created attachment 191209 [details]
Patch
Comment 4 Carlos Garcia Campos 2013-03-04 05:55:42 PST
Comment on attachment 191209 [details]
Patch

Excellent!
Comment 5 Carlos Garcia Campos 2013-03-04 11:07:32 PST
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.
Comment 6 Manuel Rego Casasnovas 2013-03-04 11:56:30 PST
Created attachment 191281 [details]
Patch
Comment 7 Carlos Garcia Campos 2013-03-05 01:25:19 PST
Comment on attachment 191281 [details]
Patch

Perfect.
Comment 8 Manuel Rego Casasnovas 2013-03-05 01:45:05 PST
Adding WebKit2 owners to the CC.
Comment 9 Carlos Garcia Campos 2013-03-05 23:43:53 PST
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.
Comment 10 Manuel Rego Casasnovas 2013-03-06 00:10:58 PST
Created attachment 191666 [details]
Patch

Fixed the issue in ChangeLog file. Sorry :-(
Comment 11 Carlos Garcia Campos 2013-03-12 07:27:30 PDT
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.
Comment 12 Manuel Rego Casasnovas 2013-03-12 08:26:45 PDT
Created attachment 192741 [details]
Patch

New patch unsubscribing signals when the test finishes.
Comment 13 Carlos Garcia Campos 2013-03-12 09:07:36 PDT
Comment on attachment 192741 [details]
Patch

Thanks!
Comment 14 Manuel Rego Casasnovas 2013-03-12 09:13:13 PDT
Comment on attachment 192741 [details]
Patch

Remove commit‑queue flag until bug #110614 lands.
Comment 15 Manuel Rego Casasnovas 2013-04-15 03:20:11 PDT
Created attachment 198047 [details]
Patch

Upload rebased patch as bug #110614 has already landed.
Comment 16 WebKit Commit Bot 2013-04-15 03:21:59 PDT
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 17 Manuel Rego Casasnovas 2013-04-17 00:58:32 PDT
Pinging owners as this patch is quite simple, it only adds the URI property to WebKitWebPage.
Comment 18 WebKit Commit Bot 2013-04-17 12:23:55 PDT
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
Comment 19 Manuel Rego Casasnovas 2013-04-18 00:12:54 PDT
Created attachment 198687 [details]
Patch

Rebased patch against current trunk r148658.
Comment 20 WebKit Commit Bot 2013-04-18 05:01:16 PDT
Comment on attachment 198687 [details]
Patch

Clearing flags on attachment: 198687

Committed r148666: <http://trac.webkit.org/changeset/148666>
Comment 21 WebKit Commit Bot 2013-04-18 05:01:22 PDT
All reviewed patches have been landed.  Closing bug.