Bug 98874

Created attachment 167948 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Attachment 167948 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 167948 [details] Patch 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?

Comment on attachment 167948 [details] Patch 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.

(In reply to comment #5) > [...] > 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? Yes, I know. It's not about avoiding the confusion, but just about removing the word ready from there, which I think it's not needed. Not a strong opinion anyway, 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. Yes, that might work too. > [...] > I'm not sure that's possible, do you know a test case where the data of the > favicon changes, but not he uri? Not really. > 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. Fair enough, I can buy this. > 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. Ok. Forget about those comments then.

Created attachment 167969 [details] Updated patch to address review comments

Attachment 167969 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 167969 [details] Updated patch to address review comments View in context: https://bugs.webkit.org/attachment.cgi?id=167969&action=review Looks good to me. Now I think it's time someone else performed an official review. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295 > return; I definitely like more this early return.

Created attachment 168190 [details] Patch updated to current git master

Attachment 168190 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 168732 [details] Updated patch Patch updated to add a change suggested by Mario, because unit tests didn't pass for him. The idea is to wait until there's an icon record in the database to emit favicon-changed. For some reason I don't need this to make unit tests pass, but it's a good idea in any case.

(In reply to comment #12) > Created an attachment (id=168732) [details] > Updated patch > > Patch updated to add a change suggested by Mario, because unit tests didn't > pass for him. The idea is to wait until there's an icon record in the database > to emit favicon-changed. For some reason I don't need this to make unit tests > pass, but it's a good idea in any case. Thanks. I've tested this patch locally and I can confirm it works fine for me, at least in the cases I've tried it out so far (running TestWebKitFavicons and testing my patches for epiphany with it) So, it looks good to me, but some formal reviewer should bless it anyway. Any takers? Gustavo? Martin? Xan?

Attachment 168732 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 168732 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=168732&action=review This seems reasonable in general, I just have a few comments and questions. Still somebody else needs to have a look at it, and I'd like to test the full thing (ephy patches included) before pushing it. > Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:182 > + return; As commented on jabber, seems weird to check for this kind of thing in a callback. Carlos told me we do it because we cannot check beforehand when the process is done, but maybe add a FIXME for it. > 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. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:326 > + return; This seems like pushing the 'early return' religion a bit too far, but no big deal. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:182 > + // and then when the icon is loaded. What happens when you ask for the favicon the first time then? You get nothing?

Comment on attachment 168732 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=168732&action=review Thanks for reviewing it. >> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:182 >> + return; > > As commented on jabber, seems weird to check for this kind of thing in a callback. Carlos told me we do it because we cannot check beforehand when the process is done, but maybe add a FIXME for it. We would need to add a callback to the C API client to get notified when the URL import is completed >> 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. >> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:182 >> + // and then when the icon is loaded. > > What happens when you ask for the favicon the first time then? You get nothing? The first time icon is NULL, so notify::favicon will be emitted when it becomes not NULL.

(In reply to comment #15) > (From update of attachment 168732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168732&action=review > > This seems reasonable in general, I just have a few comments and questions. > Still somebody else needs to have a look at it, and I'd like to test the > full thing (ephy patches included) before pushing it. As I said, the patch looks good to me as well. About the patches for ephy, those currently in [1] are already updated to take into account the patch being proposed here, so it's easy to try it out. As for me, I've just tried them out right now and I can confirm they work fine for me. [1] https://bugzilla.gnome.org/show_bug.cgi?id=679370

(In reply to comment #17) > (In reply to comment #15) > > (From update of attachment 168732 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=168732&action=review > > > > This seems reasonable in general, I just have a few comments and questions. > > Still somebody else needs to have a look at it, and I'd like to test the > > full thing (ephy patches included) before pushing it. > > As I said, the patch looks good to me as well. About the patches for ephy, those currently in [1] are already updated to take into account the patch being proposed here, so it's easy to try it out. > > As for me, I've just tried them out right now and I can confirm they work fine for me. > > [1] https://bugzilla.gnome.org/show_bug.cgi?id=679370 Ping reviewers? This patch has been almost for 2 weeks here with no feedback and I think it would be good if we could move things forward, not to let it bitrot too much. The patch works fine for me and makes possible to port one of epiphany's most visible missing features when using WK2, both in release and debug builds, so I don't see any reason why we shouldn't give it some love already, if time permits. Thanks in advance.

Comment on attachment 168732 [details] Updated patch 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.

(In reply to comment #19) > (From update of attachment 168732 [details]) > 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. Thank you very much! > >> 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. We could add a favicon-data-changed, or simply emit favicon-changed with the same URL and document that when the signal is emitted for the same icon URL is because the icon data changed.

Committed r133867: <http://trac.webkit.org/changeset/133867>