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..3425df614c8fded46c5ac4c8fe123f5f9ab5c09b 100644
--- a/base/test/scoped_feature_list.cc
+++ b/base/test/scoped_feature_list.cc
@@ -4,20 +4,23 @@
 
 #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(
+static std::vector<std::string> GetFeatureString(
     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;
 }
@@ -43,8 +46,8 @@ void ScopedFeatureList::Init() {
 void ScopedFeatureList::InitWithFeatures(
     const std::initializer_list<base::Feature>& enabled_features,
     const std::initializer_list<base::Feature>& disabled_features) {

[Ilya Sherman, 2017/04/20 20:53:07]

Could this still be the main entry point?  The API is pretty much identical to
InitWithFeatureListAndOverrides().

[chaopeng, 2017/04/21 02:36:21]

Yes.

-  InitFromCommandLine(GetFeatureString(enabled_features),
-                      GetFeatureString(disabled_features));
+  InitWithFeatureListAndOverrides(GetFeatureString(enabled_features),
+                                  GetFeatureString(disabled_features));
 }
 
 void ScopedFeatureList::InitWithFeatureList(
@@ -63,11 +66,67 @@ void ScopedFeatureList::InitFromCommandLine(
 }
 
 void ScopedFeatureList::InitAndEnableFeature(const base::Feature& feature) {
-  InitFromCommandLine(feature.name, std::string());
+  std::vector<std::string> feature_list{feature.name};
+  InitWithFeatureListAndOverrides(feature_list, std::vector<std::string>());

[Ilya Sherman, 2017/04/20 20:53:07]

nit: I think this could be simplified to

InitWithFeatures({feature}, {});

A similar transformation could be made below.

[chaopeng, 2017/04/21 02:36:21]

Done.

 }
 
 void ScopedFeatureList::InitAndDisableFeature(const base::Feature& feature) {
-  InitFromCommandLine(std::string(), feature.name);
+  std::vector<std::string> feature_list{feature.name};
+  InitWithFeatureListAndOverrides(std::vector<std::string>(), feature_list);
+}
+
+void OverrideFeature(const std::string& features,

[Ilya Sherman, 2017/04/20 20:53:06]

Please add documentation for this function.

[Ilya Sherman, 2017/04/20 20:53:07]

nit: s/Feature/Features

[chaopeng, 2017/04/21 02:36:21]

Done.

+                     std::vector<std::string>* check_list,
+                     std::vector<std::string>* add_to_list) {

[Ilya Sherman, 2017/04/20 20:53:07]

I'd suggest writing this method like so:

void AddFeatureOverrides(const std::string& features,
                         const std::set<StringPiece>&
already_overridden_feature_names,
                         std::vector<std::string>* overrides) {
  std::vector<StringPiece> features_list =
      SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
  for (const StringPiece& feature : features_list) {
    // Extract out a GetFeatureName() function.
    StringPiece feature_name = GetFeatureName(feature);
    if (already_overridden_feature_names.count(feature_name)) continue;
    overrides->push_back(feature);
  }
}

The already_overridden_feature_names set could contain both the enabled and the
disabled features -- it doesn't matter in which state they were overridden.

Writing the code this way obviates the need for some of the comments below, but
please still pay attention to my questions about correctness, and add test
coverage as needed to address them.

[chaopeng, 2017/04/21 02:36:21]

Done.

+  std::vector<StringPiece> features_list =
+      SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+
+  for (StringPiece& feature : features_list) {
+    // Start with * means use default.
+    if (feature.starts_with("*"))
+      return;

[Ilya Sherman, 2017/04/20 20:53:07]

Should this be a continue instead?  (If so, please add test coverage for this.)

Actually, if field trial state isn't dropped, it seems more appropriate to keep
these features.  And it's not clear to me why we'd want to drop field trial
state.

[chaopeng, 2017/04/21 02:36:21]

Yes, we should keep it.

+
+    // Remove field_trial info.
+    StringPiece feature_name = feature;
+    std::size_t index = feature.find("<");
+    if (index != std::string::npos)
+      feature_name = feature.substr(0, index);

[Ilya Sherman, 2017/04/20 20:53:07]

Hmm, why is the field trial info being removed?  Shouldn't it affect the feature
state sometimes?

[chaopeng, 2017/04/21 02:36:21]

Not remove it now.

+
+    if (std::any_of(

[Ilya Sherman, 2017/04/20 20:53:07]

nit: std::find?

[chaopeng, 2017/04/21 02:36:21]

change to count

+            check_list->begin(), check_list->end(),
+            [&feature_name](std::string& s) { return feature_name == s; }))
+      return;

[Ilya Sherman, 2017/04/20 20:53:07]

Ditto regarding continue vs. return.

[chaopeng, 2017/04/21 02:36:22]

Done.

+
+    if (std::any_of(
+            add_to_list->begin(), add_to_list->end(),
+            [&feature_name](std::string& s) { return feature_name == s; }))
+      return;

[Ilya Sherman, 2017/04/20 20:53:07]

Ditto regarding continue vs. return.

[chaopeng, 2017/04/21 02:36:21]

Done.

+
+    add_to_list->push_back(feature_name.as_string());
+  }
+}

[Ilya Sherman, 2017/04/20 20:53:07]

nit: Please move this into the anonymous namespace, since it's a private
implementation detail.

[chaopeng, 2017/04/21 02:36:21]

Done.

+
+void ScopedFeatureList::InitWithFeatureListAndOverrides(
+    const std::vector<std::string>& override_enabled_features,
+    const std::vector<std::string>& override_disabled_features) {
+  std::vector<std::string> merged_enabled_features(override_enabled_features);
+  std::vector<std::string> merged_disabled_features(override_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);
+    OverrideFeature(enabled_features, &merged_disabled_features,
+                    &merged_enabled_features);
+    OverrideFeature(disabled_features, &merged_disabled_features,
+                    &merged_enabled_features);

[Ilya Sherman, 2017/04/20 20:53:07]

I think the latter two args are swapped?  Please add test coverage for this if
I'm not mistaken.

[chaopeng, 2017/04/21 02:36:21]

Done.

+  }
+
+  std::string enabled = JoinString(merged_enabled_features, ",");
+  std::string disabled = JoinString(merged_disabled_features, ",");
+  InitFromCommandLine(enabled, disabled);
 }
 
 }  // namespace test