WebRuntimeFeatures overrides are inconsistently applied to features |
||||
Issue descriptionMost WebRuntimeFeatures are enabled/disabled within SetRuntimeFeaturesDefaultsAndUpdateFromArgs(), though there are some exceptions (i.e., issue 831869). However, even within this function, the precedence is unclear and inconsistent. Most of the code in SetRuntimeFeaturesDefaultsAndUpdateFromArgs() is enabling/disabling individual features based on command line switches / FeatureList, but there are also several places where categories of features are enabled/disabled. There are two main problems: 1. The order of groups vs. individual features is inconsistent and there is no obvious reason for it. Some groups are set in the middle, which is probably unintentional. 2. Some features are only enabled/disabled if a flag is present or the feature has a particular state in FeatureList while others are always set to the state in FeatureList. The result of the latter is that the group values are always overridden. There are also inconsistencies: 3. There are a few features that explicitly check for `|| enableExperimentalWebPlatformFeatures` along with an individual switch or FeatureList feature. It's not clear why or whether this check is redundant given that all experimental web platform features are enabled at the top of the function. 4. Some features use WebRuntimeFeatures::EnableFeatureFromString("<FeatureName>") rather than WebRuntimeFeatures::Enable<FeatureName>(). This is used in both patterns in (2). These features do not have a specific WebRuntimeFeatures function, but they do have a generated RuntimeEnabledFeatures function. Maybe these are not web platform features. Examples of (1) include: * ExperimentalWebPlatformFeatures are enabled at the top * SetRuntimeFeatureDefaultsForPlatform is called after setting enabling Origin Trials (overall, not individual trials) and WebUSB. * All OriginTrialControlledFeatures are disabled by kDisableOriginTrialControlledBlinkFeatures in the middle ( issue 830978 ). * kEnableBlinkFeatures then kDisableBlinkFeatures are handled slightly later in the middle. * In addition, features with status "test" are enabled after SetRuntimeFeaturesDefaultsAndUpdateFromArgs() (see issue 831869) meaning all of this logic for such features is clobbered when running layout tests. Regarding (2): Some features check command_line.HasSwitch() or base::FeatureList::IsEnabled() and only then call WebRuntimeFeatures::Enable<FeatureName>() with the appropriate value. Others, however, just call WebRuntimeFeatures::Enable<FeatureName>(base::FeatureList::IsEnabled(features::k<FeatureName>)). The latter seems a bit simpler and handles the feature being both enabled and disabled regardless of the default, which could change outside this function. However, it also overrides any values that had previously been set. See (1). Some instances of (3), such as NetInfoDownlinkMax and RootLayerScrolling, are actually stable so this is unnecessary. In the case of RootLayerScrolling, which uses base::FeatureList, there is actually a corner case where one could disable the feature but enable ExperimentalWebPlatformFeatures and override that disable. Is this intentional? Related to (4), there appears to be a mix of web platform and non-web platform features enabled by this function. Maybe the two should be clearly separated in some way. Also, it'd be nice to somehow call the RuntimeEnabledFeatures functions directly without the string indirection. For (1), we should figure out what the desired order is then implement it and use helper functions and/or comments to enforce it so it does not get unintentionally changed (i.e., issue 830978 ). We may want to consider (4) as part of this. For (2), we should pick a pattern and change all features to use it consistently.
,
Apr 13 2018
,
Apr 13 2018
I opened issue 832877 to specifically resolve (2). It also contains an explanation for at least some instances of (3). Once that is addressed, we still need to decide on the desired precedence (1) and decide how to address (4).
,
Apr 13 2018
As I just noted in https://crbug.com/832877#c2, the fix for (2) is not that simple. The use of that pattern is intentional so that flags can override field trials. We need to carefully consider where and how individual features are enabled/disabled in relation to all the other mechanisms: Origin Trials, Field Trials, --enable-experimental-web-platform-features, --disable-origin-trial-controlled-blink-features, --enable-blink-features, --disable-blink-features, SetRuntimeFeatureDefaultsForPlatform(), EnableTestOnlyFeatures(true), and whatever else there may be. IMO, it would be ideal of chrome://flags could override everything (that can be reached in production Chrome).
,
Apr 13 2018
NetInfoDownlinkMax, which I mentioned in (3) is actually yet another case. The feature is marked "stable" but disabled for all platforms except Android and Chrome OS in SetRuntimeFeatureDefaultsForPlatform(). Even though it is not "experimental", it can be enabled by `--enable-experimental-web-platform-features` because of the explicit `|| enableExperimentalWebPlatformFeatures`. The explicit experimental check was added [1][2][3] for issue 399052 where the tests were not running on all platforms because SetRuntimeFeatureDefaultsForPlatform() was overriding the enableExperimentalFeatures(true) call right before it on most platforms. This is a perfect example of how the ordering is important and needs to be carefully considered. This specific condition could maybe be fixed by swapping the enableExperimentalFeatures(true) and SetRuntimeFeatureDefaultsForPlatform() calls, though it's unclear if that is the right order (see comment 4). [1] The check was originally added in https://codereview.chromium.org/426373002/diff/40001/content/child/runtime_features.cc#newcode174. [2] The check was later changed to use the variable in https://codereview.chromium.org/2379053004/diff/40001/content/child/runtime_features.cc#newcode169. [3] The name of the feature at this point was changed in https://chromium-review.googlesource.com/c/chromium/src/+/565342/11/content/child/runtime_features.cc.
,
Apr 13 2018
RootLayerScrolling, which I mentioned in (3), appears to have been done intentionally in https://crrev.com/c/929662, though it's not clear why this "stable" and base::FEATURE_ENABLED_BY_DEFAULT feature needs to be enabled by --enable-experimental-web-platform-features if someone chose to disable it specifically. skobes@?
,
Apr 13 2018
The base::Feature is affected by Finch trials, and may flip multiple times between FEATURE_DISABLED_BY_DEFAULT and FEATURE_ENABLED_BY_DEFAULT as the feature approaches launch readiness. To keep layout tests working it's important for --enable-experimental-web-platform-features to enable the Blink REF regardless of the base::Feature status. I'd love to see a refactoring of SetRuntimeFeaturesDefaultsAndUpdateFromArgs that makes the explicit check unnecessary. This would involve moving the WebRuntimeFeatures::EnableExperimentalFeatures call to the end of the function. I think I tried to do this once but ran into issues.
,
Apr 14 2018
From what I've been able to find, SetRuntimeFeatureDefaultsForPlatform was originally[1] AdjustRuntimeFeatureDefaultsForPlatform, and was called last, in order to disable features which were not actually implemented on a given platform. I feel like all of the other switches should have been placed above that, but since it can now turn features *off* as well, it's more complicated. It might make sense to split it into two --- a "set defaults" method that actually sets the defaults for togglable features, and gets run as early as possible, and then another method that disables those that *cannot* be safely turned on on the current platform, and runs after everything else. (That second one might actually be covered by https://crbug.com/711300 if we do that right, though) [1] https://chromium.googlesource.com/chromium/src/+/78b86f9adfbd2c5a7a36824ae383b9324bb3f6db%5E%21/#F0
,
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