[webkit-reviews] review denied: [Bug 96476] [WK2][GTK] Implement new Favicons API : [Attachment 165573] 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 165573: Patch proposal + Unit test
https://bugs.webkit.org/attachment.cgi?id=165573&action=review

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


Looks great in general, there are some minor details, and I think we should try
to test as much as possible in the unit tests, when possible because this is
not easy to test.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:88
> +	 * WebKitFaviconDatabase::icon-ready:

Since we are using favicon everywhere, maybe this should be called
favicon-ready

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:92
> +	 * This signal gets emitted when the icon's data is ready to be

We could be a bit more explicit about what icon we are referring. Someting like


This signal gets emitted when the favicon of @page_uri is ready. This means
that the favicon's data ....

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:114
> +    gulong cancelledId;

Use unsigned long instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:168
> +    toImpl(wkIconDatabase)->retainIconForPageURL(pageURLString);

You can avoid the toImpl cast by using
database->priv->iconDatabase->retainIconForPageURL(pageURLString); directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:185
> +    iconDatabase->initializeIconDatabaseClient(&wkIconDatabaseClient);

We are using the C API in this particular case in other files, because we are
passing a C API struct client after all.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:255
> + * @page_uri: URI of the page containing the icon

We don't know if the page contains an icon or not. Maybe we should say this is
the page for which we want to get the favicon

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:269
> +void webkit_favicon_database_get_favicon(WebKitFaviconDatabase* database,
const gchar* pageURL, GCancellable* cancellable, GAsyncReadyCallback callback,
gpointer userData)

Use pageURI instead of pageURL, to know this is what we get from the api user.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:279
> +    data->pageURL = pageURL;

This should be String::fromUTF8(pageURI);

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:284
> +    if (cancellable) {
> +	   data->cancelledId =
> +	       g_cancellable_connect(cancellable,
G_CALLBACK(getIconSurfaceCancelledCallback), result.get(), 0);
> +    }

You also need to call g_simple_async_result_set_check_cancellable(), so that
when the operation is cancelled and completed in getIconSurfaceCancelled(),
g_simple_async_result_propagate_error() fills the error with Operation was
cancelled error.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:294
> +    String pageURLString = String::fromUTF8(pageURL);

You already have the pageURLString in data->pageURL

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:372
> +	   // If the icon URL is known it obviously means that the icon data
> +	   // hasn't been retrieved yet, so report that it's 'unknown'.
> +	   g_set_error_literal(error, WEBKIT_FAVICON_DATABASE_ERROR,
> +			       WEBKIT_FAVICON_DATABASE_ERROR_ICON_UNKNOWN,
> +			       _("Unknown favicon for page"));

Here we an include the page uri in the error message. Favicon for page %s is
unknown or something like that

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:383
> + * Obtains the URI for the favicon for the given page URI.

Maybe the URI of the favicon for the given page URI or even for @page_uri

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:68
> +/**
> + * WebKitFaviconDatabaseError:
> + * @WEBKIT_FAVICON_DATABASE_ERROR_NOT_INITIALIZED: The
#WebKitFaviconDatabase has not been initialized yet
> + * @WEBKIT_FAVICON_DATABASE_ERROR_ICON_NOT_FOUND: There is not an icon
available for the requested URL
> + * @WEBKIT_FAVICON_DATABASE_ERROR_ICON_UNKNOWN: There might be an icon for
the requested URL, but its data is unknown at the moment
> + *
> + * Enum values used to denote the various errors related to the
#WebKitFaviconDatabase.
> + **/
> +typedef enum {
> +    WEBKIT_FAVICON_DATABASE_ERROR_NOT_INITIALIZED,
> +    WEBKIT_FAVICON_DATABASE_ERROR_ICON_NOT_FOUND,
> +    WEBKIT_FAVICON_DATABASE_ERROR_ICON_UNKNOWN
> +} WebKitFaviconDatabaseError;

I think we should probably use FAVICON everywhere

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabasePrivate.h:26
> +WebKitFaviconDatabase*
webkitFaviconDatabaseCreate(WebKit::WebIconDatabase*);

You can add using namespace WebKit; here and remove it from the .cpp as we are
doing in other files.

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

This is already included by WebKitFaviconDatabasePrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:102
> +    GRefPtr<WebKitFaviconDatabase> iconDatabase;

faviconDatabase?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:339
> + * default directory for the platform.

You should mention what this means, just by pointing to g_get_user_data_dir()
docs.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:358
> +    // Do nothing if the database is already initialized, since in WK2
> +    // it's not allowed to change this path once the database is open.
> +    if (context->priv->iconDatabase || !path)
> +	   return;

Maybe we could turn this into a g_return_if_fail() since it's a programmer
error to call this method twice.

> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:74
> +	Source/WebKit2/UIProcess/API/gtk/tests/resources/blank.ico

No need to include this, you can use the icon directly from wk1 dir, like we
currently do in other tests.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:39
> +	   , m_mainLoop(g_main_loop_new(0, TRUE))
> +	   , m_webView(WEBKIT_WEB_VIEW(webkit_web_view_new()))

If you need a web view and a main loop you probably want to inherit from
WebViewTest instead of Test

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:40
> +	   , m_baseURI(soup_uri_to_string(kServer->baseURI(), FALSE))

Do you really need to cache this? you can use WebKitTestServer::getURIForPath()
when you need the base uri

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:56
> +    const char* baseURI()
> +    {
> +	   return m_baseURI.get();
> +    }

Do you really need a method for this? :-)

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:166
> +	   CString resourcesDir = Test::getResourcesDir();
> +	   GOwnPtr<char> pathToFavicon(g_build_filename(resourcesDir.data(),
"blank.ico", NULL));

You can use Test::getWebKit1TestResoucesDir() instead and use the icon from the
wk1 dir

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:170
> +	   GError* error = 0;
> +	   g_file_get_contents(pathToFavicon.get(), &contents, &length,
&error);
> +	   g_assert(!error);

g_file_get_contents returns a boolean, if you are no interested in the error,
use the result. It's very unlikely that this fail, so I don't think it's worth
it checking anyway.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:206
> +    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);

Maybe we should move some of the logic from the fixture here, I'm not sure, but
I think we want to test all possible errors (if possible).

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:215
> +    GOwnPtr<char> expectedURI(g_strdup_printf("%sfavicon.ico", baseURI));

You can use kServer->getURIForPath(/favicon.ico)