Bug 96476 – [WK2][GTK] Implement new Favicons API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

We need a way to interact with the IconDatabase from WebKit2GTK based applications, and so we need to have a favicons API available as in the case of WebKit1

I've been working a bit on this already, will attach a first iteration of a candidate patch soon.

[Mario Sanchez Prada]

Created attachment 163600 [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.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 163600 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h"
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 163600 [details]
Patch proposal (NO Unit tests yet)

View in context: https://bugs.webkit.org/attachment.cgi?id=163600&action=review

The new public header WebKitFaviconDatabase.h should be added to the main header file webkit2.h. And all new symbols should be added to the webkit2gtk-section.txt file.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:39
> +static void webkitFaviconDatabaseProcessPendingIconsForURI(WebKitFaviconDatabase*, const String&);
> +static void webkitFaviconDatabaseGetIconSurfaceCancelled(GCancellable*, GSimpleAsyncResult*);

I think these were needed in wk1 because they were used by the PendingRequest class. Now we can just move the functions before they are used and avoid the prototypes.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:85
> +static void webkit_favicon_database_class_init(WebKitFaviconDatabaseClass* findClass)

findClass?

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

Use page_uri instead of page_url

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:97
> +      * Note that this signal carries the URI of the frame that loads
> +      * the icon, while #WebKitWebView::icon-ready provides the URI
> +      * of the favicon.

I guess this is copied from wk1, there's not icon-ready signal in WebKitWebView for wk2. Here you should explain that this is emitted when the icon has been loaded from the network or imported from the database.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:102
> +                     (GSignalFlags)G_SIGNAL_RUN_LAST,

The cast is not needed, I think.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:178
> +struct GetFaviconSurfaceAsyncData {
> +    GRefPtr<WebKitFaviconDatabase> faviconDatabase;
> +    String pageURL;
> +    RefPtr<cairo_surface_t> icon;
> +    GRefPtr<GCancellable> cancellable;
> +    gulong cancelledId;
> +};

You need to provide a destructor to disconnect the cancellable signal, something like:

struct GetFaviconSurfaceAsyncData {
    ~GetFaviconSurfaceAsyncData()
    {
        if (cancelledID)
            g_cancellable_disconnect(cancellable.get(), cancelledId);
    }
.....
};

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:186
> +    if (data && data->cancelledId > 0)

data can't be NULL, add an ASSERT if you want to be extra sure.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:187
> +        g_cancellable_disconnect(data->cancellable.get(), data->cancelledId);

This should be done by the struct destructor.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:200
> +    completePendingIconRequest(result);

You can call g_simple_async_result_complete(result); directly here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:245
> + * @page_url: URL of the page containing the icon

Use page_uri

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:273
> +    data->cancelledId = cancellable
> +        ? g_cancellable_connect(cancellable, G_CALLBACK(webkitFaviconDatabaseGetIconSurfaceCancelled), result.get(), 0)
> +        : 0;

The struct is initialize to 0 when allocated, so in thsi case maybe it's clearer to use an if:

if (cancellable)
    data->cancelledId = g_cancellable_connect(cancellable, G_CALLBACK(webkitFaviconDatabaseGetIconSurfaceCancelled), result.get(), 0);

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:326
> + * Returns: (transfer full): a new reference to a #cairo_surface_t, or %NULL.

or %NULL in case of error

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:338
> +    // The following statement should always return valid data.
> +    GetFaviconSurfaceAsyncData* data = static_cast<GetFaviconSurfaceAsyncData*>(g_simple_async_result_get_op_res_gpointer(simpleResult));
> +    ASSERT(data);

I think the ASSERT already gives the idea that data shouldn't be NULL, so the comment looks redundant.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:342
> +    // Ensure we disconnect any cancellable that might have been set.
> +    if (data && data->cancelledId > 0)
> +        g_cancellable_disconnect(data->cancellable.get(), data->cancelledId);

This will be done by the destructor, and the async result will be released when the operation si completed.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:346
> +    // Return the icon is we can get it already.
> +    if (data->icon)
> +        return static_cast<cairo_surface_t*>(data->icon.release().leakRef());

Instead of leaking the ref, simple return a new ref, the one in AsyncData struct will be released by the destructor.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:363
> +        // If the icon URL is known it obviously means that the icon data
> +        // hasn't been retrieved yet, so report that it's 'unavailable'.

'unknown' instead of 'unavailable' no?

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:383
> +        completePendingIconRequest(result);

g_simple_async_result_complete()

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:394
> + * Obtains the URI for the favicon for the given page URI.
> + * See also webkit_web_view_get_icon_uri().

I guess this has been copied from wk1 too.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:396
> + * Returns: a newly allocated URI for the favicon, or %NULL

or %NULL if the database doesn't have a favicon for @page_uri

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:77
> +webkit_favicon_database_get_favicon               (WebKitFaviconDatabase* database,

WebKitFaviconDatabase* database -> WebKitFaviconDatabase *database

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:78
> +                                                   const gchar*           page_url,

page_uri, or uri

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:82
> +WEBKIT_API cairo_surface_t*

cairo_surface_t* -> cairo_surface_t *

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:90
> +webkit_favicon_database_get_favicon_finish        (WebKitFaviconDatabase* database,
> +                                                   GAsyncResult*          result,
> +                                                   GError**               error);
> +WEBKIT_API gchar*
> +webkit_favicon_database_get_favicon_uri           (WebKitFaviconDatabase* database,
> +                                                   const gchar* pageURL);
> +WEBKIT_API void
> +webkit_favicon_database_clear                     (WebKitFaviconDatabase* database);

Ok, all * are incorrectly placed.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabasePrivate.h:26
> +#include <wtf/text/CString.h>

Why do you need this here?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:46
> +#include <wtf/text/StringBuilder.h>

I don't think you need this.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:331
> + * webkit_web_context_set_favicon_database_path:

I've never known why this method expects a directory and doesn't allow to set a full path, but in case we really want it and use our own filename for the database, I would call this set_favicon_database_directory() to make clearer that the path must be a directory.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:337
> + * for @context on disk. Passing NULL as @path means using the default

%NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:342
> + * Use this before webkit_web_context_get_favicon_database() only if
> + * you want WebKitGTK+ to use a directory different than the default
> + * one for the platform.

If you want to use a different path for the favicon database, this method must be called before the database is created by webkit_web_context_get_favicon_database(). Note that a #WebKitWebView could use the favicon database, so this should also be called before loading any web page. 

Or something similar, but these cases should be clear.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:357
> +    context->priv->iconDatabasePath = WebCore::filenameToString(path).utf8();

Are you sure that if path is NULL iconDatabasePath will be null too? I would check the case in the early return:

if (context->priv->iconDatabase || !path)
    return;

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:360
> +const gchar* webkit_web_context_get_favicon_database_path(WebKitWebContext *context)

This method is undocumented.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:395
> +    StringBuilder builder;

This is unused, no?

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
>> +    GOwnPtr<gchar> iconDatabasePath(g_build_filename(webkit_web_context_get_favicon_database_path(context), "/",
>> +                                                     WebCore::IconDatabase::defaultDatabaseFilename().utf8().data(),
>> +                                                     NULL));
> 
> Use 0 instead of NULL.  [readability/null] [5]

Don't add "/", g_build_filename already adds dir separators

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:123
> +WEBKIT_API const gchar*

const gchar* -> const gchar *

[Mario Sanchez Prada]

Created attachment 165573 [details]
Patch proposal + Unit test

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

[WebKit Review Bot]

Attachment 165573 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:413:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 165573 [details]
Patch proposal + Unit test

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)

[Mario Sanchez Prada]

Comment on attachment 165573 [details]
Patch proposal + Unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=165573&action=review

Thanks Carlos for the awesome, thoroughful and incredibly helpful review. I'll work on a follow up patch for this right away.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:88
>> +      * WebKitFaviconDatabase::icon-ready:
> 
> Since we are using favicon everywhere, maybe this should be called favicon-ready

Ok.

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

Ok

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

Ok

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

Right. Thanks for pointing this out

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

I just changed it because it would mean doing a toAPI(iconDatabase) here to again have toImpl(wkIconDatabaseRef) called in the implementation of WKIconDatabaseSetIconDatabaseClient().

But yeah, I see your point and I think it's just a matter of coherence here, while not meaning any big problem anyway... so, changed!

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

Ok

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

Ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:279
>> +    data->pageURL = pageURL;
> 
> This should be String::fromUTF8(pageURI);

Right

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:284
>> +    }
> 
> 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.

Oops! Fixed!

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:294
>> +    String pageURLString = String::fromUTF8(pageURL);
> 
> You already have the pageURLString in data->pageURL

That's right. Fixed

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:372
>> +                            _("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

Ok

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

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:68
>> +} WebKitFaviconDatabaseError;
> 
> I think we should probably use FAVICON everywhere

Fixed

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

For some reason I liked it more this way, but I'm fine adding the using directive.

Fixed

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:24
>> +#include "WebIconDatabase.h"
> 
> This is already included by WebKitFaviconDatabasePrivate.h

"That is true, hacker, that is true" (http://en.wikipedia.org/wiki/Free_Software_Song)

Removed

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

Why not? Fixed!

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

Fixed

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:358
>> +        return;
> 
> Maybe we could turn this into a g_return_if_fail() since it's a programmer error to call this method twice.

Makes sense to me

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:413
>> +                                                     NULL));
> 
> Use 0 instead of NULL.  [readability/null] [5]

I'd like to but I can't, since it's a sentinel for g_build_filename() and using NULL here will spit a warning out while compiling.

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

Ok

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

Ok

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

You're probably right.

>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:56
>> +    }
> 
> Do you really need a method for this? :-)

Probably not :)

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

Ok.

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

Ok

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

You mean moving the assertions to the functions in FaviconDatabaseTest and leaving here just the test->ask*() calls?

>> 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)

Ok. I'll do

[Mario Sanchez Prada]

Created attachment 165994 [details]
Patch proposal + Unit test

New patch addressing the issues pointed out by Carlos

[WebKit Review Bot]

Attachment 165994 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:410:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:411:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h"
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:363:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:368:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 10 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 165994 [details]
Patch proposal + Unit test

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.

[Mario Sanchez Prada]

Comment on attachment 165994 [details]
Patch proposal + Unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=165994&action=review

Again thanks for the review. I'm already working on fixing these issues and will incorporate them in a follow up patch

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:23
>> +#include "WebIconDatabase.h"
> 
> This is already included by WebKitFaviconDatabasePrivate.h

Removed

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:26
>> +#include "WebKitWebContext.h"
> 
> Do we need this here?

Removed

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:33
>> +#include <wtf/gobject/GOwnPtr.h>
> 
> I think we are not using GOwnPtr here

True. Removed

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:77
>> +    priv->~WebKitFaviconDatabasePrivate();
> 
> priv is only used here so we could probably use database->priv directly.

Replaced with database->priv

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:106
>> +
> 
> What happened here with the spaces?

They changed how the check-webkit-style thing works in r129690, and it seems there's a bug related to multiline statements, that should be aligned with the opening bracket.

See https://bugs.webkit.org/show_bug.cgi?id=97602#c9

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

Fixed

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

I was taking the idea of WK1 tests, but sure I can redo this to avoid this external connection.

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

Ok, I'll try to change that then

[Mario Sanchez Prada]

Created attachment 166241 [details]
Patch proposal + Unit test

Here comes a new patch. I think (and hope) I have addressed all the remaining issues.

Eager to get some feedback

[WebKit Review Bot]

Attachment 166241 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:410:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:411:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h"
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:96:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:98:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:357:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:362:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 10 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 166241 [details]
Patch proposal + Unit test

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.

[Mario Sanchez Prada]

Thanks Carlos. I'm now pushing the patch with all those last comments addressed

[Mario Sanchez Prada]

Committed r129906: <http://trac.webkit.org/changeset/129906>

[Mario Sanchez Prada]

I have spotted some issues after landing this patch in debug builds, so I filed a new bug to keep track of the fix for that: bug 97966