Bug 146149

Summary: [GTK] Add initial WebKitWebsiteDataManager API for process configuration options
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, mcatanzaro
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 146145    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated patch svillar: review+

Description Carlos Garcia Campos 2015-06-19 09:58:10 PDT
As discussed in the mailing list, for now it will replace the different ways we have to configure data store directories.
Comment 1 Carlos Garcia Campos 2015-06-19 10:11:00 PDT
Created attachment 255195 [details]
Patch

This won't apply as it depends on bug #146145
Comment 2 Michael Catanzaro 2015-06-19 10:47:36 PDT
You forgot documentation for webkit_website_data_manager_new
Comment 3 Carlos Garcia Campos 2015-06-19 10:54:51 PDT
(In reply to comment #2)
> You forgot documentation for webkit_website_data_manager_new

Oops.
Comment 4 Carlos Garcia Campos 2015-06-21 23:49:33 PDT
Created attachment 255339 [details]
Updated patch

Just added the missing documentation.
Comment 5 Carlos Garcia Campos 2015-06-23 03:17:03 PDT
Created attachment 255402 [details]
Updated patch

Added base-data-directory and base-cache-directory properties, more API documentation and updated the unit tests.
Comment 6 Sergio Villar Senin 2015-06-24 01:26:47 PDT
Comment on attachment 255402 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255402&action=review

Provided we agree on the API this looks good to me

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:156
> +webkit_web_context_get_website_data_manager         (WebKitWebContext              *context);

Shouln't we use const here because of the immutable thing?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:158
> +            priv->webSQLDirectory.reset(g_build_filename(priv->baseDataDirectory.get(), "databases", nullptr));

Perhaps cache the value of "priv->baseDataDirectory.get()" in a local variable
Comment 7 Carlos Garcia Campos 2015-06-24 04:43:51 PDT
Comment on attachment 255402 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255402&action=review

Thanks for the review! Please, not that this depends on patch attached to bug #146145

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:156
>> +webkit_web_context_get_website_data_manager         (WebKitWebContext              *context);
> 
> Shouln't we use const here because of the immutable thing?

Well, const in C is not exactly the same as in C++. The properties in WebKitWebsiteDataManager are construct-only, so the object can't be modified no matter if the pointer is const or not.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:158
>> +            priv->webSQLDirectory.reset(g_build_filename(priv->baseDataDirectory.get(), "databases", nullptr));
> 
> Perhaps cache the value of "priv->baseDataDirectory.get()" in a local variable

What for? to make lines shorter?
Comment 8 Gustavo Noronha (kov) 2015-06-24 08:37:20 PDT
The API seems sane to me =)
Comment 9 Martin Robinson 2015-06-24 08:41:24 PDT
Comment on attachment 255402 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255402&action=review

The API looks fine to me.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:382
> +            _("The WebKitWebsiteDataManager associated to this context"),

English nit: associated to -> associated with

PS: Sorry about English.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:43
> + * a default value will be used automatically for the confoguration options

Nit: confoguration -> configuration

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:355
> + * Returns: the base directory for Website data, or %NULL if

Website is capitalized here, but not earlier.
Comment 10 Carlos Garcia Campos 2015-06-25 00:52:31 PDT
Committed r185950: <http://trac.webkit.org/changeset/185950>