[webkit-reviews] review denied: [Bug 56200] [GTK] WebKitIconDatabase doesn't keep icons cached : [Attachment 100462] Updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 12 18:32:13 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia 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 100462: Updated patch
https://bugs.webkit.org/attachment.cgi?id=100462&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100462&action=review
I like this patch a great deal. I was initially a bit worried about the
approach of hiding the behavior of the WebCore icon cache. After seeing it
done in WebKit2, I think it's a great idea.
> Source/WebKit/gtk/tests/testicondatabase.c:3
> +/*
> + * Copyright (C) 2011 Igalia S.L.
> + *
Please use WebKit style naming for this file.
> Source/WebKit/gtk/tests/testicondatabase.c:29
> +static void delete_db_file_if_exists(const gchar *db_path)
Instead of "db" this should be "database"
> Source/WebKit/gtk/tests/testicondatabase.c:34
> + gchar* db_filename = g_build_filename(db_path, "WebpageIcons.db", NULL);
Webpage -> WebPage to match the typical naming convention in WebKit.
> Source/WebKit/gtk/tests/testicondatabase.c:59
> + GError* error = NULL;
NULL -> 0
> Source/WebKit/gtk/tests/testicondatabase.c:108
> + g_signal_connect(view, "icon-loaded", G_CALLBACK (idle_quit_loop_cb),
NULL);
Extra space after G_CALLBACK
> Source/WebKit/gtk/tests/testicondatabase.c:118
> + WebKitIconDatabase* icon_database = webkit_get_icon_database();
> + webkit_icon_database_get_icon_pixbuf_async(icon_database,
"http://www.google.com/", NULL,
> +
icon_database_get_icon_valid_callback, user_data);
No need to cache icon database.
> Source/WebKit/gtk/tests/testicondatabase.c:124
> + WebKitIconDatabase* icon_database = webkit_get_icon_database();
Ditto.
> Source/WebKit/gtk/tests/testicondatabase.c:132
> +{
> + WebKitIconDatabase* icon_database = webkit_get_icon_database();
Ditto
> Source/WebKit/gtk/tests/testicondatabase.c:148
> + /* Load google to make sure favicon is added to database */
This sentence is missing a period.
> Source/WebKit/gtk/tests/testicondatabase.c:166
> + webkit_icon_database_set_path(icon_database, NULL);
NULL -> 0
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:87
> + GList* pendingIcons;
> + gboolean importFinished;
Please use bool here instead of gboolean. It looks like these values are not
initialized during instance initialization. I believe you should do that
regardless of whether anything currently depends on it. Would prefer
pendingIcons to be called pendingIconRequests and for it to be a WTF::Vector.
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:277
> +static GdkPixbuf* get_icon_pixbuf(WebKitIconDatabase* database, const
String& pageURL)
Should be called something like getIconPixbufSynchronously.
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:332
> + GSimpleAsyncResult* result = request->asyncResult();
> + ASSERT(result);
> + database->priv->pendingIcons =
g_list_remove_link(database->priv->pendingIcons, icons);
> + g_simple_async_result_set_error(result, G_IO_ERROR,
G_IO_ERROR_CANCELLED, "%s", "Operation was cancelled");
> + g_simple_async_result_complete(result);
> + delete request;
> + g_list_free(icons);
> + break;
Can this be a helper in PendingIconRequest?
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:361
> + * This is an asynchronous method. When the operation is finished, callback
will
> + * be invoked. You can then call
webkit_icon_database_get_icon_pixbuf_finish()
> + * to get the result of the operation.
> + * See also webkit_icon_database_get_icon_pixbuf().
Do you think it's useful to mention why you want to load icons asynchronously
here?
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:379
> + if (cancellable) {
> + request->m_cancellable = cancellable;
> + request->m_cancelledId = g_cancellable_connect(cancellable,
G_CALLBACK(webkit_icon_database_get_icon_pixbuf_cancelled), request, 0);
> + g_object_set_data_full(G_OBJECT(result.get()), "cancellable",
g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));
> + }
> +
I believe you should just pass the cancellable to the PendingIconRequest
constructor and make all this the responsibility of the constructor.
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:383
> + String pageURL = String::fromUTF8(pageURI);
> + GdkPixbuf* pixbuf = get_icon_pixbuf(database, pageURL);
No need to cache pageURL.
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:394
> + delete request;
Wish there was a better way to handle this than manual memory management. It's
probably okay though. :)
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:433
> + if (request->pageURI() == pageURI) {
Please use an early continue here instead of nesting.
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:441
> + database->priv->pendingIcons =
g_list_remove_link(database->priv->pendingIcons, icons);
> + if (request->frameLoaderClient())
> + request->frameLoaderClient()->dispatchDidReceiveIcon();
> + else if (request->asyncResult()) {
> + GSimpleAsyncResult* result = request->asyncResult();
> + g_simple_async_result_set_op_res_gpointer(result,
get_icon_pixbuf(database, pageURI), 0);
> + g_simple_async_result_complete(result);
> + }
Can we move this to a helper in PendingIconRequest?
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:453
> + PendingIconRequest* request = new PendingIconRequest(pageURI, client);
> + database->priv->pendingIcons =
g_list_prepend(database->priv->pendingIcons, request);
No need to cache request.
> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:493
> + if (result) {
> + g_simple_async_result_set_op_res_gpointer(result, 0, 0);
> + g_simple_async_result_complete(result);
> + }
I like the idea of pushing this kind of code into methods in
PendingIconRequest.
> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:30
> +#include "FrameLoaderClient.h"
> +#include "GRefPtr.h"
> +#include "IconDatabaseClient.h"
> +#include "webkitglobals.h"
> +
> +namespace WebCore {
> +class FrameLoaderClient;
> +class IconDatabaseClient;
Why both include headers and forward declare FrameLoaderClient and
IconDatabaseClient?
> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:38
> +void
_webkit_icon_database_register_for_icon_notification(WebKitIconDatabase*, const
gchar* pageURI, WebCore::FrameLoaderClient*);
> +void
_webkit_icon_database_unregister_for_icon_notification(WebKitIconDatabase*,
const gchar* pageURI, WebCore::FrameLoaderClient*);
> +void
_webkit_icon_database_process_pending_icons_for_uri(WebKitIconDatabase*, const
String& pageURI);
> +void _webkit_icon_database_import_finished(WebKitIconDatabase*);
> +}
We've been using WebKit-style naming for private methods.
> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:54
> +class IconDatabaseClientGtk : public WebCore::IconDatabaseClient {
> + public:
> + // IconDatabaseClient interface
> + virtual bool performImport() { return true; }
> + virtual void didRemoveAllIcons() { };
> + virtual void didImportIconURLForPageURL(const String&) { };
> + virtual void didImportIconDataForPageURL(const String& URL)
> + {
> +
_webkit_icon_database_process_pending_icons_for_uri(webkit_get_icon_database(),
URL);
> + }
> + virtual void didChangeIconForPageURL(const String&) { };
> + virtual void didFinishURLImport() {
_webkit_icon_database_import_finished(webkit_get_icon_database()); }
> +};
This is only used in one file so it shouldn't be in the header.
_webkit_icon_database_process_pending_icons_for_uri and
_webkit_icon_database_import_finished can then become static.
> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:62
> +struct PendingIconRequest {
> +public:
> + PendingIconRequest(const char* pageURI, WebCore::FrameLoaderClient*
frameLoaderClient)
> + : m_pageURI(pageURI)
> + , m_frameLoaderClient(frameLoaderClient)
> + , m_asyncResult(0)
> + , m_cancellable(0)
This class is only used in one file, so it should not be in a header.
More information about the webkit-reviews
mailing list