[Webkit-unassigned] [Bug 96476] [WK2][GTK] Implement new Favicons API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96476


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #166241|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #15 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-09-28 07:03:59 PST ---
(From update of attachment 166241)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.