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

Thu Sep 27 03:09:11 PDT 2012

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

--- Comment #8 from Mario Sanchez Prada <msanchez at igalia.com>  2012-09-27 03:09:36 PST ---
(From update of attachment 165573)
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

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