Bug 98063 – [GTK] [WK2] Add favicon support to the MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alberto Garcia]

Since r129994 there's API to get the favicon of a web view, so it
would be nice to use it in the MiniBrowser.

[Alberto Garcia]

Created attachment 166631 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 166631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166631&action=review

I guess you should also unref the favicon pixbuf in finalize().

> Tools/MiniBrowser/gtk/BrowserWindow.c:375
> +    cairo_surface_t *surface;
> +
> +    surface = webkit_web_view_get_favicon(window->webView);

Use a single line here:

cairo_surface_t *surface = webkit_web_view_get_favicon(window->webView);

> Tools/MiniBrowser/gtk/BrowserWindow.c:445
> +    g_signal_connect(window->uriEntry, "notify::text", G_CALLBACK(uriEntryTextChanged), (gpointer)window);

Don't use the gpointer cast, it's not needed, I know there are several there already, but that's old code copied from GtkLauncher, I think.

[Sergio Villar Senin]

Comment on attachment 166631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166631&action=review

> Tools/MiniBrowser/gtk/BrowserWindow.c:367
> +    }

Just a small nit. WebKit coding style prefers early returns for these cases. So better 

if (!window->favicon)
    return;

[Alberto Garcia]

(In reply to comment #3)

> Just a small nit. WebKit coding style prefers early returns for these cases. So better 
> 
> if (!window->favicon)
>     return;

If you're talking about faviconChanged(), there we want to clear the
favicon if it's NULL, not just return.

[Alberto Garcia]

Created attachment 166669 [details]
Patch v2

(In reply to comment #2)
> I guess you should also unref the favicon pixbuf in finalize().

> Use a single line here:
> cairo_surface_t *surface = webkit_web_view_get_favicon(window->webView);

> > +    g_signal_connect(window->uriEntry, "notify::text", G_CALLBACK(uriEntryTextChanged), (gpointer)window);
> Don't use the gpointer cast, it's not needed

Ok, here's the new patch with these three changes.

I also decided not to clear the icon when the URI entry is changed. So
for this to work correctly, WebKitWebView has to emit notify::favicon
also when a new page is loaded and it has no favicon.

The emission of that signal need anyway to be changed since it's not
being emitted in most cases at the moment, but it should not affect
the MiniBrowser code.

[Mario Sanchez Prada]

Comment on attachment 166669 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=166669&action=review

Thanks for taking care of this, Berto. The patch looks good to me in general, I've just have a couple of really minor comments

> Tools/ChangeLog:18
> +        (browserWindowConstructed):

You should provide a brief description for each of these functions

> Tools/MiniBrowser/gtk/BrowserWindow.c:397
> +        window->favicon = NULL;

You could use g_clear_object() here

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 166669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166669&action=review
> 
> Thanks for taking care of this, Berto. The patch looks good to me in general, I've just have a couple of really minor comments
> 
> > Tools/ChangeLog:18
> > +        (browserWindowConstructed):
> 
> You should provide a brief description for each of these functions
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:397
> > +        window->favicon = NULL;
> 
> You could use g_clear_object() here

Make sure we depend on a recent enough glib to use g_clear_object

[Carlos Garcia Campos]

Comment on attachment 166669 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=166669&action=review

> Tools/MiniBrowser/gtk/BrowserWindow.c:370
> +        cairo_surface_destroy(surface);

webkit_web_view_get_favicon() now returns transfer none, so you shouldn't destroy the surface.

[Alberto Garcia]

Created attachment 166882 [details]
Patch v3

Here's the updated patch after the API changes in WebKitWebView.

[Carlos Garcia Campos]

Comment on attachment 166882 [details]
Patch v3

Excellent

[WebKit Review Bot]

Comment on attachment 166882 [details]
Patch v3

Clearing flags on attachment: 166882

Committed r130282: <http://trac.webkit.org/changeset/130282>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.