Use base::feature for data saver server experiments

Deprecate the use of field trial, and use a feature instead.

Bug: 955544
Change-Id: I32c069d0e5960af90256382132db93250613d754
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1610582Reviewed-by: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659681}

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -307,6 +307,8 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
 
   size_t count_proxy_server_requests_received_ = 0u;
 
+  base::test::ScopedFeatureList scoped_feature_list_;
+
  private:
   std::unique_ptr<net::test_server::HttpResponse> GetConfigResponse(
       const net::test_server::HttpRequest& request) {
@@ -320,7 +322,6 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
 
   ClientConfig config_;
   std::unique_ptr<base::RunLoop> config_run_loop_;
-  base::test::ScopedFeatureList scoped_feature_list_;
   base::test::ScopedFeatureList param_feature_list_;
   net::EmbeddedTestServer secure_proxy_check_server_;
   net::EmbeddedTestServer config_server_;
@@ -695,6 +696,49 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyExpBrowsertest,
   EXPECT_LE(1u, count_proxy_server_requests_received_);
 }
 
+class DataReductionProxyExpFeatureBrowsertest
+    : public DataReductionProxyBrowsertest {
+ public:
+  void SetUp() override {
+    std::map<std::string, std::string> field_trial_params;
+    field_trial_params[data_reduction_proxy::params::
+                           GetDataSaverServerExperimentsOptionName()] =
+        experiment_name;
+
+    scoped_feature_list_.InitWithFeaturesAndParameters(
+        {{data_reduction_proxy::features::kDataReductionProxyServerExperiments,
+          {field_trial_params}},
+         {data_reduction_proxy::features::
+              kDataReductionProxyEnabledWithNetworkService,
+          {}}},
+        {});
+
+    InProcessBrowserTest::SetUp();
+  }
+
+  const std::string experiment_name = "foo_feature_experiment";
+};
+
+IN_PROC_BROWSER_TEST_F(DataReductionProxyExpFeatureBrowsertest,
+                       ChromeProxyExpHeaderSet) {
+  expect_exp_value_in_request_header_ = experiment_name;
+
+  net::EmbeddedTestServer proxy_server;
+  proxy_server.RegisterRequestMonitor(base::BindRepeating(
+      &DataReductionProxyBrowsertest::MonitorAndVerifyRequestsToProxyServer,
+      base::Unretained(this)));
+  proxy_server.RegisterRequestHandler(
+      base::BindRepeating(&BasicResponse, kPrimaryResponse));
+  ASSERT_TRUE(proxy_server.Start());
+  SetConfig(CreateConfigForServer(proxy_server));
+  // A network change forces the config to be fetched.
+  SimulateNetworkChange(network::mojom::ConnectionType::CONNECTION_3G);
+  WaitForConfig();
+
+  ui_test_utils::NavigateToURL(browser(), GURL("http://does.not.resolve/foo"));
+  EXPECT_LE(1u, count_proxy_server_requests_received_);
+}
+
 class DataReductionProxyBrowsertestWithNetworkService
     : public DataReductionProxyBrowsertest {
  public:

chrome/browser/previews/previews_lite_page_navigation_throttle_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/test/scoped_task_environment.h"
 #include "chrome/browser/previews/previews_lite_page_decider.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_features.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
 #include "components/previews/core/previews_features.h"
@@ -129,22 +130,17 @@ TEST(PreviewsLitePageNavigationThrottleTest, TestGetPreviewsURL) {
         test_case.experiment_cmd_line);
 
     base::test::ScopedFeatureList scoped_feature_list;
-    scoped_feature_list.InitAndEnableFeatureWithParameters(
-        previews::features::kLitePageServerPreviews,
-        {{"previews_host", test_case.previews_host}});
-
-    base::FieldTrialList::CreateFieldTrial(
-        data_reduction_proxy::params::
-            GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-        "enabled");
-    std::map<std::string, std::string> server_experiment;
-    server_experiment[data_reduction_proxy::params::
-                          GetDataSaverServerExperimentsOptionName()] =
+    std::map<std::string, std::string> server_experiment_params;
+    server_experiment_params[data_reduction_proxy::params::
+                                 GetDataSaverServerExperimentsOptionName()] =
         test_case.experiment_variation;
-    variations::AssociateVariationParams(
-        data_reduction_proxy::params::
-            GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-        "enabled", server_experiment);
+
+    scoped_feature_list.InitWithFeaturesAndParameters(
+        {{data_reduction_proxy::features::kDataReductionProxyServerExperiments,
+          {server_experiment_params}},
+         {previews::features::kLitePageServerPreviews,
+          {{"previews_host", test_case.previews_host}}}},
+        {});
 
     EXPECT_EQ(PreviewsLitePageNavigationThrottle::GetPreviewsURLForURL(
                   GURL(test_case.original_url)),

components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc

@@ -14,10 +14,12 @@
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/strings/string_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "build/build_config.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_features.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
 #include "components/variations/variations_associated_data.h"
@@ -260,7 +262,6 @@ TEST_F(DataReductionProxyRequestOptionsTest, ParseExperiments) {
 
 TEST_F(DataReductionProxyRequestOptionsTest, ParseExperimentsFromFieldTrial) {
   const char kFieldTrialGroupFoo[] = "enabled_foo";
-  const char kFieldTrialGroupBar[] = "enabled_bar";
   const char kExperimentFoo[] = "foo";
   const char kExperimentBar[] = "bar";
   const struct {
@@ -274,36 +275,36 @@ TEST_F(DataReductionProxyRequestOptionsTest, ParseExperimentsFromFieldTrial) {
       {"disabled_group", kExperimentFoo, false, kExperimentFoo},
       // Valid field trial groups should pick from field trial.
       {kFieldTrialGroupFoo, std::string(), false, kExperimentFoo},
-      {kFieldTrialGroupBar, std::string(), false, kExperimentBar},
       {kFieldTrialGroupFoo, std::string(), true, std::string()},
-      {kFieldTrialGroupBar, std::string(), true, std::string()},
       // Experiments from command line switch should override.
       {kFieldTrialGroupFoo, kExperimentBar, false, kExperimentBar},
-      {kFieldTrialGroupBar, kExperimentFoo, false, kExperimentFoo},
       {kFieldTrialGroupFoo, kExperimentBar, false, kExperimentBar},
-      {kFieldTrialGroupBar, kExperimentFoo, false, kExperimentFoo},
       {kFieldTrialGroupFoo, kExperimentBar, true, std::string()},
-      {kFieldTrialGroupBar, kExperimentFoo, true, std::string()},
   };
 
-  std::map<std::string, std::string> server_experiment_foo,
-      server_experiment_bar;
+  std::map<std::string, std::string> server_experiment_foo;
 
   server_experiment_foo[params::GetDataSaverServerExperimentsOptionName()] =
       kExperimentFoo;
-  server_experiment_bar[params::GetDataSaverServerExperimentsOptionName()] =
-      kExperimentBar;
-  ASSERT_TRUE(variations::AssociateVariationParams(
-      params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-      kFieldTrialGroupFoo, server_experiment_foo));
-  ASSERT_TRUE(variations::AssociateVariationParams(
-      params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-      kFieldTrialGroupBar, server_experiment_bar));
 
   for (const auto& test : tests) {
+    // Remove all related switches first to reset the test state.
+    base::CommandLine::ForCurrentProcess()->RemoveSwitch(
+        data_reduction_proxy::switches::kDataReductionProxyExperiment);
+    base::CommandLine::ForCurrentProcess()->RemoveSwitch(
+        switches::kDataReductionProxyServerExperimentsDisabled);
+
     std::string expected_experiments;
 
-    base::CommandLine::ForCurrentProcess()->InitFromArgv(0, nullptr);
+    base::test::ScopedFeatureList scoped_feature_list;
+
+    if (test.field_trial_group != "disabled_group") {
+      scoped_feature_list.InitWithFeaturesAndParameters(
+          {{data_reduction_proxy::features::
+                kDataReductionProxyServerExperiments,
+            {server_experiment_foo}}},
+          {});
+    }
 
     if (test.disable_server_experiments_via_flag) {
       base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
@@ -312,15 +313,9 @@ TEST_F(DataReductionProxyRequestOptionsTest, ParseExperimentsFromFieldTrial) {
       base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
           data_reduction_proxy::switches::kDataReductionProxyExperiment,
           test.command_line_experiment);
-      base::CommandLine::ForCurrentProcess()->RemoveSwitch(
-          switches::kDataReductionProxyServerExperimentsDisabled);
     }
 
     std::string expected_header;
-    base::FieldTrialList field_trial_list(nullptr);
-    base::FieldTrialList::CreateFieldTrial(
-        params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-        test.field_trial_group);
 
     if (!test.expected_experiment.empty())
       expected_experiments = test.expected_experiment;
@@ -342,14 +337,13 @@ TEST_F(DataReductionProxyRequestOptionsTest, TestExperimentPrecedence) {
   // Field trial has the lowest priority.
   std::map<std::string, std::string> server_experiment;
   server_experiment[params::GetDataSaverServerExperimentsOptionName()] = "foo";
-  ASSERT_TRUE(variations::AssociateVariationParams(
-      params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-      "enabled", server_experiment));
-
-  base::FieldTrialList field_trial_list(nullptr);
-  base::FieldTrialList::CreateFieldTrial(
-      params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-      "enabled");
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeaturesAndParameters(
+      {{data_reduction_proxy::features::kDataReductionProxyServerExperiments,
+        {server_experiment}}},
+      {});
+
   std::string expected_experiments = "foo";
   std::string expected_header;
   SetHeaderExpectations(std::string(), kClientStr, kExpectedBuild,

components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc

@@ -61,5 +61,11 @@ const base::Feature kDataReductionProxyDisableProxyFailedWarmup{
     "DataReductionProxyDisableProxyFailedWarmup",
     base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Enables server experiments run jointly with Chrome. The experiment
+// id should be specified using the finch parameter
+// params::GetDataSaverServerExperimentsOptionName().
+const base::Feature kDataReductionProxyServerExperiments{
+    "DataReductionProxyServerExperiments", base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/data_reduction_proxy_features.h

@@ -19,6 +19,7 @@ extern const base::Feature kDataSaverUseOnDeviceSafeBrowsing;
 extern const base::Feature kDataReductionProxyBlockOnBadGatewayResponse;
 extern const base::Feature kDataReductionProxyPopulatePreviewsPageIDToPingback;
 extern const base::Feature kDataReductionProxyDisableProxyFailedWarmup;
+extern const base::Feature kDataReductionProxyServerExperiments;
 
 }  // namespace features
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc

@@ -50,10 +50,6 @@ const char kClientConfigURL[] =
 const char kPingbackURL[] =
     "https://datasaver.googleapis.com/v1/metrics:recordPageloadMetrics";
 
-// The name of the server side experiment field trial.
-const char kServerExperimentsFieldTrial[] =
-    "DataReductionProxyServerExperiments";
-
 // LitePage black list version.
 const char kLitePageBlackListVersion[] = "lite-page-blacklist-version";
 
@@ -86,13 +82,6 @@ bool CanShowAndroidLowMemoryDevicePromo() {
   return false;
 }
 
-// Returns true if this client is part of the field trial that should enable
-// server experiments for the data reduction proxy.
-bool IsIncludedInServerExperimentsFieldTrial() {
-  return base::FieldTrialList::FindFullName(kServerExperimentsFieldTrial)
-             .find(kDisabled) != 0;
-}
-
 }  // namespace
 
 namespace data_reduction_proxy {
@@ -364,16 +353,15 @@ std::string GetDataSaverServerExperiments() {
   if (!cmd_line_experiment.empty())
     return cmd_line_experiment;
 
-  // Next, check if the experiment is set using the field trial.
-  if (!IsIncludedInServerExperimentsFieldTrial())
+  // First check if the feature is enabled.
+  if (!base::FeatureList::IsEnabled(
+          features::kDataReductionProxyServerExperiments)) {
     return std::string();
-  return variations::GetVariationParamValue(kServerExperimentsFieldTrial,
-                                            kExperimentsOption);
+  }
+  return base::GetFieldTrialParamValueByFeature(
+      features::kDataReductionProxyServerExperiments, kExperimentsOption);
 }
 
-const char* GetDataSaverServerExperimentsFieldTrialNameForTesting() {
-  return kServerExperimentsFieldTrial;
-}
 
 GURL GetSecureProxyCheckURL() {
   std::string secure_proxy_check_url =

components/data_reduction_proxy/core/common/data_reduction_proxy_params.h

@@ -110,9 +110,6 @@ std::string GetDataSaverServerExperimentsOptionName();
 // experiment is enabled.
 std::string GetDataSaverServerExperiments();
 
-// Returns the name of the server side experiment field trial.
-const char* GetDataSaverServerExperimentsFieldTrialNameForTesting();
-
 // Returns the URL to check to decide if the secure proxy origin should be
 // used.
 GURL GetSecureProxyCheckURL();

components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc

@@ -146,9 +146,8 @@ TEST_F(DataReductionProxyParamsTest, AreServerExperimentsEnabled) {
   };
 
   for (const auto& test : tests) {
-    base::FieldTrialParamAssociator::GetInstance()->ClearAllParamsForTesting();
+    base::test::ScopedFeatureList scoped_feature_list;
 
-    base::FieldTrialList field_trial_list(nullptr);
     std::map<std::string, std::string> variation_params;
     std::string exp_name;
 
@@ -156,12 +155,12 @@ TEST_F(DataReductionProxyParamsTest, AreServerExperimentsEnabled) {
       exp_name = "foobar";
       variation_params[params::GetDataSaverServerExperimentsOptionName()] =
           exp_name;
-      ASSERT_TRUE(base::AssociateFieldTrialParams(
-          params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-          test.trial_group_value, variation_params));
-      base::FieldTrialList::CreateFieldTrial(
-          params::GetDataSaverServerExperimentsFieldTrialNameForTesting(),
-          test.trial_group_value);
+
+      scoped_feature_list.InitWithFeaturesAndParameters(
+          {{data_reduction_proxy::features::
+                kDataReductionProxyServerExperiments,
+            {variation_params}}},
+          {});
     }
 
     base::CommandLine::ForCurrentProcess()->InitFromArgv(0, nullptr);