[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 #165994|review?                     |review-
               Flag|                            |




--- Comment #11 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-09-27 08:50:02 PST ---
(From update of attachment 165994)
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.

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