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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49966


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74847|                            |review-
               Flag|                            |




--- Comment #6 from Martin Robinson <mrobinson at webkit.org>  2010-12-01 07:46:11 PST ---
(From update of attachment 74847)
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?

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