Refactor redirect decision making to decider interface

This CL introduces PublicResourceDecider interface that has the redirect
decision making API that subclasses should implemeent. This class also
has the mojo interface got interacting with browser process. This
interface also has the RedirectResult enum that will have ineligible
reasons for public image hints approach and robots checking based
approach.

PublicImageHintsDeciderAgent (rename of SubresourceRedirectHintsAgent)
is introduced which is the subclass of the decider interface for public
image hints approach.

Change-Id: I5ef43c2ee821f7d5aa6501683c22e09464da7972
Bug: 1152527
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2572755
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834888}

chrome/renderer/BUILD.gn

@@ -96,12 +96,14 @@ static_library("renderer") {
     "plugins/plugin_uma.h",
     "previews/resource_loading_hints_agent.cc",
     "previews/resource_loading_hints_agent.h",
-    "subresource_redirect/public_image_hints_url_loader_throttle.cc",
-    "subresource_redirect/public_image_hints_url_loader_throttle.h",
+    "subresource_redirect/public_image_hints_decider_agent.cc",
+    "subresource_redirect/public_image_hints_decider_agent.h",
+    "subresource_redirect/public_resource_decider.h",
+    "subresource_redirect/public_resource_decider_agent.cc",
+    "subresource_redirect/public_resource_decider_agent.h",
+    "subresource_redirect/redirect_result.h",
     "subresource_redirect/robots_rules_parser.cc",
     "subresource_redirect/robots_rules_parser.h",
-    "subresource_redirect/subresource_redirect_hints_agent.cc",
-    "subresource_redirect/subresource_redirect_hints_agent.h",
     "subresource_redirect/subresource_redirect_params.cc",
     "subresource_redirect/subresource_redirect_params.h",
     "subresource_redirect/subresource_redirect_url_loader_throttle.cc",

chrome/renderer/chrome_content_renderer_client.cc

@@ -60,7 +60,7 @@
 #include "chrome/renderer/plugins/pdf_plugin_placeholder.h"
 #include "chrome/renderer/plugins/plugin_uma.h"
 #include "chrome/renderer/previews/resource_loading_hints_agent.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h"
+#include "chrome/renderer/subresource_redirect/public_image_hints_decider_agent.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_params.h"
 #include "chrome/renderer/sync_encryption_keys_extension.h"
 #include "chrome/renderer/url_loader_throttle_provider_impl.h"
@@ -607,7 +607,7 @@ void ChromeContentRendererClient::RenderFrameCreated(
   new previews::ResourceLoadingHintsAgent(associated_interfaces, render_frame);
 
   if (subresource_redirect::IsPublicImageHintsBasedCompressionEnabled())
-    new subresource_redirect::SubresourceRedirectHintsAgent(
+    new subresource_redirect::PublicImageHintsDeciderAgent(
         associated_interfaces, render_frame);
 
   if (translate::IsSubFrameTranslationEnabled()) {

chrome/renderer/previews/resource_loading_hints_agent.cc

@@ -8,7 +8,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/histogram_macros_local.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h"
+#include "chrome/renderer/subresource_redirect/public_image_hints_decider_agent.h"
 #include "content/public/renderer/render_frame.h"
 #include "third_party/blink/public/platform/web_loading_hints_provider.h"
 #include "third_party/blink/public/platform/web_string.h"

chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.cc → chrome/renderer/subresource_redirect/public_image_hints_decider_agent.cc

@@ -2,7 +2,7 @@
 // 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_hints_agent.h"
+#include "chrome/renderer/subresource_redirect/public_image_hints_decider_agent.h"
 
 #include "base/metrics/field_trial_params.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_params.h"
@@ -16,39 +16,44 @@
 
 namespace subresource_redirect {
 
-SubresourceRedirectHintsAgent::SubresourceRedirectHintsAgent(
+namespace {
+
+// Returns the url spec with username, password, ref fragment stripped to be
+// useful for public URL decision making.
+std::string GetURLForPublicDecision(const GURL& url) {
+  GURL::Replacements rep;
+  rep.ClearRef();
+  rep.ClearPassword();
+  rep.ClearUsername();
+  return url.ReplaceComponents(rep).spec();
+}
+
+}  // namespace
+
+PublicImageHintsDeciderAgent::PublicImageHintsDeciderAgent(
     blink::AssociatedInterfaceRegistry* associated_interfaces,
     content::RenderFrame* render_frame)
-    : content::RenderFrameObserver(render_frame),
-      content::RenderFrameObserverTracker<SubresourceRedirectHintsAgent>(
-          render_frame) {
-  DCHECK(render_frame);
+    : PublicResourceDeciderAgent(associated_interfaces, render_frame) {
   DCHECK(IsPublicImageHintsBasedCompressionEnabled());
-  // base::Unretained is safe here because |this| is created for the RenderFrame
-  // never destroyed.
-  associated_interfaces->AddInterface(
-      base::BindRepeating(&SubresourceRedirectHintsAgent::BindHintsReceiver,
-                          base::Unretained(this)));
 }
 
-SubresourceRedirectHintsAgent::~SubresourceRedirectHintsAgent() = default;
+PublicImageHintsDeciderAgent::~PublicImageHintsDeciderAgent() = default;
 
-bool SubresourceRedirectHintsAgent::IsMainFrame() const {
+bool PublicImageHintsDeciderAgent::IsMainFrame() const {
   return render_frame()->IsMainFrame();
 }
 
-void SubresourceRedirectHintsAgent::DidStartNavigation(
+void PublicImageHintsDeciderAgent::DidStartNavigation(
     const GURL& url,
     base::Optional<blink::WebNavigationType> navigation_type) {
   if (!IsMainFrame())
     return;
   // Clear the hints when a navigation starts, so that hints from previous
   // navigation do not apply in case the same renderframe is reused.
-  public_image_urls_.clear();
-  public_image_urls_received_ = false;
+  public_image_urls_ = base::nullopt;
 }
 
-void SubresourceRedirectHintsAgent::ReadyToCommitNavigation(
+void PublicImageHintsDeciderAgent::ReadyToCommitNavigation(
     blink::WebDocumentLoader* document_loader) {
   if (!IsMainFrame())
     return;
@@ -57,64 +62,62 @@ void SubresourceRedirectHintsAgent::ReadyToCommitNavigation(
   // destroyed.
   hint_receive_timeout_timer_.Start(
       FROM_HERE, base::TimeDelta::FromSeconds(GetHintsReceiveTimeout()),
-      base::BindOnce(&SubresourceRedirectHintsAgent::OnHintsReceiveTimeout,
+      base::BindOnce(&PublicImageHintsDeciderAgent::OnHintsReceiveTimeout,
                      base::Unretained(this)));
 }
 
-void SubresourceRedirectHintsAgent::OnDestruct() {
+void PublicImageHintsDeciderAgent::OnDestruct() {
   delete this;
 }
 
-void SubresourceRedirectHintsAgent::SetCompressPublicImagesHints(
+void PublicImageHintsDeciderAgent::SetCompressPublicImagesHints(
     mojom::CompressPublicImagesHintsPtr images_hints) {
   if (!IsMainFrame())
     return;
-  DCHECK(public_image_urls_.empty());
-  DCHECK(!public_image_urls_received_);
+  DCHECK(!public_image_urls_);
   public_image_urls_ = images_hints->image_urls;
-  public_image_urls_received_ = true;
   hint_receive_timeout_timer_.Stop();
   RecordImageHintsUnavailableMetrics();
 }
 
-SubresourceRedirectHintsAgent::RedirectResult
-SubresourceRedirectHintsAgent::ShouldRedirectImage(const GURL& url) const {
-  if (!public_image_urls_received_) {
+base::Optional<RedirectResult>
+PublicImageHintsDeciderAgent::ShouldRedirectSubresource(
+    const GURL& url,
+    ShouldRedirectDecisionCallback callback) {
+  if (!public_image_urls_)
     return RedirectResult::kIneligibleImageHintsUnavailable;
-  }
 
-  GURL::Replacements rep;
-  rep.ClearRef();
-  // TODO(rajendrant): Skip redirection if the URL contains username or password
-  if (public_image_urls_.find(url.ReplaceComponents(rep).spec()) !=
-      public_image_urls_.end()) {
+  if (public_image_urls_->find(GetURLForPublicDecision(url)) !=
+      public_image_urls_->end()) {
     return RedirectResult::kRedirectable;
   }
 
   return RedirectResult::kIneligibleMissingInImageHints;
 }
 
-void SubresourceRedirectHintsAgent::RecordMetricsOnLoadFinished(
+void PublicImageHintsDeciderAgent::RecordMetricsOnLoadFinished(
     const GURL& url,
     int64_t content_length,
     RedirectResult redirect_result) {
   if (redirect_result == RedirectResult::kIneligibleImageHintsUnavailable) {
-    GURL::Replacements rep;
-    rep.ClearRef();
     unavailable_image_hints_urls_.insert(
-        std::make_pair(url.ReplaceComponents(rep).spec(), content_length));
+        std::make_pair(GetURLForPublicDecision(url), content_length));
     return;
   }
   RecordMetrics(content_length, redirect_result);
 }
 
-void SubresourceRedirectHintsAgent::ClearImageHints() {
-  public_image_urls_.clear();
+void PublicImageHintsDeciderAgent::ClearImageHints() {
+  if (public_image_urls_)
+    public_image_urls_->clear();
 }
 
-void SubresourceRedirectHintsAgent::RecordMetrics(
+void PublicImageHintsDeciderAgent::RecordMetrics(
     int64_t content_length,
     RedirectResult redirect_result) const {
+  // TODO(1156757): Reduce the number of ukm records, by aggregating the
+  // image bytes per RedirectResult and then recording once every k seconds, or
+  // k images.
   if (!render_frame() || !render_frame()->GetWebFrame())
     return;
 
@@ -132,12 +135,12 @@ void SubresourceRedirectHintsAgent::RecordMetrics(
       public_image_compression_data_use.SetIneligibleImageHintsUnavailableBytes(
           content_length);
       break;
-    case RedirectResult::kIneligibleImageHintsUnavailableButRedirectableBytes:
+    case RedirectResult::kIneligibleImageHintsUnavailableButRedirectable:
       public_image_compression_data_use
           .SetIneligibleImageHintsUnavailableButCompressibleBytes(
               content_length);
       break;
-    case RedirectResult::kIneligibleImageHintsUnavailableAndMissingInHintsBytes:
+    case RedirectResult::kIneligibleImageHintsUnavailableAndMissingInHints:
       public_image_compression_data_use
           .SetIneligibleImageHintsUnavailableAndMissingInHintsBytes(
               content_length);
@@ -146,7 +149,9 @@ void SubresourceRedirectHintsAgent::RecordMetrics(
       public_image_compression_data_use.SetIneligibleMissingInImageHintsBytes(
           content_length);
       break;
-    case RedirectResult::kIneligibleOtherImage:
+    case RedirectResult::kUnknown:
+    case RedirectResult::kIneligibleRedirectFailed:
+    case RedirectResult::kIneligibleBlinkDisallowed:
       public_image_compression_data_use.SetIneligibleOtherImageBytes(
           content_length);
       break;
@@ -159,20 +164,21 @@ void SubresourceRedirectHintsAgent::RecordMetrics(
   public_image_compression_data_use.Record(ukm_recorder.get());
 }
 
-void SubresourceRedirectHintsAgent::OnHintsReceiveTimeout() {
+void PublicImageHintsDeciderAgent::OnHintsReceiveTimeout() {
   RecordImageHintsUnavailableMetrics();
 }
 
-void SubresourceRedirectHintsAgent::RecordImageHintsUnavailableMetrics() {
+void PublicImageHintsDeciderAgent::RecordImageHintsUnavailableMetrics() {
   for (const auto& resource : unavailable_image_hints_urls_) {
     auto redirect_result = RedirectResult::kIneligibleImageHintsUnavailable;
-    if (public_image_urls_received_) {
-      if (public_image_urls_.find(resource.first) != public_image_urls_.end()) {
-        redirect_result = RedirectResult::
-            kIneligibleImageHintsUnavailableButRedirectableBytes;
+    if (public_image_urls_) {
+      if (public_image_urls_->find(resource.first) !=
+          public_image_urls_->end()) {
+        redirect_result =
+            RedirectResult::kIneligibleImageHintsUnavailableButRedirectable;
       } else {
-        redirect_result = RedirectResult::
-            kIneligibleImageHintsUnavailableAndMissingInHintsBytes;
+        redirect_result =
+            RedirectResult::kIneligibleImageHintsUnavailableAndMissingInHints;
       }
     }
     RecordMetrics(resource.second, redirect_result);
@@ -180,21 +186,10 @@ void SubresourceRedirectHintsAgent::RecordImageHintsUnavailableMetrics() {
   unavailable_image_hints_urls_.clear();
 }
 
-void SubresourceRedirectHintsAgent::NotifyHttpsImageCompressionFetchFailed(
+void PublicImageHintsDeciderAgent::NotifyCompressedResourceFetchFailed(
     base::TimeDelta retry_after) {
-  if (!subresource_redirect_service_remote_) {
-    render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(
-        &subresource_redirect_service_remote_);
-  }
-  subresource_redirect_service_remote_->NotifyCompressedImageFetchFailed(
-      retry_after);
+  PublicResourceDeciderAgent::NotifyCompressedResourceFetchFailed(retry_after);
   ClearImageHints();
 }
 
-void SubresourceRedirectHintsAgent::BindHintsReceiver(
-    mojo::PendingAssociatedReceiver<mojom::SubresourceRedirectHintsReceiver>
-        receiver) {
-  subresource_redirect_hints_receiver_.Bind(std::move(receiver));
-}
-
 }  // namespace subresource_redirect

chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h → chrome/renderer/subresource_redirect/public_image_hints_decider_agent.h

@@ -2,89 +2,35 @@
 // 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_HINTS_AGENT_H_
-#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_HINTS_AGENT_H_
+#ifndef CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_IMAGE_HINTS_DECIDER_AGENT_H_
+#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_IMAGE_HINTS_DECIDER_AGENT_H_
 
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/timer/timer.h"
-#include "chrome/common/previews_resource_loading_hints.mojom.h"
-#include "chrome/common/subresource_redirect_service.mojom.h"
-#include "content/public/renderer/render_frame_observer.h"
-#include "content/public/renderer/render_frame_observer_tracker.h"
-#include "mojo/public/cpp/bindings/associated_receiver.h"
-#include "mojo/public/cpp/bindings/pending_associated_receiver.h"
-#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
-#include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
+#include "chrome/renderer/subresource_redirect/public_resource_decider_agent.h"
+#include "chrome/renderer/subresource_redirect/redirect_result.h"
 #include "url/gurl.h"
 
 namespace subresource_redirect {
 
-// Holds the public image URL hints to be queried by URL loader throttles. Only
-// redirects public images for mainframes.
-class SubresourceRedirectHintsAgent
-    : public content::RenderFrameObserver,
-      public mojom::SubresourceRedirectHintsReceiver,
-      public content::RenderFrameObserverTracker<
-          SubresourceRedirectHintsAgent> {
+// The decider agent implementation that allows image compression based on
+// public image hints received. Only redirects public images for mainframes.
+class PublicImageHintsDeciderAgent : public PublicResourceDeciderAgent {
  public:
-  enum class RedirectResult {
-    // The image was found in the image hints and is eligible to be redirected
-    // to compressed version.
-    kRedirectable,
-
-    // Possible reasons for ineligibility.
-    // because the image hint list was not retrieved within certain time limit
-    // of navigation start,
-    kIneligibleImageHintsUnavailable,
-
-    // because the image hint list was not retrieved at the time of image fetch,
-    // but the image URL was found in the hint list, which finished fetching
-    // later.
-    kIneligibleImageHintsUnavailableButRedirectableBytes,
-
-    // because the image hint list was not retrieved at the time of image fetch,
-    // and the image URL was not in the hint list as well, which finished
-    // fetching later.
-    kIneligibleImageHintsUnavailableAndMissingInHintsBytes,
-
-    // because the image URL was not found in the image hints.
-    kIneligibleMissingInImageHints,
-
-    // because of other reasons such as subframe images, Blink did not allow the
-    // redirect due to non <img> element, security limitations, javascript
-    // initiated image, etc.
-    kIneligibleOtherImage
-  };
-
-  explicit SubresourceRedirectHintsAgent(
+  PublicImageHintsDeciderAgent(
       blink::AssociatedInterfaceRegistry* associated_interfaces,
       content::RenderFrame* render_frame);
-  ~SubresourceRedirectHintsAgent() override;
+  ~PublicImageHintsDeciderAgent() override;
 
-  SubresourceRedirectHintsAgent(const SubresourceRedirectHintsAgent&) = delete;
-  SubresourceRedirectHintsAgent& operator=(
-      const SubresourceRedirectHintsAgent&) = delete;
-
-  // mojom::SubresourceRedirectHintsReceiver:
-  void SetCompressPublicImagesHints(
-      mojom::CompressPublicImagesHintsPtr images_hints) override;
-
-  RedirectResult ShouldRedirectImage(const GURL& url) const;
-
-  // Record metrics when the resource load is finished.
-  void RecordMetricsOnLoadFinished(const GURL& url,
-                                   int64_t content_length,
-                                   RedirectResult redirect_result);
-
-  // Notifies the browser process that https image compression fetch had failed.
-  void NotifyHttpsImageCompressionFetchFailed(base::TimeDelta retry_after);
-
-  // Clears the image hint urls.
-  void ClearImageHints();
+  PublicImageHintsDeciderAgent(const PublicImageHintsDeciderAgent&) = delete;
+  PublicImageHintsDeciderAgent& operator=(const PublicImageHintsDeciderAgent&) =
+      delete;
 
  private:
+  friend class SubresourceRedirectPublicImageHintsDeciderAgentTest;
+
   // content::RenderFrameObserver:
   void DidStartNavigation(
       const GURL& url,
@@ -93,43 +39,55 @@ class SubresourceRedirectHintsAgent
       blink::WebDocumentLoader* document_loader) override;
   void OnDestruct() override;
 
+  // mojom::SubresourceRedirectHintsReceiver:
+  void SetCompressPublicImagesHints(
+      mojom::CompressPublicImagesHintsPtr images_hints) override;
+
+  // PublicResourceDeciderAgent:
+  base::Optional<RedirectResult> ShouldRedirectSubresource(
+      const GURL& url,
+      ShouldRedirectDecisionCallback callback) override;
+  void RecordMetricsOnLoadFinished(const GURL& url,
+                                   int64_t content_length,
+                                   RedirectResult redirect_result) override;
+  void NotifyCompressedResourceFetchFailed(
+      base::TimeDelta retry_after) override;
+
   bool IsMainFrame() const;
 
+  // Clears the image hint urls.
+  void ClearImageHints();
+
+  // Called when the hint receive timer expires to flush the metrics for
+  // resources that no hint has been received and clears
+  // |unavailable_image_hints_urls_|.
   void OnHintsReceiveTimeout();
 
+  // Records the breakdown of bytes to UKM metrics.
   void RecordMetrics(int64_t content_length,
                      RedirectResult redirect_result) const;
 
   // Record the breakdown of bytes for unavailable image hints, whether the
-  // hints fetch timed out, or the image was not in the delayed hints, or the
+  // hints fetch timed out, the image was not in the delayed hints, or the
   // image was in the delayed hints.
   void RecordImageHintsUnavailableMetrics();
 
-  // Binds the mojo hints receiver pipe.
-  void BindHintsReceiver(
-      mojo::PendingAssociatedReceiver<mojom::SubresourceRedirectHintsReceiver>
-          receiver);
-
-  bool public_image_urls_received_ = false;
-  base::flat_set<std::string> public_image_urls_;
+  // The raw spec of image urls that are determined public, received from image
+  // hints. Will be base::nullopt after the navigation starts and until the
+  // hints have been received.
+  base::Optional<base::flat_set<std::string>> public_image_urls_;
 
   // To trigger the timeout for the hints to be received from the time
   // navigation starts.
   base::OneShotTimer hint_receive_timeout_timer_;
 
   // The urls and resource size of images that were not redirectable due to
-  // image hints was unavailable at the time of image fetch. This list is kept
-  // until hints are received or it times out and used to record metrics.
+  // image hints were not unavailable at the time of image fetch. This list is
+  // kept until hints are received or hints retrieval times out. This is used
+  // for metrics purposes.
   base::flat_set<std::pair<std::string, int64_t>> unavailable_image_hints_urls_;
-
-  mojo::AssociatedReceiver<mojom::SubresourceRedirectHintsReceiver>
-      subresource_redirect_hints_receiver_{this};
-
-  mojo::AssociatedRemote<
-      subresource_redirect::mojom::SubresourceRedirectService>
-      subresource_redirect_service_remote_;
 };
 
 }  // namespace subresource_redirect
 
-#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_HINTS_AGENT_H_
+#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_IMAGE_HINTS_DECIDER_AGENT_H_

chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle_browsertest.cc → chrome/renderer/subresource_redirect/public_image_hints_decider_agent_browsertest.cc

@@ -5,8 +5,8 @@
 #include "chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h"
 
 #include "base/test/scoped_feature_list.h"
-#include "chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h"
+#include "chrome/renderer/subresource_redirect/public_image_hints_decider_agent.h"
+#include "chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_util.h"
 #include "chrome/test/base/chrome_render_view_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -25,9 +25,7 @@ namespace subresource_redirect {
 
 int kRenderFrameID = 1;
 
-namespace {
-
-class SubresourceRedirectPublicImageHintsURLLoaderThrottleTest
+class SubresourceRedirectPublicImageHintsDeciderAgentTest
     : public ChromeRenderViewTest {
  public:
   void DisableSubresourceRedirectFeature() {
@@ -38,12 +36,12 @@ class SubresourceRedirectPublicImageHintsURLLoaderThrottleTest
 
   void SetCompressPublicImagesHints(
       const std::vector<std::string>& public_image_urls) {
-    subresource_redirect_hints_agent_->SetCompressPublicImagesHints(
+    public_image_hints_decider_agent_->SetCompressPublicImagesHints(
         mojom::CompressPublicImagesHints::New(public_image_urls));
   }
 
-  std::unique_ptr<PublicImageHintsURLLoaderThrottle>
-  CreatePublicImageHintsURLLoaderThrottle(
+  std::unique_ptr<SubresourceRedirectURLLoaderThrottle>
+  CreateSubresourceRedirectURLLoaderThrottle(
       const GURL& url,
       network::mojom::RequestDestination request_destination,
       int previews_state) {
@@ -55,11 +53,22 @@ class SubresourceRedirectPublicImageHintsURLLoaderThrottleTest
                request, view_->GetMainRenderFrame()->GetRoutingID())
                .get() != nullptr);
 
-    return std::make_unique<PublicImageHintsURLLoaderThrottle>(
+    return std::make_unique<SubresourceRedirectURLLoaderThrottle>(
         view_->GetMainRenderFrame()->GetRoutingID(),
         previews_state & blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON);
   }
 
+  void VerifyRedirectResult(SubresourceRedirectURLLoaderThrottle* throttle,
+                            RedirectResult redirect_result) {
+    EXPECT_EQ(throttle->redirect_result_, redirect_result);
+  }
+
+  void VerifyRedirectState(
+      SubresourceRedirectURLLoaderThrottle* throttle,
+      SubresourceRedirectURLLoaderThrottle::RedirectState redirect_state) {
+    EXPECT_EQ(throttle->redirect_state_, redirect_state);
+  }
+
  protected:
   void SetUp() override {
     ChromeRenderViewTest::SetUp();
@@ -67,16 +76,23 @@ class SubresourceRedirectPublicImageHintsURLLoaderThrottleTest
         {{blink::features::kSubresourceRedirect,
           {{"enable_subresource_server_redirect", "true"}}}},
         {});
-    subresource_redirect_hints_agent_ = new SubresourceRedirectHintsAgent(
-        &associated_interfaces_, view_->GetMainRenderFrame());
+    public_image_hints_decider_agent_ =
+        std::make_unique<PublicImageHintsDeciderAgent>(
+            &associated_interfaces_, view_->GetMainRenderFrame());
+  }
+
+  void TearDown() override {
+    public_image_hints_decider_agent_.reset();
+    ChromeRenderViewTest::TearDown();
   }
 
  private:
-  SubresourceRedirectHintsAgent* subresource_redirect_hints_agent_;
+  std::unique_ptr<PublicImageHintsDeciderAgent>
+      public_image_hints_decider_agent_;
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
+TEST_F(SubresourceRedirectPublicImageHintsDeciderAgentTest,
        TestMaybeCreateThrottle) {
   struct TestCase {
     bool data_saver_enabled;
@@ -118,19 +134,22 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
     request.SetPreviewsState(test_case.previews_state);
     request.SetUrl(GURL(test_case.url));
     request.SetRequestDestination(test_case.destination);
-    EXPECT_EQ(test_case.expected_is_throttle_created,
-              SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
-                  request, kRenderFrameID) != nullptr);
+    auto throttle = SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
+        request, kRenderFrameID);
+    EXPECT_EQ(test_case.expected_is_throttle_created, throttle != nullptr);
+    if (throttle)
+      VerifyRedirectResult(throttle.get(), RedirectResult::kRedirectable);
   }
 }
 
-TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
+TEST_F(SubresourceRedirectPublicImageHintsDeciderAgentTest,
        TestGetSubresourceURL) {
   struct TestCase {
     int previews_state;
     GURL original_url;
-    GURL redirected_subresource_url;  // Empty URL means there will be no
-                                      // redirect.
+    GURL redirected_subresource_url;  // Empty URL indicates no redirect.
+    RedirectResult expected_redirect_result;
+    SubresourceRedirectURLLoaderThrottle::RedirectState expected_redirect_state;
   };
 
   const TestCase kTestCases[]{
@@ -138,12 +157,18 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
           GURL("https://www.test.com/public_img.jpg"),
           GetSubresourceURLForURL(GURL("https://www.test.com/public_img.jpg")),
+          RedirectResult::kRedirectable,
+          SubresourceRedirectURLLoaderThrottle::RedirectState::
+              kRedirectAttempted,
       },
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
           GURL("https://www.test.com/public_img.jpg#anchor"),
           GetSubresourceURLForURL(
               GURL("https://www.test.com/public_img.jpg#anchor")),
+          RedirectResult::kRedirectable,
+          SubresourceRedirectURLLoaderThrottle::RedirectState::
+              kRedirectAttempted,
       },
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
@@ -152,17 +177,34 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
           GetSubresourceURLForURL(
               GURL("https://www.test.com/"
                    "public_img.jpg?public_arg1=bar&public_arg2")),
+          RedirectResult::kRedirectable,
+          SubresourceRedirectURLLoaderThrottle::RedirectState::
+              kRedirectAttempted,
       },
       // Private images will not be redirected.
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
           GURL("https://www.test.com/private_img.jpg"),
           GURL(),
+          RedirectResult::kIneligibleMissingInImageHints,
+          SubresourceRedirectURLLoaderThrottle::RedirectState::
+              kRedirectNotAllowedByDecider,
       },
       {
           blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
           GURL("https://www.test.com/public_img.jpg&private_arg1=foo"),
           GURL(),
+          RedirectResult::kIneligibleMissingInImageHints,
+          SubresourceRedirectURLLoaderThrottle::RedirectState::
+              kRedirectNotAllowedByDecider,
+      },
+      // Image disallowed by blink will not be redirected.
+      {
+          blink::PreviewsTypes::PREVIEWS_UNSPECIFIED,
+          GURL("https://www.test.com/public_img.jpg"),
+          GURL(),
+          RedirectResult::kIneligibleBlinkDisallowed,
+          SubresourceRedirectURLLoaderThrottle::RedirectState::kNone,
       },
   };
   blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
@@ -173,14 +215,14 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
        "https://www.test.com/public_img.jpg?public_arg1=bar&public_arg2"});
 
   for (const TestCase& test_case : kTestCases) {
-    auto throttle = CreatePublicImageHintsURLLoaderThrottle(
+    auto throttle = CreateSubresourceRedirectURLLoaderThrottle(
         test_case.original_url, network::mojom::RequestDestination::kImage,
         test_case.previews_state);
     network::ResourceRequest request;
     request.url = test_case.original_url;
     request.destination = network::mojom::RequestDestination::kImage;
     request.previews_state = test_case.previews_state;
-    bool defer = true;
+    bool defer = false;
     throttle->WillStartRequest(&request, &defer);
 
     EXPECT_FALSE(defer);
@@ -189,16 +231,18 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
     } else {
       EXPECT_EQ(request.url, test_case.original_url);
     }
+    VerifyRedirectResult(throttle.get(), test_case.expected_redirect_result);
+    VerifyRedirectState(throttle.get(), test_case.expected_redirect_state);
   }
 }
 
-TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
+TEST_F(SubresourceRedirectPublicImageHintsDeciderAgentTest,
        DeferOverridenToFalse) {
   blink::WebNetworkStateNotifier::SetSaveDataEnabled(true);
 
   SetCompressPublicImagesHints({"https://www.test.com/test.jpg"});
 
-  auto throttle = CreatePublicImageHintsURLLoaderThrottle(
+  auto throttle = CreateSubresourceRedirectURLLoaderThrottle(
       GURL("https://www.test.com/test.jpg"),
       network::mojom::RequestDestination::kImage,
       blink::PreviewsTypes::SUBRESOURCE_REDIRECT_ON);
@@ -212,5 +256,4 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
   EXPECT_FALSE(defer);
 }
 
-}  // namespace
 }  // namespace subresource_redirect

chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle.cc

-// Copyright 2020 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/public_image_hints_url_loader_throttle.h"
-
-#include "base/memory/ptr_util.h"
-#include "base/metrics/field_trial_params.h"
-#include "base/metrics/histogram_macros.h"
-#include "base/time/time.h"
-#include "chrome/renderer/previews/resource_loading_hints_agent.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_params.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_util.h"
-#include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
-#include "content/public/renderer/render_frame.h"
-#include "net/base/escape.h"
-#include "net/base/load_flags.h"
-#include "net/http/http_status_code.h"
-#include "net/http/http_util.h"
-#include "services/network/public/mojom/fetch_api.mojom-shared.h"
-#include "services/network/public/mojom/url_response_head.mojom.h"
-#include "third_party/blink/public/common/features.h"
-#include "third_party/blink/public/common/loader/previews_state.h"
-#include "third_party/blink/public/platform/web_network_state_notifier.h"
-#include "third_party/blink/public/platform/web_url.h"
-#include "third_party/blink/public/platform/web_url_request.h"
-
-namespace subresource_redirect {
-
-PublicImageHintsURLLoaderThrottle::PublicImageHintsURLLoaderThrottle(
-    int render_frame_id,
-    bool allowed_to_redirect)
-    : SubresourceRedirectURLLoaderThrottle(render_frame_id) {
-  redirect_result_ =
-      allowed_to_redirect
-          ? SubresourceRedirectHintsAgent::RedirectResult::kRedirectable
-          : SubresourceRedirectHintsAgent::RedirectResult::
-                kIneligibleOtherImage;
-}
-
-PublicImageHintsURLLoaderThrottle::~PublicImageHintsURLLoaderThrottle() =
-    default;
-
-SubresourceRedirectHintsAgent*
-PublicImageHintsURLLoaderThrottle::GetSubresourceRedirectHintsAgent() {
-  return subresource_redirect::SubresourceRedirectHintsAgent::Get(
-      GetRenderFrame());
-}
-
-base::Optional<bool> PublicImageHintsURLLoaderThrottle::ShouldRedirectImage(
-    const GURL& url,
-    SubresourceRedirectURLLoaderThrottle::RedirectDecisionCallback callback) {
-  if (redirect_result_ !=
-      SubresourceRedirectHintsAgent::RedirectResult::kRedirectable) {
-    return false;
-  }
-
-  auto* subresource_redirect_hints_agent = GetSubresourceRedirectHintsAgent();
-  if (!subresource_redirect_hints_agent)
-    return false;
-
-  redirect_result_ = subresource_redirect_hints_agent->ShouldRedirectImage(url);
-  if (redirect_result_ !=
-      SubresourceRedirectHintsAgent::RedirectResult::kRedirectable) {
-    return false;
-  }
-  return true;
-}
-
-void PublicImageHintsURLLoaderThrottle::OnRedirectedLoadCompleteWithError() {
-  redirect_result_ =
-      SubresourceRedirectHintsAgent::RedirectResult::kIneligibleOtherImage;
-}
-
-void PublicImageHintsURLLoaderThrottle::RecordMetricsOnLoadFinished(
-    const GURL& url,
-    int64_t content_length) {
-  auto* subresource_redirect_hints_agent = GetSubresourceRedirectHintsAgent();
-  if (subresource_redirect_hints_agent) {
-    subresource_redirect_hints_agent->RecordMetricsOnLoadFinished(
-        url, content_length, redirect_result_);
-  }
-}
-
-}  // namespace subresource_redirect

chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle.h

-// Copyright 2020 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_PUBLIC_IMAGE_HINTS_URL_LOADER_THROTTLE_H_
-#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_IMAGE_HINTS_URL_LOADER_THROTTLE_H_
-
-#include "base/macros.h"
-#include "base/timer/timer.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h"
-#include "chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h"
-#include "third_party/blink/public/common/loader/url_loader_throttle.h"
-
-namespace subresource_redirect {
-
-// This class allows image compression based on public image hints received.
-class PublicImageHintsURLLoaderThrottle
-    : public SubresourceRedirectURLLoaderThrottle {
- public:
-  PublicImageHintsURLLoaderThrottle(int render_frame_id,
-                                    bool allowed_to_redirect);
-
-  ~PublicImageHintsURLLoaderThrottle() override;
-
-  PublicImageHintsURLLoaderThrottle(const PublicImageHintsURLLoaderThrottle&) =
-      delete;
-  PublicImageHintsURLLoaderThrottle& operator=(
-      const PublicImageHintsURLLoaderThrottle&) = delete;
-
-  // SubresourceRedirectURLLoaderThrottle:
-  base::Optional<bool> ShouldRedirectImage(
-      const GURL& url,
-      SubresourceRedirectURLLoaderThrottle::RedirectDecisionCallback callback)
-      override;
-  void OnRedirectedLoadCompleteWithError() override;
-  void RecordMetricsOnLoadFinished(const GURL& url,
-                                   int64_t content_length) override;
-
- private:
-  friend class TestPublicImageHintsURLLoaderThrottle;
-
-  SubresourceRedirectHintsAgent* GetSubresourceRedirectHintsAgent();
-
-  // Whether the subresource can be redirected or not and what was the reason if
-  // its not eligible.
-  SubresourceRedirectHintsAgent::RedirectResult redirect_result_;
-};
-
-}  // namespace subresource_redirect
-
-#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_IMAGE_HINTS_URL_LOADER_THROTTLE_H_

chrome/renderer/subresource_redirect/public_resource_decider.h

+// Copyright 2020 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_PUBLIC_RESOURCE_DECIDER_H_
+#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_RESOURCE_DECIDER_H_
+
+#include "base/bind.h"
+#include "base/optional.h"
+#include "chrome/renderer/subresource_redirect/redirect_result.h"
+#include "url/gurl.h"
+
+namespace subresource_redirect {
+
+// Interface for the decider agent classes that decide whether a resource is
+// considered public and eligible for redirection for compression. Also allows
+// coverage metrics to be recorded for the resource load.
+class PublicResourceDecider {
+ public:
+  using ShouldRedirectDecisionCallback =
+      base::OnceCallback<void(RedirectResult)>;
+
+  // Determine whether the subresource url should be redirected. When the
+  // determination can be made immediately, the decision should be returned.
+  // Otherwise base::nullopt should be returned and the callback should be
+  // invoked with the decision asynchronously.
+  virtual base::Optional<RedirectResult> ShouldRedirectSubresource(
+      const GURL& url,
+      ShouldRedirectDecisionCallback callback) = 0;
+
+  // Notifies the decider that the subresource load finished.
+  virtual void RecordMetricsOnLoadFinished(const GURL& url,
+                                           int64_t content_length,
+                                           RedirectResult redirect_result) = 0;
+
+  // Notifies that compressed resource fetch had failed. |retry_after| indicates
+  // the duration returned by the compression server, until which subsequent
+  // fetches to compression server should be blocked.
+  virtual void NotifyCompressedResourceFetchFailed(
+      base::TimeDelta retry_after) = 0;
+};
+
+}  // namespace subresource_redirect
+
+#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_RESOURCE_DECIDER_H_

chrome/renderer/subresource_redirect/public_resource_decider_agent.cc

+// Copyright 2020 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/public_resource_decider_agent.h"
+
+#include "content/public/renderer/render_frame.h"
+
+namespace subresource_redirect {
+
+PublicResourceDeciderAgent::PublicResourceDeciderAgent(
+    blink::AssociatedInterfaceRegistry* associated_interfaces,
+    content::RenderFrame* render_frame)
+    : content::RenderFrameObserver(render_frame),
+      content::RenderFrameObserverTracker<PublicResourceDeciderAgent>(
+          render_frame) {
+  DCHECK(render_frame);
+  // base::Unretained is safe here because |this| is created for the RenderFrame
+  // and never destroyed.
+  associated_interfaces->AddInterface(base::BindRepeating(
+      &PublicResourceDeciderAgent::BindHintsReceiver, base::Unretained(this)));
+}
+
+PublicResourceDeciderAgent::~PublicResourceDeciderAgent() = default;
+
+void PublicResourceDeciderAgent::OnDestruct() {
+  delete this;
+}
+
+void PublicResourceDeciderAgent::NotifyCompressedResourceFetchFailed(
+    base::TimeDelta retry_after) {
+  if (!subresource_redirect_service_remote_) {
+    render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(
+        &subresource_redirect_service_remote_);
+  }
+  subresource_redirect_service_remote_->NotifyCompressedImageFetchFailed(
+      retry_after);
+}
+
+void PublicResourceDeciderAgent::BindHintsReceiver(
+    mojo::PendingAssociatedReceiver<mojom::SubresourceRedirectHintsReceiver>
+        receiver) {
+  subresource_redirect_hints_receiver_.Bind(std::move(receiver));
+}
+
+}  // namespace subresource_redirect

chrome/renderer/subresource_redirect/public_resource_decider_agent.h

+// Copyright 2020 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_PUBLIC_RESOURCE_DECIDER_AGENT_H_
+#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_RESOURCE_DECIDER_AGENT_H_
+
+#include "base/bind.h"
+#include "base/optional.h"
+#include "chrome/common/subresource_redirect_service.mojom.h"
+#include "chrome/renderer/subresource_redirect/public_resource_decider.h"
+#include "chrome/renderer/subresource_redirect/redirect_result.h"
+#include "content/public/renderer/render_frame_observer.h"
+#include "content/public/renderer/render_frame_observer_tracker.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/pending_associated_receiver.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
+#include "url/gurl.h"
+
+namespace content {
+class RenderFrame;
+}  // namespace content
+
+namespace subresource_redirect {
+
+// Base class for the decider agent classes that decide whether a resource is
+// considered public and eligible for redirection for compression. Also allows
+// coverage metrics to be recorded for the resource load. This class is also the
+// point of contact for browser mojo.
+class PublicResourceDeciderAgent
+    : public content::RenderFrameObserver,
+      public mojom::SubresourceRedirectHintsReceiver,
+      public content::RenderFrameObserverTracker<PublicResourceDeciderAgent>,
+      public PublicResourceDecider {
+ public:
+  PublicResourceDeciderAgent(
+      blink::AssociatedInterfaceRegistry* associated_interfaces,
+      content::RenderFrame* render_frame);
+  ~PublicResourceDeciderAgent() override;
+
+  void NotifyCompressedResourceFetchFailed(
+      base::TimeDelta retry_after) override;
+
+ private:
+  // content::RenderFrameObserver:
+  void OnDestruct() override;
+
+  // Binds the mojo hints receiver pipe.
+  void BindHintsReceiver(
+      mojo::PendingAssociatedReceiver<mojom::SubresourceRedirectHintsReceiver>
+          receiver);
+
+  mojo::AssociatedReceiver<mojom::SubresourceRedirectHintsReceiver>
+      subresource_redirect_hints_receiver_{this};
+
+  mojo::AssociatedRemote<
+      subresource_redirect::mojom::SubresourceRedirectService>
+      subresource_redirect_service_remote_;
+};
+
+}  // namespace subresource_redirect
+
+#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_PUBLIC_RESOURCE_DECIDER_AGENT_H_

chrome/renderer/subresource_redirect/redirect_result.h

+// Copyright 2020 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_REDIRECT_RESULT_H_
+#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_REDIRECT_RESULT_H_
+
+namespace subresource_redirect {
+
+// Enumerates the different results possible for subresource redirection, such
+// as redirectable or different reasons of ineligibility.
+enum class RedirectResult {
+  kUnknown = 0,
+
+  // The image was determined as public and is eligible to be redirected
+  // to a compressed version.
+  kRedirectable,
+
+  // Possible reasons for ineligibility:
+
+  // Because of reasons Blink could disallow compression such as non <img>
+  // element, CSP/CORS security restrictions, javascript initiated image, etc.
+  kIneligibleBlinkDisallowed,
+
+  // Because the compressed subresource fetch failed, and then the original
+  // subresource was loaded.
+  kIneligibleRedirectFailed,
+
+  // Possible reasons for ineligibility due to public image hints approach:
+
+  // Because the image hint list was not retrieved within certain time limit
+  // of navigation start,
+  kIneligibleImageHintsUnavailable,
+
+  // Because the image hint list was not retrieved at the time of image fetch,
+  // but the image URL was found in the hint list, which finished fetching
+  // later.
+  kIneligibleImageHintsUnavailableButRedirectable,
+
+  // Because the image hint list was not retrieved at the time of image fetch,
+  // and the image URL was not in the hint list as well, which finished
+  // fetching later.
+  kIneligibleImageHintsUnavailableAndMissingInHints,
+
+  // Because the image URL was not found in the image hints.
+  kIneligibleMissingInImageHints,
+
+  kMaxValue = RedirectResult::kIneligibleMissingInImageHints
+};
+
+}  // namespace subresource_redirect
+
+#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_REDIRECT_RESULT_H_

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.cc

@@ -9,7 +9,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "chrome/renderer/previews/resource_loading_hints_agent.h"
-#include "chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle.h"
+#include "chrome/renderer/subresource_redirect/redirect_result.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_params.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_util.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
@@ -37,6 +37,12 @@ bool IsCompressionServerOrigin(const GURL& url) {
          (url.scheme() == compression_server.scheme());
 }
 
+// Returns the decider for the render frame
+PublicResourceDeciderAgent* GetPublicResourceDeciderAgent(int render_frame_id) {
+  return PublicResourceDeciderAgent::Get(
+      content::RenderFrame::FromRoutingID(render_frame_id));
+}
+
 }  // namespace
 
 // static
@@ -52,7 +58,7 @@ SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
       request.GetRequestContext() !=
           blink::mojom::RequestContextType::FAVICON) {
     return base::WrapUnique<SubresourceRedirectURLLoaderThrottle>(
-        new PublicImageHintsURLLoaderThrottle(
+        new SubresourceRedirectURLLoaderThrottle(
             render_frame_id, request.GetPreviewsState() &
                                  blink::PreviewsTypes::kSubresourceRedirectOn));
   }
@@ -60,8 +66,13 @@ SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
 }
 
 SubresourceRedirectURLLoaderThrottle::SubresourceRedirectURLLoaderThrottle(
-    int render_frame_id)
-    : render_frame_id_(render_frame_id) {}
+    int render_frame_id,
+    bool allowed_to_redirect)
+    : render_frame_id_(render_frame_id) {
+  redirect_result_ = allowed_to_redirect
+                         ? RedirectResult::kRedirectable
+                         : RedirectResult::kIneligibleBlinkDisallowed;
+}
 
 SubresourceRedirectURLLoaderThrottle::~SubresourceRedirectURLLoaderThrottle() =
     default;
@@ -73,6 +84,9 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
   DCHECK_EQ(request->destination, network::mojom::RequestDestination::kImage);
   DCHECK(request->url.SchemeIs(url::kHttpsScheme));
 
+  if (redirect_result_ != RedirectResult::kRedirectable)
+    return;
+
   // Do not redirect if its already a litepage subresource.
   if (IsCompressionServerOrigin(request->url))
     return;
@@ -80,17 +94,22 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
   if (!ShouldCompressionServerRedirectSubresource())
     return;
 
-  auto redirect_decision = ShouldRedirectImage(
-      request->url,
-      base::BindOnce(
-          &SubresourceRedirectURLLoaderThrottle::NotifyRedirectDeciderDecision,
-          weak_ptr_factory_.GetWeakPtr()));
-  if (!redirect_decision) {
+  auto* public_resource_decider_agent =
+      GetPublicResourceDeciderAgent(render_frame_id_);
+  if (!public_resource_decider_agent)
+    return;
+
+  auto redirect_result =
+      public_resource_decider_agent->ShouldRedirectSubresource(
+          request->url, base::BindOnce(&SubresourceRedirectURLLoaderThrottle::
+                                           NotifyRedirectDeciderDecision,
+                                       weak_ptr_factory_.GetWeakPtr()));
+  if (!redirect_result) {
     // Decision cannot be made yet. Defer the subresource and change the URL to
     // compression server URL. The NotifyRedirectDeciderDecision callback will
     // continue with compression or disable compression by resetting to original
     // URL.
-    redirect_state_ = RedirectState::kDeciderDecisionPending;
+    redirect_state_ = RedirectState::kRedirectDecisionPending;
     *defer = true;
     request->url = GetSubresourceURLForURL(request->url);
     return;
@@ -98,28 +117,35 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
 
   // The decider decision has been made.
   *defer = false;
-  if (*redirect_decision) {
-    redirect_state_ = RedirectState::kRedirectAttempted;
-    request->url = GetSubresourceURLForURL(request->url);
-    StartRedirectTimeoutTimer();
-  } else {
-    redirect_state_ = RedirectState::kDeciderDisallowed;
+  redirect_result_ = *redirect_result;
+  if (redirect_result_ != RedirectResult::kRedirectable) {
+    redirect_state_ = RedirectState::kRedirectNotAllowedByDecider;
+    return;
   }
+
+  // Redirect is allowed.
+  redirect_state_ = RedirectState::kRedirectAttempted;
+  request->url = GetSubresourceURLForURL(request->url);
+  StartRedirectTimeoutTimer();
 }
 
 void SubresourceRedirectURLLoaderThrottle::NotifyRedirectDeciderDecision(
-    bool is_allowed) {
-  DCHECK_EQ(RedirectState::kDeciderDecisionPending, redirect_state_);
+    RedirectResult redirect_result) {
+  DCHECK_EQ(RedirectState::kRedirectDecisionPending, redirect_state_);
+  redirect_result_ = redirect_result;
 
-  if (is_allowed) {
-    redirect_state_ = RedirectState::kRedirectAttempted;
-    delegate_->Resume();
-    StartRedirectTimeoutTimer();
-  } else {
+  if (redirect_result_ != RedirectResult::kRedirectable) {
     // Restart the fetch to the original URL.
-    redirect_state_ = RedirectState::kDeciderDisallowed;
+    redirect_state_ = RedirectState::kRedirectNotAllowedByDecider;
     delegate_->RestartWithURLResetAndFlags(net::LOAD_NORMAL);
+    delegate_->Resume();
+    return;
   }
+
+  // Redirect is allowed.
+  redirect_state_ = RedirectState::kRedirectAttempted;
+  delegate_->Resume();
+  StartRedirectTimeoutTimer();
 }
 
 void SubresourceRedirectURLLoaderThrottle::WillRedirectRequest(
@@ -132,7 +158,7 @@ void SubresourceRedirectURLLoaderThrottle::WillRedirectRequest(
   // Check if the redirect is in some terminal state.
   DCHECK((redirect_state_ == RedirectState::kNone) ||
          (redirect_state_ == RedirectState::kRedirectAttempted) ||
-         (redirect_state_ == RedirectState::kDeciderDisallowed) ||
+         (redirect_state_ == RedirectState::kRedirectNotAllowedByDecider) ||
          redirect_state_ == RedirectState::kRedirectFailed);
   if (redirect_state_ == RedirectState::kRedirectAttempted &&
       redirect_timeout_timer_) {
@@ -154,7 +180,7 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
   // Check if the redirect is in some terminal state.
   DCHECK((redirect_state_ == RedirectState::kNone) ||
          (redirect_state_ == RedirectState::kRedirectAttempted) ||
-         (redirect_state_ == RedirectState::kDeciderDisallowed) ||
+         (redirect_state_ == RedirectState::kRedirectNotAllowedByDecider) ||
          redirect_state_ == RedirectState::kRedirectFailed);
   if (redirect_state_ != RedirectState::kRedirectAttempted)
     return;
@@ -177,7 +203,7 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
       response_head.headers->response_code() == 304) {
     return;
   }
-  OnRedirectedLoadCompleteWithError();
+  redirect_result_ = RedirectResult::kIneligibleRedirectFailed;
 
   // 503 response code indicates loadshed from the compression server. Notify
   // the browser process which will bypass subresource redirect for subsequent
@@ -191,9 +217,9 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
       net::HttpUtil::ParseRetryAfterHeader(retry_after_string,
                                            base::Time::Now(), &retry_after);
     }
-    if (auto* subresource_redirect_hints_agent =
-            SubresourceRedirectHintsAgent::Get(GetRenderFrame())) {
-      subresource_redirect_hints_agent->NotifyHttpsImageCompressionFetchFailed(
+    if (auto* public_resource_decider_agent =
+            GetPublicResourceDeciderAgent(render_frame_id_)) {
+      public_resource_decider_agent->NotifyCompressedResourceFetchFailed(
           retry_after);
     }
   }
@@ -211,7 +237,7 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
   // Check if the redirect is in some terminal state.
   DCHECK((redirect_state_ == RedirectState::kNone) ||
          (redirect_state_ == RedirectState::kRedirectAttempted) ||
-         (redirect_state_ == RedirectState::kDeciderDisallowed) ||
+         (redirect_state_ == RedirectState::kRedirectNotAllowedByDecider) ||
          redirect_state_ == RedirectState::kRedirectFailed);
   // If response was not from the compression server, don't record any
   // metrics.
@@ -223,7 +249,11 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
   if (content_length < 0)
     return;
 
-  RecordMetricsOnLoadFinished(response_url, content_length);
+  if (auto* public_resource_decider_agent =
+          GetPublicResourceDeciderAgent(render_frame_id_)) {
+    public_resource_decider_agent->RecordMetricsOnLoadFinished(
+        response_url, content_length, redirect_result_);
+  }
 
   if (redirect_state_ != RedirectState::kRedirectAttempted)
     return;
@@ -259,7 +289,7 @@ void SubresourceRedirectURLLoaderThrottle::WillOnCompleteWithError(
   if (redirect_state_ != RedirectState::kRedirectAttempted)
     return;
   DCHECK(ShouldCompressionServerRedirectSubresource());
-  OnRedirectedLoadCompleteWithError();
+  redirect_result_ = RedirectResult::kIneligibleRedirectFailed;
 
   // If the server fails, restart the request to the original resource, and
   // record it.
@@ -283,9 +313,9 @@ void SubresourceRedirectURLLoaderThrottle::OnRedirectTimeout() {
   DCHECK_EQ(RedirectState::kRedirectAttempted, redirect_state_);
   redirect_state_ = RedirectState::kRedirectFailed;
   delegate_->RestartWithURLResetAndFlagsNow(net::LOAD_NORMAL);
-  if (auto* subresource_redirect_hints_agent =
-          SubresourceRedirectHintsAgent::Get(GetRenderFrame())) {
-    subresource_redirect_hints_agent->NotifyHttpsImageCompressionFetchFailed(
+  if (auto* public_resource_decider_agent =
+          GetPublicResourceDeciderAgent(render_frame_id_)) {
+    public_resource_decider_agent->NotifyCompressedResourceFetchFailed(
         base::TimeDelta());
   }
   UMA_HISTOGRAM_BOOLEAN("SubresourceRedirect.CompressionFetchTimeout", true);

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
+#include "chrome/renderer/subresource_redirect/public_resource_decider_agent.h"
 #include "content/public/renderer/render_frame.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
 
@@ -25,10 +26,30 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
  public:
   using RedirectDecisionCallback = base::OnceCallback<void(bool)>;
 
+  // Different states the subresource redirection can be in.
+  enum class RedirectState {
+    kNone,
+
+    // The redirect decision is pending from the underlying decider.
+    kRedirectDecisionPending,
+
+    // Redirect was disallowed by the underlying decider e.g., robots rules
+    // decider.
+    kRedirectNotAllowedByDecider,
+
+    // The subresource request was redirected to attempt to compress it.
+    kRedirectAttempted,
+
+    // Failed due to http response codes, net errors, and the subresource was
+    // fetched from original origin.
+    kRedirectFailed
+  };
+
   static std::unique_ptr<SubresourceRedirectURLLoaderThrottle>
   MaybeCreateThrottle(const blink::WebURLRequest& request, int render_frame_id);
 
-  explicit SubresourceRedirectURLLoaderThrottle(int render_frame_id);
+  SubresourceRedirectURLLoaderThrottle(int render_frame_id,
+                                       bool allowed_to_redirect);
   ~SubresourceRedirectURLLoaderThrottle() override;
 
   SubresourceRedirectURLLoaderThrottle(
@@ -58,51 +79,11 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
   // Overridden to do nothing as the default implementation is NOT_REACHED()
   void DetachFromCurrentSequence() override;
 
-  // Determine whether the image url should be redirected. When the
-  // determination can be made immediately, the decision should be returned.
-  // Otherwise base::nullopt should be returned and the callback should be
-  // invoked with the decision asynchronously.
-  virtual base::Optional<bool> ShouldRedirectImage(
-      const GURL& url,
-      RedirectDecisionCallback callback) = 0;
-
-  // Indicates the subresource redirect failed, and the image will be fetched
-  // directly from the  origin instead. The failures can be due to non-2xx
-  // http responses or other net errors
-  virtual void OnRedirectedLoadCompleteWithError() = 0;
-
-  // Notifies the image load finished.
-  virtual void RecordMetricsOnLoadFinished(const GURL& url,
-                                           int64_t content_length) = 0;
-
-  content::RenderFrame* GetRenderFrame() const {
-    return content::RenderFrame::FromRoutingID(render_frame_id_);
-  }
-
  private:
-  // Different states the subresource redirection can be in.
-  enum class RedirectState {
-    kNone,
-
-    // The redirect decision is pending from the underlying decider.
-    kDeciderDecisionPending,
-
-    // Redirect was disallowed by the underlying decider e.g., robots rules
-    // decider.
-    kDeciderDisallowed,
-
-    // The decider allowed redirect, and was attempted.
-    kRedirectAttempted,
-
-    // Failed due to http response codes, net errors, and the subresource was
-    // fetched from original origin.
-    kRedirectFailed
-  };
-
-  friend class TestPublicImageHintsURLLoaderThrottle;
+  friend class SubresourceRedirectPublicImageHintsDeciderAgentTest;
 
   // Callback to notify the decision of decider subclasses.
-  void NotifyRedirectDeciderDecision(bool is_allowed);
+  void NotifyRedirectDeciderDecision(RedirectResult);
 
   // Start the timer for redirect fetch timeout.
   void StartRedirectTimeoutTimer();
@@ -119,6 +100,10 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
   // Timer to detect whether the response from compression server has timed out.
   std::unique_ptr<base::OneShotTimer> redirect_timeout_timer_;
 
+  // Whether the subresource can be redirected or not and what was the reason if
+  // its not eligible.
+  RedirectResult redirect_result_;
+
   // Used to get a weak pointer to |this|.
   base::WeakPtrFactory<SubresourceRedirectURLLoaderThrottle> weak_ptr_factory_{
       this};

chrome/test/BUILD.gn

@@ -1569,7 +1569,7 @@ if (!is_android) {
       "../renderer/chrome_content_settings_agent_delegate_browsertest.cc",
       "../renderer/chrome_render_frame_observer_browsertest.cc",
       "../renderer/lite_video/lite_video_hint_agent_browsertest.cc",
-      "../renderer/subresource_redirect/public_image_hints_url_loader_throttle_browsertest.cc",
+      "../renderer/subresource_redirect/public_image_hints_decider_agent_browsertest.cc",
       "../renderer/translate/per_frame_translate_agent_browsertest.cc",
       "../renderer/translate/translate_agent_browsertest.cc",
       "../renderer/translate/translate_script_browsertest.cc",