Add switch-dependent feature overrides to FeatureList initialization

This adds the ability to set up feature overrides that depend on the
presence of a command-line switch. These overrides are processed
during FeatureList initialization, after explicit enables/disables,
but before field trial setup. This allows the initialization of
a base::Feature according to a command-line switch, which is superseded
by explicit --enable-features and --disable-features, but which
supersedes variations/field trials.

Bug: 988603
Change-Id: Id808325d94689d272b9bc00b400b44d0560d6136
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1724704
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685668}

android_webview/browser/aw_feature_list_creator.cc

@@ -36,6 +36,7 @@
 #include "components/variations/pref_names.h"
 #include "components/variations/service/safe_seed_manager.h"
 #include "components/variations/service/variations_service.h"
+#include "content/public/common/content_switch_dependent_feature_overrides.h"
 #include "services/preferences/tracked/segregated_pref_store.h"
 
 namespace android_webview {
@@ -144,9 +145,11 @@ void AwFeatureListCreator::SetUpFieldTrials() {
   variations_field_trial_creator_->SetupFieldTrials(
       cc::switches::kEnableGpuBenchmarking, switches::kEnableFeatures,
       switches::kDisableFeatures, unforceable_field_trials,
-      std::vector<std::string>(), /*low_entropy_provider=*/nullptr,
-      std::make_unique<base::FeatureList>(), aw_field_trials_.get(),
-      &ignored_safe_seed_manager);
+      std::vector<std::string>(),
+      content::GetSwitchDependentFeatureOverrides(
+          *base::CommandLine::ForCurrentProcess()),
+      /*low_entropy_provider=*/nullptr, std::make_unique<base::FeatureList>(),
+      aw_field_trials_.get(), &ignored_safe_seed_manager);
 }
 
 void AwFeatureListCreator::CreateLocalState() {

base/feature_list.cc

@@ -161,6 +161,14 @@ void FeatureList::RegisterFieldTrialOverride(const std::string& feature_name,
   RegisterOverride(feature_name, override_state, field_trial);
 }
 
+void FeatureList::RegisterExtraFeatureOverrides(
+    const std::vector<FeatureOverrideInfo>& extra_overrides) {
+  for (const FeatureOverrideInfo& override_info : extra_overrides) {
+    RegisterOverride(override_info.first.get().name, override_info.second,
+                     /* field_trial = */ nullptr);
+  }
+}
+
 void FeatureList::AddFeaturesToAllocator(PersistentMemoryAllocator* allocator) {
   DCHECK(initialized_);
 
@@ -223,6 +231,15 @@ std::vector<StringPiece> FeatureList::SplitFeatureListString(
 // static
 bool FeatureList::InitializeInstance(const std::string& enable_features,
                                      const std::string& disable_features) {
+  return InitializeInstance(enable_features, disable_features,
+                            std::vector<FeatureOverrideInfo>());
+}
+
+// static
+bool FeatureList::InitializeInstance(
+    const std::string& enable_features,
+    const std::string& disable_features,
+    const std::vector<FeatureOverrideInfo>& extra_overrides) {
   // We want to initialize a new instance here to support command-line features
   // in testing better. For example, we initialize a dummy instance in
   // base/test/test_suite.cc, and override it in content/browser/
@@ -247,6 +264,7 @@ bool FeatureList::InitializeInstance(const std::string& enable_features,
 
   std::unique_ptr<FeatureList> feature_list(new FeatureList);
   feature_list->InitializeFromCommandLine(enable_features, disable_features);
+  feature_list->RegisterExtraFeatureOverrides(extra_overrides);
   FeatureList::SetInstance(std::move(feature_list));
   return !instance_existed_before;
 }

base/feature_list.h

@@ -96,6 +96,18 @@ class BASE_EXPORT FeatureList {
   FeatureList();
   ~FeatureList();
 
+  // Specifies whether a feature override enables or disables the feature.
+  enum OverrideState {
+    OVERRIDE_USE_DEFAULT,
+    OVERRIDE_DISABLE_FEATURE,
+    OVERRIDE_ENABLE_FEATURE,
+  };
+
+  // Describes a feature override. The first member is a Feature that will be
+  // overridden with the state given by the second member.
+  using FeatureOverrideInfo =
+      std::pair<const std::reference_wrapper<const Feature>, OverrideState>;
+
   // Initializes feature overrides via command-line flags |enable_features| and
   // |disable_features|, each of which is a comma-separated list of features to
   // enable or disable, respectively. If a feature appears on both lists, then
@@ -114,15 +126,10 @@ class BASE_EXPORT FeatureList {
   // of the associated field trial.
   void InitializeFromSharedMemory(PersistentMemoryAllocator* allocator);
 
-  // Specifies whether a feature override enables or disables the feature.
-  enum OverrideState {
-    OVERRIDE_USE_DEFAULT,
-    OVERRIDE_DISABLE_FEATURE,
-    OVERRIDE_ENABLE_FEATURE,
-  };
-
   // Returns true if the state of |feature_name| has been overridden via
-  // |InitializeFromCommandLine()|.
+  // |InitializeFromCommandLine()|. This includes features explicitly
+  // disabled/enabled with --disable-features and --enable-features, as well as
+  // any extra feature overrides that depend on command line switches.
   bool IsFeatureOverriddenFromCommandLine(const std::string& feature_name,
                                           OverrideState state) const;
 
@@ -146,6 +153,15 @@ class BASE_EXPORT FeatureList {
                                   OverrideState override_state,
                                   FieldTrial* field_trial);
 
+  // Adds extra overrides (not associated with a field trial). Should be called
+  // before SetInstance().
+  // The ordering of calls with respect to InitializeFromCommandLine(),
+  // RegisterFieldTrialOverride(), etc. matters. The first call wins out,
+  // because the |overrides_| map uses insert(), which retains the first
+  // inserted entry and does not overwrite it on subsequent calls to insert().
+  void RegisterExtraFeatureOverrides(
+      const std::vector<FeatureOverrideInfo>& extra_overrides);
+
   // Loops through feature overrides and serializes them all into |allocator|.
   void AddFeaturesToAllocator(PersistentMemoryAllocator* allocator);
 
@@ -188,6 +204,14 @@ class BASE_EXPORT FeatureList {
   static bool InitializeInstance(const std::string& enable_features,
                                  const std::string& disable_features);
 
+  // Like the above, but also adds extra overrides. If a feature appears in
+  // |extra_overrides| and also |enable_features| or |disable_features|, the
+  // disable/enable will supersede the extra overrides.
+  static bool InitializeInstance(
+      const std::string& enable_features,
+      const std::string& disable_features,
+      const std::vector<FeatureOverrideInfo>& extra_overrides);
+
   // Returns the singleton instance of FeatureList. Will return null until an
   // instance is registered via SetInstance().
   static FeatureList* GetInstance();

base/feature_list_unittest.cc

@@ -8,6 +8,7 @@
 
 #include <algorithm>
 #include <utility>
+#include <vector>
 
 #include "base/format_macros.h"
 #include "base/memory/read_only_shared_memory_region.h"
@@ -378,12 +379,64 @@ TEST_F(FeatureListTest, AssociateReportingFieldTrial) {
   }
 }
 
+TEST_F(FeatureListTest, RegisterExtraFeatureOverrides) {
+  ClearFeatureListInstance();
+
+  auto feature_list = std::make_unique<FeatureList>();
+  std::vector<FeatureList::FeatureOverrideInfo> overrides;
+  overrides.push_back({std::cref(kFeatureOnByDefault),
+                       FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE});
+  overrides.push_back({std::cref(kFeatureOffByDefault),
+                       FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE});
+  feature_list->RegisterExtraFeatureOverrides(std::move(overrides));
+  RegisterFeatureListInstance(std::move(feature_list));
+
+  EXPECT_FALSE(FeatureList::IsEnabled(kFeatureOnByDefault));
+  EXPECT_TRUE(FeatureList::IsEnabled(kFeatureOffByDefault));
+}
+
+TEST_F(FeatureListTest, InitializeFromCommandLineThenRegisterExtraOverrides) {
+  ClearFeatureListInstance();
+
+  auto feature_list = std::make_unique<FeatureList>();
+  feature_list->InitializeFromCommandLine(kFeatureOnByDefaultName,
+                                          kFeatureOffByDefaultName);
+  std::vector<FeatureList::FeatureOverrideInfo> overrides;
+  overrides.push_back({std::cref(kFeatureOnByDefault),
+                       FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE});
+  overrides.push_back({std::cref(kFeatureOffByDefault),
+                       FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE});
+  feature_list->RegisterExtraFeatureOverrides(std::move(overrides));
+  RegisterFeatureListInstance(std::move(feature_list));
+
+  // The InitializeFromCommandLine supersedes the RegisterExtraFeatureOverrides
+  // because it was called first.
+  EXPECT_TRUE(FeatureList::IsEnabled(kFeatureOnByDefault));
+  EXPECT_FALSE(FeatureList::IsEnabled(kFeatureOffByDefault));
+
+  std::string enable_features;
+  std::string disable_features;
+  FeatureList::GetInstance()->GetFeatureOverrides(&enable_features,
+                                                  &disable_features);
+  EXPECT_EQ(kFeatureOnByDefaultName, SortFeatureListString(enable_features));
+  EXPECT_EQ(kFeatureOffByDefaultName, SortFeatureListString(disable_features));
+}
+
 TEST_F(FeatureListTest, GetFeatureOverrides) {
   ClearFeatureListInstance();
   FieldTrialList field_trial_list(nullptr);
   std::unique_ptr<FeatureList> feature_list(new FeatureList);
   feature_list->InitializeFromCommandLine("A,X", "D");
 
+  Feature feature_b = {"B", FEATURE_ENABLED_BY_DEFAULT};
+  Feature feature_c = {"C", FEATURE_DISABLED_BY_DEFAULT};
+  std::vector<FeatureList::FeatureOverrideInfo> overrides;
+  overrides.push_back({std::cref(feature_b),
+                       FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE});
+  overrides.push_back({std::cref(feature_c),
+                       FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE});
+  feature_list->RegisterExtraFeatureOverrides(std::move(overrides));
+
   FieldTrial* trial = FieldTrialList::CreateFieldTrial("Trial", "Group");
   feature_list->RegisterFieldTrialOverride(kFeatureOffByDefaultName,
                                            FeatureList::OVERRIDE_ENABLE_FEATURE,
@@ -395,13 +448,13 @@ TEST_F(FeatureListTest, GetFeatureOverrides) {
   std::string disable_features;
   FeatureList::GetInstance()->GetFeatureOverrides(&enable_features,
                                                   &disable_features);
-  EXPECT_EQ("A,OffByDefault<Trial,X", SortFeatureListString(enable_features));
-  EXPECT_EQ("D", SortFeatureListString(disable_features));
+  EXPECT_EQ("A,C,OffByDefault<Trial,X", SortFeatureListString(enable_features));
+  EXPECT_EQ("B,D", SortFeatureListString(disable_features));
 
   FeatureList::GetInstance()->GetCommandLineFeatureOverrides(&enable_features,
                                                              &disable_features);
-  EXPECT_EQ("A,X", SortFeatureListString(enable_features));
-  EXPECT_EQ("D", SortFeatureListString(disable_features));
+  EXPECT_EQ("A,C,X", SortFeatureListString(enable_features));
+  EXPECT_EQ("B,D", SortFeatureListString(disable_features));
 }
 
 TEST_F(FeatureListTest, GetFeatureOverrides_UseDefault) {

chrome/browser/metrics/chrome_feature_list_creator.cc

@@ -41,6 +41,7 @@
 #include "components/variations/pref_names.h"
 #include "components/variations/service/variations_service.h"
 #include "components/variations/variations_crash_keys.h"
+#include "content/public/common/content_switch_dependent_feature_overrides.h"
 #include "services/service_manager/embedder/result_codes.h"
 #include "ui/base/resource/resource_bundle.h"
 
@@ -194,6 +195,8 @@ void ChromeFeatureListCreator::SetupFieldTrials() {
   variations_service->SetupFieldTrials(
       cc::switches::kEnableGpuBenchmarking, switches::kEnableFeatures,
       switches::kDisableFeatures, unforceable_field_trials, variation_ids,
+      content::GetSwitchDependentFeatureOverrides(
+          *base::CommandLine::ForCurrentProcess()),
       std::move(feature_list), browser_field_trials_.get());
   variations::InitCrashKeys();
 

components/variations/service/variations_field_trial_creator.cc

@@ -444,6 +444,7 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
     const char* kDisableFeatures,
     const std::set<std::string>& unforceable_field_trials,
     const std::vector<std::string>& variation_ids,
+    const std::vector<base::FeatureList::FeatureOverrideInfo>& extra_overrides,
     std::unique_ptr<const base::FieldTrial::EntropyProvider>
         low_entropy_provider,
     std::unique_ptr<base::FeatureList> feature_list,
@@ -508,6 +509,13 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
       command_line->GetSwitchValueASCII(kEnableFeatures),
       command_line->GetSwitchValueASCII(kDisableFeatures));
 
+  // This needs to happen here: After the InitializeFromCommandLine() call,
+  // because the explicit cmdline --disable-features and --enable-features
+  // should take precedence over these extra overrides. Before the call to
+  // SetInstance(), because overrides cannot be registered after the FeatureList
+  // instance is set.
+  feature_list->RegisterExtraFeatureOverrides(extra_overrides);
+
   bool used_testing_config = false;
 #if BUILDFLAG(FIELDTRIAL_TESTING_ENABLED)
   if (!command_line->HasSwitch(switches::kDisableFieldTrialTestingConfig) &&

components/variations/service/variations_field_trial_creator.h

@@ -73,16 +73,26 @@ class VariationsFieldTrialCreator {
   // |safe_seed_manager| should be notified of the combined server and client
   // state that was activated to create the field trials (only when the return
   // value is true).
-  bool SetupFieldTrials(const char* kEnableGpuBenchmarking,
-                        const char* kEnableFeatures,
-                        const char* kDisableFeatures,
-                        const std::set<std::string>& unforceable_field_trials,
-                        const std::vector<std::string>& variation_ids,
-                        std::unique_ptr<const base::FieldTrial::EntropyProvider>
-                            low_entropy_provider,
-                        std::unique_ptr<base::FeatureList> feature_list,
-                        PlatformFieldTrials* platform_field_trials,
-                        SafeSeedManager* safe_seed_manager);
+  // |extra_overrides| gives a list of feature overrides that should be applied
+  // after the features explicitly disabled/enabled from the command line via
+  // --disable-features and --enable-features, but before field trials.
+  // Note: The ordering of the FeatureList method calls is such that the
+  // explicit --disable-features and --enable-features from the command line
+  // take precedence over the |extra_overrides|, which take precedence over the
+  // field trials.
+  bool SetupFieldTrials(
+      const char* kEnableGpuBenchmarking,
+      const char* kEnableFeatures,
+      const char* kDisableFeatures,
+      const std::set<std::string>& unforceable_field_trials,
+      const std::vector<std::string>& variation_ids,
+      const std::vector<base::FeatureList::FeatureOverrideInfo>&
+          extra_overrides,
+      std::unique_ptr<const base::FieldTrial::EntropyProvider>
+          low_entropy_provider,
+      std::unique_ptr<base::FeatureList> feature_list,
+      PlatformFieldTrials* platform_field_trials,
+      SafeSeedManager* safe_seed_manager);
 
   // Returns all of the client state used for filtering studies.
   // As a side-effect, may update the stored permanent consistency country.

components/variations/service/variations_field_trial_creator_unittest.cc

@@ -260,7 +260,8 @@ class TestVariationsFieldTrialCreator : public VariationsFieldTrialCreator {
     TestPlatformFieldTrials platform_field_trials;
     return VariationsFieldTrialCreator::SetupFieldTrials(
         "", "", "", std::set<std::string>(), std::vector<std::string>(),
-        nullptr, std::make_unique<base::FeatureList>(), &platform_field_trials,
+        std::vector<base::FeatureList::FeatureOverrideInfo>(), nullptr,
+        std::make_unique<base::FeatureList>(), &platform_field_trials,
         safe_seed_manager_);
   }
 
@@ -512,7 +513,8 @@ TEST_F(FieldTrialCreatorTest, SetupFieldTrials_LoadsCountryOnFirstRun) {
   // |initial_seed| included the country code for India, this study should be
   // active.
   EXPECT_TRUE(field_trial_creator.SetupFieldTrials(
-      "", "", "", std::set<std::string>(), std::vector<std::string>(), nullptr,
+      "", "", "", std::set<std::string>(), std::vector<std::string>(),
+      std::vector<base::FeatureList::FeatureOverrideInfo>(), nullptr,
       std::make_unique<base::FeatureList>(), &platform_field_trials,
       &safe_seed_manager));
 

components/variations/service/variations_service.cc

@@ -908,12 +908,14 @@ bool VariationsService::SetupFieldTrials(
     const char* kDisableFeatures,
     const std::set<std::string>& unforceable_field_trials,
     const std::vector<std::string>& variation_ids,
+    const std::vector<base::FeatureList::FeatureOverrideInfo>& extra_overrides,
     std::unique_ptr<base::FeatureList> feature_list,
     variations::PlatformFieldTrials* platform_field_trials) {
   return field_trial_creator_.SetupFieldTrials(
       kEnableGpuBenchmarking, kEnableFeatures, kDisableFeatures,
-      unforceable_field_trials, variation_ids, CreateLowEntropyProvider(),
-      std::move(feature_list), platform_field_trials, &safe_seed_manager_);
+      unforceable_field_trials, variation_ids, extra_overrides,
+      CreateLowEntropyProvider(), std::move(feature_list),
+      platform_field_trials, &safe_seed_manager_);
 }
 
 void VariationsService::OverrideCachedUIStrings() {

components/variations/service/variations_service.h

@@ -186,13 +186,16 @@ class VariationsService
   }
 
   // Wrapper around VariationsFieldTrialCreator::SetupFieldTrials().
-  bool SetupFieldTrials(const char* kEnableGpuBenchmarking,
-                        const char* kEnableFeatures,
-                        const char* kDisableFeatures,
-                        const std::set<std::string>& unforceable_field_trials,
-                        const std::vector<std::string>& variation_ids,
-                        std::unique_ptr<base::FeatureList> feature_list,
-                        variations::PlatformFieldTrials* platform_field_trials);
+  bool SetupFieldTrials(
+      const char* kEnableGpuBenchmarking,
+      const char* kEnableFeatures,
+      const char* kDisableFeatures,
+      const std::set<std::string>& unforceable_field_trials,
+      const std::vector<std::string>& variation_ids,
+      const std::vector<base::FeatureList::FeatureOverrideInfo>&
+          extra_overrides,
+      std::unique_ptr<base::FeatureList> feature_list,
+      variations::PlatformFieldTrials* platform_field_trials);
 
   // Overrides cached UI strings on the resource bundle once it is initialized.
   void OverrideCachedUIStrings();

content/browser/startup_helper.cc

@@ -16,6 +16,7 @@
 #include "base/task/thread_pool/thread_pool.h"
 #include "build/build_config.h"
 #include "content/common/thread_pool_util.h"
+#include "content/public/common/content_switch_dependent_feature_overrides.h"
 #include "content/public/common/content_switches.h"
 
 namespace content {
@@ -40,7 +41,8 @@ std::unique_ptr<base::FieldTrialList> SetUpFieldTrialsAndFeatureList() {
 
   base::FeatureList::InitializeInstance(
       command_line->GetSwitchValueASCII(switches::kEnableFeatures),
-      command_line->GetSwitchValueASCII(switches::kDisableFeatures));
+      command_line->GetSwitchValueASCII(switches::kDisableFeatures),
+      GetSwitchDependentFeatureOverrides(*command_line));
   return field_trial_list;
 }