Bug 30653 – Add a temporary flag to the Database class that will be used by V8 to decide if window.openDatabase() should be exposed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

We need to add a flag (somewhere) that V8 can use to determine if the window.openDatabase() function should be enabled or not. This would be a temporary flag, that will be removed when Chromium's DB implementation is complete (in a couple of weeks, hopefully). The best place to put this temporary flag seems to be the Database class.

[Dumitru Daniliuc]

Created attachment 41623 [details]
patch

[Jeremy Orlow]

LGTM

[Eric Seidel (no email)]

Comment on attachment 41623 [details]
patch

Tabs in the ChangeLog will cause the commit to fail.

[Dumitru Daniliuc]

Created attachment 41685 [details]
patch

Got rid of the #if USE(V8) ugliness by introducing a generic Experiments class that should be used to store flags for experimental features that determine at runtime if the feature should be enabled or not.

Eric: please CC some people from Apple, I don't know who the appropriate people are.

[Eric Seidel (no email)]

Interesting idea.  I'm honestly not sure who at Apple does the database stuff.

According to http://trac.webkit.org/log/trunk/WebCore/storage/Database.h is might be ap and Brady.

In general the change looks good to me.  I'm not sure what the current style for static members is:
 45     static bool s_isDatabaseEnabled;

I'm ready to r+ this, but we should wait a few more minutes in case others have comments.

[Dimitri Glazkov (Google)]

Can I nit? I think it should be just databaseEnabled/setDatabaseEnabled.

[Dumitru Daniliuc]

Created attachment 41686 [details]
patch

Changed function names to setDatabaseEnabled()/databaseEnabled() and removed the s_ in the static member -- I see some s_ static members in WebCore/platform/*.cpp, but most of them seem to not start with s_.

[Eric Seidel (no email)]

Comment on attachment 41686 [details]
patch

I wonder if Experiments should be in Page instead.  It's sorta like a light-weight Settings object.  Makes me wonder a bit why this wouldn't actually just be in Settings...  If this is only for V8 maybe your initial approach was better of just shoving this off in v8 code.

[Dmitry Titov]

(In reply to comment #8)
> (From update of attachment 41686 [details])
> I wonder if Experiments should be in Page instead.  It's sorta like a
> light-weight Settings object.  Makes me wonder a bit why this wouldn't actually
> just be in Settings... 

Unlike Settings, these are static for the lifetime of the process (as in command-line flags) so it makes sense to have a single static class that exposes them - this way things like Workers can check those flags easily too.

[Sam Weinig]

The experiments class is a layering violation.  Databases (in this sense) are a DOM concept and not something the platform layer should know about.

[Dumitru Daniliuc]

Created attachment 41702 [details]
patch

Moved and renamed the class as agreed on IRC.

[Dmitry Titov]

r=me

On IRC, there was an idea floating about moving those bits to static members of Settings class. If the runtime enablement is something that ports other then Chromium will be interested in, we can easily do that, otherwise it is unclear why make it even visible on WebCore level vs internal to v8 bindings. Please comment if I got that wrong, it's easy to change.

[Jeremy Orlow]

(In reply to comment #12)
> r=me
> 
> On IRC, there was an idea floating about moving those bits to static members of
> Settings class. If the runtime enablement is something that ports other then
> Chromium will be interested in, we can easily do that, otherwise it is unclear
> why make it even visible on WebCore level vs internal to v8 bindings. Please
> comment if I got that wrong, it's easy to change.

Moving into settings is a mistake.  Settings are attached to pages, but these runtime flags apply to workers and other things that have nothing to do with a page.

None of the other ports have expressed any interest in enabling/disabling stuff at runtime like Chromium.  I don't think there's any reason to make things more generic until such an interest is expressed.

The reason to make this visible in the WebCore namespace is because that's where the bindings are.

[Dumitru Daniliuc]

Landed as r49989.

[Eric Seidel (no email)]

I don't like that this can't be tested.  At least Settings now have the ability to be tested from DumpRenderTree. :(