[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 #4 from Martin Robinson <mrobinson at webkit.org>  2011-10-27 12:31:32 PST ---
(From update of attachment 112326)
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.

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