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
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.
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.
Created attachment 96048 [details] Add API to allow setting local storage database path Fixed another possible memory leak. Think I got them all now.
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.
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.
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.
Created attachment 96504 [details] Add API to allow setting local storage database path Hopefully fixing the style failures.
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.
Created attachment 97174 [details] Add API to allow setting local storage database path Changes have been made from previous review to prevent memory leaks.
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.
Created attachment 97199 [details] Add API to allow setting local storage database path Made comments more clear and fixed indentation.
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.
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
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 =).
(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!
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.
Created attachment 100318 [details] Now with much more magicz! Here you are.
Created attachment 100321 [details] Now with magicz and actually buildable!
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
Comment on attachment 100321 [details] Now with magicz and actually buildable! Landed r90776 manually because of the unusual reviewer line.