Summary: | [GTK][WK2] Add webkit_web_page_get_uri to WebKit2 GTK+ API | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | andersca, benjamin, cgarcia, commit-queue, gustavo, 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
Manuel Rego Casasnovas
2013-03-04 00:09:45 PST
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. |