Bug 70814 - [GTK] Add webkit_web_view_get_uri() to WebKit2 GTK+ API
Summary: [GTK] Add webkit_web_view_get_uri() to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-10-25 05:33 PDT by Carlos Garcia Campos
Modified: 2011-10-28 01:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.54 KB, patch)
2011-10-25 06:09 PDT, Carlos Garcia Campos
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-10-25 05:33:49 PDT
It's currently missing.
Comment 1 Carlos Garcia Campos 2011-10-25 06:09:17 PDT
Created attachment 112326 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-25 06:12:14 PDT
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 3 Gustavo Noronha (kov) 2011-10-27 12:13:02 PDT
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 4 Martin Robinson 2011-10-27 12:31:32 PDT
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.
Comment 5 Carlos Garcia Campos 2011-10-27 23:26:08 PDT
(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.
Comment 6 Martin Robinson 2011-10-27 23:57:50 PDT
(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.
Comment 7 Carlos Garcia Campos 2011-10-28 01:13:12 PDT
Committed r98707: <http://trac.webkit.org/changeset/98707>