Toggling RuntimeEnabledFeatures in LayoutTests

[Dominic Mazzoni]

I'm having trouble with toggling RuntimeEnabledFeatures in a test - the tests I landed are passing when run serially, but not when run in parallel. The issue seems to be that attributes annotated with [RuntimeEnabled] in web IDL don't seem to work if you access them before enabling them.

The feature in question is AccessibilityObjectModel, defined in RuntimeEnabledFeatures.json5 like this:

{ name: "AccessibilityObjectModel", settable_from_internals: true, status: "experimental", },

The interface I'm trying to call is Element.accessibleNode, defined in Element.idl like this:

[RuntimeEnabled=AccessibilityObjectModel] readonly attribute AccessibleNode? accessibleNode;

Here's what I'm observing:

1. In an empty test, document.body.accessibleNode returns undefined.

2. If I first call internals.runtimeFlags.accessibilityObjectModelEnabled = true, then document.body.accessibleNode returns an object.

3. However, if I first access document.body.accessibleNode first, and then call internals.runtimeFlags.accessibilityObjectModelEnabled = true, then document.body.accessibleNode continues to return undefined.

Is there a step I'm missing, or a better way to do this?

[Daniel Cheng]

This is a limitation of the bindings system. Accessing document.body forces the creation of the V8 wrapper for Element, which forces bindings to create the object template for the Element interface. At that time, bindings has to make a determination of whether or not the runtime feature is enabled--and if it isn't, the object template won't set up any of the glue for that attribute/method.

Daniel

[Daniel Cheng]

Note: one way to work around this is to implement the layout test as a C++ unit test (perhaps using the SimTest framework).

Daniel

[Dominic Mazzoni]

Switching to SimTest for most new tests is fine with me, but is there also a workaround for layout tests? We'd like to have cross-browser web platform tests, for example.

Would it be reasonable to just enable this feature in the test runner so that it's enabled by default for all layout tests?

[TAMURA, Kent]

A flag with status:"experimental" should be enabled by default during running layout tests. If it's not enabled, it's a bug of our test environment setup.

--

TAMURA Kent
Software Engineer, Google

[Philip Jägenstedt]

status: "test" is also enough, if it's too early to enable the feature for any users at all.

[Dominic Mazzoni]

OK, I see what's going on. When I initially set the flag to status: "experimental", that did enable it by default during layout tests. However, when I added that flag to content features (https://codereview.chromium.org/2787843003), that caused it to be switched off by default. So is that what "test" is for?

[Philip Jägenstedt]

Did you figure it out? I presume that the thing that switched it off doesn't differentiate between "experimental" and "test". "test" is for enabling it while running LayoutTests without enabling it for users who have "experimental web platform features" enabled.

[Dominic Mazzoni]

I can see what's going on in the code, but I'm not sure the correct fix.

If you add a feature to RuntimeEnabledFeatures, it works as advertised: either "experimental" or "test" enables it during tests. You can toggle it from the command-line using --enable-blink-features=

However, if you also add a content flag (content/public/common/content_features.cc) and use that to enable the blink flag in content/child/runtime_features.cc, that turns the feature on or off based on that flag, bypassing the default set in RuntimeEnabledFeatures.json5. (One reason I gave it a content flag is to be able to enable it dynamically in content tests, and to have the option of a future finch experiment or field trial.)

However, if you set the feature to "test", then RuntimeEnabledFeatures::setTestFeaturesEnabled will set the Blink feature to true when running layout tests, even if it was set to false by the content layer.

Possible fixes - I'm not sure which of these is right:

1. RuntimeEnabledFeatures::setTestFeaturesEnabled could also enable experimental features?

2. Add a new method RuntimeEnabledFeatures::setExperimentalFeaturesEnabled that's also called when running layout tests?

3. content/child/runtime_features.cc should have a way to leave web platform features as their default if not explicitly set by a command-line switch or other explicit experiment (i.e. the choices should be true/false/default)

- Dominic

[Daniel Cheng]

On Tue, Apr 25, 2017 at 8:24 AM Dominic Mazzoni <dmaz...@chromium.org> wrote:

I can see what's going on in the code, but I'm not sure the correct fix.

If you add a feature to RuntimeEnabledFeatures, it works as advertised: either "experimental" or "test" enables it during tests. You can toggle it from the command-line using --enable-blink-features=

However, if you also add a content flag (content/public/common/content_features.cc) and use that to enable the blink flag in content/child/runtime_features.cc, that turns the feature on or off based on that flag, bypassing the default set in RuntimeEnabledFeatures.json5. (One reason I gave it a content flag is to be able to enable it dynamically in content tests, and to have the option of a future finch experiment or field trial.)

However, if you set the feature to "test", then RuntimeEnabledFeatures::setTestFeaturesEnabled will set the Blink feature to true when running layout tests, even if it was set to false by the content layer.

Possible fixes - I'm not sure which of these is right:

1. RuntimeEnabledFeatures::setTestFeaturesEnabled could also enable experimental features?

2. Add a new method RuntimeEnabledFeatures::setExperimentalFeaturesEnabled that's also called when running layout tests?

3. content/child/runtime_features.cc should have a way to leave web platform features as their default if not explicitly set by a command-line switch or other explicit experiment (i.e. the choices should be true/false/default)

It seems to me runtime_features.cc should prefer the pattern:

if (condition)

  WebRuntimeFeatures::EnableFoo(true);  // or false

over

WebRuntimeFeatures::EnableFoo(condition);

Daniel

[Dominic Mazzoni]

On Tue, Apr 25, 2017 at 5:54 AM Daniel Cheng <dch...@chromium.org> wrote:

It seems to me runtime_features.cc should prefer the pattern:

if (condition)

  WebRuntimeFeatures::EnableFoo(true);  // or false

That would mean you couldn't disable the feature?

Perhaps to elaborate, is this what you had in mind:

* If RuntimeFeatures has status "test" or "experimental", runtime_features.cc should only call EnableFoo(true),

* If RuntimeFeatures has status "stable", runtime_features.cc should only call EnableFoo(false), and then the switch should be deleted when the experimentation is over

[Daniel Cheng]

Yes: the command-line flag should be used for flipping the runtime-enabled feature away from its default state, whatever that may be, rather than unconditionally setting it.

Daniel 

[TAMURA, Kent]

WebRuntimeFeatures::EnableExperimentalFeatures() is called at the beginning of SetRuntimeFeaturesDefaultsAndUpdateFromArgs(), then SetRuntimeFeaturesDefaultsAndUpdateFromArgs() overrides flags with command-line or base::FeatureList.

After that, LayoutTestContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() calls only WebRuntimeFeatures::EnableTestOnlyFeatures().  It should call WebRuntimeFeatures::EnableExperimentalFeatures() too.  base::FeatureList should not affect layout tests.

[Daniel Cheng]

I think we still want to have some way of allowing flags to change layout test behavior, no?

Daniel