RESOLVED FIXED 146149
[GTK] Add initial WebKitWebsiteDataManager API for process configuration options
https://bugs.webkit.org/show_bug.cgi?id=146149
Summary [GTK] Add initial WebKitWebsiteDataManager API for process configuration options
Carlos Garcia Campos
Reported 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.
Attachments
Patch (45.92 KB, patch)
2015-06-19 10:11 PDT, Carlos Garcia Campos
no flags
Updated patch (46.36 KB, patch)
2015-06-21 23:49 PDT, Carlos Garcia Campos
no flags
Updated patch (55.56 KB, patch)
2015-06-23 03:17 PDT, Carlos Garcia Campos
svillar: review+
Carlos Garcia Campos
Comment 1 2015-06-19 10:11:00 PDT
Created attachment 255195 [details] Patch This won't apply as it depends on bug #146145
Michael Catanzaro
Comment 2 2015-06-19 10:47:36 PDT
You forgot documentation for webkit_website_data_manager_new
Carlos Garcia Campos
Comment 3 2015-06-19 10:54:51 PDT
(In reply to comment #2) > You forgot documentation for webkit_website_data_manager_new Oops.
Carlos Garcia Campos
Comment 4 2015-06-21 23:49:33 PDT
Created attachment 255339 [details] Updated patch Just added the missing documentation.
Carlos Garcia Campos
Comment 5 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.
Sergio Villar Senin
Comment 6 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
Carlos Garcia Campos
Comment 7 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?
Gustavo Noronha (kov)
Comment 8 2015-06-24 08:37:20 PDT
The API seems sane to me =)
Martin Robinson
Comment 9 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.
Carlos Garcia Campos
Comment 10 2015-06-25 00:52:31 PDT
Note You need to log in before you can comment on or make changes to this bug.