Bug 146149 – [GTK] Add initial WebKitWebsiteDataManager API for process configuration options

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

As discussed in the mailing list, for now it will replace the different ways we have to configure data store directories.

[Carlos Garcia Campos]

Created attachment 255195 [details]
Patch

This won't apply as it depends on bug #146145

[Michael Catanzaro]

You forgot documentation for webkit_website_data_manager_new

[Carlos Garcia Campos]

(In reply to comment #2)
> You forgot documentation for webkit_website_data_manager_new

Oops.

[Carlos Garcia Campos]

Created attachment 255339 [details]
Updated patch

Just added the missing documentation.

[Carlos Garcia Campos]

Created attachment 255402 [details]
Updated patch

Added base-data-directory and base-cache-directory properties, more API documentation and updated the unit tests.

[Sergio Villar Senin]

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

[Carlos Garcia Campos]

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?

[Gustavo Noronha (kov)]

The API seems sane to me =)

[Martin Robinson]

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.

[Carlos Garcia Campos]

Committed r185950: <http://trac.webkit.org/changeset/185950>