[webkit-reviews] review denied: [Bug 56200] [GTK] WebKitIconDatabase doesn't keep icons cached : [Attachment 106123] Updated patch

[bugzilla-daemon at webkit.org]

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 106123: Updated patch
https://bugs.webkit.org/attachment.cgi?id=106123&action=review

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


Sorry for the long wait for the review. r-, because of some nits and modifying
a hash while iterating through it. If you fix all this little stuff though,
it's r+. We just need another review to approve the API.

> Source/WebKit/gtk/ChangeLog:7
> +	   Reviewed by NOBODY (OOPS!).
> +

Please put a short overview of your change here.

> Source/WebKit/gtk/ChangeLog:47
> +	   (server_callback):
> +	   (deleteDatabaseFileIfExists):
> +	   (testWebkitIconDatabaseSetPath):
> +	   (iconDatabaseGetIconValidCallback):
> +	   (iconDatabaseGetIconInvalidCallback):
> +	   (iconDatabaseGetIconCancelledCallback):
> +	   (idleQuitLoopCallback):
> +	   (loadURI):
> +	   (iconDatabaseGetIconValidIdle):
> +	   (iconDatabaseGetIconInvalidIdle):
> +	   (iconDatabaseGetIconCancelledIdle):
> +	   (testWebkitIconDatabaseGetIconAsync):
> +	   (testWebkitIconDatabaseGetIconURI):
> +	   (testWebkitIconDatabaseRemoveAll):
> +	   (testWebkitIconDatabaseCloseDatabase):
> +	   (main):
> +	   * webkit/webkiticondatabase.cpp:
> +	   (IconDatabaseClientGtk::performImport):
> +	   (IconDatabaseClientGtk::didRemoveAllIcons):
> +	   (IconDatabaseClientGtk::didImportIconURLForPageURL):
> +	   (IconDatabaseClientGtk::didImportIconDataForPageURL):
> +	   (IconDatabaseClientGtk::didChangeIconForPageURL):
> +	   (IconDatabaseClientGtk::didFinishURLImport):
> +	   (PendingIconRequest::PendingIconRequest):
> +	   (PendingIconRequest::~PendingIconRequest):
> +	   (PendingIconRequest::pageURI):
> +	   (PendingIconRequest::frameLoaderClient):
> +	   (PendingIconRequest::asyncResult):
> +	   (PendingIconRequest::asyncResultComplete):
> +	   (PendingIconRequest::asyncResultCompleteInIdle):
> +	   (PendingIconRequest::asyncResultCancel):
> +	   (PendingIconRequest::frameLoaderDispatchDidReceiveIcon):
> +	   (webkitIconDatabaseIconLoaded): Retain the icon when loaded to

Sicne the method descriptions are all empty, they are just noise. In these
situations I think it's fine to just leave the filename.

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

Extra newlines here. No need to split the signature across multiple lines.

> Source/WebKit/gtk/tests/testicondatabase.c:89
> +    GError *error = 0;

0 should be NULL here.

> Source/WebKit/gtk/tests/testicondatabase.c:142
> +    webkit_web_view_load_uri(view, uri);
> +    //    g_signal_connect(view, "icon-loaded",
G_CALLBACK(idleQuitLoopCallback), NULL);
> +    g_signal_connect(view, "notify::load-status",
G_CALLBACK(idleQuitLoopCallback), NULL);

If this line isn't needed, just remove it instead of commenting it out.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:71
> +struct PendingIconRequest;

You should say:

class PendingIconRequest.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:146
> +	   g_simple_async_result_set_error(m_asyncResult.get(), G_IO_ERROR,
G_IO_ERROR_CANCELLED, "%s", "Operation was cancelled");

The string "Operation was cancelled" is missing the l10n underscore.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:181
> +typedef HashMap<String, PendingIconRequestVector* > PendingIconRequestMap;

Extra space after PendingIconRequestVector*

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:392
> +    // We must pass something greater than 0, 0 to get a pixbuf.

0, 0 -> 0x0 or 0 by 0

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:440
> +    database->priv->pendingIconRequests.remove(pageURL);
> +    delete requests;

I think I'd rather this take an iterator or just a pageURI, but not both the
value and the index. Maybe just a pageURI. You could do:

iterator toDelete = pendingIconRequests.remove(pageURL);
delete toDelete->second;
database->priv->pendingIconRequests.remove(toDelete);

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:467
> +static void webkitIconDatabaseGetIconPixbufCancelled(GCancellable*
cancellable, PendingIconRequest* request)
> +{
> +    // Handle cancelled in a in idle since it might be called from any
thread.
> +    g_idle_add(getIconPixbufCancelledIdle, request);
> +}

I think it would be better to use the wtf::callOnMainThread here.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:483
> + * 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().

I'm not sure I like the use of GIO style APIs here. At this point though, we
should get this patch in and then reconsider the APIs for WebKit2.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:485
> + * Since: 1.5.3

This will need to change now. :(

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:495
> +    String pageURL(String::fromUTF8(pageURI));

I think the preferred style is:  String pageURL = String::fromUTF8(pageURI);

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:505
> +	   // Import is ongoing, there might be an icon.

I think a comment like: " Import is ongoing, the icon we want might load
later." would be less confusing.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:527
> + * Since: 1.5.3

Should change again.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:539
> +	   g_set_error_literal(error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
"Operation was cancelled");

I think "Operation was	cancelled" needs to be prefixed by an underscore for
l10n.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:544
> +    return icon ? static_cast<GdkPixbuf*>(g_object_ref(icon)) : 0;

I think it's clearer here to use an early return:
if (!icon)
    return 0;
return g_object_ref(icon);

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:547
> +void webkitIconDatabaseRegisterForIconNotification(WebKitIconDatabase*
database, const gchar* pageURI, WebCore::FrameLoaderClient* client)

I'd rather this took a const String& pageURI instead of converting it here and
in webkitIconDatabaseUnregisterForIconNotification.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:557
> +void webkitIconDatabaseUnregisterForIconNotification(WebKitIconDatabase*
database, const gchar* pageURI, WebCore::FrameLoaderClient* client)
> +{
> +    String pageURL(String::fromUTF8(pageURI));

Ditto.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:601
> +    // requests we only need to delete the request so that icon-loaded won't
be emitted.
> +    PendingIconRequestMap::const_iterator end =
database->priv->pendingIconRequests.end();
> +    for (PendingIconRequestMap::const_iterator iter =
database->priv->pendingIconRequests.begin(); iter != end; ++iter) {
> +	   String iconURL =
WebCore::iconDatabase().synchronousIconURLForPageURL(iter->first);
> +	   if (!iconURL.isEmpty())
> +	       continue;
> +
> +	   PendingIconRequestVector* icons = iter->second;
> +	   for (size_t i = 0; i < icons->size(); ++i) {
> +	       PendingIconRequest* request = icons->at(i).get();
> +	       if (request->asyncResult())
> +		   request->asyncResultComplete(0);
> +	   }
> +
> +	   webkitIconDatabaseDeleteRequests(database, icons, iter->first);

I'm not sure it's safe to modify the HashSet while you are using an iterator to
it. Better to keep a list of defunct pageURIs and then delete them afterward.

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:30
> +extern "C" {
> +void webkitIconDatabaseRegisterForIconNotification(WebKitIconDatabase*,
const gchar* pageURI, WebCore::FrameLoaderClient*);
> +void webkitIconDatabaseUnregisterForIconNotification(WebKitIconDatabase*,
const gchar* pageURI, WebCore::FrameLoaderClient*);
> +}

Please omit the extern "C". It's fine that these are mangled.