Summary: | [GTK] [WK2] Add favicon support to the MiniBrowser | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alberto Garcia <berto> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Minor | CC: | cgarcia, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 98153 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Alberto Garcia
2012-10-01 13:52:16 PDT
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. |