[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 #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-10-10 03:00:19 PST ---
(From update of attachment 167948)
View in context: https://bugs.webkit.org/attachment.cgi?id=167948&action=review

>> 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 "

Good catch thanks

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

Oops

>> 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)

This doesn't come from favicon-ready, but from webkit_favicon_database_get_favicon(), it's the GAsyncReadyCallback. We can call it gotFaviconCallback for example, but since there's favicon-ready signal anymore there's no confusion, no?

>> 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 :)

Yes we don't want to reset the favicon when the request has been cancelled. Maybe it's easier to understand if we return early when the error is cancelled.

>> 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?

I'm not sure that's possible, do you know a test case where the data of the favicon changes, but not he uri? Anyway, the idea is to not ask for the favicon unless the favicon URI has changed. If it's possible to change the icon contents in the database, but not the uri, this method shouldn't be called anyway, In such case webkitWebViewUpdateFavicon() will be called. I guess we would need a new signal in the database favicon-data-changed or something like that. Note that the iconChanged callback is not called in this page http://www.p01.org/releases/DEFENDER_of_the_favicon/ after the favicon has been added to the database.

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