Bug 55335 – [GTK] Extended application cache database API and added unit tests file.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Lukasz Slachciak]

1. Declared public webkit_application_cache_set_maximum_size API.
2. Declared and implemented webkit_application_cache_get_maximum_size API.
3. Added unit test file covering all API's from webkitapplicationcache.h

[Lukasz Slachciak]

Created attachment 83999 [details]
Application cache max storage API and unit tests file

[Martin Robinson]

Comment on attachment 83999 [details]
Application cache max storage API and unit tests file

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.

[Lukasz Slachciak]

Created attachment 84289 [details]
Application cache max storage API and unit tests file

[Lukasz Slachciak]

Thank you Martin for review.
I applied your suggestions.

[Martin Robinson]

CCing other GTK+ reviewers so that one of them may cast vote 2 of 2.

[Collabora GTK+ EWS bot]

Attachment 84289 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8070956

[Lukasz Slachciak]

Created attachment 84454 [details]
Application cache max storage API and unit tests file

[Martin Robinson]

Ah, one thing I neglected to mention is that you should also remove the declaration of webkit_application_cache_set_maximum_size from LayoutTestControllerGtk.cpp. It should no longer be needed.

[Lukasz Slachciak]

Created attachment 84478 [details]
Application cache max storage API and unit tests file

Additionally removed external declaration of webkit_application_cache_set_maximum_size as Martin suggested.

[Xan Lopez]

Comment on attachment 84478 [details]
Application cache max storage API and unit tests file

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 84478 [details]
Application cache max storage API and unit tests file

Clearing flags on attachment: 84478

Committed r80225: <http://trac.webkit.org/changeset/80225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lukasz Slachciak]

Thank you Martin and Xan for review!