This is required to implement the adblock extension in Epiphnay.
Created attachment 191169 [details] Patch Patch to be applied after bug #110614.
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"
Created attachment 191209 [details] Patch
Comment on attachment 191209 [details] Patch Excellent!
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.
Created attachment 191281 [details] Patch
Comment on attachment 191281 [details] Patch Perfect.
Adding WebKit2 owners to the CC.
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.
Created attachment 191666 [details] Patch Fixed the issue in ChangeLog file. Sorry :-(
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.
Created attachment 192741 [details] Patch New patch unsubscribing signals when the test finishes.
Comment on attachment 192741 [details] Patch Thanks!
Comment on attachment 192741 [details] Patch Remove commit‑queue flag until bug #110614 lands.
Created attachment 198047 [details] Patch Upload rebased patch as bug #110614 has already landed.
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
Pinging owners as this patch is quite simple, it only adds the URI property to WebKitWebPage.
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
Created attachment 198687 [details] Patch Rebased patch against current trunk r148658.
Comment on attachment 198687 [details] Patch Clearing flags on attachment: 198687 Committed r148666: <http://trac.webkit.org/changeset/148666>
All reviewed patches have been landed. Closing bug.