Change ScopedFeatureList to overrides FeatureList not reset

base/test/scoped_feature_list.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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;
 }
 
 }  // namespace
 
 ScopedFeatureList::ScopedFeatureList() {}
 
 ScopedFeatureList::~ScopedFeatureList() {
   if (original_feature_list_) {
     base::FeatureList::ClearInstanceForTesting();
     base::FeatureList::RestoreInstanceForTesting(
         std::move(original_feature_list_));
   }
 }
 
 void ScopedFeatureList::Init() {
   std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
   feature_list->InitializeFromCommandLine(std::string(), std::string());
   InitWithFeatureList(std::move(feature_list));
 }
 
 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(
     std::unique_ptr<FeatureList> feature_list) {

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

Is this method really needed, or could it be replaced by calls to other Init()
functions?  It seems like an unnecessary way to circumvent any previously set
FeatureList state.

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

This method has lot calls. I think it is better to have a separate CL to remove
it. 
Do we still provider methods for reset features in case developers want only
enable/disable the features they need?

   DCHECK(!original_feature_list_);
   original_feature_list_ = base::FeatureList::ClearInstanceForTesting();
   base::FeatureList::SetInstance(std::move(feature_list));
 }
 
 void ScopedFeatureList::InitFromCommandLine(
     const std::string& enable_features,
     const std::string& disable_features) {

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

Could this either be removed or call into InitWithFeatures()?  As above, it
seems like an unnecessary escape from previously set FeatureList state.

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

same as below.

   std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
   feature_list->InitializeFromCommandLine(enable_features, disable_features);
   InitWithFeatureList(std::move(feature_list));
 }
 
 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
 }  // namespace base