[webkit-reviews] review denied: [Bug 49966] [GTK] Add an easy API to fetch a page's favicon : [Attachment 74847] favicon api for gtk webview

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied  review:
Bug 49966: [GTK] Add an easy API to fetch a page's favicon
https://bugs.webkit.org/show_bug.cgi?id=49966

Attachment 74847: favicon api for gtk webview 
https://bugs.webkit.org/attachment.cgi?id=74847&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?