Enable DRP config service when lite page redirect is enabled

Enable data reduction proxy config service fetch
when lite page redirect is enabled.

Currently, the fetch is disabled if the user is in
the DRP holdback group even if lite page redirect
feature is enabled.

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

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -35,6 +35,8 @@
 #include "components/data_reduction_proxy/proto/client_config.pb.h"
 #include "components/page_load_metrics/browser/page_load_metrics_test_waiter.h"
 #include "components/prefs/pref_service.h"
+#include "components/previews/core/previews_experiments.h"
+#include "components/previews/core/previews_features.h"
 #include "components/proxy_config/proxy_config_dictionary.h"
 #include "components/proxy_config/proxy_config_pref_names.h"
 #include "components/variations/variations_associated_data.h"
@@ -299,17 +301,27 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
 
   void SetConfig(const ClientConfig& config) {
     config_ = config;
-    // Config is not fetched in the holdback group. So, return early.
-    if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial())
+    // 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()) {
       return;
+    }
 
     config_waiter_ = std::make_unique<ScopedConfigWaiter>(browser()->profile());
   }
 
   void WaitForConfig() {
-    // Config is not fetched in the holdback group. So, return early.
-    if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial())
+    // 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()) {
       return;
+    }
+    // Destructor of |config_waiter_| waits for the config fetch request to
+    // arrive. For that check to work correctly, |config_waiter_| should be
+    // non-null.
+    ASSERT_TRUE(config_waiter_ != nullptr);
     config_waiter_.reset();
   }
 
@@ -320,11 +332,14 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
   base::test::ScopedFeatureList scoped_feature_list_;
 
  private:
+  // Called when |config_server_| receives a request for config fetch.
   std::unique_ptr<net::test_server::HttpResponse> GetConfigResponse(
       const net::test_server::HttpRequest& request) {
-    // Config should not be fetched when in holdback.
-    EXPECT_FALSE(
-        data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial());
+    // Config should be fetched only when holdback is disabled or lite page
+    // redirect previews are enabled.
+    EXPECT_TRUE(
+        !data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial() ||
+        previews::params::IsLitePageServerPreviewsEnabled());
 
     auto response = std::make_unique<net::test_server::BasicHttpResponse>();
     response->set_content(config_.SerializeAsString());
@@ -658,29 +673,38 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyBrowsertest, UMAMetricsRecorded) {
       1);
 }
 
-// Test that enabling the holdback disables the proxy.
-// Parameter is true if the data reduction proxy holdback should be enabled.
+// 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<bool>,
+    : public ::testing::WithParamInterface<std::tuple<bool, bool>>,
       public DataReductionProxyBrowsertest {
  public:
   DataReductionProxyWithHoldbackBrowsertest()
       : DataReductionProxyBrowsertest(),
-        data_reduction_proxy_holdback_enabled_(GetParam()) {}
+        data_reduction_proxy_holdback_enabled_(std::get<0>(GetParam())),
+        lite_page_redirect_previews_enabled_(std::get<1>(GetParam())) {}
 
   void SetUp() override {
     if (data_reduction_proxy_holdback_enabled_) {
       scoped_feature_list_.InitAndEnableFeature(
           data_reduction_proxy::features::kDataReductionProxyHoldback);
     }
+    if (lite_page_redirect_previews_enabled_) {
+      previews_lite_page_redirect_feature_list_.InitAndEnableFeature(
+          previews::features::kLitePageServerPreviews);
+    }
 
     InProcessBrowserTest::SetUp();
   }
 
   const bool data_reduction_proxy_holdback_enabled_;
+  const bool lite_page_redirect_previews_enabled_;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
+  base::test::ScopedFeatureList previews_lite_page_redirect_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_P(DataReductionProxyWithHoldbackBrowsertest,
@@ -697,6 +721,8 @@ IN_PROC_BROWSER_TEST_P(DataReductionProxyWithHoldbackBrowsertest,
   // A network change forces the config to be fetched.
   SimulateNetworkChange(network::mojom::ConnectionType::CONNECTION_3G);
 
+  // Ensure that the client config is fetched when lite page redirect preview is
+  // enabled or DRP holdback is disabled.
   WaitForConfig();
 
   // Load a webpage in holdback group as well. This ensures that while in
@@ -705,17 +731,19 @@ 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 (GetParam()) {
+  if (data_reduction_proxy_holdback_enabled_) {
     EXPECT_NE(GetBody(), kPrimaryResponse);
   } else {
     EXPECT_EQ(GetBody(), kPrimaryResponse);
   }
 }
 
-// Parameter is true if the data reduction proxy holdback should be 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.
 INSTANTIATE_TEST_SUITE_P(,
                          DataReductionProxyWithHoldbackBrowsertest,
-                         ::testing::Values(false, true));
+                         ::testing::Combine(::testing::Bool(),
+                                            ::testing::Bool()));
 
 class DataReductionProxyExpBrowsertest : public DataReductionProxyBrowsertest {
  public:

chrome/browser/previews/previews_lite_page_redirect_browsertest.cc

@@ -141,6 +141,8 @@ class BasePreviewsLitePageRedirectServerBrowserTest
 
   virtual bool UseOptimizationGuideKeyedServiceImplementation() const = 0;
 
+  virtual bool ShouldEnableDRPHoldback() const = 0;
+
   enum PreviewsServerAction {
     // Previews server will respond with HTTP 200 OK, OFCL=60,
     // Content-Length=20.
@@ -321,6 +323,10 @@ class BasePreviewsLitePageRedirectServerBrowserTest
       opt_guide_keyed_service_feature_list_.InitWithFeatures(
           {}, {optimization_guide::features::kOptimizationGuideKeyedService});
     }
+
+    drp_holdback_feature_list_.InitWithFeatureState(
+        data_reduction_proxy::features::kDataReductionProxyHoldback,
+        ShouldEnableDRPHoldback());
   }
 
   void SetUpOnMainThread() override {
@@ -895,6 +901,7 @@ class BasePreviewsLitePageRedirectServerBrowserTest
   base::test::ScopedFeatureList scoped_feature_list_;
   base::test::ScopedFeatureList url_loader_feature_list_;
   base::test::ScopedFeatureList opt_guide_keyed_service_feature_list_;
+  base::test::ScopedFeatureList drp_holdback_feature_list_;
   std::unique_ptr<net::EmbeddedTestServer> previews_server_;
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
   std::unique_ptr<net::EmbeddedTestServer> http_server_;
@@ -922,23 +929,27 @@ class BasePreviewsLitePageRedirectServerBrowserTest
   base::OnceClosure waiting_for_report_closure_;
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 class PreviewsLitePageRedirectServerBrowserTest
     : public BasePreviewsLitePageRedirectServerBrowserTest,
-      public testing::WithParamInterface<bool> {
+      public ::testing::WithParamInterface<std::tuple<bool, bool>> {
  public:
   bool UseOptimizationGuideKeyedServiceImplementation() const override {
-    return GetParam();
+    return std::get<0>(GetParam());
+  }
+  bool ShouldEnableDRPHoldback() const override {
+    return std::get<1>(GetParam());
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be
+// enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectServerBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 // Previews InfoBar (which these tests trigger) does not work on Mac.
 // See https://crbug.com/782322 for detail.
@@ -1596,12 +1607,12 @@ class PreviewsLitePageRedirectServerTimeoutBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectServerTimeoutBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(PreviewsLitePageRedirectServerTimeoutBrowserTest,
                        DISABLE_ON_WIN_MAC_CHROMESOS(LitePagePreviewsTimeout)) {
@@ -1642,12 +1653,12 @@ class PreviewsLitePageRedirectServerBadServerBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectServerBadServerBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(
     PreviewsLitePageRedirectServerBadServerBrowserTest,
@@ -1690,12 +1701,12 @@ class PreviewsLitePageRedirectServerDataSaverBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectServerDataSaverBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(
     PreviewsLitePageRedirectServerDataSaverBrowserTest,
@@ -1728,12 +1739,12 @@ class PreviewsLitePageRedirectServerNoDataSaverHeaderBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectServerNoDataSaverHeaderBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(
     PreviewsLitePageRedirectServerNoDataSaverHeaderBrowserTest,
@@ -1770,12 +1781,12 @@ class PreviewsLitePageRedirectNotificationDSEnabledBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectNotificationDSEnabledBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(
     PreviewsLitePageRedirectNotificationDSEnabledBrowserTest,
@@ -1831,12 +1842,12 @@ class PreviewsLitePageRedirectDSDisabledBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectDSDisabledBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(
     PreviewsLitePageRedirectDSDisabledBrowserTest,
@@ -1864,12 +1875,12 @@ class PreviewsLitePageRedirectControlBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectControlBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(
     PreviewsLitePageRedirectControlBrowserTest,
@@ -1924,6 +1935,8 @@ class PreviewsLitePageRedirectServerNetworkIsolationBrowserTest
     return false;
   }
 
+  bool ShouldEnableDRPHoldback() const override { return false; }
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 };
@@ -1999,12 +2012,12 @@ class PreviewsLitePageRedirectAndPageHintsBrowserTest
   }
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     PreviewsLitePageRedirectAndPageHintsBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 // Regression test for crbug.com/954554.
 IN_PROC_BROWSER_TEST_P(
@@ -2223,12 +2236,12 @@ class CoinFlipHoldbackExperimentBrowserTest
   base::test::ScopedFeatureList ukm_feature_list_;
 };
 
-// Param is true if testing using the OptimizationGuideKeyedService
-// implementation.
+// First param is true if testing using the OptimizationGuideKeyedService
+// implementation. Second param is true if DRP holdback should be enabled.
 INSTANTIATE_TEST_SUITE_P(
     /* no prefix */,
     CoinFlipHoldbackExperimentBrowserTest,
-    testing::Bool());
+    ::testing::Combine(::testing::Bool(), ::testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(CoinFlipHoldbackExperimentBrowserTest,
                        DISABLE_ON_WIN_MAC_CHROMESOS(NoPreviews_NoCoinFlip)) {

components/data_reduction_proxy/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+components/data_use_measurement/core",
   "+components/pref_registry",
   "+components/prefs",
+  "+components/previews/core",
   "+components/variations",
   "+crypto",
   "+google_apis",

components/data_reduction_proxy/core/browser/BUILD.gn

@@ -56,6 +56,7 @@ if (is_android) {
       "//components/data_use_measurement/core:ascriber",
       "//components/pref_registry",
       "//components/prefs",
+      "//components/previews/core:core",
       "//components/variations",
       "//components/variations/net",
       "//content/public/browser",
@@ -91,6 +92,7 @@ static_library("browser") {
     "//components/data_use_measurement/core:ascriber",
     "//components/pref_registry",
     "//components/prefs",
+    "//components/previews/core:core",
     "//components/variations",
     "//components/variations/net",
     "//content/public/browser",

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc

@@ -33,6 +33,7 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
 #include "components/data_reduction_proxy/proto/client_config.pb.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/previews/core/previews_experiments.h"
 #include "components/variations/net/variations_http_headers.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
@@ -178,7 +179,8 @@ DataReductionProxyConfigServiceClient::DataReductionProxyConfigServiceClient(
   DCHECK(config);
   DCHECK(service);
   DCHECK(config_service_url_.is_valid());
-  DCHECK(!params::IsIncludedInHoldbackFieldTrial());
+  DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
+         previews::params::IsLitePageServerPreviewsEnabled());
 
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
@@ -431,7 +433,8 @@ void DataReductionProxyConfigServiceClient::OnURLLoadComplete(
 
 void DataReductionProxyConfigServiceClient::RetrieveRemoteConfig() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!params::IsIncludedInHoldbackFieldTrial());
+  DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
+         previews::params::IsLitePageServerPreviewsEnabled());
 
   CreateClientConfigRequest request;
   std::string serialized_request;

components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc

@@ -159,7 +159,8 @@ void DataReductionProxyRequestOptions::UpdateCredentials() {
   RegenerateRequestHeaderValue();
 }
 
-void DataReductionProxyRequestOptions::SetKey(const std::string& key) {
+void DataReductionProxyRequestOptions::SetKeyForTesting(
+    const std::string& key) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if(!key.empty()) {
     key_ = key;

components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h

@@ -79,7 +79,7 @@ class DataReductionProxyRequestOptions {
   // this class has been constructed. Android WebView is a platform that does
   // this. The caller needs to make sure |this| pointer is valid when
   // SetKey is called.
-  void SetKey(const std::string& key);
+  void SetKeyForTesting(const std::string& key);
 
   // Sets the credentials for sending to the Data Reduction Proxy.
   void SetSecureSession(const std::string& secure_session);

components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc

@@ -195,7 +195,7 @@ TEST_F(DataReductionProxyRequestOptionsTest, Authorization) {
   test_context_->RunUntilIdle();
 
   // Now set a key.
-  request_options()->SetKey(kTestKey2);
+  request_options()->SetKeyForTesting(kTestKey2);
 
   // Write headers.
   VerifyExpectedHeader(expected_header, kPageIdValue);
@@ -211,7 +211,7 @@ TEST_F(DataReductionProxyRequestOptionsTest, AuthorizationIgnoresEmptyKey) {
 
   // Now set an empty key. The auth handler should ignore that, and the key
   // remains |kTestKey|.
-  request_options()->SetKey(std::string());
+  request_options()->SetKeyForTesting(std::string());
   VerifyExpectedHeader(expected_header, kPageIdValue);
 }
 

components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc

@@ -34,6 +34,7 @@
 #include "components/data_reduction_proxy/proto/data_store.pb.h"
 #include "components/data_use_measurement/core/data_use_measurement.h"
 #include "components/prefs/pref_service.h"
+#include "components/previews/core/previews_experiments.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
 namespace data_reduction_proxy {
@@ -93,7 +94,8 @@ DataReductionProxyService::DataReductionProxyService(
   // It is safe to use base::Unretained here, since it gets executed
   // synchronously on the UI thread, and |this| outlives the caller (since the
   // caller is owned by |this|.
-  if (!params::IsIncludedInHoldbackFieldTrial()) {
+  if (!params::IsIncludedInHoldbackFieldTrial() ||
+      previews::params::IsLitePageServerPreviewsEnabled()) {
     config_client_ = std::make_unique<DataReductionProxyConfigServiceClient>(
         GetBackoffPolicy(), request_options_.get(), raw_mutable_config,
         config_.get(), this, network_connection_tracker_,
@@ -441,6 +443,9 @@ void DataReductionProxyService::Clone(
 }
 
 void DataReductionProxyService::UpdateCustomProxyConfig() {
+  if (params::IsIncludedInHoldbackFieldTrial())
+    return;
+
   network::mojom::CustomProxyConfigPtr config = CreateCustomProxyConfig(
       !base::FeatureList::IsEnabled(
           features::kDataReductionProxyDisableProxyFailedWarmup),
@@ -506,6 +511,9 @@ DataReductionProxyService::CreateCustomProxyConfig(
 void DataReductionProxyService::StoreSerializedConfig(
     const std::string& serialized_config) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
+         previews::params::IsLitePageServerPreviewsEnabled());
+
   SetStringPref(prefs::kDataReductionProxyConfig, serialized_config);
   SetInt64Pref(prefs::kDataReductionProxyLastConfigRetrievalTime,
                (base::Time::Now() - base::Time()).InMicroseconds());

components/previews/DEPS

@@ -3,5 +3,6 @@ include_rules = [
   # allowances of //content dependencies as appropriate.
   "-components",
   "-components/data_reduction_proxy/",
+  "+components/data_reduction_proxy/core/browser/",
   "-components/previews/content",
 ]
\ No newline at end of file