Move UnescapeValue to base/metrics/field_trial_params

After moving some functions from net// to base//, the UnescapeValue no
longer needs to depend on net// so it can be moved to
base/metrics/field_trial_params where it gets used. Also replace the
temporary NoOpDecodeFunc with the UnescapeValue as it's available now.

Bug: 1068052
Change-Id: I1b1790a3e1491a834822d017a5f2f4bfa34ffbec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298942
Commit-Queue: Weilun Shi <sweilun@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789696}

base/feature_list.cc

@@ -199,20 +199,6 @@ void FeatureList::InitializeFromCommandLine(
     const std::string& disable_features) {
   DCHECK(!initialized_);
 
-  // Process disabled features first, so that disabled ones take precedence over
-  // enabled ones (since RegisterOverride() uses insert()).
-  RegisterOverridesFromCommandLine(disable_features, OVERRIDE_DISABLE_FEATURE);
-  RegisterOverridesFromCommandLine(enable_features, OVERRIDE_ENABLE_FEATURE);
-
-  initialized_from_command_line_ = true;
-}
-
-void FeatureList::InitializeFromCommandLineWithFeatureParams(
-    const std::string& enable_features,
-    const std::string& disable_features,
-    FieldTrialParamsDecodeStringFunc decode_data_func) {
-  DCHECK(!initialized_);
-
   std::string parsed_enable_features;
   std::string force_fieldtrials;
   std::string force_fieldtrial_params;
@@ -222,20 +208,32 @@ void FeatureList::InitializeFromCommandLineWithFeatureParams(
   DCHECK(parse_enable_features_result) << StringPrintf(
       "The --%s list is unparsable or invalid, please check the format.",
       ::switches::kEnableFeatures);
-  bool associate_params_result = AssociateFieldTrialParamsFromString(
-      force_fieldtrial_params, decode_data_func);
-  DCHECK(associate_params_result) << StringPrintf(
-      "The field trial parameters part of the --%s list is invalid. Make sure "
-      "you %%-encode the following characters in param values: %%:/.,",
-      ::switches::kEnableFeatures);
 
-  bool create_trials_result =
-      FieldTrialList::CreateTrialsFromString(force_fieldtrials);
-  DCHECK(create_trials_result)
-      << StringPrintf("Invalid field trials are specified in --%s.",
-                      ::switches::kEnableFeatures);
+  // Only create field trials when field_trial_list is available. Some tests
+  // don't have field trial list available.
+  if (FieldTrialList::GetInstance()) {
+    bool associate_params_result = AssociateFieldTrialParamsFromString(
+        force_fieldtrial_params, &UnescapeValue);
+    DCHECK(associate_params_result) << StringPrintf(
+        "The field trial parameters part of the --%s list is invalid. Make "
+        "sure "
+        "you %%-encode the following characters in param values: %%:/.,",
+        ::switches::kEnableFeatures);
+
+    bool create_trials_result =
+        FieldTrialList::CreateTrialsFromString(force_fieldtrials);
+    DCHECK(create_trials_result)
+        << StringPrintf("Invalid field trials are specified in --%s.",
+                        ::switches::kEnableFeatures);
+  }
+
+  // Process disabled features first, so that disabled ones take precedence over
+  // enabled ones (since RegisterOverride() uses insert()).
+  RegisterOverridesFromCommandLine(disable_features, OVERRIDE_DISABLE_FEATURE);
+  RegisterOverridesFromCommandLine(parsed_enable_features,
+                                   OVERRIDE_ENABLE_FEATURE);
 
-  InitializeFromCommandLine(parsed_enable_features, disable_features);
+  initialized_from_command_line_ = true;
 }
 
 void FeatureList::InitializeFromSharedMemory(
@@ -461,11 +459,6 @@ void FeatureList::RestoreInstanceForTesting(
   g_feature_list_instance = instance.release();
 }
 
-// static
-std::string FeatureList::NoOpDecodeFunc(const std::string& str) {
-  return str;
-}
-
 void FeatureList::FinalizeInitialization() {
   DCHECK(!initialized_);
   // Store the field trial list pointer for DCHECKing.

base/feature_list.h

@@ -128,25 +128,12 @@ class BASE_EXPORT FeatureList {
   using FeatureOverrideInfo =
       std::pair<const std::reference_wrapper<const Feature>, OverrideState>;
 
-  // Initializes feature overrides via command-line flags |enable_features| and
-  // |disable_features|, each of which is a comma-separated list of features to
-  // enable or disable, respectively. If a feature appears on both lists, then
-  // it will be disabled. If a list entry has the format "FeatureName<TrialName"
-  // then this initialization will also associate the feature state override
-  // with the named field trial, if it exists. If a feature name is prefixed
-  // with the '*' character, it will be created with OVERRIDE_USE_DEFAULT -
-  // which is useful for associating with a trial while using the default state.
-  // Must only be invoked during the initialization phase (before
-  // FinalizeInitialization() has been called).
-  void InitializeFromCommandLine(const std::string& enable_features,
-                                 const std::string& disable_features);
-
   // Initializes feature overrides via command-line flags |enable_features| and
   // |disable_features|, each of which is a comma-separated list of features to
   // enable or disable, respectively. This function also allows users to set
-  // feature's field trial params via |enable_features|. |decode_data_func|
-  // allows specifying a custom decoding function. Must only be invoked during
-  // the initialization phase (before FinalizeInitialization() has been called).
+  // feature's field trial params via |enable_features|. Must only be invoked
+  // during the initialization phase (before FinalizeInitialization() has been
+  // called).
   //
   // If a feature appears on both lists, then it will be disabled. If
   // a list entry has the format "FeatureName<TrialName" then this
@@ -162,10 +149,8 @@ class BASE_EXPORT FeatureList {
   // If a feature name is prefixed with the '*' character, it will be created
   // with OVERRIDE_USE_DEFAULT - which is useful for associating with a trial
   // while using the default state.
-  void InitializeFromCommandLineWithFeatureParams(
-      const std::string& enable_features,
-      const std::string& disable_features,
-      FieldTrialParamsDecodeStringFunc decode_data_func);
+  void InitializeFromCommandLine(const std::string& enable_features,
+                                 const std::string& disable_features);
 
   // Initializes feature overrides through the field trial allocator, which
   // we're using to store the feature names, their override state, and the name
@@ -279,10 +264,6 @@ class BASE_EXPORT FeatureList {
   // to support base::test::ScopedFeatureList helper class.
   static void RestoreInstanceForTesting(std::unique_ptr<FeatureList> instance);
 
-  // TODO(crbug.com/1068052): Port the UnescapeValue function and cleanup this.
-  // A no-op function that takes a string and returns it.
-  static std::string NoOpDecodeFunc(const std::string& str);
-
  private:
   FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity);
   FRIEND_TEST_ALL_PREFIXES(FeatureListTest,

base/feature_list_unittest.cc

@@ -119,8 +119,7 @@ TEST_F(FeatureListTest, InitializeFromCommandLineWithFeatureParams) {
     SCOPED_TRACE(test_case.enable_features);
 
     auto feature_list = std::make_unique<FeatureList>();
-    feature_list->InitializeFromCommandLineWithFeatureParams(
-        test_case.enable_features, "", &FeatureList::NoOpDecodeFunc);
+    feature_list->InitializeFromCommandLine(test_case.enable_features, "");
     test::ScopedFeatureList scoped_feature_list;
     scoped_feature_list.InitWithFeatureList(std::move(feature_list));
 

base/metrics/field_trial_params.cc

@@ -11,12 +11,19 @@
 #include "base/feature_list.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/field_trial_param_associator.h"
+#include "base/strings/escape.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 
 namespace base {
 
+std::string UnescapeValue(const std::string& value) {
+  return UnescapeURLComponent(
+      value, UnescapeRule::PATH_SEPARATORS |
+                 UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS);
+}
+
 bool AssociateFieldTrialParams(const std::string& trial_name,
                                const std::string& group_name,
                                const FieldTrialParams& params) {

base/metrics/field_trial_params.h

@@ -23,6 +23,11 @@ typedef std::map<std::string, std::string> FieldTrialParams;
 // Param string decoding function for AssociateFieldTrialParamsFromString().
 typedef std::string (*FieldTrialParamsDecodeStringFunc)(const std::string& str);
 
+// Unescapes special characters from the given string. Used in
+// AssociateFieldTrialParamsFromString() as one of the feature params decoding
+// functions.
+BASE_EXPORT std::string UnescapeValue(const std::string& value);
+
 // Associates the specified set of key-value |params| with the field trial
 // specified by |trial_name| and |group_name|. Fails and returns false if the
 // specified field trial already has params associated with it or the trial

base/test/scoped_feature_list.cc

@@ -179,8 +179,7 @@ void ScopedFeatureList::InitFromCommandLine(
     const std::string& enable_features,
     const std::string& disable_features) {
   std::unique_ptr<FeatureList> feature_list(new FeatureList);
-  feature_list->InitializeFromCommandLineWithFeatureParams(
-      enable_features, disable_features, &FeatureList::NoOpDecodeFunc);
+  feature_list->InitializeFromCommandLine(enable_features, disable_features);
   InitWithFeatureList(std::move(feature_list));
 }
 

components/variations/field_trial_config/field_trial_util.cc

@@ -167,19 +167,14 @@ void ChooseExperiment(
 
 }  // namespace
 
-std::string UnescapeValue(const std::string& value) {
-  return net::UnescapeURLComponent(
-      value, net::UnescapeRule::PATH_SEPARATORS |
-                 net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS);
-}
-
 std::string EscapeValue(const std::string& value) {
-  // This needs to be the inverse of UnescapeValue in the anonymous namespace
-  // above.
+  // This needs to be the inverse of UnescapeValue in
+  // base/metrics/field_trial_params.
   std::string net_escaped_str =
       net::EscapeQueryParamValue(value, true /* use_plus */);
 
-  // net doesn't escape '.' and '*' but UnescapeValue() covers those cases.
+  // net doesn't escape '.' and '*' but base::UnescapeValue() covers those
+  // cases.
   std::string escaped_str;
   escaped_str.reserve(net_escaped_str.length());
   for (const char ch : net_escaped_str) {
@@ -195,7 +190,7 @@ std::string EscapeValue(const std::string& value) {
 
 bool AssociateParamsFromString(const std::string& varations_string) {
   return base::AssociateFieldTrialParamsFromString(varations_string,
-                                                   &UnescapeValue);
+                                                   &base::UnescapeValue);
 }
 
 void AssociateParamsFromFieldTrialConfig(

components/variations/field_trial_config/field_trial_util_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -903,6 +904,7 @@ TEST_F(FieldTrialUtilTest, TestEscapeValue) {
   EXPECT_EQ(escaped_str.find(','), std::string::npos);
   EXPECT_EQ(escaped_str.find('*'), std::string::npos);
 
-  EXPECT_EQ(str, UnescapeValue(escaped_str));
+  // Make sure the EscapeValue function is the inverse of base::UnescapeValue.
+  EXPECT_EQ(str, base::UnescapeValue(escaped_str));
 }
 }  // namespace variations

components/variations/service/variations_field_trial_creator.cc

@@ -509,9 +509,9 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
                                        switches::kForceDisableVariationIds));
   }
 
-  feature_list->InitializeFromCommandLineWithFeatureParams(
+  feature_list->InitializeFromCommandLine(
       command_line->GetSwitchValueASCII(kEnableFeatures),
-      command_line->GetSwitchValueASCII(kDisableFeatures), &UnescapeValue);
+      command_line->GetSwitchValueASCII(kDisableFeatures));
 
   // This needs to happen here: After the InitializeFromCommandLine() call,
   // because the explicit cmdline --disable-features and --enable-features