Bug 96477 – [WK2][GTK] Add API to get the favicon for a WebKitWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

It would be nice if, besides of having a Favicons API implemented in WK2 (mostly as a wrapper of the IconDatabase), we would have also a way to easily get the favicon for a webview and get notified when it changes.

[Mario Sanchez Prada]

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.

[Carlos Garcia Campos]

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

[Mario Sanchez Prada]

Created attachment 165591 [details]
Patch proposal + Unit test

New patch proposal, addressing the issues pointed out by Carlos and adding new unit tests

[Carlos Garcia Campos]

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.

[Mario Sanchez Prada]

(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.

[Carlos Garcia Campos]

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;

[Mario Sanchez Prada]

Committed r129994: <http://trac.webkit.org/changeset/129994>