Prevent RuntimeEnabledFeature checks for features under origin trial |
||
Issue descriptionThe integration instructions for origin trial recommend replacing all checks for RuntimeEnabledFeature with the appropriate origin trial check (either via IDL or C++): https://chromium.googlesource.com/chromium/src/+/master/docs/origin_trials_integration.md Even with instruction, that is a manual process in which references can be missed by developers and code reviewers. For example, see this thread: https://groups.google.com/a/chromium.org/d/topic/experimentation-dev/nba0pV8iDLw/discussion ... and this CL: https://chromium-review.googlesource.com/c/chromium/src/+/849254 iclelland@ suggested making it a compile-time error to use any RuntimeEnabledFeature checks when there is an origin trial configured. The thinking is to implement by changing the code generation to omit RuntimeEnabledFeature functions for those with an origin trial. There may be complications in the bindings code, but seems like a good starting point.
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/71d9865af943ad83bc44f70289b4ad059a416f8b commit 71d9865af943ad83bc44f70289b4ad059a416f8b Author: Ian Clelland <iclelland@google.com> Date: Tue Jan 30 18:54:34 2018 Remove redundant check for WebVR runtime flag. The code checks both the runtime flag and the origin trial status, when the origin trial check implicitly checks the runtime flag first. Removing this antipattern will allow us to eventually mark the inappropriate checks as private, and reduce inadvertent bugs. Bug: 799949 Change-Id: I8af29c46a0f5eefacb366e87d378c86b7d4ac71f Reviewed-on: https://chromium-review.googlesource.com/891573 Reviewed-by: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Ian Clelland <iclelland@chromium.org> Cr-Commit-Position: refs/heads/master@{#532975} [modify] https://crrev.com/71d9865af943ad83bc44f70289b4ad059a416f8b/third_party/WebKit/Source/modules/ModulesInitializer.cpp
,
Feb 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7bb2e57ab78d896fc38b1a9da7854abf27b9193d commit 7bb2e57ab78d896fc38b1a9da7854abf27b9193d Author: Ian Clelland <iclelland@chromium.org> Date: Wed Feb 14 19:57:37 2018 Fix Origin Trial integration for Low-Latency Canvas There were two places in code which relied on the state of the runtime flag for low-latency canvas. Because it is under an origin trial, the code should actually be checking OriginTrials::lowLatencyCanvasEnabled(), since the runtime flag is actually always on for the trial, but the state of the feature can differ between frames. Bug: 799949 Change-Id: I747c259fe4dd734a13ae2644497cf3e136354289 Reviewed-on: https://chromium-review.googlesource.com/893521 Reviewed-by: Justin Novosad <junov@chromium.org> Commit-Queue: Ian Clelland <iclelland@chromium.org> Cr-Commit-Position: refs/heads/master@{#536787} [modify] https://crrev.com/7bb2e57ab78d896fc38b1a9da7854abf27b9193d/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp [modify] https://crrev.com/7bb2e57ab78d896fc38b1a9da7854abf27b9193d/third_party/WebKit/Source/core/html/canvas/HTMLCanvasElement.cpp
,
Feb 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ecdce4160726827b731d0c029c00b967bd76bb8 commit 2ecdce4160726827b731d0c029c00b967bd76bb8 Author: Ian Clelland <iclelland@chromium.org> Date: Thu Feb 22 16:09:33 2018 Rename the RuntimeEnabledFeatures check for Origin Trials When a feature is part of an origin trial, it is almost always incorrect to call RuntimeEnabledFeatures::<feature>Enabled. Instead, the corresponding method on OriginTrials should be called. This change renames the method in RuntimeEnabledFeatures, when a feature is made part of a trial, so that any existing checks in code will trigger compiler errors until changed. Bug: 799949 Change-Id: I81ea3f569e0cd250cc155db25cf381f9de032bc2 Reviewed-on: https://chromium-review.googlesource.com/929644 Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Jason Chase <chasej@chromium.org> Commit-Queue: Ian Clelland <iclelland@chromium.org> Cr-Commit-Position: refs/heads/master@{#538447} [modify] https://crrev.com/2ecdce4160726827b731d0c029c00b967bd76bb8/third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl [modify] https://crrev.com/2ecdce4160726827b731d0c029c00b967bd76bb8/third_party/WebKit/Source/build/scripts/templates/RuntimeEnabledFeaturesTestHelpers.h.tmpl [modify] https://crrev.com/2ecdce4160726827b731d0c029c00b967bd76bb8/third_party/WebKit/Source/build/scripts/templates/origin_trials.cc.tmpl [modify] https://crrev.com/2ecdce4160726827b731d0c029c00b967bd76bb8/third_party/WebKit/Source/build/scripts/templates/runtime_enabled_features.cc.tmpl [modify] https://crrev.com/2ecdce4160726827b731d0c029c00b967bd76bb8/third_party/WebKit/Source/build/scripts/templates/runtime_enabled_features.h.tmpl
,
Apr 18 2018
Calling this fixed for now. If there is additional work that needs to be done, re-open or (better yet) file new bugs with the specific work required. |
||
►
Sign in to add a comment |
||
Comment 1 by iclell...@chromium.org
, Jan 30 2018Owner: iclell...@chromium.org
Status: Started (was: Available)