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(

[Ilya Sherman, 2017/04/21 22:43:33]

nit: s/String/Vector

[Ilya Sherman, 2017/04/21 22:43:34]

nit: No need for "static" within an anonymous namespace.  (Applies to the
functions defined below as well.)

     const std::initializer_list<base::Feature>& features) {
-  std::string output;
-  for (const base::Feature& feature : features) {
-    if (!output.empty())
-      output += ",";
-    output += feature.name;
+  std::vector<std::string> output;
+  for (const base::Feature& feature : features)

[Ilya Sherman, 2017/04/21 22:43:33]

nit: Please add curly braces.  I'm not sure whether they're optional for loops
as well as if-stmts; but within the metrics code, we prefer to always include
them for loop constructs.

+    output.push_back(feature.name);
+
+  return output;
+}
+
+static StringPiece GetFeatureName(const StringPiece& feature) {

[Ilya Sherman, 2017/04/21 22:43:34]

Please add documentation for this function.

[Ilya Sherman, 2017/04/21 22:43:34]

Per the StringPiece documentation, it's preferable to pass StringPieces by value
rather than const-ref:
https://cs.chromium.org/chromium/src/base/strings/string_piece.h?l=14-20

+  StringPiece feature_name = feature;
+
+  // Remove started *.

[Ilya Sherman, 2017/04/21 22:43:33]

This just repeats what the code does.  Please instead document what the idea is,
i.e. what the asterisk means.

+  if (feature_name.starts_with("*"))
+    feature_name.substr(1);

[Ilya Sherman, 2017/04/21 22:43:34]

Did you mean to assign this back to feature_name?  If so, please add test
coverage.

+
+  // 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;
+}
+
+// Given features from FeatureList::GetFeatureOverrides, add them to overrides
+// if it is not existing in already_overridden_feature_names or overrides.

[Ilya Sherman, 2017/04/21 22:43:33]

If you take my advice about passing a unioned set as the second param, you can
simplify this implementation not to separately consider the overrides list.

+static void OverrideFeatures(
+    const std::string& features,
+    std::vector<std::string>* already_overridden_feature_names,
+    std::vector<std::string>* overrides) {
+  std::vector<StringPiece> features_list =
+      SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+
+  for (StringPiece& feature : features_list) {

[Ilya Sherman, 2017/04/21 22:43:34]

nit: Chromium strongly frowns upon non-const references.  Since it's preferable
to pass StringPiece by value, this should probably be a copy as well.  But if
you were to want a reference, it should be a const-ref.

+    StringPiece feature_name = GetFeatureName(feature);
+
+    if (std::count(already_overridden_feature_names->begin(),
+                   already_overridden_feature_names->end(), feature_name) > 0)
+      continue;
+
+    if (std::count(overrides->begin(), overrides->end(), feature_name) > 0)
+      continue;
+
+    overrides->push_back(feature.as_string());
   }
-  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) {
-  InitFromCommandLine(GetFeatureString(enabled_features),
-                      GetFeatureString(disabled_features));
-}
-
 void ScopedFeatureList::InitWithFeatureList(
     std::unique_ptr<FeatureList> feature_list) {
   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) {
   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());
+  InitWithFeatures({feature}, {});
 }
 
 void ScopedFeatureList::InitAndDisableFeature(const base::Feature& feature) {
-  InitFromCommandLine(std::string(), feature.name);
+  InitWithFeatures({}, {feature});
+}
+
+void ScopedFeatureList::InitWithFeatures(

[Ilya Sherman, 2017/04/21 22:43:34]

Please position this within the .cc file in the same order as it appears in the
.h file.

+    const std::initializer_list<base::Feature>& enabled_features,
+    const std::initializer_list<base::Feature>& disabled_features) {
+  std::vector<std::string> merged_enabled_features =
+      GetFeatureString(enabled_features);
+  std::vector<std::string> merged_disabled_features =
+      GetFeatureString(disabled_features);

[Ilya Sherman, 2017/04/21 22:43:34]

Please construct a set containing the union of these features, and pass it as
the second param to OverrideFeatures.  In addition to increasing code clarity
(IMO), it also guards against the case that a feature is being overridden to be
enabled in a separate state than it was in the existing FeatureList.  (e.g. with
different field trial params)

+
+  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, &merged_disabled_features,

[Ilya Sherman, 2017/04/21 22:43:34]

The second param is not an outparam, so please pass it by const-reference rather
than as a pointer.

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