[webkit-reviews] review denied: [Bug 55335] [GTK] Extended application cache database API and added unit tests file. : [Attachment 83999] Application cache max storage API and unit tests file

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Lukasz Slachciak
<l.slachciak at samsung.com>'s request for review:
Bug 55335: [GTK] Extended application cache database API and added unit tests
file.
https://bugs.webkit.org/show_bug.cgi?id=55335

Attachment 83999: Application cache max storage API and unit tests file
https://bugs.webkit.org/attachment.cgi?id=83999&action=review

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

Nice work. There are a couple small things I'd like to see fixed. We need
another GTK+ reviewer to okay this API addition.

> Source/WebKit/gtk/tests/testapplicationcache.c:23
> +#include <gtk/gtk.h>
> +#include <glib.h>
> +#include <glib/gprintf.h>
> +#include <webkit/webkit.h>

Includes should be in alphabetical order.

> Source/WebKit/gtk/tests/testapplicationcache.c:27
> +static void test_applicationcache_maximum_size()

applicationcache should be application_cache everywhere, I think.

> Source/WebKit/gtk/tests/testapplicationcache.c:54
> +    g_assert(g_strcmp0(databaseDirectorySet,databaseDirectoryGet) == 0);

Missing a space after the comma here. It's probably better to use
g_assert_cmpstr here.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:36
> + * By default it is set to UINT_MAX i.e. no quota

Missing a period at the end of this sentence.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:56
> + * It clears cache and rebuilds storage.

You should clarify this sentence a bit. "Changing the application cache storage
size will clear the cache."

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:62
>  void webkit_application_cache_set_maximum_size(unsigned long long size)
>  {
>  #if ENABLE(OFFLINE_WEB_APPLICATIONS)

Here it makes sense to first check if the maximum size has changed before doing
anything. Now that this API is public it needs to be more fault-tolerant.