[webkit-reviews] review granted: [Bug 96476] [WK2][GTK] Implement new Favicons API : [Attachment 166241] Patch proposal + Unit test

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 96476: [WK2][GTK] Implement new Favicons API
https://bugs.webkit.org/show_bug.cgi?id=96476

Attachment 166241: Patch proposal + Unit test
https://bugs.webkit.org/attachment.cgi?id=166241&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166241&action=review


Great!

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:344
> +    // Return the icon is we can get it already.

is -> if maybe the whole comment is a bit obvious.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:83
> +WEBKIT_API cairo_surface_t *
> +webkit_favicon_database_get_favicon_finish	     (WebKitFaviconDatabase
*database,

extra spaces between function name and (, this is the longer function so one
space is enough

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:353
> +

Extra new line here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:379
> +    WebKitWebContextPrivate* priv = context->priv;
> +

Extra line here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:405
> +    WebKitWebContextPrivate* priv = context->priv;
> +
> +    if (priv->faviconDatabase)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:58
> +	   FaviconDatabaseTest* test = static_cast<FaviconDatabaseTest*>(data);

> +	   test->m_getFaviconResult = result;
> +	   g_main_loop_quit(test->m_mainLoop);

Call webkit_favicon_database_get_favicon_finish here and save the cairo surface
in the fixture instead of the async result.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:69
> +	   g_signal_handlers_disconnect_matched(m_webView, G_SIGNAL_MATCH_DATA,
0, 0, 0, 0, this);

You are not connecting to any web view signal here, what do you want to
disconnect?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:157
> +    GError* error = 0;
> +    favicon =
webkit_favicon_database_get_favicon_finish(WEBKIT_FAVICON_DATABASE(database),
test->m_getFaviconResult.get(), &error);
> +    g_assert(!favicon);
> +    g_assert(error && WEBKIT_FAVICON_DATABASE_ERROR_FAVICON_NOT_FOUND);

You could save the error in the fixture and check it here, it's a bit weird
that the fixtures starts the operation and the test finishes it.