Since r129994 there's API to get the favicon of a web view, so it would be nice to use it in the MiniBrowser.
Created attachment 166631 [details] Patch
Comment on attachment 166631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166631&action=review I guess you should also unref the favicon pixbuf in finalize(). > Tools/MiniBrowser/gtk/BrowserWindow.c:375 > + cairo_surface_t *surface; > + > + surface = webkit_web_view_get_favicon(window->webView); Use a single line here: cairo_surface_t *surface = webkit_web_view_get_favicon(window->webView); > Tools/MiniBrowser/gtk/BrowserWindow.c:445 > + g_signal_connect(window->uriEntry, "notify::text", G_CALLBACK(uriEntryTextChanged), (gpointer)window); Don't use the gpointer cast, it's not needed, I know there are several there already, but that's old code copied from GtkLauncher, I think.
Comment on attachment 166631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166631&action=review > Tools/MiniBrowser/gtk/BrowserWindow.c:367 > + } Just a small nit. WebKit coding style prefers early returns for these cases. So better if (!window->favicon) return;
(In reply to comment #3) > Just a small nit. WebKit coding style prefers early returns for these cases. So better > > if (!window->favicon) > return; If you're talking about faviconChanged(), there we want to clear the favicon if it's NULL, not just return.
Created attachment 166669 [details] Patch v2 (In reply to comment #2) > I guess you should also unref the favicon pixbuf in finalize(). > Use a single line here: > cairo_surface_t *surface = webkit_web_view_get_favicon(window->webView); > > + g_signal_connect(window->uriEntry, "notify::text", G_CALLBACK(uriEntryTextChanged), (gpointer)window); > Don't use the gpointer cast, it's not needed Ok, here's the new patch with these three changes. I also decided not to clear the icon when the URI entry is changed. So for this to work correctly, WebKitWebView has to emit notify::favicon also when a new page is loaded and it has no favicon. The emission of that signal need anyway to be changed since it's not being emitted in most cases at the moment, but it should not affect the MiniBrowser code.
Comment on attachment 166669 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=166669&action=review Thanks for taking care of this, Berto. The patch looks good to me in general, I've just have a couple of really minor comments > Tools/ChangeLog:18 > + (browserWindowConstructed): You should provide a brief description for each of these functions > Tools/MiniBrowser/gtk/BrowserWindow.c:397 > + window->favicon = NULL; You could use g_clear_object() here
(In reply to comment #6) > (From update of attachment 166669 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166669&action=review > > Thanks for taking care of this, Berto. The patch looks good to me in general, I've just have a couple of really minor comments > > > Tools/ChangeLog:18 > > + (browserWindowConstructed): > > You should provide a brief description for each of these functions > > > Tools/MiniBrowser/gtk/BrowserWindow.c:397 > > + window->favicon = NULL; > > You could use g_clear_object() here Make sure we depend on a recent enough glib to use g_clear_object
Comment on attachment 166669 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=166669&action=review > Tools/MiniBrowser/gtk/BrowserWindow.c:370 > + cairo_surface_destroy(surface); webkit_web_view_get_favicon() now returns transfer none, so you shouldn't destroy the surface.
Created attachment 166882 [details] Patch v3 Here's the updated patch after the API changes in WebKitWebView.
Comment on attachment 166882 [details] Patch v3 Excellent
Comment on attachment 166882 [details] Patch v3 Clearing flags on attachment: 166882 Committed r130282: <http://trac.webkit.org/changeset/130282>
All reviewed patches have been landed. Closing bug.