Migrate FeatureSwitches using Finch to use the Features API |
||
Issue descriptionFeatureSwitch is an extensions concept that was introduced a few years back to detect if a feature was enabled when it could be enabled through different mechanisms (e.g., command line switch, runtime variable, and more recently finch). Currently, FeatureSwitch uses the FieldTrial API to query for "Enable"+experiment name to see if a feature is enabled via finch. This is fragile, and should be updated to use the Feature API. More ambitiously, the finch framework now accommodates most of the same requirements - runtime manipulation, commandline integration, etc. We might be able to entirely remove, or at least reduce, the FeatureSwitch interface.
,
Nov 13 2017
I'm not familiar with FeatureSwitch, but here are some thoughts. 1. We also have field trial APIs under metricsPrivate: - getFieldTrial(), getVariationParams() I assume that's a separate thing from FeatureSwitch? I was thinking if we add APIs for querying features, they would be alongside the field trial ones above - but not sure what's the best way to unify things if we also have a different mechanism already. 2. If we want to query base::Features from extensions, we will need to maintain a separate list of Features that can be queried. This is because there's no central registration for features - you need access to the actual base::Feature struct to query its state (if there's no override, the state from the struct is used). So for an API that would query them by name, you'd need a separate mapping. We do the same thing for features we expose through JNI to the Java layer on Android. See chrome_feature_list.cc. One nice thing about this is it will reduce the surface space of what's exposed to extensions - as we should only list the features that we want to expose.
,
Nov 13 2017
To be clear, FeatureSwitch is not exposed to extensions. It is an internal C++ class used by the Extensions system code in various places: https://cs.chromium.org/chromium/src/extensions/common/feature_switch.cc?q=feature_switch.cc&sq=package:chromium&l=1 Basically, all it does is provide a struct that can generically encapsulate the logic of: if (runtime_overridde_value) return *runtime_override_value; if (command_line_value) return *command_line_value; if (feature_switch_value) return *feature_switch_value; return default_value; for an arbitrary feature. My understanding is that all of these are now fairly easily achieved by features, and it seems good to converge on that (since it is far more widely used).
,
Nov 13 2017
Ah gotcha! Yeah, in that case we can probably get rid of the helper entirely (once all uses of it are eliminated).
,
Nov 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b81f44f2583034a47ab26b46b41166dbc59b0e2 commit 3b81f44f2583034a47ab26b46b41166dbc59b0e2 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Nov 15 17:24:32 2017 Update YieldBetweenContentScriptRuns in fieldtrial_testing_config.json Update YieldBetweenContentScriptRuns to include the enable_features entry for "YieldBetweenContentScriptRuns". Bug: 784476 Change-Id: I195578b82669561564a00a11f2466539989fbe28 Reviewed-on: https://chromium-review.googlesource.com/769304 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#516731} [modify] https://crrev.com/3b81f44f2583034a47ab26b46b41166dbc59b0e2/testing/variations/fieldtrial_testing_config.json
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4 commit a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Nov 22 04:47:43 2017 [Extensions Features] Migrate FeatureSwitch native_crx_bindings FeatureSwitch::native_crx_bindings() controls the native bindings experiment. Now, the metrics Features API (base::Feature[List]) has all the same capabilities as FeatureSwitch (commandline, runtime, and variation toggling). Migrate native_crx_bindings to use the Features API. Bug: 784476 Change-Id: Ic57e258d1d3b6cacc153eba915b2386109dd2a69 Reviewed-on: https://chromium-review.googlesource.com/772840 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#518529} [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/extensions/api_binding_perf_browsertest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/extensions/error_console/error_console_browsertest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/extensions/extension_bindings_apitest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/extensions/extension_messages_apitest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/extensions/native_bindings_apitest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/browser/extensions/service_worker_apitest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/BUILD.gn [add] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/extension_features.cc [add] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/extension_features.h [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/feature_switch.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/feature_switch.h [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/switches.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/common/switches.h [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/renderer/api_test_base.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/renderer/dispatcher.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/renderer/module_system_test.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/renderer/utils_unittest.cc [modify] https://crrev.com/a3fe3d605cbbfa8c12fa89d68eae5537acfc8ed4/extensions/renderer/worker_thread_dispatcher.cc
,
Dec 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cd6bc10775b703e05353d8d11358658e0a78241 commit 4cd6bc10775b703e05353d8d11358658e0a78241 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Tue Dec 05 23:46:49 2017 [Extensions Features] Migrate yield_between_content_script_runs FeatureSwitch::yield_between_content_script_runs() controls the experiment to yield between subsequent content script injections. Now, the metrics Features API (base::Feature[List]) has all the same capabilities as FeatureSwitch (commandline, runtime, and variation toggling). Migrate yield_between_content_script_runs to use the Features API, and remove the FeatureSwitch member. Note: a followup will remove FeatureSwitch finch support, which will be unused with the removal of this FeatureSwitch. Bug: 784476 Change-Id: I780695e90b83b2037b7f601efa42f0871da24303 Reviewed-on: https://chromium-review.googlesource.com/797893 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#521902} [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/chrome/browser/extensions/content_script_apitest.cc [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/common/extension_features.cc [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/common/extension_features.h [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/common/feature_switch.cc [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/common/feature_switch.h [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/common/switches.cc [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/common/switches.h [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/renderer/script_injection.cc [modify] https://crrev.com/4cd6bc10775b703e05353d8d11358658e0a78241/extensions/renderer/script_injection_manager.cc
,
Dec 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05026d51d9fb58298658d669f22a32e70b259f54 commit 05026d51d9fb58298658d669f22a32e70b259f54 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Dec 06 23:20:20 2017 [Extensions] Remove FeatureSwitch support for Field Trials The Finch Features API (base::Feature and base::FeatureList) now satisfy all the requirements that FeatureSwitch needed to (the ability to toggle from command line, runtime flag, or field trial). The two FeatureSwitch items that used the Field Trial API (yield_between_content_scripts and native_crx_bindings) have been migrated to the Features API. Remove FeatureSwitch support for field trials. Bug: 784476 Bug: 585569 Change-Id: I265b5f687707aeee2e7daf2f45df44ab094e479c Reviewed-on: https://chromium-review.googlesource.com/811290 Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#522244} [modify] https://crrev.com/05026d51d9fb58298658d669f22a32e70b259f54/chrome/common/extensions/feature_switch_unittest.cc [modify] https://crrev.com/05026d51d9fb58298658d669f22a32e70b259f54/extensions/common/feature_switch.cc [modify] https://crrev.com/05026d51d9fb58298658d669f22a32e70b259f54/extensions/common/feature_switch.h
,
Dec 6 2017
Updating title to be more accurate, and closing out. FeatureSwitch and Features no longer overlap. I think we probably don't want to entirely kill FeatureSwitch yet, since it's still got some use (for things that don't need to be Finch'd). |
||
►
Sign in to add a comment |
||
Comment 1 by isherman@chromium.org
, Nov 13 2017