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

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied 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 165994: Patch proposal + Unit test
https://bugs.webkit.org/attachment.cgi?id=165994&action=review

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


Patch looks good, only unit tests need to be improved a bit, sorry for not
replying to your question about unit tests before you posted the new patch.
Also the style issues related to spaces.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:23
> +#include "WebIconDatabase.h"

This is already included by WebKitFaviconDatabasePrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:26
> +#include "WebKitWebContext.h"

Do we need this here?

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:33
> +#include <wtf/gobject/GOwnPtr.h>

I think we are not using GOwnPtr here

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:77
> +    priv->~WebKitFaviconDatabasePrivate();

priv is only used here so we could probably use database->priv directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:106
> +

What happened here with the spaces?

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:133
> +    WebCore::NativeImagePtr icon =
priv->iconDatabase->nativeImageForPageURL(pageURL, WebCore::IntSize(1, 1));

Same here, priv is only used here so maybe we can just use database->priv
directly

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:130
> +	   webkit_favicon_database_get_favicon(database,
"http://www.webkitgtk.org/", 0, getInvalidFaviconCallback, this);

We should avoid depending on internet connection for unit tests.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:191
> +    test->m_iconReadySignalReceived = false;
> +    test->askForIconAndWaitForIconReadySignal();
> +    g_assert(test->m_iconReadySignalReceived);
> +
> +    test->m_iconIsValid = false;
> +    test->askAndWaitForValidFavicon();
> +    g_assert(test->m_iconIsValid);
> +
> +    test->askAndWaitForInvalidFavicon();
> +    g_assert(!test->m_iconIsValid);

Ok, what I meant is that there's too much logic in the fixture. This could be
something like:

// Check that icon-ready is emitted when a page with a favicon is loaded.
test->loadURI();
test->waitUntilIconReady();
g_assert(test->m_iconReadySignalReceived);

cairo_surface_t* icon = test->getFavicon(kServer->getURIForPath("/").data())
check icon is not null and even it's == blank.ico or at least has the same size


We need to figure out a way to not return a favicon from soup server callback
in this case
icon =	test->getFavicon(kServer->getURIForPath("/foo").data());
check icon is NULL

See the Downloads tests for an example.