Bug 28036 - [Qt] Public API to configure the storage path for HTML5 localStorage
Summary: [Qt] Public API to configure the storage path for HTML5 localStorage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-05 21:20 PDT by Laszlo Gombos
Modified: 2009-08-14 07:21 PDT (History)
2 users (show)

See Also:


Attachments
proposed api (4.05 KB, patch)
2009-08-05 21:27 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
second try (5.40 KB, patch)
2009-08-10 05:50 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
3rd try (10.44 KB, patch)
2009-08-10 20:12 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
4th try. (10.44 KB, patch)
2009-08-10 20:24 PDT, Laszlo Gombos
hausmann: review-
Details | Formatted Diff | Diff
5th try (13.17 KB, patch)
2009-08-14 05:05 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
6th try (13.26 KB, patch)
2009-08-14 06:33 PDT, Laszlo Gombos
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-08-05 21:20:28 PDT
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.
Comment 1 Laszlo Gombos 2009-08-05 21:27:57 PDT
Created attachment 34198 [details]
proposed api
Comment 2 Kenneth Rohde Christiansen 2009-08-06 05:18:39 PDT
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?
Comment 3 Simon Hausmann 2009-08-06 07:57:38 PDT
(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?
Comment 4 Simon Hausmann 2009-08-06 07:58:55 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2009-08-06 09:24:36 PDT
Yes, I also think that we need that.
Comment 6 Eric Seidel (no email) 2009-08-07 08:45:39 PDT
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 7 Laszlo Gombos 2009-08-07 09:04:58 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2009-08-07 10:45:46 PDT
..as well as update the documentation and link to the webstorage spec.
Comment 9 Laszlo Gombos 2009-08-10 05:50:08 PDT
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.
Comment 10 Kenneth Rohde Christiansen 2009-08-10 06:09:13 PDT
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?
Comment 11 Kenneth Rohde Christiansen 2009-08-10 09:39:40 PDT
<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
Comment 12 Laszlo Gombos 2009-08-10 20:12:01 PDT
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
Comment 13 Laszlo Gombos 2009-08-10 20:24:14 PDT
Created attachment 34538 [details]
4th try.

Found a typo in the 3rd path, please review this one instead.
Comment 14 Kenneth Rohde Christiansen 2009-08-11 06:42:50 PDT
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 15 Kenneth Rohde Christiansen 2009-08-11 06:48:52 PDT
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?
Comment 16 Laszlo Gombos 2009-08-12 07:51:40 PDT
(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 17 Simon Hausmann 2009-08-13 06:48:29 PDT
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
Comment 18 Laszlo Gombos 2009-08-14 05:05:25 PDT
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.
Comment 19 Laszlo Gombos 2009-08-14 06:33:02 PDT
Created attachment 34837 [details]
6th try

Documentation for LocalStorageEnabled now mentions that it's disabled by default
Enable qdocs for setLocalStoragePath and localStoragePath.
Comment 20 Simon Hausmann 2009-08-14 07:19:06 PDT
Comment on attachment 34837 [details]
6th try

r=me
Comment 21 Simon Hausmann 2009-08-14 07:21:18 PDT
Landed in r47283