Change ScopedFeatureList to overrides FeatureList not reset

base/test/scoped_feature_list.cc

Index: base/test/scoped_feature_list.cc
diff --git a/base/test/scoped_feature_list.cc b/base/test/scoped_feature_list.cc
index f0f3f4edfba1f1d333ef3188836cf14273795739..da7e9ffa40d3bb22f9b98916daa8a89607ed1332 100644
--- a/base/test/scoped_feature_list.cc
+++ b/base/test/scoped_feature_list.cc
@@ -4,24 +4,78 @@
 
 #include "base/test/scoped_feature_list.h"
 
+#include <algorithm>
 #include <string>
+#include <vector>
+
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 
 namespace base {
 namespace test {
 
 namespace {
 
-static std::string GetFeatureString(
+std::vector<std::string> GetFeatureVector(
     const std::initializer_list<base::Feature>& features) {
-  std::string output;
+  std::vector<std::string> output;

[Alexei Svitkine (slow), 2017/04/24 15:55:11]

Can this be a StringPiece vector?

   for (const base::Feature& feature : features) {
-    if (!output.empty())
-      output += ",";
-    output += feature.name;
+    output.push_back(feature.name);
   }
+
   return output;
 }
 
+// Get feature name for feature string from FeatureList::GetFeatureOverrides.

[Ilya Sherman, 2017/04/24 19:49:51]

nit: I'd rephrase this to something like

  // Extracts a feature name from a feature state string.  For example, given
  // the input "*MyLovelyFeature<SomeFieldTrial", returns "MyLovelyFeature".

+StringPiece GetFeatureName(StringPiece feature) {
+  StringPiece feature_name = feature;
+
+  // Remove default info.
+  if (feature_name.starts_with("*"))
+    feature_name = feature_name.substr(1);
+
+  // Remove field_trial info.
+  std::size_t index = feature_name.find("<");
+  if (index != std::string::npos)
+    feature_name = feature_name.substr(0, index);
+
+  return feature_name;
+}
+
+struct Features {
+  std::vector<std::string> enabled_feature_list;
+  std::vector<std::string> disabled_feature_list;
+};
+
+// Given features from FeatureList::GetFeatureOverrides, add them to
+// merged_features if it is not existing in merged_features.

[Ilya Sherman, 2017/04/24 19:49:51]

The semantics of the params are not entirely obvious (at least, not to me), so
please document the expected semantics.

+void OverrideFeatures(const std::string& features,
+                      base::FeatureList::OverrideState override_state,
+                      Features* merged_features) {

[chaopeng, 2017/04/22 03:18:15]

Is this same as your comment?

[Ilya Sherman, 2017/04/24 19:49:51]

It's not really what I had in mind, but I guess this way is ok too.

+  std::vector<StringPiece> features_list =
+      SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+
+  for (StringPiece feature : features_list) {
+    StringPiece feature_name = GetFeatureName(feature);
+
+    if (std::count(merged_features->enabled_feature_list.begin(),

[Alexei Svitkine (slow), 2017/04/24 15:55:11]

Nit: Use ContainsValue() from base/stl_util.h

+                   merged_features->enabled_feature_list.end(),
+                   feature_name) > 0)
+      continue;
+
+    if (std::count(merged_features->disabled_feature_list.begin(),
+                   merged_features->disabled_feature_list.end(),
+                   feature_name) > 0)
+      continue;
+
+    if (override_state == base::FeatureList::OVERRIDE_ENABLE_FEATURE) {
+      merged_features->enabled_feature_list.push_back(feature.as_string());
+    } else {
+      merged_features->disabled_feature_list.push_back(feature.as_string());

[Alexei Svitkine (slow), 2017/04/24 15:55:11]

DCHECK that it's OVERRIDE_DISABLE_FEATURE?

+    }
+  }
+}
+
 }  // namespace
 
 ScopedFeatureList::ScopedFeatureList() {}
@@ -40,13 +94,6 @@ void ScopedFeatureList::Init() {
   InitWithFeatureList(std::move(feature_list));
 }
 
-void ScopedFeatureList::InitWithFeatures(
-    const std::initializer_list<base::Feature>& enabled_features,
-    const std::initializer_list<base::Feature>& disabled_features) {
-  InitFromCommandLine(GetFeatureString(enabled_features),
-                      GetFeatureString(disabled_features));
-}
-
 void ScopedFeatureList::InitWithFeatureList(
     std::unique_ptr<FeatureList> feature_list) {
   DCHECK(!original_feature_list_);
@@ -62,12 +109,38 @@ void ScopedFeatureList::InitFromCommandLine(
   InitWithFeatureList(std::move(feature_list));
 }
 
+void ScopedFeatureList::InitWithFeatures(
+    const std::initializer_list<base::Feature>& enabled_features,
+    const std::initializer_list<base::Feature>& disabled_features) {
+  Features merged_features;
+  merged_features.enabled_feature_list = GetFeatureVector(enabled_features);
+  merged_features.disabled_feature_list = GetFeatureVector(disabled_features);
+
+  base::FeatureList* feature_list = base::FeatureList::GetInstance();
+  if (feature_list) {
+    std::string enabled_features;
+    std::string disabled_features;
+    base::FeatureList::GetInstance()->GetFeatureOverrides(&enabled_features,
+                                                          &disabled_features);
+    OverrideFeatures(enabled_features,
+                     FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+                     &merged_features);
+    OverrideFeatures(disabled_features,
+                     FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE,
+                     &merged_features);
+  }
+
+  std::string enabled = JoinString(merged_features.enabled_feature_list, ",");
+  std::string disabled = JoinString(merged_features.disabled_feature_list, ",");
+  InitFromCommandLine(enabled, disabled);
+}
+
 void ScopedFeatureList::InitAndEnableFeature(const base::Feature& feature) {
-  InitFromCommandLine(feature.name, std::string());
+  InitWithFeatures({feature}, {});
 }
 
 void ScopedFeatureList::InitAndDisableFeature(const base::Feature& feature) {
-  InitFromCommandLine(std::string(), feature.name);
+  InitWithFeatures({}, {feature});
 }
 
 }  // namespace test