Bug 28036 – [Qt] Public API to configure the storage path for HTML5 localStorage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Laszlo Gombos]

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.

[Laszlo Gombos]

Created attachment 34198 [details]
proposed api

[Kenneth Rohde Christiansen]

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?

[Simon Hausmann]

(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?

[Simon Hausmann]

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.

[Kenneth Rohde Christiansen]

Yes, I also think that we need that.

[Eric Seidel (no email)]

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.

[Laszlo Gombos]

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.

[Kenneth Rohde Christiansen]

..as well as update the documentation and link to the webstorage spec.

[Laszlo Gombos]

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.

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

<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

[Laszlo Gombos]

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

[Laszlo Gombos]

Created attachment 34538 [details]
4th try.

Found a typo in the 3rd path, please review this one instead.

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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?

[Laszlo Gombos]

(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 )

[Simon Hausmann]

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

[Laszlo Gombos]

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.

[Laszlo Gombos]

Created attachment 34837 [details]
6th try

Documentation for LocalStorageEnabled now mentions that it's disabled by default
Enable qdocs for setLocalStoragePath and localStoragePath.

[Simon Hausmann]

Comment on attachment 34837 [details]
6th try

r=me

[Simon Hausmann]

Landed in r47283