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

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied 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 130683: Patch
https://bugs.webkit.org/attachment.cgi?id=130683&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=130683&action=review


Looks good to me. I think maybe you could split the expiration stuff in a
second patch, since it's an improvement over the first iteration and would make
it easier to review it. I'm marking r- to not be the  "scumbag reviewer" that
makes comments and does not mark the patch.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:46
> + * SECTION:webkitwebdatabase

webkitfavicondatabase

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:47
> + * @short_description: A WebKit web application database

Hrm, a favicon database right? Copy&pasta error I guess.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:51
> + * the same icon database.

I don't think it makes much sense to use browser UI terms to explain what a
favicon is here. Just explain in technically, or not at all.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:54
> + * loaded, you will need to set an icon database path using

s/loaded/stored/ ?

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

And I'd put the paragraph about enabling storage after this.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:112
> +    }

Hrm, I suppose you need to define this because C++ complains otherwise?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:136
> +	       return;

What does it mean exactly to 'return;' from a ctor? Shouldn't you ASSERT or
something?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:139
> +	   g_object_set_data_full(G_OBJECT(result), "cancellable",
g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));

Hrm, why do you need to do this if the class has both objects as members?

Added later: OK, you use it in _finish. Where is it unrefed exactly?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:221
> +}

This is doing nothing ,no need to define it.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:283
> +	 * only need the favicon of a particular #WebKitWebView.

This is confusing. Is it only for the main frame or any frame? @frame_uri: says
"the frame" and later you say main frame.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:307
> +    new (database->priv) WebKitFaviconDatabasePrivate();

Hrm, isn't this smashing the stuff you get from database->priv ? If you are
using the new() thing do you need the GObject system?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:314
> +    if (database->priv->expirationTimesDatabase) {

if (!database->...) and early return?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:356
> +{

Perhaps check with argc before accessing argv[0], just in case.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:370
> +    int lastValidTime = (int) currentTime() - ICON_EXPIRATION_TIME;

No space for the casting, right?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:496
> + * to WebKitFaviconDatabase::icon-loaded and call this in the callback.

s/call this/use this function/?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:500
> + * different size the icon will be scaled on every single call to this

I'd say better "will be scaled each time you call this function".

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

Can we add a comment here about removing the request we just created? It's
confusing otherwise.