Allow client configs to be fetched for all data saver users.

Curretly, client configs are not fetched if data saver holdback
is enabled AND lite page redirect preview is not enabled.
This CL changes the logic to allow client configs to be
fetched for all data saver users.

This is expected to ensure that the client config
is available to lite page redirect users when they switch
from control finch arm to enabled arm.

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

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -302,7 +302,8 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
     // Config is not fetched if the holdback group is enabled and lite page
     // redirect previews are not enabled. So, return early.
     if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial() &&
-        !previews::params::IsLitePageServerPreviewsEnabled()) {
+        !previews::params::IsLitePageServerPreviewsEnabled() &&
+        !params::ForceEnableClientConfigServiceForAllDataSaverUsers()) {
       return;
     }
 
@@ -313,7 +314,8 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
     // Config is not fetched if the holdback group is enabled and lite page
     // redirect previews are not enabled. So, return early.
     if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial() &&
-        !previews::params::IsLitePageServerPreviewsEnabled()) {
+        !previews::params::IsLitePageServerPreviewsEnabled() &&
+        !params::ForceEnableClientConfigServiceForAllDataSaverUsers()) {
       return;
     }
     // Destructor of |config_waiter_| waits for the config fetch request to
@@ -337,7 +339,8 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
     // redirect previews are enabled.
     EXPECT_TRUE(
         !data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial() ||
-        previews::params::IsLitePageServerPreviewsEnabled());
+        previews::params::IsLitePageServerPreviewsEnabled() ||
+        params::ForceEnableClientConfigServiceForAllDataSaverUsers());
 
     auto response = std::make_unique<net::test_server::BasicHttpResponse>();
     response->set_content(config_.SerializeAsString());
@@ -672,19 +675,27 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyBrowsertest, UMAMetricsRecorded) {
 
 // Test that enabling the holdback disables the proxy and that the client config
 // is fetched when lite page redirect preview is enabled.
-// First parameter is true if the data reduction proxy holdback should be
-// enabled. Second parameter is true if lite page redirect preview is enabled.
 class DataReductionProxyWithHoldbackBrowsertest
-    : public ::testing::WithParamInterface<std::tuple<bool, bool>>,
+    : public ::testing::WithParamInterface<std::tuple<bool, bool, bool>>,
       public DataReductionProxyBrowsertest {
  public:
   DataReductionProxyWithHoldbackBrowsertest()
       : DataReductionProxyBrowsertest(),
-        data_reduction_proxy_holdback_enabled_(std::get<0>(GetParam())),
-        lite_page_redirect_previews_enabled_(std::get<1>(GetParam())) {}
+        // Consider the holdback as enabled if holdback is enabled or the
+        // |force_enable_config_service_fetches_| is enabled.
+        data_reduction_proxy_holdback_enabled_(std::get<2>(GetParam()) ||
+                                               std::get<0>(GetParam())),
+        lite_page_redirect_previews_enabled_(std::get<1>(GetParam())),
+        force_enable_config_service_fetches_(std::get<2>(GetParam())) {}
 
   void SetUp() override {
-    if (data_reduction_proxy_holdback_enabled_) {
+    if (force_enable_config_service_fetches_) {
+      scoped_feature_list_.InitAndEnableFeatureWithParameters(
+          data_reduction_proxy::features::kDataReductionProxyHoldback,
+          {{"force_enable_config_service_fetches", "true"}});
+    }
+    if (!force_enable_config_service_fetches_ &&
+        data_reduction_proxy_holdback_enabled_) {
       scoped_feature_list_.InitAndEnableFeature(
           data_reduction_proxy::features::kDataReductionProxyHoldback);
     }
@@ -698,6 +709,7 @@ class DataReductionProxyWithHoldbackBrowsertest
 
   const bool data_reduction_proxy_holdback_enabled_;
   const bool lite_page_redirect_previews_enabled_;
+  const bool force_enable_config_service_fetches_;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -728,7 +740,8 @@ IN_PROC_BROWSER_TEST_P(DataReductionProxyWithHoldbackBrowsertest,
   // holdback is not enabled would trigger and cause the test to fail.
   ui_test_utils::NavigateToURL(browser(), GURL("http://does.not.resolve/foo"));
 
-  if (data_reduction_proxy_holdback_enabled_) {
+  if (data_reduction_proxy_holdback_enabled_ ||
+      force_enable_config_service_fetches_) {
     EXPECT_NE(GetBody(), kPrimaryResponse);
   } else {
     EXPECT_EQ(GetBody(), kPrimaryResponse);
@@ -737,9 +750,12 @@ IN_PROC_BROWSER_TEST_P(DataReductionProxyWithHoldbackBrowsertest,
 
 // First parameter is true if the data reduction proxy holdback should be
 // enabled. Second parameter is true if lite page redirect preview is enabled.
+// Third parameter is true if data reduction proxy config should always be
+// fetched.
 INSTANTIATE_TEST_SUITE_P(,
                          DataReductionProxyWithHoldbackBrowsertest,
                          ::testing::Combine(::testing::Bool(),
+                                            ::testing::Bool(),
                                             ::testing::Bool()));
 
 class DataReductionProxyExpBrowsertest : public DataReductionProxyBrowsertest {

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc

@@ -180,7 +180,8 @@ DataReductionProxyConfigServiceClient::DataReductionProxyConfigServiceClient(
   DCHECK(service);
   DCHECK(config_service_url_.is_valid());
   DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
-         previews::params::IsLitePageServerPreviewsEnabled());
+         previews::params::IsLitePageServerPreviewsEnabled() ||
+         params::ForceEnableClientConfigServiceForAllDataSaverUsers());
 
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
@@ -434,7 +435,8 @@ void DataReductionProxyConfigServiceClient::OnURLLoadComplete(
 void DataReductionProxyConfigServiceClient::RetrieveRemoteConfig() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
-         previews::params::IsLitePageServerPreviewsEnabled());
+         previews::params::IsLitePageServerPreviewsEnabled() ||
+         params::ForceEnableClientConfigServiceForAllDataSaverUsers());
 
   CreateClientConfigRequest request;
   std::string serialized_request;

components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc

@@ -95,7 +95,8 @@ DataReductionProxyService::DataReductionProxyService(
   // synchronously on the UI thread, and |this| outlives the caller (since the
   // caller is owned by |this|.
   if (!params::IsIncludedInHoldbackFieldTrial() ||
-      previews::params::IsLitePageServerPreviewsEnabled()) {
+      previews::params::IsLitePageServerPreviewsEnabled() ||
+      params::ForceEnableClientConfigServiceForAllDataSaverUsers()) {
     config_client_ = std::make_unique<DataReductionProxyConfigServiceClient>(
         GetBackoffPolicy(), request_options_.get(), raw_mutable_config,
         config_.get(), this, network_connection_tracker_,
@@ -512,7 +513,8 @@ void DataReductionProxyService::StoreSerializedConfig(
     const std::string& serialized_config) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
-         previews::params::IsLitePageServerPreviewsEnabled());
+         previews::params::IsLitePageServerPreviewsEnabled() ||
+         params::ForceEnableClientConfigServiceForAllDataSaverUsers());
 
   SetStringPref(prefs::kDataReductionProxyConfig, serialized_config);
   SetInt64Pref(prefs::kDataReductionProxyLastConfigRetrievalTime,

components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc

@@ -112,6 +112,16 @@ std::string HoldbackFieldTrialGroup() {
   return base::FieldTrialList::FindFullName("DataCompressionProxyHoldback");
 }
 
+bool ForceEnableClientConfigServiceForAllDataSaverUsers() {
+  // Client config is enabled for all data users that are not in the
+  // kDataReductionProxyHoldback. Users that have kDataReductionProxyHoldback
+  // enabled have config service client enabled only if
+  // |force_enable_config_service_fetches| is set to true.
+  return GetFieldTrialParamByFeatureAsBool(
+      data_reduction_proxy::features::kDataReductionProxyHoldback,
+      "force_enable_config_service_fetches", false);
+}
+
 bool FetchWarmupProbeURLEnabled() {
   return !base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kDisableDataReductionProxyWarmupURLFetch);

components/data_reduction_proxy/core/common/data_reduction_proxy_params.h

@@ -48,6 +48,10 @@ bool IsIncludedInHoldbackFieldTrial();
 // not included in a group.
 std::string HoldbackFieldTrialGroup();
 
+// Returns true if DRP config service should always be fetched even if DRP
+// holdback is enabled.
+bool ForceEnableClientConfigServiceForAllDataSaverUsers();
+
 // Returns true if this client has the command line switch to enable forced
 // pageload metrics pingbacks on every page load.
 bool IsForcePingbackEnabledViaFlags();