[Webkit-unassigned] [Bug 98874] [GTK] WebKitWebView doesn't notify of favicon changes for known favicons but new pages

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Mario Sanchez Prada <msanchez at igalia.com>  2012-10-10 01:59:54 PST ---
(From update of attachment 167948)
View in context: https://bugs.webkit.org/attachment.cgi?id=167948&action=review

Looks good to me, just have some minor comments

> Source/WebKit2/ChangeLog:36
> +        (_WebKitWebViewPrivate): Add favicon URI to make we only ask for a

" to make we only ask " -> " to make sure we only ask "

> Source/WebKit2/ChangeLog:45
> +        (faviconChangedCallback): Call webkitWebViewUpdateFaviconURI) with

"webkitWebViewUpdateFaviconURI)" -> "webkitWebViewUpdateFaviconURI()"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:290
> +static void getFaviconReadyCallback(GObject* object, GAsyncResult* result, gpointer userData)

Maybe rename it to getFaviconCallback, getFaviconAsyncCallback or getFaviconFromDatabaseCallback ?

getFaviconReadyCallback sounds strange to me (seems like it comes from the previous favicon-ready signal)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:294
> +    if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED)) {

I guess the reason to only check if the error is G_IO_ERROR_CANCELLED is because in any other case (error or not) we want to update the favicon, setting it to 0 if an error other than 'cancelled' has actually happened, right?

If so, I think a brief comment would be nice here, since at a first glance it surprised me not to see something like a plain "if (!error)". Maybe it's just me, though :)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:313
> +    if (webView->priv->faviconURI == faviconURI)

I'm not sure whether you want this check here. What if the URI is the same but the actual data for the favicon has changed? I'm afraid in that case you might find yourself trying to update the favicon and getting no results.

I suppose the reason for this is that this method is actually called webkitWebViewUpdateFaviconURI(), which leds to my second comment: supposing you consider my comment of removing this early return, and as the method is actually requesting a favicon from the database in its last line, perhaps it would be better to rename it to webkitWebViewUpdateFavicon()?

Or maybe you just want to keep the URI updated and update the icon's data _only_ when that URI changes?

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