[webkit-reviews] review granted: [Bug 56200] [GTK] WebKitIconDatabase doesn't keep icons cached : [Attachment 131879] Patch

Thu Mar 15 18:16:06 PDT 2012

Martin Robinson <mrobinson at webkit.org> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 56200: [GTK] WebKitIconDatabase doesn't keep icons cached
https://bugs.webkit.org/show_bug.cgi?id=56200

Attachment 131879: Patch
https://bugs.webkit.org/attachment.cgi?id=131879&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131879&action=review

Okay. Looks good. I see lots of style errors so be sure to run
check-webkit-style directly on the new files to find the rest of them. Please
fix them all before landing and considering addressing the lingering questions
Xan and I have.

> Source/WebKit/gtk/tests/testfavicondatabase.c:26
> +#define ICON_SIZE 16

You shoulduse const int gIconSize here instead of a #define.

> Source/WebKit/gtk/tests/testfavicondatabase.c:30
> +GMainLoop *loop;
> +char* base_uri;

Asterisk craziness!

base_uri -> baseURI

> Source/WebKit/gtk/tests/testfavicondatabase.c:33
> +static void
> +server_callback(SoupServer *server, SoupMessage *msg, const char *path,
GHashTable *query, SoupClientContext *context, gpointer data)

Even though this is C code, you should still use WebKit coding style so this
declaration should be on one line and you should use camelCase and
non-abbreviated names so:

server_callback -> serverCallback
msg -> message

> Source/WebKit/gtk/tests/testfavicondatabase.c:45
> +
> +    if (g_str_equal(path, "/favicon.ico")) {

You should define length and contents here, right before you use them.

> Source/WebKit/gtk/tests/testfavicondatabase.c:73
> +static void testWebKitFaviconDatabaseSetPath(void)

Omit the void argument here.

> Source/WebKit/gtk/tests/testfavicondatabase.c:89
> +static void faviconDatabaseGetFaviconValidCallback(GObject *sourceObject,
GAsyncResult *result, gpointer userData)

faviconDatabaseGetValidFaviconCallback?

> Source/WebKit/gtk/tests/testfavicondatabase.c:102
> +static void faviconDatabaseGetFaviconInvalidCallback(GObject *sourceObject,
GAsyncResult *result, gpointer userData)

faviconDatabaseGetInvalidFaviconCallback?

> Source/WebKit/gtk/tests/testfavicondatabase.c:127
> +static inline void mainLoopQuitIfLoadCompleted(gboolean* iconOrPageLoaded)

quitMainLoopIfLoadCompleted

> Source/WebKit/gtk/tests/testfavicondatabase.c:135
> +static void idleQuitLoopCallback(WebKitWebView *webView, GParamSpec *pspec,
gboolean* iconOrPageLoaded)

pspec -> paramSpec

> Source/WebKit/gtk/tests/testfavicondatabase.c:143
> +static void webkitWebViewIconLoaded(WebKitFaviconDatabase* database, const
char* frameURI, gboolean* iconOrPageLoaded)

The asterisks are in the wrong place here.

> Source/WebKit/gtk/tests/testfavicondatabase.c:163
> +static gboolean faviconDatabaseGetFaviconValidIdle(gpointer userData)

faviconDatabaseGetValidFaviconIdle

> Source/WebKit/gtk/tests/testfavicondatabase.c:167
> +   
webkit_favicon_database_get_favicon_pixbuf(webkit_get_favicon_database(),
base_uri,
> +					       ICON_SIZE, ICON_SIZE, NULL,
> +					      
faviconDatabaseGetFaviconValidCallback, userData);

The indentation seems off here.

> Source/WebKit/gtk/tests/testfavicondatabase.c:171
> +static gboolean faviconDatabaseGetFaviconInvalidIdle(gpointer userData)

faviconDatabaseGetInvalidFaviconIdle

> Source/WebKit/gtk/tests/testfavicondatabase.c:176
> +					       ICON_SIZE, ICON_SIZE, NULL,
> +					      
faviconDatabaseGetFaviconInvalidCallback, userData);
> +    return FALSE;

The indetation looks off here as well.

> Source/WebKit/gtk/tests/testfavicondatabase.c:184
> +   
webkit_favicon_database_get_favicon_pixbuf(webkit_get_favicon_database(),
base_uri,
> +					       ICON_SIZE, ICON_SIZE,
cancellable,
> +					      
faviconDatabaseGetFaviconCancelledCallback, userData);

Ditto.

> Source/WebKit/gtk/tests/testfavicondatabase.c:190
> +static void testWebKitFaviconDatabaseGetFavicon(void)

Omit the second "void"

> Source/WebKit/gtk/tests/testfavicondatabase.c:196
> +    /* Load uri to make sure favicon is added to database */

Missing a period on this comment.

> Source/WebKit/gtk/tests/testfavicondatabase.c:246
> +    SoupServer* server;

Define this where you first use it:

SoupServer *server = soup_server...

> Source/WebKit/gtk/tests/testfavicondatabase.c:247
> +    SoupURI* soup_uri;

soup_uri -> soupURI

Your asterisk is in the wrong place. Define this where you first use it.

> Source/WebKit/gtk/tests/testfavicondatabase.c:249
> +    gtk_test_init(&argc, &argv, 0);

You should use NULL here instead of 0. This could very well crash a 64-bit
machine.

> Source/WebKit/gtk/tests/testfavicondatabase.c:252
> +    /* Hopefully make test independent of the path it's called from. */
> +    testutils_relative_chdir("Source/WebKit/gtk/tests/resources/test.html",
argv[0]);

Hopefully make test -> This hopefully makes the test

You might double-check that this works during make distcheck

> Source/WebKit/gtk/tests/testfavicondatabase.c:254
> +    server = soup_server_new(SOUP_SERVER_PORT, 0, NULL);

What does it meant to pass zero for the port here? That might deserve a
comment.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:48
> + * a particular Web page or Web site.

In this circumstance, web is not typically capitalized. You can probably just
say:

#WebKitFaviconDatabase provides access to the icons associated with web sites.

The way it is now suggests that each page or site has multiple icons.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:52
> + * images found into the memory cache if possible.

into the memory cache -> into a memory cache

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:57
> + * The database is disabled by default. In order for icons to be
> + * stored and accessed, you will need to set an icon database path
> + * using webkit_favicon_database_set_path(). Disable the database
> + * again passing %NULL to the previous call.

I think it's good to be explicit that when the database is enabled, the in
memory cache is also frozen to an on-disk database. It's a bit unclear. You
should probably put that clarification at the end of the previous paragraph.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:61
> + * If WebKitWebSettings::enable-private-browsing is %TRUE new icons
> + * won't be added to the database on disk and no existing icons will
> + * be deleted from it.

What about the in-memory cache?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:64
> + * Icons will be automatically purged after 4 days without being
> + * used.

Is this still true?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:81
> +

Extra newline here.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:87
> +    // Called when the an icon is requested while the initial
> +    // import is going on.

Called when the an icon -> Called when an icon

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:165
> +    String m_pageURI;

This is not API, so this should probably be call m_pageURL and the accessor
updated to match.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:269
>> +	  * only need the favicon of a particular #WebKitWebView.
> 
> Isn't this just the same? What's the difference between the favicon for the
mainframe and the view?

I guess he's making the distinction between something that can happen to any
WebView and a particular instance of a WebView. Agree that it's a bit unclear
here. Perhaps change  

See #WebKitWebView::icon-loaded if you only need the favicon of a particular
#WebKitWebView.

to

This signal is fired if an icon is loaded on any #WebKitWebView. If you are
only interested in a particular #WebKitWebView see #WebKitWebView::icon-loaded.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:273
>> +	  * of the favicon.
> 
> This seems more like the actual difference between the signals.

Out of curiosity, what's the reason for this difference? It seems very
arbitrary.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:283
> +								 0,
> +								 NULL,
> +								 NULL,
> +								
webkit_marshal_VOID__STRING,

ELIMINATE NULL. ;)

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:314
> + **/

I think you can omit the first asterisk.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:341
> + **/

Ditto.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:350
> +    if (!(path && path[0])) {

I think that popping open a nice cold can of DeMorgan's Law on this makes it a
little clearer:

if (!path || !path[0]) {

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:378
> + **/

Can remove one asterisk.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:407
> +    if (width && height && (icon->width() != (int) width || icon->height()
!= (int) height))

Please use static_cast here instead of C-style casts.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:427
> + * If (@width, @height) is (0, 0) then this method will return the

I think this reads better as "If @width and @height are both zero." (0, 0) can
be confused as vector notation.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:500
> + * webkit_favicon_database_try_get_favicon_pixbuf() is that it always return
the

always return -> always returns

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:509
> + * If (@width, @height) is (0, 0) then this method will return the

Same comment as above.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:540
> +    GdkPixbuf* pixbuf = getIconPixbufSynchronously(database, pageURL, 16,
16);

Why do you pass 16 pixels here for the width and height? Won't this resize the
icon to something other than @width and @height?

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:586
>> +	return static_cast<GdkPixbuf*>(g_object_ref(icon));
> 
> I checked briefly the life cycle of this and it seems you could just return
the icon as-is here and remove the extra g_object_unref destroy notifies when
setting up things?

Good point. If you remove the new reference be sure to change the annotation
above.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5176
> +    webkitWebViewRegisterForIconNotification(webView, FALSE);

You should use false here.