Allow subresource redirect to be controlled via finch

We need to be able to disable just the redirect part of the feature, so
that the users will still see the original image, but other coverage
metrics will be recorded for analysis.

This CL also removes detecting images based on file extensions.

Bug: 1042869
Change-Id: I6842a09386664d9ce61e364bbba2b2aef9000195
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2017671
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735168}

chrome/browser/data_saver/subresource_redirect_browsertest.cc

@@ -57,9 +57,8 @@ void RetryForHistogramUntilCountReached(base::HistogramTester* histogram_tester,
 
 class SubresourceRedirectBrowserTest : public InProcessBrowserTest {
  public:
-  explicit SubresourceRedirectBrowserTest(
-      const std::string& included_path_suffixes = "")
-      : included_path_suffixes_(included_path_suffixes),
+  explicit SubresourceRedirectBrowserTest(bool enable_lite_page_redirect = true)
+      : enable_lite_page_redirect_(enable_lite_page_redirect),
         https_server_(net::EmbeddedTestServer::TYPE_HTTPS),
         compression_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
 
@@ -86,7 +85,8 @@ class SubresourceRedirectBrowserTest : public InProcessBrowserTest {
 
     scoped_feature_list_.InitWithFeaturesAndParameters(
         {{blink::features::kSubresourceRedirect,
-          {{"included_path_suffixes", included_path_suffixes_},
+          {{"enable_lite_page_redirect",
+            enable_lite_page_redirect_ ? "true" : "false"},
            {"lite_page_subresource_origin", compression_url_.spec()}}},
          {optimization_guide::features::kOptimizationHints, {}}},
         {});
@@ -222,7 +222,7 @@ class SubresourceRedirectBrowserTest : public InProcessBrowserTest {
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;
-  const std::string included_path_suffixes_;
+  bool enable_lite_page_redirect_ = false;
 
   GURL compression_url_;
   GURL http_url_;
@@ -243,11 +243,11 @@ class SubresourceRedirectBrowserTest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(SubresourceRedirectBrowserTest);
 };
 
-class DifferentMediaInclusionSubresourceRedirectBrowserTest
+class RedirectDisabledSubresourceRedirectBrowserTest
     : public SubresourceRedirectBrowserTest {
  public:
-  DifferentMediaInclusionSubresourceRedirectBrowserTest()
-      : SubresourceRedirectBrowserTest(".svg") {}
+  RedirectDisabledSubresourceRedirectBrowserTest()
+      : SubresourceRedirectBrowserTest(false) {}
 };
 
 //  NOTE: It is indirectly verified that correct requests are being sent to
@@ -537,11 +537,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectBrowserTest,
             https_url().port());
 }
 
-//  This test loads image.html, but with
-//  SubresourceRedirectIncludedMediaSuffixes set to only allow .svg, so no
-//  internal redirect should occur.
-IN_PROC_BROWSER_TEST_F(DifferentMediaInclusionSubresourceRedirectBrowserTest,
-                       NoTriggerWhenNotIncludedInMediaSuffixes) {
+// This test verifies that the image redirect to lite page is disabled via finch
+IN_PROC_BROWSER_TEST_F(RedirectDisabledSubresourceRedirectBrowserTest,
+                       ImagesNotRedirected) {
   EnableDataSaver(true);
   SetUpPublicImageURLPaths({"/load_image/image.png"});
   ui_test_utils::NavigateToURL(browser(),
@@ -552,6 +550,10 @@ IN_PROC_BROWSER_TEST_F(DifferentMediaInclusionSubresourceRedirectBrowserTest,
 
   histogram_tester()->ExpectTotalCount(
       "SubresourceRedirect.CompressionAttempt.ResponseCode", 0);
+  histogram_tester()->ExpectTotalCount(
+      "SubresourceRedirect.CompressionAttempt.ServerResponded", 0);
+  histogram_tester()->ExpectTotalCount(
+      "SubresourceRedirect.DidCompress.CompressionPercent", 0);
 
   EXPECT_TRUE(RunScriptExtractBool("checkImage()"));
 

chrome/renderer/BUILD.gn

@@ -94,8 +94,6 @@ jumbo_static_library("renderer") {
     "previews/resource_loading_hints_agent.h",
     "sandbox_status_extension_android.cc",
     "sandbox_status_extension_android.h",
-    "subresource_redirect/subresource_redirect_experiments.cc",
-    "subresource_redirect/subresource_redirect_experiments.h",
     "subresource_redirect/subresource_redirect_hints_agent.cc",
     "subresource_redirect/subresource_redirect_hints_agent.h",
     "subresource_redirect/subresource_redirect_params.cc",

chrome/renderer/subresource_redirect/subresource_redirect_experiments.cc

-// Copyright 2019 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 "chrome/renderer/subresource_redirect/subresource_redirect_experiments.h"
-
-#include "base/feature_list.h"
-#include "base/metrics/field_trial.h"
-#include "base/metrics/field_trial_params.h"
-#include "base/strings/string_split.h"
-#include "third_party/blink/public/common/features.h"
-
-namespace subresource_redirect {
-
-bool ShouldIncludeMediaSuffix(const GURL& url) {
-  std::vector<std::string> suffixes = {".jpg", ".jpeg", ".png", ".svg",
-                                       ".webp"};
-
-  std::string csv = base::GetFieldTrialParamValueByFeature(
-      blink::features::kSubresourceRedirect, "included_path_suffixes");
-  if (csv != "") {
-    suffixes = base::SplitString(csv, ",", base::TRIM_WHITESPACE,
-                                 base::SPLIT_WANT_NONEMPTY);
-  }
-
-  for (const std::string& suffix : suffixes) {
-    if (base::EndsWith(url.path(), suffix,
-                       base::CompareCase::INSENSITIVE_ASCII)) {
-      return true;
-    }
-  }
-  return false;
-}
-
-}  // namespace subresource_redirect

chrome/renderer/subresource_redirect/subresource_redirect_experiments.h

-// Copyright 2019 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.
-
-#ifndef CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_EXPERIMENTS_H_
-#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_EXPERIMENTS_H_
-
-#include "url/gurl.h"
-
-namespace subresource_redirect {
-
-// Returns true if the given url matches an included media suffix.
-bool ShouldIncludeMediaSuffix(const GURL& url);
-
-}  // namespace subresource_redirect
-
-#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_EXPERIMENTS_H_

chrome/renderer/subresource_redirect/subresource_redirect_experiments_unittest.cc

-// Copyright 2019 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 "chrome/renderer/subresource_redirect/subresource_redirect_experiments.h"
-
-#include "base/test/scoped_feature_list.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/blink/public/common/features.h"
-
-namespace subresource_redirect {
-
-namespace {
-
-TEST(SubresourceRedirectExperimentsTest, TestDefaultShouldIncludeMediaSuffix) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      blink::features::kSubresourceRedirect);
-
-  EXPECT_FALSE(ShouldIncludeMediaSuffix(GURL("http://chromium.org/path/")));
-
-  std::vector<std::string> default_suffixes = {".jpg", ".jpeg", ".png", ".svg",
-                                               ".webp"};
-  for (const std::string& suffix : default_suffixes) {
-    GURL url("http://chromium.org/image" + suffix);
-    EXPECT_TRUE(ShouldIncludeMediaSuffix(url));
-  }
-}
-
-TEST(SubresourceRedirectExperimentsTest, TestShouldIncludeMediaSuffix) {
-  struct TestCase {
-    std::string msg;
-    std::string varaiation_value;
-    std::vector<std::string> urls;
-    bool want_return;
-  };
-  const TestCase kTestCases[]{
-      {
-          .msg = "Default values are overridden by variations",
-          .varaiation_value = ".html",
-          .urls = {"http://chromium.org/image.jpeg",
-                   "http://chromium.org/image.png",
-                   "http://chromium.org/image.jpg",
-                   "http://chromium.org/image.svg"},
-          .want_return = false,
-      },
-      {
-          .msg = "Variation value whitespace should be trimmed",
-          .varaiation_value = " .svg , \t .png\n",
-          .urls = {"http://chromium.org/image.svg",
-                   "http://chromium.org/image.png"},
-          .want_return = true,
-      },
-      {
-          .msg = "Variation value empty values should be excluded",
-          .varaiation_value = ".svg,,.png,",
-          .urls = {"http://chromium.org/image.svg",
-                   "http://chromium.org/image.png"},
-          .want_return = true,
-      },
-      {
-          .msg = "URLs should be compared case insensitive",
-          .varaiation_value = ".svg,.png,",
-          .urls = {"http://chromium.org/image.SvG",
-                   "http://chromium.org/image.PNG"},
-          .want_return = true,
-      },
-      {
-          .msg = "Query params and fragments don't matter",
-          .varaiation_value = ".svg,.png,",
-          .urls = {"http://chromium.org/image.svg?hello=world",
-                   "http://chromium.org/image.png#test"},
-          .want_return = true,
-      },
-      {
-          .msg = "Query params and fragments shouldn't be considered",
-          .varaiation_value = ".svg,.png,",
-          .urls = {"http://chromium.org/?image=image.svg",
-                   "http://chromium.org/#image.png"},
-          .want_return = false,
-      },
-  };
-  for (const TestCase& test_case : kTestCases) {
-    SCOPED_TRACE(test_case.msg);
-
-    base::test::ScopedFeatureList scoped_feature_list;
-    scoped_feature_list.InitAndEnableFeatureWithParameters(
-        blink::features::kSubresourceRedirect,
-        {{"included_path_suffixes", test_case.varaiation_value}});
-
-    for (const std::string& url : test_case.urls) {
-      EXPECT_EQ(test_case.want_return, ShouldIncludeMediaSuffix(GURL(url)));
-    }
-  }
-}
-
-}  // namespace
-}  // namespace subresource_redirect

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.cc

@@ -4,9 +4,9 @@
 
 #include "chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h"
 
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
 #include "chrome/renderer/previews/resource_loading_hints_agent.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_experiments.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_params.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_util.h"
@@ -67,11 +67,11 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
   if (!subresource_redirect_hints_agent->ShouldRedirectImage(request->url))
     return;
 
-  // Image subresources that have paths that do not end in one of the
-  // following common formats are commonly single pixel images that will not
-  // benefit from being sent to the compression server.
-  if (!ShouldIncludeMediaSuffix(request->url))
+  if (!base::GetFieldTrialParamByFeatureAsBool(
+          blink::features::kSubresourceRedirect, "enable_lite_page_redirect",
+          false)) {
     return;
+  }
 
   request->url = GetSubresourceURLForURL(request->url);
   *defer = false;

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle_unittest.cc

@@ -91,8 +91,10 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestMaybeCreateThrottle) {
   for (const TestCase& test_case : kTestCases) {
     base::test::ScopedFeatureList scoped_feature_list;
     if (test_case.is_subresource_redirect_feature_enabled) {
-      scoped_feature_list.InitAndEnableFeature(
-          blink::features::kSubresourceRedirect);
+      scoped_feature_list.InitWithFeaturesAndParameters(
+          {{blink::features::kSubresourceRedirect,
+            {{"enable_lite_page_redirect", "true"}}}},
+          {});
     } else {
       scoped_feature_list.InitAndDisableFeature(
           blink::features::kSubresourceRedirect);
@@ -142,8 +144,10 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestGetSubresourceURL) {
       },
   };
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      blink::features::kSubresourceRedirect);
+  scoped_feature_list.InitWithFeaturesAndParameters(
+      {{blink::features::kSubresourceRedirect,
+        {{"enable_lite_page_redirect", "true"}}}},
+      {});
 
   for (const TestCase& test_case : kTestCases) {
     auto throttle = CreateSubresourceRedirectURLLoaderThrottle(
@@ -170,8 +174,10 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestGetSubresourceURL) {
 
 TEST(SubresourceRedirectURLLoaderThrottleTest, DeferOverridenToFalse) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      blink::features::kSubresourceRedirect);
+  scoped_feature_list.InitWithFeaturesAndParameters(
+      {{blink::features::kSubresourceRedirect,
+        {{"enable_lite_page_redirect", "true"}}}},
+      {});
 
   auto throttle = CreateSubresourceRedirectURLLoaderThrottle(
       GURL("https://www.test.com/test.jpg"), content::ResourceType::kImage,

chrome/test/BUILD.gn

@@ -3481,7 +3481,6 @@ test("unit_tests") {
     "../renderer/net/net_error_helper_core_unittest.cc",
     "../renderer/plugins/plugin_uma_unittest.cc",
     "../renderer/prerender/prerender_dispatcher_unittest.cc",
-    "../renderer/subresource_redirect/subresource_redirect_experiments_unittest.cc",
     "../renderer/subresource_redirect/subresource_redirect_url_loader_throttle_unittest.cc",
     "../renderer/subresource_redirect/subresource_redirect_util_unittest.cc",
     "../renderer/v8_unwinder_unittest.cc",