Summary: | [WK2][GTK] Add API to get the favicon for a WebKitWebView | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mario Sanchez Prada <mario> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cgarcia | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 96476 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Mario Sanchez Prada
2012-09-12 00:50:16 PDT
Created attachment 163601 [details] Patch proposal (NO Unit tests yet) Attaching a patch proposal for this issue, based on some work I've been already doing locally and some comments/discussions with Carlos as well. Not asking for a formal review yet because my intention with this attachment is to provide an easy way to review this initial version and to start iterating from it. In case everything kind of makes sense, I will start working on unit tests for this new API. This patch depends on bug 96476 being fixed. Comment on attachment 163601 [details] Patch proposal (NO Unit tests yet) View in context: https://bugs.webkit.org/attachment.cgi?id=163601&action=review Add webkit_web_view_get_favicon to webkit2gtk-sections.txt file. > Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:155 > +cairo_surface_t* webkitFaviconDatabaseGetFaviconSync(WebKitFaviconDatabase* database, const CString& pageURL) Remove the Sync suffix, it's private api and we know it's sync. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:254 > +void webkitWebViewIconReadyCallback(WebKitFaviconDatabase *database, const char *uri, WebKitWebView *webView) This should be static > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1797 > + * Returns favicon currently associated to @web_view, if any. The > + * favicon might change during a load operation, but you can monitor > + * those changes by connecting to the notify::favicon signal of > + * @web_view. Instead of might change, I would use something similar to other methods in WebKitWebView. You can connect to notify::favicon signal of @web_view to be notified when the favicon is available. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1800 > + * Returns: (transfer full): the favicon currently associated the > + * @web_view or %NULL if no favicon has been associated yet. You should mention it's a #cairo_surface_t what is returned. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:254 > +webkit_web_view_get_favicon (WebKitWebView* webView); WebKitWebView* webView -> WebKitWebView *webView Created attachment 165591 [details]
Patch proposal + Unit test
New patch proposal, addressing the issues pointed out by Carlos and adding new unit tests
Comment on attachment 165591 [details] Patch proposal + Unit test View in context: https://bugs.webkit.org/attachment.cgi?id=165591&action=review Patch looks great, the unit tests part will probably need to be updated to the changes in bug #96476. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:241 > + iconFromWebView = webkit_web_view_get_favicon(test->m_webView.get()); > + g_assert(iconFromWebView); > + cairo_surface_destroy(iconFromWebView); I could check the icon contents, or simply that cairo_image_surface_get_width/height are > 0 > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:272 > + FaviconDatabaseTest::add("WebKitFaviconDatabase", "get-favicon-from-webview", testGetFaviconFromWebView); This could be FaviconDatabaseTest::add("WebKitWebView", "favicon", testWebViewFavicon); since wew are testing web view api even if it's run in the favicon database test. We do that in other tests. (In reply to comment #4) > (From update of attachment 165591 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165591&action=review > > Patch looks great, the unit tests part will probably need to be updated to the changes in bug #96476. > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:241 > > + iconFromWebView = webkit_web_view_get_favicon(test->m_webView.get()); > > + g_assert(iconFromWebView); > > + cairo_surface_destroy(iconFromWebView); > > I could check the icon contents, or simply that cairo_image_surface_get_width/height are > 0 > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:272 > > + FaviconDatabaseTest::add("WebKitFaviconDatabase", "get-favicon-from-webview", testGetFaviconFromWebView); > > This could be FaviconDatabaseTest::add("WebKitWebView", "favicon", testWebViewFavicon); since wew are testing web view api even if it's run in the favicon database test. We do that in other tests. Thanks for the review. I'll address those issues when landing, as soon as bug 96476 is fixed. Comment on attachment 165591 [details] Patch proposal + Unit test View in context: https://bugs.webkit.org/attachment.cgi?id=165591&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:297 > + WebKitWebViewPrivate* priv = webView->priv; > + WebKitFaviconDatabase* database = webkit_web_context_get_favicon_database(priv->context); I've noticed another minor detail here, you could move the webkit_web_context_get_favicon_database() after the early return, since most of the times you won't need the database. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:314 > + WebKitWebViewPrivate* priv = webView->priv; > + WebKitFaviconDatabase* database = webkit_web_context_get_favicon_database(priv->context); > + > + if (priv->watchForChangesInFaviconHandlerID) > + g_signal_handler_disconnect(database, priv->watchForChangesInFaviconHandlerID); > + priv->watchForChangesInFaviconHandlerID = 0; Similar here. You could use an early return: WebKitWebViewPrivate* priv = webView->priv; if (!priv->watchForChangesInFaviconHandlerID) return; WebKitFaviconDatabase* database = webkit_web_context_get_favicon_database(priv->context); g_signal_handler_disconnect(database, priv->watchForChangesInFaviconHandlerID); priv->watchForChangesInFaviconHandlerID = 0; Committed r129994: <http://trac.webkit.org/changeset/129994> |