Do not use WebRuntimeFeatures::EnableFEATURE(base::FeatureList::IsEnabled(kFEATURE)) pattern |
|||
Issue descriptionAs 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.
,
Apr 13 2018
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.
,
Apr 13 2018
,
Apr 13 2018
,
Jan 18
(4 days ago)
This is a code health issue that we still intend to address. |
|||
►
Sign in to add a comment |
|||
Comment 1 by cha...@chromium.org
, Apr 13 2018