Reworks PreviewsDecider to defer ECT checks for Hint-based previews.

This is meant to allow Slow Page Triggering of Previews - that is, for
previews that may specify variable network quality thresholds based
on the URL (via OptimizationHints), the ECT is deferred until commit
time.

It renames and refactors the ShouldAllow* and IsURLAllowed* methods
to give them more power (it pushes down the ECT threshold determination
instead of having multiple call sites try to provide). There is some
reordering of the PreviewsAvailablity passed reasons.

Some more cleanup is merited but deferred to keep the diffs simpler for
this change.


Bug: 895581
Change-Id: Id8636312a9e707dd1181ca0df514f3f743347671
Reviewed-on: https://chromium-review.googlesource.com/c/1310562
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606243}

chrome/browser/chrome_content_browser_client.cc

@@ -5063,15 +5063,12 @@ content::PreviewsState ChromeContentBrowserClient::DetermineAllowedPreviews(
     previews_state |= (previews_data->allowed_previews_state() &
                        server_previews_enabled_state);
   } else {
-    if (previews_decider_impl->ShouldAllowPreviewAtECT(
+    if (previews_decider_impl->ShouldAllowPreviewAtNavigationStart(
             previews_data, current_navigation_url, is_reload,
-            previews::PreviewsType::LITE_PAGE,
-            net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector<std::string>(),
-            true) &&
-        previews_decider_impl->ShouldAllowPreviewAtECT(
+            previews::PreviewsType::LITE_PAGE, true /* is_server_preview */) &&
+        previews_decider_impl->ShouldAllowPreviewAtNavigationStart(
             previews_data, current_navigation_url, is_reload,
-            previews::PreviewsType::LOFI, net::EFFECTIVE_CONNECTION_TYPE_4G,
-            std::vector<std::string>(), true)) {
+            previews::PreviewsType::LOFI, true /* is_server_preview */)) {
       previews_state |= server_previews_enabled_state;
     }
   }

components/previews/content/previews_content_util.cc

@@ -42,9 +42,9 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
     // Keep the same OFFLINE previews bit as the original URL.
     previews_state |=
         (previews_data->allowed_previews_state() & content::OFFLINE_PAGE_ON);
-  } else if (previews_decider->ShouldAllowPreview(
-                 previews_data, url, is_reload,
-                 previews::PreviewsType::OFFLINE)) {
+  } else if (previews_decider->ShouldAllowPreviewAtNavigationStart(
+                 previews_data, url, is_reload, previews::PreviewsType::OFFLINE,
+                 false /* is_server_preview */)) {
     previews_state |= content::OFFLINE_PAGE_ON;
   }
 
@@ -52,9 +52,9 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
   if (!is_data_saver_user)
     return previews_state;
 
-  if (previews_decider->ShouldAllowPreview(
+  if (previews_decider->ShouldAllowPreviewAtNavigationStart(
           previews_data, url, is_reload,
-          previews::PreviewsType::RESOURCE_LOADING_HINTS)) {
+          previews::PreviewsType::RESOURCE_LOADING_HINTS, false)) {
     previews_state |= content::RESOURCE_LOADING_HINTS_ON;
     // Initiate load of any applicable hint details.
     previews_decider->LoadResourceHints(url);
@@ -63,17 +63,16 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
   // Check for client-side previews in precedence order.
   // Note: this is for the beginning of navigation so we should not
   // check for https here (since an http request may redirect to https).
-  if (previews_decider->ShouldAllowPreview(previews_data, url, is_reload,
-                                           previews::PreviewsType::NOSCRIPT)) {
+  if (previews_decider->ShouldAllowPreviewAtNavigationStart(
+          previews_data, url, is_reload, previews::PreviewsType::NOSCRIPT,
+          false /* is_server_preview */)) {
     previews_state |= content::NOSCRIPT_ON;
   }
 
   if (previews::params::IsClientLoFiEnabled() &&
-      previews_decider->ShouldAllowPreviewAtECT(
+      previews_decider->ShouldAllowClientPreviewWithFinchBlacklist(
           previews_data, url, is_reload, previews::PreviewsType::LOFI,
-          previews::params::EffectiveConnectionTypeThresholdForClientLoFi(),
-          previews::params::GetBlackListedHostsForClientLoFiFieldTrial(),
-          false)) {
+          previews::params::GetBlackListedHostsForClientLoFiFieldTrial())) {
     previews_state |= content::CLIENT_LOFI_ON;
   }
 
@@ -124,7 +123,7 @@ content::PreviewsState DetermineCommittedClientPreviewsState(
     // Resource loading hints was chosen for the original URL but only continue
     // with it if the committed URL has HTTPS scheme and is allowed by decider.
     if (is_https && previews_decider &&
-        previews_decider->IsURLAllowedForPreview(
+        previews_decider->ShouldCommitPreview(
             previews_data, url,
             previews::PreviewsType::RESOURCE_LOADING_HINTS)) {
       return content::RESOURCE_LOADING_HINTS_ON;
@@ -138,7 +137,7 @@ content::PreviewsState DetermineCommittedClientPreviewsState(
     // NoScript was chosen for the original URL but only continue with it
     // if the committed URL has HTTPS scheme and is allowed by decider.
     if (is_https && previews_decider &&
-        previews_decider->IsURLAllowedForPreview(
+        previews_decider->ShouldCommitPreview(
             previews_data, url, previews::PreviewsType::NOSCRIPT)) {
       return content::NOSCRIPT_ON;
     }

components/previews/content/previews_content_util_unittest.cc

@@ -25,29 +25,28 @@ class PreviewEnabledPreviewsDecider : public PreviewsDecider {
   PreviewEnabledPreviewsDecider() {}
   ~PreviewEnabledPreviewsDecider() override {}
 
-  bool ShouldAllowPreviewAtECT(
+  bool ShouldAllowPreviewAtNavigationStart(
       PreviewsUserData* previews_data,
       const GURL& url,
       bool is_reload,
       PreviewsType type,
-      net::EffectiveConnectionType effective_connection_type_threshold,
-      const std::vector<std::string>& host_blacklist_from_finch,
       bool is_server_preview) const override {
     return IsEnabled(type);
   }
 
-  bool ShouldAllowPreview(PreviewsUserData* previews_data,
-                          const GURL& url,
-                          bool is_reload,
-                          PreviewsType type) const override {
-    return ShouldAllowPreviewAtECT(previews_data, url, is_reload, type,
-                                   params::GetECTThresholdForPreview(type),
-                                   std::vector<std::string>(), false);
+  bool ShouldAllowClientPreviewWithFinchBlacklist(
+      PreviewsUserData* previews_data,
+      const GURL& url,
+      bool is_reload,
+      PreviewsType type,
+      const std::vector<std::string>& host_blacklist_from_finch)
+      const override {
+    return IsEnabled(type);
   }
 
-  bool IsURLAllowedForPreview(PreviewsUserData* previews_data,
-                              const GURL& url,
-                              PreviewsType type) const override {
+  bool ShouldCommitPreview(PreviewsUserData* previews_data,
+                           const GURL& url,
+                           PreviewsType type) const override {
     EXPECT_TRUE(type == PreviewsType::NOSCRIPT ||
                 type == PreviewsType::RESOURCE_LOADING_HINTS);
     return IsEnabled(type);

components/previews/content/previews_decider_impl.h

@@ -104,25 +104,23 @@ class PreviewsDeciderImpl : public PreviewsDecider,
   PreviewsBlackList* black_list() const { return previews_black_list_.get(); }
 
   // PreviewsDecider implementation:
-  bool ShouldAllowPreview(PreviewsUserData* previews_data,
-                          const GURL& url,
-                          bool is_reload,
-                          PreviewsType type) const override;
-  bool ShouldAllowPreviewAtECT(
+  bool ShouldAllowPreviewAtNavigationStart(
       PreviewsUserData* previews_data,
       const GURL& url,
       bool is_reload,
       PreviewsType type,
-      net::EffectiveConnectionType effective_connection_type_threshold,
-      const std::vector<std::string>& host_blacklist_from_finch,
       bool is_server_preview) const override;
-  bool IsURLAllowedForPreview(PreviewsUserData* previews_data,
-                              const GURL& url,
-                              PreviewsType type) const override;
+  bool ShouldAllowClientPreviewWithFinchBlacklist(
+      PreviewsUserData* previews_data,
+      const GURL& url,
+      bool is_reload,
+      PreviewsType type,
+      const std::vector<std::string>& host_blacklist_from_finch) const override;
+  bool ShouldCommitPreview(PreviewsUserData* previews_data,
+                           const GURL& committed_url,
+                           PreviewsType type) const override;
 
-  // Set whether ignoring the long term blacklist rules is allowed for calls to
-  // ShouldAllowPreviewAtECT that have |can_ignore_long_term_black_list_rules|
-  // set to true.
+  // Set whether to ignore the long term blacklist rules for server previews.
   void SetIgnoreLongTermBlackListForServerPreviews(
       bool ignore_long_term_blacklist_for_server_previews);
 
@@ -148,6 +146,22 @@ class PreviewsDeciderImpl : public PreviewsDecider,
       std::unique_ptr<PreviewsBlackList> previews_back_list);
 
  private:
+  // Returns whether the preview |type| should be considered for |url|.
+  // This is an initial check on the preview |type| being enabled and the
+  // |url| not being a local URL.
+  bool ShouldConsiderPreview(PreviewsType type,
+                             const GURL& url,
+                             PreviewsUserData* previews_data) const;
+
+  // Determines the eligibility of the preview |type| for |url|.
+  PreviewsEligibilityReason DeterminePreviewEligibility(
+      PreviewsUserData* previews_data,
+      const GURL& url,
+      bool is_reload,
+      PreviewsType type,
+      bool is_server_preview,
+      std::vector<PreviewsEligibilityReason>* passed_reasons) const;
+
   // Whether the preview |type| should be allowed to be considered for |url|
   // subject to any server provided optimization hints. This is meant for
   // checking the initial navigation URL. Returns ALLOWED if no reason found
@@ -161,7 +175,7 @@ class PreviewsDeciderImpl : public PreviewsDecider,
   // Whether |url| is allowed for |type| according to server provided
   // optimization hints, if available. This is meant for checking the committed
   // navigation URL against any specific hint details.
-  PreviewsEligibilityReason IsURLAllowedForPreviewByOptimizationHints(
+  PreviewsEligibilityReason ShouldCommitPreviewPerOptimizationHints(
       PreviewsUserData* previews_data,
       const GURL& url,
       PreviewsType type,
@@ -186,9 +200,7 @@ class PreviewsDeciderImpl : public PreviewsDecider,
   // set it in flags. See previews::IsPreviewsBlacklistIgnoredViaFlag.
   bool blacklist_ignored_;
 
-  // Whether ignoring the blacklist is allowed for calls to
-  // ShouldAllowPreviewAtECT that have
-  // |is_server_preview| true.
+  // Whether to ignore the blacklist for server previews.
   bool ignore_long_term_blacklist_for_server_previews_ = false;
 
   // The estimate of how slow a user's connection is. Used for triggering

components/previews/content/previews_optimization_guide.cc

@@ -31,17 +31,18 @@ PreviewsOptimizationGuide::~PreviewsOptimizationGuide() {
   optimization_guide_service_->RemoveObserver(this);
 }
 
-bool PreviewsOptimizationGuide::IsWhitelisted(PreviewsUserData* previews_data,
-                                              const GURL& url,
-                                              PreviewsType type) const {
+bool PreviewsOptimizationGuide::IsWhitelisted(
+    PreviewsUserData* previews_data,
+    const GURL& url,
+    PreviewsType type,
+    net::EffectiveConnectionType* out_ect_threshold) const {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
   if (!hints_)
     return false;
 
+  *out_ect_threshold = params::GetECTThresholdForPreview(type);
   int inflation_percent = 0;
-  net::EffectiveConnectionType ect_threshold =
-      params::GetECTThresholdForPreview(type);
-  if (!hints_->IsWhitelisted(url, type, &inflation_percent, &ect_threshold))
+  if (!hints_->IsWhitelisted(url, type, &inflation_percent, out_ect_threshold))
     return false;
 
   if (inflation_percent != 0 && previews_data)

components/previews/content/previews_optimization_guide.h

@@ -46,12 +46,15 @@ class PreviewsOptimizationGuide
 
   ~PreviewsOptimizationGuide() override;
 
-  // Returns whether |type| is whitelisted for |url|.|previews_data| can be
-  // modified.
+  // Returns whether |type| is whitelisted for |url|. If so |out_ect_threshold|
+  // provides the maximum effective connection type to trigger the preview for.
+  // |previews_data| can be modified (for further details provided by hints).
   // Virtual so it can be mocked in tests.
-  virtual bool IsWhitelisted(PreviewsUserData* previews_data,
-                             const GURL& url,
-                             PreviewsType type) const;
+  virtual bool IsWhitelisted(
+      PreviewsUserData* previews_data,
+      const GURL& url,
+      PreviewsType type,
+      net::EffectiveConnectionType* out_ect_threshold) const;
 
   // Returns whether |type| is blacklisted for |url|.
   // Virtual so it can be mocked in tests.

components/previews/content/previews_ui_service_unittest.cc

@@ -23,6 +23,11 @@ namespace previews {
 
 namespace {
 
+// Dummy method for creating TestPreviewsUIService.
+bool MockedPreviewsIsEnabled(previews::PreviewsType type) {
+  return true;
+}
+
 class TestPreviewsUIService : public PreviewsUIService {
  public:
   TestPreviewsUIService(
@@ -34,7 +39,7 @@ class TestPreviewsUIService : public PreviewsUIService {
       : PreviewsUIService(std::move(previews_decider_impl),
                           std::move(previews_opt_out_store),
                           std::move(previews_opt_guide),
-                          PreviewsIsEnabledCallback(),
+                          base::BindRepeating(&MockedPreviewsIsEnabled),
                           std::move(logger),
                           blacklist::BlacklistData::AllowedTypesAndVersions(),
                           test_network_quality_tracker) {}

components/previews/content/previews_user_data.h

@@ -25,11 +25,20 @@ class PreviewsUserData {
   // A session unique ID related to this navigation.
   uint64_t page_id() const { return page_id_; }
 
+  // The effective connection type value for the navigation.
+  net::EffectiveConnectionType navigation_ect() const {
+    return navigation_ect_;
+  }
+  void set_navigation_ect(net::EffectiveConnectionType navigation_ect) {
+    navigation_ect_ = navigation_ect;
+  }
+
   // Returns the data savings inflation percent to use for this navigation
   // instead of the default if it is not 0.
   int data_savings_inflation_percent() const {
     return data_savings_inflation_percent_;
   }
+
   // Sets a data savings inflation percent value to use instead of the default
   // if there is a committed preview. Note that this is expected to be used for
   // specific preview types (such as NoScript) that don't have better data use
@@ -100,6 +109,11 @@ class PreviewsUserData {
   // A session unique ID related to this navigation.
   const uint64_t page_id_;
 
+  // The effective connection type at the time of navigation. This is the value
+  // to compare to the preview's triggering ect threshold.
+  net::EffectiveConnectionType navigation_ect_ =
+      net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+
   // A previews data savings inflation percent for the navigation if not 0.
   int data_savings_inflation_percent_ = 0;
 

components/previews/core/previews_decider.h

@@ -18,37 +18,39 @@ class PreviewsUserData;
 
 class PreviewsDecider {
  public:
-  // Whether |url| is allowed to show a preview of |type|. If the current
-  // ECT is strictly faster than |effective_connection_type_threshold|, the
-  // preview will be disallowed; preview types that check network quality before
-  // calling ShouldAllowPreviewAtECT should pass in
-  // EFFECTIVE_CONNECTION_TYPE_4G.
-  // |is_server_preview| means that the blacklist does
+  // Whether |url| is allowed to show a preview of |type| as can be determined
+  // at the start of a navigation (or start of a redirection). This can be
+  // further checked at navigation commit time via |ShouldCommitPreview|.
+  // Some types of previews will be checked for an applicable network quality
+  // threshold - these are client previews that do not have optimization hint
+  // support. Previews with optimization hint support can have variable
+  // network quality thresholds based on the committed URL. Server previews
+  // perform a network quality check on the server. |is_server_preview| is used
+  // to identify such server previews and also means that the blacklist does
   // not need to be checked for long term rules when Previews has been
   // configured to allow skipping the blacklist.
-  virtual bool ShouldAllowPreviewAtECT(
+  virtual bool ShouldAllowPreviewAtNavigationStart(
       PreviewsUserData* previews_data,
       const GURL& url,
       bool is_reload,
       PreviewsType type,
-      net::EffectiveConnectionType effective_connection_type_threshold,
-      const std::vector<std::string>& host_blacklist_from_finch,
       bool is_server_preview) const = 0;
 
-  // Same as ShouldAllowPreviewAtECT, but uses the previews default
-  // EffectiveConnectionType and no blacklisted hosts from the server.
-  virtual bool ShouldAllowPreview(PreviewsUserData* previews_data,
-                                  const GURL& url,
-                                  bool is_reload,
-                                  PreviewsType type) const = 0;
+  // Whether |url| is allowed to show a preview of |type| as can be determined
+  // at the start of a navigation (or start of a redirection). The checks a
+  // provided server blacklist |host_blacklist_from_finch| that is used by
+  // ClientLoFi which pre-dated the optimization hints mechanism.
+  virtual bool ShouldAllowClientPreviewWithFinchBlacklist(
+      PreviewsUserData* previews_data,
+      const GURL& url,
+      bool is_reload,
+      PreviewsType type,
+      const std::vector<std::string>& host_blacklist_from_finch) const = 0;
 
-  // Whether the |url| is allowed to show a preview of |type|.
-  // This only considers whether the URL is constrained/allowed in
-  // blacklists/whitelists. It does not include other constraints such
-  // as the effective connection type.
-  virtual bool IsURLAllowedForPreview(PreviewsUserData* previews_data,
-                                      const GURL& url,
-                                      PreviewsType type) const = 0;
+  // Whether the |committed_url| is allowed to show a preview of |type|.
+  virtual bool ShouldCommitPreview(PreviewsUserData* previews_data,
+                                   const GURL& committed_url,
+                                   PreviewsType type) const = 0;
 
   // Requests that any applicable detailed resource hints be loaded.
   virtual void LoadResourceHints(const GURL& url) = 0;

components/previews/core/test_previews_decider.cc

@@ -11,28 +11,27 @@ TestPreviewsDecider::TestPreviewsDecider(bool allow_previews)
 
 TestPreviewsDecider::~TestPreviewsDecider() {}
 
-bool TestPreviewsDecider::ShouldAllowPreviewAtECT(
+bool TestPreviewsDecider::ShouldAllowPreviewAtNavigationStart(
     PreviewsUserData* previews_data,
     const GURL& url,
     bool is_reload,
     PreviewsType type,
-    net::EffectiveConnectionType effective_connection_type_threshold,
-    const std::vector<std::string>& host_blacklist_from_finch,
     bool is_server_preview) const {
   return allow_previews_;
 }
 
-bool TestPreviewsDecider::ShouldAllowPreview(PreviewsUserData* previews_data,
-                                             const GURL& url,
-                                             bool is_reload,
-                                             PreviewsType type) const {
+bool TestPreviewsDecider::ShouldAllowClientPreviewWithFinchBlacklist(
+    PreviewsUserData* previews_data,
+    const GURL& url,
+    bool is_reload,
+    PreviewsType type,
+    const std::vector<std::string>& host_blacklist_from_finch) const {
   return allow_previews_;
 }
 
-bool TestPreviewsDecider::IsURLAllowedForPreview(
-    PreviewsUserData* previews_data,
-    const GURL& url,
-    PreviewsType type) const {
+bool TestPreviewsDecider::ShouldCommitPreview(PreviewsUserData* previews_data,
+                                              const GURL& url,
+                                              PreviewsType type) const {
   return allow_previews_;
 }
 

components/previews/core/test_previews_decider.h

@@ -16,21 +16,21 @@ class TestPreviewsDecider : public previews::PreviewsDecider {
   ~TestPreviewsDecider() override;
 
   // previews::PreviewsDecider:
-  bool ShouldAllowPreviewAtECT(
+  bool ShouldAllowPreviewAtNavigationStart(
       PreviewsUserData* previews_data,
       const GURL& url,
       bool is_reload,
       PreviewsType type,
-      net::EffectiveConnectionType effective_connection_type_threshold,
-      const std::vector<std::string>& host_blacklist_from_finch,
       bool is_server_preview) const override;
-  bool ShouldAllowPreview(PreviewsUserData* previews_data,
-                          const GURL& url,
-                          bool is_reload,
-                          PreviewsType type) const override;
-  bool IsURLAllowedForPreview(PreviewsUserData* previews_data,
-                              const GURL& url,
-                              PreviewsType type) const override;
+  bool ShouldAllowClientPreviewWithFinchBlacklist(
+      PreviewsUserData* previews_data,
+      const GURL& url,
+      bool is_reload,
+      PreviewsType type,
+      const std::vector<std::string>& host_blacklist_from_finch) const override;
+  bool ShouldCommitPreview(PreviewsUserData* previews_data,
+                           const GURL& url,
+                           PreviewsType type) const override;
   void LoadResourceHints(const GURL& url) override;
   void LogHintCacheMatch(const GURL& url, bool is_committed) const override;