[Webkit-unassigned] [Bug 70814] [GTK] Add webkit_web_view_get_uri() to WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70814





--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-10-27 23:26:08 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.