GNOME Bugzilla – Bug 679370
Port favicons to WebKit2
Last modified: 2012-12-04 12:35:29 UTC
This is probably the most visible regeression of WebKit2 in this moment. We need API in WebKit2 first to implement this in ephy.
New API has landed in WebKitGTK+ for this: https://bugs.webkit.org/show_bug.cgi?id=96476 https://bugs.webkit.org/show_bug.cgi?id=96477
Created attachment 225456 [details] [review] 1. Initialize the favicon database from WebKit2GTK+ as soon as possible.
Created attachment 225457 [details] [review] 2. Add new helper functions WebKit2GTK+'s favicons API.
Created attachment 225458 [details] [review] 3. Remove comment about missing code related to WebKit2GTK+'s favicons API.
Created attachment 225459 [details] [review] 4. Port EphyWebView to WebKit2GTK+ favicons API.
Created attachment 225460 [details] [review] 5. Port EphyHostsStore to WebKit2GTK+ favicons API.
Created attachment 225461 [details] [review] 6. Port EphyBookmarks and EphyBookmarkAction to WebKit2GTK+ favicons API.
Created attachment 225462 [details] [review] 7. Port EphyCompletionModel to WebKit2GTK+ favicons API.
Created attachment 225463 [details] [review] 8. Port EphyNavigationHistoryAction to WebKit2GTK+ favicons API.
Created attachment 225464 [details] [review] 9. Port PdmDialog to WebKit2GTK+ favicons API.
Created attachment 225465 [details] [review] 10. Avoid a crash in EphyBookmarksEditor when using WebKit2. This patch just avoids a crash when opening the bookmarks editor, but does not show favicons for each entry in there yet. Still need to figure out a way to port this to the new WebKit2GTK+ favicons API, as the current code relies on the try_get_favicon_pixbuf() synchronous API from WebKit1
Review of attachment 225456 [details] [review]: ::: src/ephy-shell.c @@ +593,3 @@ + /* Initialize the favicon cache as early as possible, or further + calls to webkit_web_context_get_favicon_database will fail. */ + favicon_db_path = g_build_filename (g_get_user_data_dir (), g_get_prgname (), "icondatabase", NULL); Two comments. - We probably want to respect the private session thing here. See commit f5a70c9a7c7fe5470b30cf556fe0f3938c4eb1e6 for how it was done for the HTTP cache. - Just wondering, but I think this should go to g_get_user_cache_dir ()? The favicons are not really just 'data' aren't they? Dunno.
Review of attachment 225457 [details] [review]: ::: lib/Makefile.am @@ +85,3 @@ +if WITH_WEBKIT2 +libephymisc_la_SOURCES += ephy-favicon-helpers.c +endif I think this is overkill, just add the sources unconditionally. ::: lib/ephy-favicon-helpers.c @@ +50,3 @@ + } + + return pixbuf; This seems to be using WK style all over the place (no space before (, // comments). Also I'd add "scaled" somewhere in the name of the method, since what it does is basically scale the result of gdk_pixbug_get_from_surface. Maybe 'ephy_pixbuf_get_from_surface_scaled' (note the ephy_ prefix, instead of gdk_).
Review of attachment 225458 [details] [review]: OK, I think this is too much ;) Just fold this in some other patch.
Review of attachment 225457 [details] [review]: ::: lib/ephy-favicon-helpers.c @@ +36,3 @@ + return NULL; + + pixbuf = gdk_pixbuf_get_from_surface(surface, 0, 0, You could initialize favicon_width and favicon_height using cairo_image_surface_get_width(surface) and cairo_image_surface_get_height(surface), and use it directly here so that you don't need to get the size again from the pixbuf. The size should be the same. @@ +43,3 @@ + favicon_height = gdk_pixbuf_get_height(pixbuf); + + // A size of (0, 0) means the maximum available size. I guess it means favicon original size, no?
Review of attachment 225459 [details] [review]: ::: embed/ephy-web-view.c @@ +34,3 @@ #include "ephy-embed-utils.h" #include "ephy-embed.h" +#include "ephy-favicon-helpers.h" This is OK if you add this file to the source unconditionally, otherwise also needs #ifdefs. @@ +108,3 @@ EphyHistoryPageVisitType visit_type; + WebKitFaviconDatabase *favicon_database; Needs #ifdefs around? @@ +147,3 @@ +static void icon_changed_cb (EphyWebView *view, GParamSpec *pspec, gpointer user_data); +#endif + Can we just move the callback above where it's used to get rid of this? I hate forward declarations. @@ +1706,3 @@ +#ifdef HAVE_WEBKIT2 + icon_surface = webkit_web_view_get_favicon (WEBKIT_WEB_VIEW (view)); + priv->icon = gdk_pixbuf_from_cairo_surface(icon_surface, FAVICON_SIZE, FAVICON_SIZE); space before ( @@ +1708,3 @@ + priv->icon = gdk_pixbuf_from_cairo_surface(icon_surface, FAVICON_SIZE, FAVICON_SIZE); + if (icon_surface) + cairo_surface_destroy (icon_surface); if icon_surface can be NULL shouldn't you check before trying to do the scaling? @@ +1743,3 @@ + /* The icon database will report about any icon loaded. */ + gchar *icon_uri = webkit_favicon_database_get_favicon_uri (view->priv->favicon_database, + view->priv->address); s/gchar/char/ @@ +1745,3 @@ + view->priv->address); + _ephy_web_view_set_icon_address (view, icon_uri); + g_free(icon_uri); space before ( @@ +2284,3 @@ +#ifdef HAVE_WEBKIT2 + /* Ensure we load the icon for this web view, if available */ Period after available.
Review of attachment 225460 [details] [review]: Looks OK to me, but Claudio should have a look too. ::: lib/widgets/ephy-hosts-store.c @@ +28,2 @@ #include <glib/gi18n.h> +#include <libsoup/soup.h> This is not needed, it seems? @@ +41,3 @@ + GtkTreeIter iter; + GtkTreePath *path; + IconLoadData *data = (IconLoadData *) user_data; No space there.
Review of attachment 225459 [details] [review]: ::: embed/ephy-web-view.c @@ +108,3 @@ EphyHistoryPageVisitType visit_type; + WebKitFaviconDatabase *favicon_database; He is saving the favicon database now for wk1 too. Not needed but I guess is more convenient? @@ -653,3 @@ g_object_unref(elements); } -#endif What #if does this close? You are removing it, but not the #if @@ +1099,3 @@ +#ifdef HAVE_WEBKIT2 + g_signal_handlers_disconnect_by_func (EPHY_WEB_VIEW (object), This expects a gpointer so no need to cast. @@ +2878,2 @@ #ifdef HAVE_WEBKIT2 + web_context = webkit_web_context_get_default(); Use the web view context instead, for now it's the same, but we won't have to change this if we eventually support multiple web processes.
(In reply to comment #18) > @@ -653,3 @@ > g_object_unref(elements); > } > -#endif > > What #if does this close? You are removing it, but not the #if Forgot this, I saw later that you were extending a previous #ifdef, but forgot to remove the comment, sorry.
Review of attachment 225462 [details] [review]: ::: src/ephy-completion-model.c @@ +186,3 @@ + favicon = gdk_pixbuf_from_cairo_surface (icon_surface, FAVICON_SIZE, FAVICON_SIZE); + if (icon_surface) + cairo_surface_destroy (icon_surface); I think I've seen this code snippet at least 4 or 5 times by now. Probably worth making a helper method at this point.
Review of attachment 225463 [details] [review]: ::: src/ephy-navigation-history-action.c @@ +269,3 @@ } + + g_object_unref (item); Either there was a leak here before or this cannot be shared between WK1 and WK2, no? @@ +296,3 @@ + webkit_favicon_database_get_favicon (database, address, + NULL, + (GAsyncReadyCallback) icon_loaded_cb, No space.
Review of attachment 225464 [details] [review]: ::: src/pdm-dialog.c @@ +308,3 @@ EphyEmbedShell *shell; EphyEmbedSingle *single; + WebKitFaviconDatabase* database; * in the wrong place.
Review of attachment 225465 [details] [review]: I guess the crash happens because we don't set value to anything?
Review of attachment 225461 [details] [review]: ::: src/bookmarks/ephy-bookmark-action.c @@ +109,3 @@ #endif + if (!pixbuf) + return; Leaks proxy.
Review of attachment 225456 [details] [review]: Thanks for the review. I'll work on this right away. See some comments below... ::: src/ephy-shell.c @@ +593,3 @@ + /* Initialize the favicon cache as early as possible, or further + calls to webkit_web_context_get_favicon_database will fail. */ + favicon_db_path = g_build_filename (g_get_user_data_dir (), g_get_prgname (), "icondatabase", NULL); Agreed on the first comment. I was just doing the same thing that it's being done in ephy-embed-single.c for WebKit1, but I think respecting the private session for this is a good idea anyway. I'll change that. About the second comment, I'm afraid I'm not sure what you mean. You mean avoiding the g_get_prgname() part and placing it directly in g_get_user_data_dir(). If that's the case I'm not sure that's a good idea. In my view, having this in places like ~/.local/share/epiphany (or the whatever the private session is stored) makes sense since it's, after all, data. I will keep this part as it is for the time being.
(In reply to comment #13) > Review of attachment 225457 [details] [review]: > > ::: lib/Makefile.am > @@ +85,3 @@ > +if WITH_WEBKIT2 > +libephymisc_la_SOURCES += ephy-favicon-helpers.c > +endif > > I think this is overkill, just add the sources unconditionally. Ok. > ::: lib/ephy-favicon-helpers.c > @@ +50,3 @@ > + } > + > + return pixbuf; > > This seems to be using WK style all over the place (no space before (, // > comments). Oops! Sorry about this. Same issue is probably all over the place in the other patches. Will fix it. > Also I'd add "scaled" somewhere in the name of the method, since > what it does is basically scale the result of gdk_pixbug_get_from_surface. > Maybe 'ephy_pixbuf_get_from_surface_scaled' (note the ephy_ prefix, instead of > gdk_). Makes sense. I'll do that.
(In reply to comment #14) > Review of attachment 225458 [details] [review]: > > OK, I think this is too much ;) Just fold this in some other patch. You wanted me to split it or not? :-) You're right, sorry. I'll merge it in the first commit.
(In reply to comment #15) > Review of attachment 225457 [details] [review]: > > ::: lib/ephy-favicon-helpers.c > @@ +36,3 @@ > + return NULL; > + > + pixbuf = gdk_pixbuf_get_from_surface(surface, 0, 0, > > You could initialize favicon_width and favicon_height using > cairo_image_surface_get_width(surface) and > cairo_image_surface_get_height(surface), and use it directly here so that you > don't need to get the size again from the pixbuf. The size should be the same. Right. Will change that too. > @@ +43,3 @@ > + favicon_height = gdk_pixbuf_get_height(pixbuf); > + > + // A size of (0, 0) means the maximum available size. > > I guess it means favicon original size, no? Yes, although the code is not consistent with either of those comments. I'll fix it too.
(In reply to comment #16) > Review of attachment 225459 [details] [review]: > > ::: embed/ephy-web-view.c > @@ +34,3 @@ > #include "ephy-embed-utils.h" > #include "ephy-embed.h" > +#include "ephy-favicon-helpers.h" > > This is OK if you add this file to the source unconditionally, otherwise also > needs #ifdefs. I'm no longer adding that file conditionally, so I think it's fine now. > @@ +108,3 @@ > EphyHistoryPageVisitType visit_type; > > + WebKitFaviconDatabase *favicon_database; > > Needs #ifdefs around? No. I made the most of this variable in the private structure and now I use it too in WK1 code. > @@ +147,3 @@ > +static void icon_changed_cb (EphyWebView *view, GParamSpec *pspec, gpointer > user_data); > +#endif > + > > Can we just move the callback above where it's used to get rid of this? I hate > forward declarations. Sure. > @@ +1706,3 @@ > +#ifdef HAVE_WEBKIT2 > + icon_surface = webkit_web_view_get_favicon (WEBKIT_WEB_VIEW (view)); > + priv->icon = gdk_pixbuf_from_cairo_surface(icon_surface, FAVICON_SIZE, > FAVICON_SIZE); > > space before ( Fixed. > @@ +1708,3 @@ > + priv->icon = gdk_pixbuf_from_cairo_surface(icon_surface, FAVICON_SIZE, > FAVICON_SIZE); > + if (icon_surface) > + cairo_surface_destroy (icon_surface); > > if icon_surface can be NULL shouldn't you check before trying to do the > scaling? You got a point :) > @@ +1743,3 @@ > + /* The icon database will report about any icon loaded. */ > + gchar *icon_uri = webkit_favicon_database_get_favicon_uri > (view->priv->favicon_database, > + > view->priv->address); > > s/gchar/char/ Ok. > @@ +1745,3 @@ > + > view->priv->address); > + _ephy_web_view_set_icon_address (view, icon_uri); > + g_free(icon_uri); > > space before ( Ok. > @@ +2284,3 @@ > > +#ifdef HAVE_WEBKIT2 > + /* Ensure we load the icon for this web view, if available */ > > Period after available. Ok.
(In reply to comment #18) > Review of attachment 225459 [details] [review]: > > ::: embed/ephy-web-view.c > @@ +108,3 @@ > EphyHistoryPageVisitType visit_type; > > + WebKitFaviconDatabase *favicon_database; > > He is saving the favicon database now for wk1 too. Not needed but I guess is > more convenient? I think it's more convenient to use it too in WK1, now we need it anyway for WK2. > +#ifdef HAVE_WEBKIT2 > + g_signal_handlers_disconnect_by_func (EPHY_WEB_VIEW (object), > > This expects a gpointer so no need to cast. Ok. > @@ +2878,2 @@ > #ifdef HAVE_WEBKIT2 > + web_context = webkit_web_context_get_default(); > > Use the web view context instead, for now it's the same, but we won't have to > change this if we eventually support multiple web processes. Ok.
(In reply to comment #17) > Review of attachment 225460 [details] [review]: > > Looks OK to me, but Claudio should have a look too. > > ::: lib/widgets/ephy-hosts-store.c > @@ +28,2 @@ > #include <glib/gi18n.h> > +#include <libsoup/soup.h> > > This is not needed, it seems? WebKit2 won't pull libsoup's headers and so we need this to be able to use libsoup from some places (common to WK1 and WK2), such as SoupURI and related functions. I can put the include inside an #ifdef, though. > @@ +41,3 @@ > + GtkTreeIter iter; > + GtkTreePath *path; > + IconLoadData *data = (IconLoadData *) user_data; > > No space there. Ok.
(In reply to comment #20) > Review of attachment 225462 [details] [review]: > > ::: src/ephy-completion-model.c > @@ +186,3 @@ > + favicon = gdk_pixbuf_from_cairo_surface (icon_surface, FAVICON_SIZE, > FAVICON_SIZE); > + if (icon_surface) > + cairo_surface_destroy (icon_surface); > > I think I've seen this code snippet at least 4 or 5 times by now. Probably > worth making a helper method at this point. I already thought of it, but eventually came up with the idea it was a bit overkill. I think I will leave it as it is for the time being and will get back to this later, after providing a new set of patches, if we really think it's worth it.
(In reply to comment #21) > Review of attachment 225463 [details] [review]: > > ::: src/ephy-navigation-history-action.c > @@ +269,3 @@ > } > + > + g_object_unref (item); > > Either there was a leak here before or this cannot be shared between WK1 and > WK2, no? You are right. This comes from the fact I added an extra reference to item before calling the asynchronous method (call me paranoid). I will add the extra reference in WK1 code too, so we both don't need an ifdef and makes it more secure also there. > @@ +296,3 @@ > + webkit_favicon_database_get_favicon (database, address, > + NULL, > + (GAsyncReadyCallback) icon_loaded_cb, > > No space. Ok.
(In reply to comment #23) > Review of attachment 225465 [details] [review]: > > I guess the crash happens because we don't set value to anything? Yes. That's what I think too.
Created attachment 226122 [details] [review] 1. Initialize the favicon database from WebKit2GTK+ as soon as possible.
Created attachment 226123 [details] [review] 2. Add new helper functions WebKit2GTK+'s favicons API.
Created attachment 226124 [details] [review] 3. Port EphyWebView to WebKit2GTK+ favicons API.
Created attachment 226125 [details] [review] 4. Port EphyHostsStore to WebKit2GTK+ favicons API.
Created attachment 226127 [details] [review] 5. Port EphyBookmarks and EphyBookmarkAction to WebKit2GTK+ favicons API.
Created attachment 226129 [details] [review] 6. Port EphyCompletionModel to WebKit2GTK+ favicons API.
Created attachment 226130 [details] [review] 7. Port EphyNavigationHistoryAction to WebKit2GTK+ favicons API.
Created attachment 226131 [details] [review] 8. Port PdmDialog to WebKit2GTK+ favicons API.
There are still issues with the implementation of favicons in wk2. I'm probably going to remove the favicon-ready signal, so please don't commit anything yet
See https://bugs.webkit.org/show_bug.cgi?id=98874
Created attachment 226155 [details] [review] 3. Port EphyWebView to WebKit2GTK+ favicons API. Attaching a new patch for porting EphyWebView, since I forgot to adapt the code to (transfer none) for webkit_web_view_get_favicon(). (In reply to comment #43) > There are still issues with the implementation of favicons in wk2. I'm probably > going to remove the favicon-ready signal, so please don't commit anything yet Ok. No problem. (In reply to comment #44) > See https://bugs.webkit.org/show_bug.cgi?id=98874 Thanks for the link
Review of attachment 226155 [details] [review]: This is probably too complex because it tries to reuse the existing wk1 approach. WebKit2 should be a lot easier, you only need to connect to notify::favicon in constructed and update the icon in the callaback. ::: embed/ephy-web-view.c @@ +108,3 @@ EphyHistoryPageVisitType visit_type; + WebKitFaviconDatabase *favicon_database; I don't think we need this. WebKitWebView:favicon propoerty should be enough for EphyWebView. @@ +1099,3 @@ + icon_surface = webkit_web_view_get_favicon (WEBKIT_WEB_VIEW (view)); + if (icon_surface) + priv->icon = ephy_pixbuf_get_from_surface_scaled (icon_surface, FAVICON_SIZE, FAVICON_SIZE); Maybe you could make ephy_pixbuf_get_from_surface_scaled return early if surface is null, to simply this. @@ +1110,3 @@ +static void +_ephy_web_view_set_icon_address (EphyWebView *view, + const char *icon_address) This is wrong, this is called set_icon_address and it's supposed to received the icon uri, but instead receives the page uri in wk1. And it doesn't set any icon_adress, it simply checks whether it's null or not to call _ephy_web_view_load_icon(). @@ +1133,3 @@ +{ + /* The icon database will report about any icon loaded. */ + char *icon_uri = webkit_favicon_database_get_favicon_uri (view->priv->favicon_database, We don't need the icon uri @@ +1146,3 @@ + gpointer user_data) +{ + _ephy_web_view_set_icon_address (view, address); The adress is the frame uri, not the icon uri. @@ +1368,3 @@ + won't be available until the web view is constructed. */ + web_context = webkit_web_view_get_context (WEBKIT_WEB_VIEW (object)); + priv->favicon_database = webkit_web_context_get_favicon_database (web_context); we shouldn't need this @@ +2296,3 @@ +#ifdef HAVE_WEBKIT2 + /* Ensure we load the icon for this web view, if available. */ + _ephy_web_view_load_icon (view); load_changed_cb is already inside a HAVE_WEBKIT2 block. But anyway, you shouldn't do this, simply connect to notify::favicon and update the icon.
Created attachment 226269 [details] [review] 3. Port EphyWebView to WebKit2GTK+ favicons API. New patch addressing the issues commented by Carlos
Review of attachment 226269 [details] [review]: ::: embed/ephy-web-view.c @@ +1092,3 @@ + + if (priv->icon != NULL) + return; I think icon is always NULL here, since this is called from _ephy_web_view_update_icon that always sets it to NULL. @@ +1118,3 @@ + priv->icon = NULL; + + g_object_notify (object, "icon"); I think we should merge update and load methods, you are notifying the icon change twice, first here and then in _ephy_web_view_load_icon(). Or you could move the notify at the end of this function and remove it from _ephy_web_view_load_icon().
Created attachment 226550 [details] [review] 3. Port EphyWebView to WebKit2GTK+ favicons API. New patch addressing the issues pointed out by Carlos
Review of attachment 226125 [details] [review]: ::: lib/widgets/ephy-hosts-store.c @@ +56,3 @@ + favicon = icon_surface ? ephy_pixbuf_get_from_surface_scaled (icon_surface, FAVICON_SIZE, FAVICON_SIZE) : 0; + if (icon_surface) + cairo_surface_destroy (icon_surface); I think it would be easier to read something like: if (icon_surface) { favicon = ephy_pixbuf_get_from_surface_scaled (icon_surface, FAVICON_SIZE, FAVICON_SIZE); cairo_surface_destroy (icon_surface); } else favicon = NULL; or you can even initialize favicon to NULL when decalred and you don't need the else.
Mario, can you confirm that with https://bugs.webkit.org/show_bug.cgi?id=99492 (already landed in wk) and https://bugs.webkit.org/show_bug.cgi?id=98874 (not yet landed) and your ephy patches everything works as expected? I can give it a try too if needed. It would be great to have favicons for 3.7.1
Yes, it works fine with those revisions of WK and current patches. Still need to address the issues you pointed out in the comment #50, though.
WebKit patch landed finally.
Yeah! Tomorrow morning I'll review all the patches here, address the issues from comment #50 and provide new ones if needed. Can't wait! :)
Created attachment 228527 [details] [review] 4. Port EphyHostsStore to WebKit2GTK+ favicons API New patch addressing issues from comment #50. I have locally tested all these patches again with latest code from WebKit and everything seems to work. Please review :)
Ping? :)
Don't want to be too insistent, but I think it would be nice to integrate this as soon as possible so we can test it and get feedback over it properly. Just saying O:-)
Review of attachment 226122 [details] [review]: Looks good.
Review of attachment 226123 [details] [review]: OK to commit with the minor nits fixed. ::: lib/ephy-favicon-helpers.c @@ +1,2 @@ +/* + * Copyright � 2012 Igalia S.L. The (c) symbol seems screwed here, not sure if it's bz's fault. @@ +18,3 @@ + */ + +#include <config.h> This is "config.h", not <config.h> @@ +23,3 @@ +#include <glib.h> + +#include "ephy-favicon-helpers.h" This goes after "config.h", see HACKING. @@ +36,3 @@ + return NULL; + + favicon_width = cairo_image_surface_get_width(surface); Space missing before (.
Review of attachment 226550 [details] [review]: Looks good to me.
Review of attachment 228527 [details] [review]: OK.
Review of attachment 226127 [details] [review]: OK.
Review of attachment 226131 [details] [review]: OK.
Review of attachment 226129 [details] [review]: Also looks good.
Review of attachment 226130 [details] [review]: Yes.
Thanks for the review, Xan. I've already addressed the issues you pointed out and committed the patches, so I'm now resolving this bug.