Add a public API to enable/disable blink runtime features by name

Add a public API to enable/disable blink runtime features by name

This will be used to add an --enable-blink-features=feature1,feature2
flag to Chromium to avoid plumbing separate flags each time a feature
needs to be exposed individually (eg. for the purposes of a virtual test
suite).

For a flag to be toggled by this API it must have the
'command_line_flag' option set in RuntimeEnabledFeatures.in

[abarth-chromium, 2013-07-08 04:09:39]

I'm not sure we want this CL.  It's rare that we need to expose a separate flag
for testing runtime enabled features.  Mostly they're just turned on during
testing (e.g., status=test).

Having an explicit list in the API makes it clearer which flags are used outside
of the repo.

not lgtm

[dstockwell, 2013-07-08 04:34:07]

On 2013/07/08 04:09:39, abarth wrote:
> I'm not sure we want this CL.  It's rare that we need to expose a separate
flag
> for testing runtime enabled features.  Mostly they're just turned on during
> testing (e.g., status=test).

The engineering effort required to add a new flag is non trivial. Enabling a
virtual suite for a new feature requires at least 3 changes.

> Having an explicit list in the API makes it clearer which flags are used
outside
> of the repo.

There are already 53 separate flags exposed by WebRuntimeFeatures (there are
only 70 features defined in RuntimeEnabledFeatures.in).

> not lgtm

For reference here is my discussion with Eric on this idea:
http://echelog.com/logs/browse/blink/1369260000
And Eric's notes on a related change: https://codereview.chromium.org/15807002

[abarth-chromium, 2013-07-08 04:41:11]

On 2013/07/08 04:34:07, dstockwell wrote:
> The engineering effort required to add a new flag is non trivial. Enabling a
> virtual suite for a new feature requires at least 3 changes.

Which flag are you particularly interested in creating a virtual test suite for?
 It's a pretty rare case.

Perhaps there's a way of addressing this use case more directly?  For example,
we could teach TestRunner.dll about the existence of virtual test suites, which
would let you introduce and use the flag in one atomic commit.

[abarth-chromium, 2013-07-08 04:47:44]

By way of context, we should probably remove almost all the entries in
WebRuntimeFeatures.h.  The
enableStableFeatures/enableExperimentalFeatures/enableTestOnlyFeatures entry
points should cover 90% of the cases and the remaining entries should just be
oddballs.  We're pretty far from that situation currently, but adding a
universal string-based API is heading the opposite direction.

[dstockwell, 2013-07-08 05:11:02]

On 2013/07/08 04:41:11, abarth wrote:
> On 2013/07/08 04:34:07, dstockwell wrote:
> > The engineering effort required to add a new flag is non trivial. Enabling a
> > virtual suite for a new feature requires at least 3 changes.
> 
> Which flag are you particularly interested in creating a virtual test suite
for?
>  It's a pretty rare case.

We're going to be adding a separate flag for CSS Animations backed by the Web
Animations model and later the same for SVG Animations. We're rebuilding
functionality that already exists and is shipping -- I agree this is not the
common case.

We can't set these particular flags to 'test' since we would immediately lose
coverage for the currently shipping implementation.
 
> Perhaps there's a way of addressing this use case more directly?  For example,
> we could teach TestRunner.dll about the existence of virtual test suites,
which
> would let you introduce and use the flag in one atomic commit.

I don't quite follow. We would still need to land changes to both Blink and
Chromium wouldn't we?

This isn't the only use case of per-feature flags I'm aware of. We would also
like to be able to turn on/off individual features for fuzzing. I know this case
recently came up when trying to run a fuzzer against the new Animated WebP
feature.

On 2013/07/08 04:47:44, abarth wrote:
> By way of context, we should probably remove almost all the entries in
> WebRuntimeFeatures.h.  The
> enableStableFeatures/enableExperimentalFeatures/enableTestOnlyFeatures entry
> points should cover 90% of the cases and the remaining entries should just be
> oddballs.  We're pretty far from that situation currently, but adding a
> universal string-based API is heading the opposite direction.

I'm not sure I understand the objection. Are you concerned that someone might
use this API to inadvertently ship or expose a feature?

[dstockwell, 2013-07-08 05:28:26]

Perhaps we could expose only a subset of features to this API, the "oddballs" as
you call them.

This might happen by adding an additional annotation in
RuntimeEnabledFeatures.in.

[eseidel, 2013-07-08 19:13:29]

I'm not sure I understand the virtual test suite usecase.  I would expect very
few features would need/want a virtual test suite. Is this an in-progress
feature which you don't want on for normal testing?

I do think that having a patch like this could make sense.  I think tagging
which features are allowed via this API also makes sense.

I agree with Adam that we should be moving away from command line flags when
possible.

[eseidel, 2013-07-08 21:02:02]

For better or worse we're developing Animations on trunk.  They need a way to
test their code w/o disturbing the rest of Layout tests.

Presumably they need a way to test their new animation engine while still
shipping the old one, hence the command line flag.  Presumably their changes
need to be set at boot time before the page is loaded? (otherwise they could
just use a setting).

[eseidel, 2013-07-08 21:03:05]

This change doesn't seem that bad to me.  I think we should discourage having
these togglable this way by making it dependent on some flag in
RuntimeEnabledFeatures.in.

It would be nice to see us remove a bunch of unused flags for these which exist
up in Content for historical reasons.

[dstockwell, 2013-07-08 21:17:54]

On 2013/07/08 21:03:05, eseidel wrote:
> This change doesn't seem that bad to me.  I think we should discourage having
> these togglable this way by making it dependent on some flag in
> RuntimeEnabledFeatures.in.

I've uploaded a new patch that requires a 'command_line_flag' option to be
specified in RuntimeEnabledFeatures.in

> It would be nice to see us remove a bunch of unused flags for these which
exist
> up in Content for historical reasons.

I'll take an initial stab at cleaning up the ones that are not used at all.

[eseidel, 2013-07-08 21:24:47]

lgtm.  Please wait for Adam's lgtm too.

[abarth-chromium, 2013-07-08 22:28:21]

On 2013/07/08 05:11:02, dstockwell wrote:
> On 2013/07/08 04:41:11, abarth wrote:
> > Perhaps there's a way of addressing this use case more directly?  For
example,
> > we could teach TestRunner.dll about the existence of virtual test suites,
> which
> > would let you introduce and use the flag in one atomic commit.
> 
> I don't quite follow. We would still need to land changes to both Blink and
> Chromium wouldn't we?

Nope.  TestRunner.dll is in the Blink repo.  You could have the mapping from
virtual test suites to command-line arguments or WebRuntimeEnabled functions in
TestRunner.dll, which means you could add the API and call it in the same
commit.

> On 2013/07/08 04:47:44, abarth wrote:
> > By way of context, we should probably remove almost all the entries in
> > WebRuntimeFeatures.h.  The
> > enableStableFeatures/enableExperimentalFeatures/enableTestOnlyFeatures entry
> > points should cover 90% of the cases and the remaining entries should just
be
> > oddballs.  We're pretty far from that situation currently, but adding a
> > universal string-based API is heading the opposite direction.
> 
> I'm not sure I understand the objection. Are you concerned that someone might
> use this API to inadvertently ship or expose a feature?

I'd like to limit Blink's configurations space to something that's reasonably
testable.  Having independent flags for every feature is useful internally so
that we can find all the code that's related to a given feature, but
gratuitously exposing all those flags externally increases the configuration
space exponentially.  In some cases there are good reasons for exposing flags,
and that's fine, but that doesn't mean we should expose all flags by default.

[abarth-chromium, 2013-07-08 22:30:36]

On 2013/07/08 21:02:02, eseidel wrote:
> For better or worse we're developing Animations on trunk.  They need a way to
> test their code w/o disturbing the rest of Layout tests.

Yes, I agree that the new animation engine is a good use case for a virtual test
suite and an individual configuration flag.  My concerns with the original CL
were that it exposed all possible configuration states to the embedder rather
than just the ones we plan to use.  Over time, I think we should aim to reduce
the number of configuration states we expose, which I plan to do by removing
each unnecessary function in this file.  Having a universal API to toggle each
flag will make that impossible.

[abarth-chromium, 2013-07-08 22:38:30]

The updated CL is better, but I'd still prefer the TestRunner.dll approach.

The approach I'd recommend is the following:

1) When running a virtual test suite, run-webkit-test passes a
--test-blink-feature=foo flag to content_shell (or whatever name you like).
2) When content_shell receives an --test-blink-feature flag together with
--dump-render-tree, it calls into TestRunner.dll with the value of the
parameter.
3) TestRunner.dll then knows how to translate the string (e.g., "foo") to a
WebRuntimeFeatures call.

The work to add a new virtual test suite would be as follows:

A) Add a new WebRuntimeFeatures API.
B) Extend TestRunner's table to know how to set that WebRuntimeFeatures.

Notice that A and B can be done in one atomic commit because
WebRuntimeFeatures.h and TestRunner are in the same repo.  It's also now clear
in the API which configurations combinations are exposed to embedders.  It's
also clear that this functionality should be used only by tests.