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..ccb8ad163d15f64fdc6a67fb3853509cd6e769b2 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/stl_util.h"
+#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;
   for (const base::Feature& feature : features) {
-    if (!output.empty())
-      output += ",";
-    output += feature.name;
+    output.push_back(feature.name);
   }
+
   return output;
 }
 
+// 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;
+};
+
+// features is from current FeatureList as a string concat features with comma.
+// override_state indicate features is enable list or disable list.
+// This method add feature in features to merged_features if it is not exist in
+// merged_features.

[Ilya Sherman, 2017/04/25 21:41:58]

nit: Suggested rephrasing:

// Merges previously-specified feature overrides with those passed into one of
// the Init() methods. |features| should be a list of features previously
// overridden to be in the |override_state|. |merged_features| should contain
// the enabled and disabled features passed into the Init() method, plus any
// overrides merged as a result of previous calls to this function.

Of course, feel free to refine this further.

+void OverrideFeatures(const std::string& features,
+                      base::FeatureList::OverrideState override_state,
+                      Features* merged_features) {
+  std::vector<StringPiece> features_list =
+      SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+
+  for (StringPiece feature : features_list) {
+    StringPiece feature_name = GetFeatureName(feature);
+
+    if (ContainsValue(merged_features->enabled_feature_list, feature_name) ||
+        ContainsValue(merged_features->disabled_feature_list, feature_name))
+      continue;
+
+    if (override_state == FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE) {
+      merged_features->enabled_feature_list.push_back(feature.as_string());
+    } else {
+      DCHECK(override_state ==

[Ilya Sherman, 2017/04/25 21:41:58]

nit: DCHECK_EQ

+             FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE);
+      merged_features->disabled_feature_list.push_back(feature.as_string());
+    }
+  }
+}
+
 }  // 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,39 @@ 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 enables;
+    std::string disables;

[chaopeng, 2017/04/25 21:21:07]

Re Alexei: I need to move this 2 line out of if-scope if I want to change
GetFeatureVector returns vector<StringPiece>.

[Ilya Sherman, 2017/04/25 21:41:58]

nit: Please restore the previous variable names of "enabled_features" and
"disabled_features".  They were clearer.

[Ilya Sherman, 2017/04/25 21:41:58]

Hmm, why do you need to move these in order to make that change?  I'm not
following the connection.

[chaopeng, 2017/04/26 01:04:39]

these names conflict with the function arguments. I rename them to current_*.

[chaopeng, 2017/04/26 01:04:39]

StringPiece is not alloc new memory but save a pointer to reuse the string's
memory.
The StringPiece we push_back at line 70 is from these two line and will
out-of-scope before JoinString if I change |struct Features| and
|GetFeatureVector| to using StringPiece.

[Ilya Sherman, 2017/04/26 07:19:20]

Ah, yes, that makes sense.  Thanks for the explanation.

I think it would be fine to hoist the two variable up above the if-stmt, with a
brief comment mentioning that it's necessary for StringPiece references to
remain valid.  I also don't feel strongly about using a StringPiece vector in
the first place; will defer to Alexei on that.

[Alexei Svitkine (slow), 2017/04/27 14:58:08]

Moving out of the if SGTM to me, with a corresponding comment.

+
+    base::FeatureList::GetInstance()->GetFeatureOverrides(&enables, &disables);
+    OverrideFeatures(enables,
+                     FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+                     &merged_features);
+    OverrideFeatures(disables,
+                     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