Bug 49966 – [GTK] Add an easy API to fetch a page's favicon

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

Created attachment 74641 [details]
favicon api for gtk webivew

## add to WebKit-r68242/WebKit/gtk/webkit/webkitwebview.h

WEBKIT_API GdkPixbuf *
webkit_web_view_get_icon                        (WebKitWebView        *webView);


## add to WebKit-r68242/WebKit/gtk/webkit/webkitwebview.cpp

GdkPixbuf * webkit_web_view_get_icon(WebKitWebView* webView)
{
        g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), 0);
        String PageURL = core(webView)->mainFrame()->loader()->url().prettyURL();
        Image * icon; 

        icon = iconDatabase()->iconForPageURL(PageURL, IntSize(16,16));
        if (! icon->isNull() ){
                return icon->getGdkPixbuf();
        }else{
                icon = iconDatabase()->defaultIcon(IntSize(16,16));
                if (icon->isNull())
                        return NULL;
                return icon->getGdkPixbuf();
        }
}

Created attachment 74642 [details]
modify GtkLauncher for testing favicon.

[Martin Robinson]

Please have a look at http://webkit.org/coding/contributing.html. These patches do not follow WebKit coding conventions and do not have a ChangeLog. If you'd like to see this code get into the tree, I recommend fixing these issues.

It might also be good to provide a little bit of context here: does this change expose some behavior that was impossible before or is just to make things easier?

thanks, I'll try to fix these issues. (about coding conventions and ChangeLog)

It is hard for application to show a page's favicon. 
there is a api in mac to get the page's favicon.

- (NSImage *)mainFrameIcon;

some application use webkit_web_view_get_icon_uri(WebKitWebView *webView) 
to get favicon's uri and try to get it by itself.
(this is hard, and it may fail due to different session, cookie...)
webkit have retrive the favicon. but no api for application to get favicon in gtk.

[Martin Robinson]

IMO a GTK+ version of this method should be explicit that this is the main frame icon as well. Why not return the natural size of the icon too? That would be more flexible for anyone using the API.

Created attachment 74847 [details]
favicon api for gtk webview 

provide a api to get page's favicon for gtk webview

GdkPixbuf * webkit_web_view_get_icon(WebKitWebView* webView)

I also modify GtkLauncher to show an example.

The width and height of returned GdkPixbuf is same with what web site favicon.ico.
if website have a 48x48 favicon. the size of GdkPixbuf will be 48x48
the parameter IntSize(16,16) does not make it to return a 16x16 icon. (I don't know why?)

it try to get website favicon, if fail will return a default icon.
if fail to get a default icon. it will return NULL.

[Martin Robinson]

Comment on attachment 74847 [details]
favicon api for gtk webview 

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

I like this API addition. Great work! There are a couple style cleanups needed. Take a look at the style guidelines at http://webkit.org/coding/coding-style.html. The style for C and C++ code is slightly different. main.c isn't yet fully in WebKit style, but all new code should be.

If you'd like your patch to be reviewed, be sure to mark it with the r? flag.

> WebKit/gtk/webkit/webkitwebview.cpp:4794
> + * Obtains the favicon for the given #WebKitWebView, or defaultIcon, or
> + * %NULL if there is none.

We probably want to expand the documentation here explaining that this is the favicon of the main frame. What is "defaultIcon?" It would also be useful to talk about situations where the icon is null (no page loaded, etc). It would be worth mentioned the icon-loaded (or whatever it's called) signal, to know when to fetch the icon.

> WebKit/gtk/webkit/webkitwebview.cpp:4796
> + * Return value: the GdkPixbuf for the favicon, or %NULL

Perhaps "a GdkPixbuf containing the favicon or %NULL" ?

> WebKit/gtk/webkit/webkitwebview.cpp:4799
> +GdkPixbuf * webkit_web_view_get_icon(WebKitWebView* webView)

WebKit style says this should be GdkPixbuf* webkit_web_view_get_icon(WebKitWebView* webView).

> WebKit/gtk/webkit/webkitwebview.cpp:4805
> +    Image * icon;
> +
> +    icon = iconDatabase()->iconForPageURL(pageURL, IntSize(16, 16));

If this size is ignored, you shouldn't use the magic number 16, 16. Also there should be a comment explaining what's happening here. Please just use Image* icon = ...

> WebKit/gtk/webkit/webkitwebview.cpp:4809
> +    icon = iconDatabase()->defaultIcon(IntSize(16, 16));

Probably should have the a comment about the size here too.

> WebKitTools/ChangeLog:8
> +        show page's favicon in Toolbar

Might want to make this a bit more in depth. Be sure to use capitalization and a period too.

> WebKitTools/GtkLauncher/main.c:68
> +static void 
> +icon_loaded_cb (WebKitWebView* web_view, const gchar* icon_url, GtkWidget* favicon)

This shold be static void iconLoadedCallback(WebKitWebView *webView, const gchar *iconURL, GtkWidget *favIcon) to meet WebKit style guidelines. In C files, the asterisks go to the variable name and in C++ they go next to the type (crazy...I know).

> WebKitTools/GtkLauncher/main.c:70
> +    GdkPixbuf * icon = webkit_web_view_get_icon (web_view);

GdkPixbuf *icon =

> WebKitTools/GtkLauncher/main.c:74
> +        gtk_image_set_from_pixbuf (GTK_IMAGE (favicon), gdk_pixbuf_scale_simple (icon, 24, 24, GDK_INTERP_BILINEAR));

gtk_image_set_from_pixbuf(GTK_IMAGE(favIcon), gdk_pixbuf_scale_simple(icon, 24, 24, GDK_INTERP_BILINEAR))

> WebKitTools/GtkLauncher/main.c:76
> +        gtk_image_set_from_pixbuf (GTK_IMAGE (favicon), icon);

Same changes here.

> WebKitTools/GtkLauncher/main.c:206
> +    /* The favicon icon */
> +    item = gtk_tool_item_new();
> +    gtk_tool_item_set_expand (item, FALSE);
> +    gtk_container_add (GTK_CONTAINER (item), favicon);
> +    gtk_toolbar_insert (GTK_TOOLBAR (toolbar), item, -1);
> +
>      /* The URL entry */

Why not change the window icon instead?

[Sergio Villar Senin]

Does this make sense if https://bugs.webkit.org/show_bug.cgi?id=56200 is accepted?

[Sergio Villar Senin]

After bug 56200 we can conclude that we have an easy API to fetch page's favicon.