Bug 30653 - Add a temporary flag to the Database class that will be used by V8 to decide if window.openDatabase() should be exposed
Summary: Add a temporary flag to the Database class that will be used by V8 to decide ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-21 17:10 PDT by Dumitru Daniliuc
Modified: 2009-10-23 13:04 PDT (History)
8 users (show)

See Also:


Attachments
patch (5.24 KB, patch)
2009-10-21 17:19 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (14.14 KB, patch)
2009-10-22 13:51 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (14.12 KB, patch)
2009-10-22 14:23 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (8.20 KB, patch)
2009-10-22 17:42 PDT, Dumitru Daniliuc
dimich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2009-10-21 17:10:10 PDT
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.
Comment 1 Dumitru Daniliuc 2009-10-21 17:19:50 PDT
Created attachment 41623 [details]
patch
Comment 2 Jeremy Orlow 2009-10-21 17:28:50 PDT
LGTM
Comment 3 Eric Seidel (no email) 2009-10-22 10:48:30 PDT
Comment on attachment 41623 [details]
patch

Tabs in the ChangeLog will cause the commit to fail.
Comment 4 Dumitru Daniliuc 2009-10-22 13:51:26 PDT
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.
Comment 5 Eric Seidel (no email) 2009-10-22 14:02:47 PDT
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.
Comment 6 Dimitri Glazkov (Google) 2009-10-22 14:03:39 PDT
Can I nit? I think it should be just databaseEnabled/setDatabaseEnabled.
Comment 7 Dumitru Daniliuc 2009-10-22 14:23:12 PDT
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_.
Comment 8 Eric Seidel (no email) 2009-10-22 14:32:40 PDT
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.
Comment 9 Dmitry Titov 2009-10-22 15:00:14 PDT
(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.
Comment 10 Sam Weinig 2009-10-22 15:48:14 PDT
The experiments class is a layering violation.  Databases (in this sense) are a DOM concept and not something the platform layer should know about.
Comment 11 Dumitru Daniliuc 2009-10-22 17:42:10 PDT
Created attachment 41702 [details]
patch

Moved and renamed the class as agreed on IRC.
Comment 12 Dmitry Titov 2009-10-23 12:19:47 PDT
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.
Comment 13 Jeremy Orlow 2009-10-23 12:24:07 PDT
(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.
Comment 14 Dumitru Daniliuc 2009-10-23 12:41:13 PDT
Landed as r49989.
Comment 15 Eric Seidel (no email) 2009-10-23 13:04:14 PDT
I don't like that this can't be tested.  At least Settings now have the ability to be tested from DumpRenderTree. :(