This API allows to change the path for the HTML5 localstorage per page. The proposal essentially converts the existing private API into a public API.
Created attachment 34198 [details] proposed api
Actually the database part is just the way it is stored, but AFAIK you dont rerally use it as a database (as with the WebDatabase API) and as such I think we should just call it LocalStorage and mention in the docs that it is the local part of the WebStorage API. Input Simon?
(In reply to comment #2) > Actually the database part is just the way it is stored, but AFAIK you dont > rerally use it as a database (as with the WebDatabase API) and as such I think > we should just call it LocalStorage and mention in the docs that it is the > local part of the WebStorage API. Right, only the local storge as part of WebStorage is persistent and therefore requires disk access. One thing I'm concerned about though in this context is that we're providing this path setting per QWebPage, where as all our existing path settings are global. In WebKit however the local storage is not a property of the page, it is a property of the page group. If we want to introduce support for page groups in the future, then we have to decide what we want to do with these path settings per page. Should a page group result in all associated QWebPage instances also sharing the same QWebSettings object, i.e. QWebPage::settings() on all the pages in the group returning the same shared object?
More generally speaking get the impression that need to have support for page grouping in our API, to make some of the newer features more controllable in environments where sandboxing is important.
Yes, I also think that we need that.
Comment on attachment 34198 [details] proposed api Looks fine to me. I'm not a Qt guy. One of the Qt guys can (and should!) make noises if they disagree.
Comment on attachment 34198 [details] proposed api Remove review+ for now as Kenneth would like to change the name (remove the "Database" part) before this gets landed.
..as well as update the documentation and link to the webstorage spec.
Created attachment 34448 [details] second try - remove the "Database" part from the name (e.g. renamed setLocalStorageDatabasePath to setLocalStoragePath) - add reference to webstorage spec Note that WebAttribute enum is still called LocalStorageDatabaseEnabled; since the enum has been part of the QtWebKit 4.5 API and changing it would be compatibility break I believe. This means that with this proposal there will be some inconsistency between the name of the WebAttribute enum and the rest of the APIs.
Regarding LocalStorageDatabaseEnabled. It would be possible to hide the enum from the docs and add a new one. Effectively deprecate it. What do you think Simon?
<tronical> kenneth: we could deprecated the old and hide it in the docs and add new functions. I have talked with Simon earlier about default paths. If you enable the feature, you should not have to set a path. If you don't it should default to [quoting Simon] <base path>/IconDatabase/ <base path>/WebDatabase/ <base path>/ApplicationCache/ <base path>/WebStorage/ <- this one
Created attachment 34537 [details] 3rd try Additional changes in the 3rd revision of the patch: - LocalStorageDatabaseEnabled enum value is now deprecated, the new name is LocalStorageEnabled - LocalStoragePath now has a default (<base path>/LocalStorage/) Here are some reasons to keep the LocalStorage name (instead of switching to WebStorage). - The LocalStorageEnabled setting does not control SessionStorage and as such WebStorageEnabled name would be misleading - WebStorage is indeed the name of the specification, but the relevant JS interface is called window.localStrorage - I believe the JS interface name is equally/more relevant - WebStorage spec is referenced not only in the HTML5 spec but also in Widgets APIs and Events spec (see http://www.w3.org/TR/widgets-apis, widget.preferences). Since the interface that is defined in this patch is only for the HTML5 LocalStorage I think the name of the interface should be also specific to HTML5 to distinguish it from other WebStorage use-cases
Created attachment 34538 [details] 4th try. Found a typo in the 3rd path, please review this one instead.
I guess on Mac you should add the organization name to the path? like mentioned in the docs for QString QDesktopServices::storageLocation ( StandardLocation type ) Any mac guy have some input?
Comment on attachment 34538 [details] 4th try. > + // Set HTML5 Application Cache default location > + QWebSettings::setOfflineWebApplicationCachePath(defaultCachePath()); I think we want to append /ApplicationCache or so. Simon, how do we hide the deprecated methods from the docs?
(In reply to comment #15) > (From update of attachment 34538 [details]) > > + // Set HTML5 Application Cache default location > > + QWebSettings::setOfflineWebApplicationCachePath(defaultCachePath()); > I think we want to append /ApplicationCache or so. > Simon, how do we hide the deprecated methods from the docs? Kenneth, I'd like to keep this patch focused on LocalStorage. I would prefer to create a separate patch for the HTML5 Application Cache changes (which I plan to do). My proposal for deprecating follows Qt conventions I belive (see for example http://qt.gitorious.org/+qt-s60-developers/qt/qt-s60/blobs/master/src/gui/dialogs/qfiledialog.h )
Comment on attachment 34538 [details] 4th try. r- as discussed. It would be best to have one isolated patch that 1) Fixes the naming of the enum 2) and introduces the storage path without any defaults or without enabling it by default. > @@ -62,7 +62,10 @@ public: > PrintElementBackgrounds, > OfflineStorageDatabaseEnabled, > OfflineWebApplicationCacheEnabled, > +#ifdef QT_DEPRECATED > LocalStorageDatabaseEnabled, > +#endif > + LocalStorageEnabled, > LocalContentCanAccessRemoteUrls > }; A slighty more backwards-compatible way of deprecating the old value would be to use the following notation: enum { ... LocalStorageEnabled, #ifdef QT_DEPRECATED LocalStorageDatabaseEnabled = LocalStorageEnabled, #endif
Created attachment 34829 [details] 5th try Disables LocalStorage for QtWebKit by default by setting QWebSettings::LocalStorageEnabled to false. Sets up a default for the LocalStorage path so that clients would only need to enable the LocalStorageEnabled setting to turn on LocalStoragre support. Turn on LocalStorage support for QtLauncher and the relevant test since LocalStorage is now disabled by default for QtWebkit.
Created attachment 34837 [details] 6th try Documentation for LocalStorageEnabled now mentions that it's disabled by default Enable qdocs for setLocalStoragePath and localStoragePath.
Comment on attachment 34837 [details] 6th try r=me
Landed in r47283