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

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Mario Sanchez Prada <msanchez at igalia.com>  2012-09-28 00:44:33 PST ---
(From update of attachment 165994)
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

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