Bug 25629 – [Gtk] Enable database and localStorage support

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

Add settings to enable database and localStorage support and enable it by default. This allows databases and storage to be shown in the WebInspector and at the same time allow us to unskip most of the storage/ layout tests.

[Jan Alonzo]

Created attachment 30114 [details]
add settings for database and localStorage

[Christian Dywan]

These properties shouldn't be conditional. Rather document that the feature may be unavailable.

As for the two options, could you elaborate on what exactly these affect? I have a rough idea of DOM storage but I'm not clear why this need two different settings.

[Jan Alonzo]

Created attachment 30117 [details]
remove DATABASE conditional guards

This patch removes the ENABLE(DATABASE) guard. Also improved on the documentation as suggested in irc by xan

[Jan Alonzo]

(In reply to comment #2)
> These properties shouldn't be conditional. Rather document that the feature may
> be unavailable.

Updated the patch to remove the conditional.

> 
> As for the two options, could you elaborate on what exactly these affect? I
> have a rough idea of DOM storage but I'm not clear why this need two different
> settings.

"enable-databases" is for the Database API which uses SQL and is asynchronous.
"enable-local-storage" is synchronous and for simple storage. Examples in the web for localStorage seems that it's for storing key-value pairs i.e, localStorage["presence"] = "online".

[Gustavo Noronha (kov)]

Comment on attachment 30117 [details]
remove DATABASE conditional guards

> +    /**
> +    * WebKitWebSettings:enable-databases:
> +    *
> +    * Whether to enable HTML5 client-side SQL database support. Client-side
> +    * SQL database allows web pages to store structured data and be able to
> +    * use SQL to manipulate that data asynchronously. Disabling this setting
> +    * will prevent if from appearing in the Database profile of the
> +    * WebInspector.
> +    *
> +    * Since 1.1.7
> +    */

The indentation of the *'s seems to be off (I think this is a problem elsewhere in our code, so probably caused by copy/paste).

The patch looks good to me, so I'll say r=me, if Xan is not against. We have agreed to have at least two people look at the API bits, so I'm not marking it r+ as of yet.

[Christian Dywan]

Come to think of it, we need something like databases-max-age as well. Otherwise this will be stored forever - which isn't good. Or alternatively a signal like "database-changed" in the way the SoupCookie works.
Jan, would you know how to arrange that?

[Christian Dywan]

And thanks a lot for adding the documentation.

[Jan Alonzo]

(In reply to comment #6)
> Come to think of it, we need something like databases-max-age as well.
> Otherwise this will be stored forever - which isn't good. Or alternatively a
> signal like "database-changed" in the way the SoupCookie works.
> Jan, would you know how to arrange that?

You raised a good point but I think storage duration should be handled in WebCore and should be spec'd as part of HTML5 database (if not already). Maybe something to ask webkit-dev as well?

[Jan Alonzo]

Created attachment 30224 [details]
updated patch to rename to properties to enable-html5-*

Updated patch to add 'html5' to the property names to remove ambiguity with icon-database. Also remove mention of WebInspector in the doc as suggested by Xan in IRC.

[Christian Dywan]

I sent a message to the WebKit devel list now, subject "Controling HTML5 local storage/ databases programmatically".

[Xan Lopez]

Comment on attachment 30224 [details]
updated patch to rename to properties to enable-html5-*

This looks good to me now. Since it's new API I'll wait for kov to give it the final thumbs up.

[Gustavo Noronha (kov)]

Comment on attachment 30224 [details]
updated patch to rename to properties to enable-html5-*

Yeah, looks good. Perhaps you could split the fix to case PROP_ENABLE_PRIVATE_BROWSING in a different commit, though? And remember to fix Since to be 1.1.8.

[Jan Alonzo]

(In reply to comment #12)
> (From update of attachment 30224 [details] [review])
> Yeah, looks good. Perhaps you could split the fix to case
> PROP_ENABLE_PRIVATE_BROWSING in a different commit, though? And remember to fix
> Since to be 1.1.8.
> 

Thanks! Landed in r44105. Private browsing case fix landed in r44106.