New issue
Advanced search Search tips

Issue 832877 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 832393


Show other hotlists

Hotlists containing this issue:
Feature-Control-Code-Health


Sign in to add a comment

Do not use WebRuntimeFeatures::EnableFEATURE(base::FeatureList::IsEnabled(kFEATURE)) pattern

Project Member Reported by ddorwin@chromium.org, Apr 13 2018

Issue description

As described in problem (2) in issue 832393, some features are enabled/disabled in SetRuntimeFeaturesDefaultsAndUpdateFromArgs() using the following pattern:
WebRuntimeFeatures::Enable<FeatureName>(base::FeatureList::IsEnabled(features::k<FeatureName>)). This pattern appears to be harmful and should be avoided.

Quickly reviewing features that use it, most appear to happen to mostly work correctly due to a variety of current states and workarounds, some which I've described below. By avoiding this pattern, though, we can avoid the need to update SetRuntimeFeaturesDefaultsAndUpdateFromArgs() when runtime_enabled_features.json5 or the base::Feature's default is updated, eliminate workarounds, and avoid potential unexpected behavior.

The following is a sampling of features that use this pattern and how it affects them. They focus on "experimental" features when --enable-experimental-web-platform-features is used, but similar problems could exist for features affected by SetRuntimeFeatureDefaultsForPlatform() or elsewhere.

* UserActivationV2 is fine now since it is not "experimental" (it has no status), but this code will need to change if/when its status becomes "experimental".
* WebXR is a concrete example of such a change, as it had to be switched to use an if statement when its status was changed to "experimental" in https://crrev.com/c/1003041.
* WebAuth gets away with this pattern mostly because the base::Feature is enabled by default except on Android. However, on Android, it probably does not respect --enable-experimental-web-platform-features
* WebAssemblyStreaming is "experimental" but the base::Feature enabled by default.
* ScrollAnchoring uses the pattern but adds `|| enableExperimentalWebPlatformFeatures`, so it actually behaves correctly. However, this means this code and runtime_enabled_features.json5 need to be kept in sync and really just duplicates the work that the earlier EnableExperimentalFeatures(true) call tried to handle earlier.
* RootLayerScrolling is "stable" and thus unaffected by --enable-experimental-web-platform-features.
 

Comment 1 by cha...@chromium.org, Apr 13 2018

Status: Available (was: Untriaged)
At least one instance of this pattern was an intentional change to overcome other precedence problems. We'll need to resolve the overall plan for precedence first (issue 832393).

ScrollAnchoring was changed to this pattern in https://codereview.chromium.org/2379053004/diff/40001/content/child/runtime_features.cc#newcode234 to address  issue 651874  where chrome://flags could not override field trials.
Blocking: -832393
Blockedon: 832393

Comment 5 by cha...@chromium.org, Jan 18 (4 days ago)

This is a code health issue that we still intend to address.

Sign in to add a comment