Summary: | Add API to WebKit-GTK to allow setting localStorage database path | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Stegeman <michael> | ||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | eric, gustavo, james.r.campos, michael, mrobinson, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Attachments: |
|
Description
Michael Stegeman
2011-06-04 08:49:21 PDT
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. |