It's currently missing.
Created attachment 112326 [details] Patch
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 on attachment 112326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112326&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:307 > WKPageLoadURL(toAPI(page), url.get()); > + webkitWebViewUpdateURI(webView); This made me a bit nervous, but notice I didn't read WKPageLoadURL's code yet. You are sure there isn't a race here, I assume? Other than that looks great.
Comment on attachment 112326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112326&action=review Looks good to me too. I have a couple quick comments: > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:455 > + * Returns the current active URI of @web_view. The active URI might change during > + * a load operation: Nice doc. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:506 > + g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL); NULL -> 0 here. > Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp:88 > + g_assert(!webkit_web_view_get_uri(m_webView)); IMO when we want to test behavior we should try to keep the assertions in the test rather than in the fixture. Having them in the fixture means that other tests will break, making it harder to find the source of the failure. I don't think this should block the patch though. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:130 > + static void uriChanged(GObject*, GParamSpec*, ViewURITrackingTest* test) Extra space after GObject*, > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:143 > + void checkActiveURI(const char* uri) I guess this should be private. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:145 > + // g_assert_cmpstr is a macro, so we need to cache the temporary string. Might want to clarify this a bit. The issue isn't that g_assert_cmpstr is a macro, but that it's a macro that uses it's values across several expressions. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:150 > + virtual void provisionalLoadStarted(WebKitWebLoaderClient*) These don't need to be virtual unless you expect someone to override them.
(In reply to comment #4) > (From update of attachment 112326 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112326&action=review > > Looks good to me too. I have a couple quick comments: > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:455 > > + * Returns the current active URI of @web_view. The active URI might change during > > + * a load operation: > > Nice doc. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:506 > > + g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL); > > NULL -> 0 here. oops, I wonder why style bot didn't complain. > > Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp:88 > > + g_assert(!webkit_web_view_get_uri(m_webView)); > > IMO when we want to test behavior we should try to keep the assertions in the test rather than in the fixture. Having them in the fixture means that other tests will break, making it harder to find the source of the failure. I don't think this should block the patch though. As long as other tests use the new methods in WebViewTest it will work, I added it to the fixture on purpose. LoadTrackingTest inherits from WebViewTest so any test using LoadTRackingTest will have those methods available. Estimated progress is also checked in the fixture, for example. One advantage of testing global stuff in the fixture is that it will be checked for other tests that are not thought to test it. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:130 > > + static void uriChanged(GObject*, GParamSpec*, ViewURITrackingTest* test) > > Extra space after GObject*, Ok. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:143 > > + void checkActiveURI(const char* uri) > > I guess this should be private. Ok, I've made everything public in fixtures by default. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:145 > > + // g_assert_cmpstr is a macro, so we need to cache the temporary string. > > Might want to clarify this a bit. The issue isn't that g_assert_cmpstr is a macro, but that it's a macro that uses it's values across several expressions. Well, that fact that it's a macro means that you can't expect it to be a single expression, even if current implementation is a single expression it might change, so it's safer to always cache the temp alue for macros. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:150 > > + virtual void provisionalLoadStarted(WebKitWebLoaderClient*) > > These don't need to be virtual unless you expect someone to override them. Ok.
(In reply to comment #5) > As long as other tests use the new methods in WebViewTest it will work, I added it to the fixture on purpose. LoadTrackingTest inherits from WebViewTest so any test using LoadTRackingTest will have those methods available. Estimated progress is also checked in the fixture, for example. One advantage of testing global stuff in the fixture is that it will be checked for other tests that are not thought to test it. There's an interesting tradeoff because maybe you cover some corner case that you didn't think of when you have the assertions in the fixture. On the other hand that becomes like a "hidden" test. You also have the issue where if it fails, every single test starts failing and you don't know where to start.
Committed r98707: <http://trac.webkit.org/changeset/98707>