[webkit-reviews] review granted: [Bug 98874] [GTK] WebKitWebView doesn't notify of favicon changes for known favicons but new pages : [Attachment 168732] Updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 98874: [GTK] WebKitWebView doesn't notify of favicon changes for known
favicons but new pages
https://bugs.webkit.org/show_bug.cgi?id=98874

Attachment 168732: Updated patch
https://bugs.webkit.org/attachment.cgi?id=168732&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168732&action=review


This looks sane to me. I don't think we should block the patch any longer. If
it needs to be tested in epiphany, we can always land after that's finished.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:108
>> +			 G_TYPE_STRING,
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

Please fix the style here before landing.

>>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:195
>>> +
>> 
>> Also as commented on jabber, seems we are never notified when a favicon
changen but its URIs stays the same. Not sure where this could be added as a
FIXME, but it would be good to document somewhere this kind of thing.
> 
> The API doc for favicon-changed signals says: "This signal is emitted when
the favicon URI of @page_uri has been changed to @favicon_uri in the database."
so it's documented somewhere :-)
> 
> I was assuming we were not notified when the icon data changes, because this
callback is never called for this page
http://www.p01.org/releases/DEFENDER_of_the_favicon/ that changes the favicon
dynamically, but I haven't tried other test cases, so I can be wrong.

I think this is an interesting question to answer indeed. The callback name
suggests that it should be called when the data changes, but not the URL. How
exactly do we plan to support pages like
http://www.p01.org/releases/DEFENDER_of_the_favicon/ if not with the
favicon-changed signal? If this will never be called when the URL is the same
perhaps it's better named favicon-uri-changed.