Summary: | Add a temporary flag to the Database class that will be used by V8 to decide if window.openDatabase() should be exposed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dumitru Daniliuc <dumi> | ||||||||||
Component: | New Bugs | Assignee: | Dumitru Daniliuc <dumi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, beidson, dglazkov, dimich, eric, fishd, jorlow, sam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Dumitru Daniliuc
2009-10-21 17:10:10 PDT
Created attachment 41623 [details]
patch
LGTM Comment on attachment 41623 [details]
patch
Tabs in the ChangeLog will cause the commit to fail.
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.
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. Can I nit? I think it should be just databaseEnabled/setDatabaseEnabled. 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 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.
(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. The experiments class is a layering violation. Databases (in this sense) are a DOM concept and not something the platform layer should know about. Created attachment 41702 [details]
patch
Moved and renamed the class as agreed on IRC.
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. (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. Landed as r49989. I don't like that this can't be tested. At least Settings now have the ability to be tested from DumpRenderTree. :( |