Bug 55335 - [GTK] Extended application cache database API and added unit tests file.
Summary: [GTK] Extended application cache database API and added unit tests file.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 54823
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-27 15:27 PST by Lukasz Slachciak
Modified: 2011-03-03 10:21 PST (History)
5 users (show)

See Also:


Attachments
Application cache max storage API and unit tests file (9.00 KB, patch)
2011-02-27 15:33 PST, Lukasz Slachciak
mrobinson: review-
Details | Formatted Diff | Diff
Application cache max storage API and unit tests file (6.61 KB, patch)
2011-03-01 13:51 PST, Lukasz Slachciak
no flags Details | Formatted Diff | Diff
Application cache max storage API and unit tests file (deleted)
2011-03-02 13:18 PST, Lukasz Slachciak
no flags Details | Formatted Diff | Diff
Application cache max storage API and unit tests file (10.71 KB, patch)
2011-03-02 15:43 PST, Lukasz Slachciak
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Slachciak 2011-02-27 15:27:10 PST
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
Comment 1 Lukasz Slachciak 2011-02-27 15:33:14 PST
Created attachment 83999 [details]
Application cache max storage API and unit tests file
Comment 2 Martin Robinson 2011-03-01 09:31:10 PST
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.
Comment 3 Lukasz Slachciak 2011-03-01 13:51:27 PST
Created attachment 84289 [details]
Application cache max storage API and unit tests file
Comment 4 Lukasz Slachciak 2011-03-01 13:52:28 PST
Thank you Martin for review.
I applied your suggestions.
Comment 5 Martin Robinson 2011-03-01 17:20:16 PST
CCing other GTK+ reviewers so that one of them may cast vote 2 of 2.
Comment 6 Collabora GTK+ EWS bot 2011-03-02 00:16:19 PST
Attachment 84289 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8070956
Comment 7 Lukasz Slachciak 2011-03-02 13:18:20 PST
Created attachment 84454 [details]
Application cache max storage API and unit tests file
Comment 8 Martin Robinson 2011-03-02 15:07:18 PST
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.
Comment 9 Lukasz Slachciak 2011-03-02 15:43:14 PST
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.
Comment 10 Xan Lopez 2011-03-02 16:15:35 PST
Comment on attachment 84478 [details]
Application cache max storage API and unit tests file

Looks good to me.
Comment 11 WebKit Commit Bot 2011-03-03 03:06:51 PST
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>
Comment 12 WebKit Commit Bot 2011-03-03 03:06:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Lukasz Slachciak 2011-03-03 10:21:28 PST
Thank you Martin and Xan for review!