kDisableOriginTrialControlledBlinkFeatures is no longer checked at the end of SetRuntimeFeaturesDefaultsAndUpdateFromArgs() |
||||
Issue descriptionWhen kDisableOriginTrialControlledBlinkFeatures was added [1], it was checked at the end of SetRuntimeFeaturesDefaultsAndUpdateFromArgs(). Only checks for kEnableBlinkFeatures and kDisableBlinkFeatures followed it. Over time, however, many new features have been added after it check - and about half of those have been after the checks for kEnableBlinkFeatures and kDisableBlinkFeatures. It's possible that features checked after the kDisableOriginTrialControlledBlinkFeatures check will behave differently than those checked before it. This may not be an issue in practice, but we should avoid any possibility. The same is true of kEnableBlinkFeatures and kDisableBlinkFeatures. Therefore, I think we should move all three of these to the end (assuming they should take precedence) and add a block comment that says to not add checks after these. [1] https://chromium-review.googlesource.com/c/chromium/src/+/568807/4/content/child/runtime_features.cc [2] https://cs.chromium.org/chromium/src/content/child/runtime_features.cc?type=cs&q=kDisableOriginTrialControlledBlinkFeatures+kDisableBlinkFeatures&sq=package:chromium&l=366
,
Apr 13 2018
,
Apr 13 2018
I'm going to extend this bug to cover the following from issue 832393: * SetRuntimeFeatureDefaultsForPlatform is called after setting enabling Origin Trials (overall, not individual trials) and WebUSB. History: 1. Web Bluetooth was inserted above SetRuntimeFeatureDefaultsForPlatform() [1] for unknown reasons. Perhaps it was just added to the "top." 2. Origin Trials (then Experimental Framework) was added above Web Bluetooth for unknown reasons. Perhaps it was just added to the "top," with it not being obvious that SetRuntimeFeatureDefaultsForPlatform was called below. 3. WebUSB was then added immediately after Web Bluetooth [3], which makes sense since the APIs are similar in purpose. 4. The Web Bluetooth check was eventually removed, leaving just WebUSB [1] https://chromium.googlesource.com/chromium/src/+/89be0ad96e7968a977db955441ddab26685b7cc8%5E%21/#F4 [2] https://chromium.googlesource.com/chromium/src/+/f80232f795d48d28f317d2c26e02feb12501de8b%5E%21/#F2 [3] https://chromium.googlesource.com/chromium/src/+/1bf9f7c6c0334454c1dcf6bd45c517b6ffe5876b%5E%21/#F4
,
Apr 13 2018
While kDisableOriginTrialControlledBlinkFeatures was originally before kEnableBlinkFeatures and kDisableBlinkFeatures, it probably makes sense for it to be last since it is a hammer for testing. It might also make sense to add "-for-testing" to the name to make the intent clear.
,
Apr 14 2018
I think that the ordering we have now is roughly right -- disabling origin trial features as a test measure, then --enable, and finally --disable. --disable should probably always trump any other flags, and I can imagine uaing --enable to add back a specific origin trial feature once all of them have been turned off.
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a82f49e4c456d5298443fa3126ca723f3e5bc65 commit 9a82f49e4c456d5298443fa3126ca723f3e5bc65 Author: David Dorwin <ddorwin@chromium.org> Date: Tue Apr 17 01:09:28 2018 Move setting of feature groups to the beginning and end of SetRuntimeFeatureDefaultsForPlatform Over time, individual features were added before or after the calls that set groups of features. This could lead to different behavior for individual features. Therefore, this CL moves them back to their original position and adds comments to hopefully deter such future changes. This CL does not attempt to modify overall order (issue 832393). Bug: 830978 Change-Id: I83df46b733751a0e9f1272c214518a3bdc67b199 Reviewed-on: https://chromium-review.googlesource.com/1013212 Reviewed-by: Ian Clelland <iclelland@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: David Dorwin <ddorwin@chromium.org> Cr-Commit-Position: refs/heads/master@{#551203} [modify] https://crrev.com/9a82f49e4c456d5298443fa3126ca723f3e5bc65/content/child/runtime_features.cc
,
Apr 17 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by ddorwin@chromium.org
, Apr 13 2018