ScopedFeatureList: Add support for enabling/disabling multiple features with parameters

Currently, there is no way to enable a feature with parameters while
enabling/disabling additional features. A feature with parameters can
be configured using InitAndEnableFeatureWithParameters. However, that
method restricts ScopedFeatureList to control only one feature and
provides no support to enable/disable additional features.

ScopedFeatureList::InitWithFeaturesAndParameters was introduced to
support configuring multiple features with parameters. As a result,
users of ScopedFeatureList can now enable more than one feature with
parameters, and disable features while enabling other features with
parameters.

Bug: 933769
Change-Id: I1c2c44c4e1058e4b62deca1f439fbfb577e64c98
Reviewed-on: https://chromium-review.googlesource.com/c/1478479
Commit-Queue: Aditya Keerthi <adityakeerthi@google.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634183}

base/test/scoped_feature_list.cc

@@ -20,6 +20,8 @@ namespace test {
 
 namespace {
 
+constexpr char kTrialGroup[] = "scoped_feature_list_trial_group";
+
 std::vector<StringPiece> GetFeatureVector(
     const std::vector<Feature>& features) {
   std::vector<StringPiece> output;
@@ -89,10 +91,13 @@ ScopedFeatureList::~ScopedFeatureList() {
   if (!init_called_)
     return;
 
-  if (field_trial_override_) {
-    base::FieldTrialParamAssociator::GetInstance()->ClearParamsForTesting(
-        field_trial_override_->trial_name(),
-        field_trial_override_->group_name());
+  auto* field_trial_param_associator = FieldTrialParamAssociator::GetInstance();
+  for (auto field_trial_override : field_trial_overrides_) {
+    if (field_trial_override) {
+      field_trial_param_associator->ClearParamsForTesting(
+          field_trial_override->trial_name(),
+          field_trial_override->group_name());
+    }
   }
 
   FeatureList::ClearInstanceForTesting();
@@ -132,12 +137,6 @@ void ScopedFeatureList::InitAndEnableFeature(const Feature& feature) {
   InitWithFeaturesAndFieldTrials({feature}, {}, {});
 }
 
-void ScopedFeatureList::InitAndEnableFeatureWithFieldTrialOverride(
-    const Feature& feature,
-    FieldTrial* trial) {
-  InitWithFeaturesAndFieldTrials({feature}, {trial}, {});
-}
-
 void ScopedFeatureList::InitAndDisableFeature(const Feature& feature) {
   InitWithFeaturesAndFieldTrials({}, {}, {feature});
 }
@@ -203,28 +202,46 @@ void ScopedFeatureList::InitWithFeaturesAndFieldTrials(
 void ScopedFeatureList::InitAndEnableFeatureWithParameters(
     const Feature& feature,
     const std::map<std::string, std::string>& feature_parameters) {
+  InitWithFeaturesAndParameters({{feature, feature_parameters}}, {});
+}
+
+void ScopedFeatureList::InitWithFeaturesAndParameters(
+    const std::vector<FeatureAndParams>& enabled_features,
+    const std::vector<Feature>& disabled_features) {
+  DCHECK(field_trial_overrides_.empty());
+
   if (!FieldTrialList::IsGlobalSetForTesting()) {
     field_trial_list_ = std::make_unique<base::FieldTrialList>(nullptr);
   }
 
+  // Enabled features and field trials to use with
+  // InitWithFeaturesAndFieldTrials.
+  std::vector<Feature> features;
+  std::vector<FieldTrial*> field_trials;
+
+  auto* field_trial_param_associator = FieldTrialParamAssociator::GetInstance();
+
   // TODO(crbug.com/794021) Remove this unique field trial name hack when there
   // is a cleaner solution.
   // Ensure that each call to this method uses a distinct field trial name.
   // Otherwise, nested calls might fail due to the shared FieldTrialList
   // already having the field trial registered.
   static int num_calls = 0;
-  ++num_calls;
-  std::string kTrialName =
-      "scoped_feature_list_trial_name" + base::NumberToString(num_calls);
-  std::string kTrialGroup = "scoped_feature_list_trial_group";
-
-  field_trial_override_ =
-      base::FieldTrialList::CreateFieldTrial(kTrialName, kTrialGroup);
-  DCHECK(field_trial_override_);
-  FieldTrialParamAssociator::GetInstance()->AssociateFieldTrialParams(
-      kTrialName, kTrialGroup, feature_parameters);
-  InitAndEnableFeatureWithFieldTrialOverride(feature,
-                                             field_trial_override_.get());
+  for (auto& enabled_feature : enabled_features) {
+    ++num_calls;
+    std::string trial_name =
+        "scoped_feature_list_trial_name" + base::NumberToString(num_calls);
+    scoped_refptr<FieldTrial> field_trial_override =
+        base::FieldTrialList::CreateFieldTrial(trial_name, kTrialGroup);
+    field_trial_overrides_.push_back(field_trial_override);
+    DCHECK(field_trial_overrides_.back());
+    field_trial_param_associator->AssociateFieldTrialParams(
+        trial_name, kTrialGroup, enabled_feature.params);
+    features.push_back(enabled_feature.feature);
+    field_trials.push_back(field_trial_override.get());
+  }
+
+  InitWithFeaturesAndFieldTrials(features, field_trials, disabled_features);
 }
 
 }  // namespace test

base/test/scoped_feature_list.h

@@ -37,6 +37,11 @@ class ScopedFeatureList final {
   ScopedFeatureList();
   ~ScopedFeatureList();
 
+  struct FeatureAndParams {
+    const Feature& feature;
+    const std::map<std::string, std::string>& params;
+  };
+
   // WARNING: This method will reset any globally configured features to their
   // default values, which can hide feature interaction bugs. Please use
   // sparingly.  https://crbug.com/713390
@@ -79,6 +84,15 @@ class ScopedFeatureList final {
       const Feature& feature,
       const std::map<std::string, std::string>& feature_parameters);
 
+  // Initializes and registers a FeatureList instance based on present
+  // FeatureList and overridden with the given enabled features and the
+  // specified field trial parameters, and the given disabled features.
+  // Note: This creates a scoped global field trial list if there is not
+  // currently one.
+  void InitWithFeaturesAndParameters(
+      const std::vector<FeatureAndParams>& enabled_features,
+      const std::vector<Feature>& disabled_features);
+
   // Initializes and registers a FeatureList instance based on present
   // FeatureList and overridden with single disabled feature.
   void InitAndDisableFeature(const Feature& feature);
@@ -112,7 +126,7 @@ class ScopedFeatureList final {
 
   bool init_called_ = false;
   std::unique_ptr<FeatureList> original_feature_list_;
-  scoped_refptr<FieldTrial> field_trial_override_;
+  std::vector<scoped_refptr<FieldTrial>> field_trial_overrides_;
   std::unique_ptr<base::FieldTrialList> field_trial_list_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedFeatureList);

base/test/scoped_feature_list_unittest.cc

@@ -181,6 +181,65 @@ TEST_F(ScopedFeatureListTest, OverrideWithFeatureParameters) {
   EXPECT_EQ("", GetFieldTrialParamValueByFeature(kTestFeature2, kParam));
 }
 
+TEST_F(ScopedFeatureListTest, OverrideMultipleFeaturesWithParameters) {
+  FieldTrialList field_trial_list(nullptr);
+  scoped_refptr<FieldTrial> trial1 =
+      FieldTrialList::CreateFieldTrial("foo1", "bar1");
+  const char kParam[] = "param_1";
+  const char kValue1[] = "value_1";
+  const char kValue2[] = "value_2";
+  std::map<std::string, std::string> parameters1;
+  parameters1[kParam] = kValue1;
+  std::map<std::string, std::string> parameters2;
+  parameters2[kParam] = kValue2;
+
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitFromCommandLine("TestFeature1<foo1,TestFeature2",
+                                    std::string());
+
+  // Check initial state.
+  ExpectFeatures("TestFeature1<foo1,TestFeature2", std::string());
+  EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature1));
+  EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature2));
+  EXPECT_EQ(trial1.get(), FeatureList::GetFieldTrial(kTestFeature1));
+  EXPECT_EQ(nullptr, FeatureList::GetFieldTrial(kTestFeature2));
+  EXPECT_EQ("", GetFieldTrialParamValueByFeature(kTestFeature1, kParam));
+  EXPECT_EQ("", GetFieldTrialParamValueByFeature(kTestFeature2, kParam));
+
+  {
+    // Override multiple features with parameters.
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeaturesAndParameters(
+        {{kTestFeature1, parameters1}, {kTestFeature2, parameters2}}, {});
+
+    EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature1));
+    EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature2));
+    EXPECT_EQ(kValue1, GetFieldTrialParamValueByFeature(kTestFeature1, kParam));
+    EXPECT_EQ(kValue2, GetFieldTrialParamValueByFeature(kTestFeature2, kParam));
+  }
+
+  {
+    // Override a feature with a parameter and disable another one.
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeaturesAndParameters({{kTestFeature1, parameters2}},
+                                                {kTestFeature2});
+
+    EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature1));
+    EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature2));
+    EXPECT_EQ(kValue2, GetFieldTrialParamValueByFeature(kTestFeature1, kParam));
+    EXPECT_EQ("", GetFieldTrialParamValueByFeature(kTestFeature2, kParam));
+  }
+
+  // Check that initial state is restored.
+  ExpectFeatures("TestFeature1<foo1,TestFeature2", std::string());
+  EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature1));
+  EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature2));
+  EXPECT_EQ(trial1.get(), FeatureList::GetFieldTrial(kTestFeature1));
+  EXPECT_EQ(nullptr, FeatureList::GetFieldTrial(kTestFeature2));
+  EXPECT_EQ("", GetFieldTrialParamValueByFeature(kTestFeature1, kParam));
+  EXPECT_EQ("", GetFieldTrialParamValueByFeature(kTestFeature2, kParam));
+}
+
 TEST_F(ScopedFeatureListTest, EnableFeatureOverrideDisable) {
   test::ScopedFeatureList feature_list1;
   feature_list1.InitWithFeatures({}, {kTestFeature1});