Move various runtime enabled experiments into a WebExperiments class....

Move various runtime enabled features into a WebRuntimeFeatures class.

There are separate functions to set/test each feature.

R=jorlow
BUG=25286
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30564

[jorlow, 2009-10-27 18:42:36]

LGTM.

Let me know what you think about passing in a boolean to say whether it's
enabled (rather than it being disabled by default).

http://codereview.chromium.org/343001/diff/1/9
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/343001/diff/1/9#newcode520
Line 520: WebExperiments::enableFeature(WebExperiments::MediaPlayer);
What about

WebExperiments::setFeatureEnabled(WebExperiments::MediaPlayer,
RenderProcess::current()->initialized_media_library());

?

This would allow us to make these default to on inside WebCore.

http://codereview.chromium.org/343001/diff/1/7
File webkit/api/public/WebExperiments.h (right):

http://codereview.chromium.org/343001/diff/1/7#newcode38
Line 38: class WebExperiments {
Add a comment.

http://codereview.chromium.org/343001/diff/1/7#newcode40
Line 40: enum Feature {
Add a comment that these can never be removed.

Is it convention to explicitly make the first one = 0?

http://codereview.chromium.org/343001/diff/1/10
File webkit/api/src/WebExperiments.cpp (right):

http://codereview.chromium.org/343001/diff/1/10#newcode54
Line 54: }
default should maybe log something?

http://codereview.chromium.org/343001/diff/1/5
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/343001/diff/1/5#newcode44
Line 44: return s_isEnabled;
I guess using s_ for global/static variables is out of favor in WebKit.  Just
make it camel case.

http://codereview.chromium.org/343001/diff/1/8
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/343001/diff/1/8#newcode56
Line 56: WebExperiments::isFeatureEnabled(WebExperiments::Database) ||
I'm prety sure WebExperiments::isFeatureEnabled(WebExperiments::Database)is not
needed....but it's not hurting anything.

[darin (slow to review), 2009-10-30 05:52:45]

I decided to take a departure from the original design.  Now, I'm going with
something more consistent with RuntimeEnabledFeatures, and I'm much happier with
it.  I hope you will be too :-)

[jorlow, 2009-10-30 06:19:06]

Will these functions ever go away?  Maybe if they've been deprecated for a
sufficient amount of time or something?  My only worry is that this will get
really bloated over time.  But I guess we'll have that with enums as well.

LGTM no matter what you decide.

http://codereview.chromium.org/343001/diff/5001/2005
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/343001/diff/5001/2005#newcode40
Line 40: static bool s_isEnabled = false;
Change to isEnabled maybe?

http://codereview.chromium.org/343001/diff/5001/2010
File webkit/api/src/WebRuntimeFeatures.cpp (right):

http://codereview.chromium.org/343001/diff/5001/2010#newcode42
Line 42: void WebRuntimeFeatures::enableDatabase(bool enable)
What's the plan when these are enabled by default?  Would the embedder need to
try setting it and then poll?  Maybe it should return a boolean for whether or
not it was successfully set?

[darin (slow to review), 2009-10-30 06:38:25]

I was thinking we might at some point mark some of these as deprecated, and then
after a major milestone or so we can go about removing them.

http://codereview.chromium.org/343001/diff/5001/2005
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/343001/diff/5001/2005#newcode40
Line 40: static bool s_isEnabled = false;
On 2009/10/30 06:19:06, Jeremy Orlow wrote:
> Change to isEnabled maybe?

Done.

http://codereview.chromium.org/343001/diff/5001/2010
File webkit/api/src/WebRuntimeFeatures.cpp (right):

http://codereview.chromium.org/343001/diff/5001/2010#newcode42
Line 42: void WebRuntimeFeatures::enableDatabase(bool enable)
On 2009/10/30 06:19:06, Jeremy Orlow wrote:
> What's the plan when these are enabled by default?  Would the embedder need to
> try setting it and then poll?  Maybe it should return a boolean for whether or
> not it was successfully set?

I just did what you suggested earlier.  The embedder always calls enableFoo with
the value it wants.  So, the defaults don't matter.

However, if we want to remove the code that calls enableFoo, then
we would probably do that once we know for sure that the feature is
enabled by default, or we would just always have code to call enableFoo(true).

The defaults do matter when the embedder doesn't call enableBar, which might
happen when you mix an old Chrome with a newer WebKit.  I'm okay with the
default floating like that.

[jorlow, 2009-10-30 07:08:02]

http://codereview.chromium.org/343001/diff/5001/2010
File webkit/api/src/WebRuntimeFeatures.cpp (right):

http://codereview.chromium.org/343001/diff/5001/2010#newcode42
Line 42: void WebRuntimeFeatures::enableDatabase(bool enable)
On 2009/10/30 06:38:25, darin wrote:
> On 2009/10/30 06:19:06, Jeremy Orlow wrote:
> > What's the plan when these are enabled by default?  Would the embedder need
to
> > try setting it and then poll?  Maybe it should return a boolean for whether
or
> > not it was successfully set?
> 
> I just did what you suggested earlier.  The embedder always calls enableFoo
with
> the value it wants.  So, the defaults don't matter.
> 
> However, if we want to remove the code that calls enableFoo, then
> we would probably do that once we know for sure that the feature is
> enabled by default, or we would just always have code to call enableFoo(true).
> 
> The defaults do matter when the embedder doesn't call enableBar, which might
> happen when you mix an old Chrome with a newer WebKit.  I'm okay with the
> default floating like that.

I imagined that the WebCore side of the runtime features would outlive the
WebKit API side of them...especially when more than just Chrome is using the
Chromium API.

I think you should commit this as is, but that we should think this through more
before we commit to it for the long term.