Bug 62091 – Add API to WebKit-GTK to allow setting localStorage database path

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

[Michael Stegeman]

Created attachment 96023 [details]
Add API to allow setting local storage database path

Overview:
WebKit-GTK currently has no way to set the path for localStorage databases to be kept. As such, localStorage is never persistent in GTK browsers, such as Epiphany, Midori, luakit, and uzbl (all 4 have been confirmed to suffer from this same issue).

Attached is a small patch against WebKit-GTK 1.4.0 that adds an API to set this path. By default, it will use XDG_DATA_HOME/wekit/databases. However, this can be overridden with the "local-storage-database-path", which accepts a string input.

Steps to Reproduce:
1. Go to http://people.w3.org/mike/localstorage.html
2. Enter a key/value pair, click setItem()
3. Close and reopen browser, returning to this page

Actual Results:
No items are stored.

Expected Results:
Key/value pair should reappear.

Build Date & Platform:
WebKit-GTK 1.4.0 on Arch Linux

Additional Builds & Platforms:
Should occur on every platform supporting WebKit-GTK.

Related bugs from other projects:
http://www.uzbl.org/bugs/index.php?do=details&task_id=212&project=1&opened=129
http://luakit.org/issues/18

[Martin Robinson]

Comment on attachment 96023 [details]
Add API to allow setting local storage database path

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

Thanks for your contribution! One thing this patch is missing is a ChangeLog. See here for information about generating one: http://www.webkit.org/coding/contributing.html

Another thing is that this patch doesn't properly handle webkit_web_settings_copy.

> webkit-1.4.0-fix/Source/WebKit/gtk/webkit/webkitwebsettings.cpp:96
> +    gchar* local_storage_database_path;

I think that html5_local_storage_database_path might be more consistent here.

> webkit-1.4.0-fix/Source/WebKit/gtk/webkit/webkitwebsettings.cpp:151
> +    PROP_LOCAL_STORAGE_DATABASE_PATH,

Ditto.

> webkit-1.4.0-fix/Source/WebKit/gtk/webkit/webkitwebsettings.cpp:580
> +    * Path to HTML5 localStorage databases.

This should probably be expanded. You should probably link to the local storage standard and mention what the default value is.

> webkit-1.4.0-fix/Source/WebKit/gtk/webkit/webkitwebsettings.cpp:590
> +                                    g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL),

Isn't this a memory leak?

> webkit-1.4.0-fix/Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:407
> +                 "local-storage-database-path", g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL),

Another memory leak here.

[Michael Stegeman]

Created attachment 96045 [details]
Add API to allow setting local storage database path

Tried to fix all of the things from the last review. Hopefully I got the memory leak fixed. Please let me know if the g_strdup method is not what I should be using, as I'm fairly new to GTK/Glib.

[Michael Stegeman]

Created attachment 96048 [details]
Add API to allow setting local storage database path

Fixed another possible memory leak. Think I got them all now.

[WebKit Review Bot]

Attachment 96048 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 96023 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 96045 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Stegeman]

Created attachment 96504 [details]
Add API to allow setting local storage database path

Hopefully fixing the style failures.

[Martin Robinson]

Comment on attachment 96504 [details]
Add API to allow setting local storage database path

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

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:594
> +                                    g_strdup(g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL)),

Hrm. The issue here is that g_build_filename return a newly-allocated string. Now you've wrapped it in g_strdup which creates another newly allocated string.

Probably what you should do is something like this:

GOwnPtr<gchar> localStoragePath(g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL));
g_object_class_install_property(gobject_class,
...etc etc..
                                       localStoragePath.get(),
                                       flags);

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:413
> +                 "local-storage-database-path", g_strdup(g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL)),

You should probably set this as something like g_build_filename(g_get_user_data_dir(), "DumpRenderTreeGtk", "databases", NULL) to avoid mixing databases with the WebKit installation. Please use the same approach that I mentioned above to avoid the memory leak as well.

[Michael Stegeman]

Created attachment 97174 [details]
Add API to allow setting local storage database path

Changes have been made from previous review to prevent memory leaks.

[Martin Robinson]

Comment on attachment 97174 [details]
Add API to allow setting local storage database path

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

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:585
> +    * is $XDG_DATA_HOME/webkit/databases/. If this property is not defined
> +    * (or left as default), the localStorage will not be persistent.

What do you mean by  "the localStorage will not be persistent?" From looking at the code it seems that if the value is left as the default, it local stroage will simply persist in the default location.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:597
> +                                    "html5-local-storage-database-path",
> +                                    _("Local Storage Database Path"),
> +                                    _("The path to where HTML5 Local Storage databases are stored."),
> +                                    localStoragePath.get(),
> +                                    flags));

Need to fix the indent on the arguments to g_param_spec_string.

[Michael Stegeman]

Created attachment 97199 [details]
Add API to allow setting local storage database path

Made comments more clear and fixed indentation.

[Martin Robinson]

This looks good to me! We just need a GTK+ reviewer to give the final r+ (API changes need two reviewers to sign off). Thank you for being so responsive to review comments.

[Gustavo Noronha (kov)]

Comment on attachment 97199 [details]
Add API to allow setting local storage database path

It looks good to me as well =) we'll have to update the Since tags and land it, I'll see if I can do it

[Gustavo Noronha (kov)]

Created attachment 100223 [details]
Patch for landing

This is the patch adapted for landing - given recent changes to WebKitWebSettings the changes were not that trivial, but I believe it's good =).

[Michael Stegeman]

(In reply to comment #14)
> Created an attachment (id=100223) [details]
> Patch for landing
> 
> This is the patch adapted for landing - given recent changes to WebKitWebSettings the changes were not that trivial, but I believe it's good =).

Looks good to me!

[Martin Robinson]

Comment on attachment 100223 [details]
Patch for landing

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

Isn't this missing some logic for webkit_web_view_update_settings in webkitwebview.cpp?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:530
> +    * "enable-html5-local-storage". The default path is $XDG_DATA_HOME/webkit/databases/

This sentence is missing a period.

[Gustavo Noronha (kov)]

Created attachment 100318 [details]
Now with much more magicz!

Here you are.

[Gustavo Noronha (kov)]

Created attachment 100321 [details]
Now with magicz and actually buildable!

[WebKit Review Bot]

Comment on attachment 100321 [details]
Now with magicz and actually buildable!

Rejecting attachment 100321 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 1

Last 500 characters of output:
geLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/gtk/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9009643

[Gustavo Noronha (kov)]

Comment on attachment 100321 [details]
Now with magicz and actually buildable!

Landed r90776 manually because of the unusual reviewer line.