[Previews] Moves local blocklist check to after other triggering checks

Currently the blocklist checks hide other interesting EligibilityReasons
from being logged to UKM (such as hint match and triggering ECT) that
could prove helpful in knowing more about hint availability and also
more clearly that when a preview would have been triggered except for
the local blocklist. This refactor pushes the blocklist checks to
after these other triggering checks (but still before the coinflip
check).


Bug: 1023611
Change-Id: Ica2e75f8099674a2767141e5234747fd10a7d4bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1912143Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714754}

components/previews/content/previews_decider_impl.cc

@@ -263,43 +263,6 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
   passed_reasons->push_back(
       PreviewsEligibilityReason::EXCLUDED_BY_MEDIA_SUFFIX);
 
-  // Skip blacklist checks if the blacklist is ignored.
-  if (!blacklist_ignored_) {
-    // Trigger the USER_RECENTLY_OPTED_OUT rule when a reload on a preview has
-    // occurred recently. No need to push_back the eligibility reason as it will
-    // be added in IsLoadedAndAllowed as the first check.
-    if (recent_preview_reload_time_ &&
-        recent_preview_reload_time_.value() + params::SingleOptOutDuration() >
-            clock_->Now()) {
-      return PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT;
-    }
-
-    // If the blacklist is unavailable and we are checking a commit-time
-    // preview, we will check for the blacklist at commit-time.
-    if (!IsCommitTimePreview(type)) {
-      if (!previews_black_list_)
-        return PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE;
-      passed_reasons->push_back(
-          PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE);
-
-      // The blacklist will disallow certain hosts for periods of time based on
-      // user's opting out of the preview.
-      PreviewsEligibilityReason status =
-          previews_black_list_->IsLoadedAndAllowed(
-              url, type,
-              is_drp_server_preview &&
-                  ignore_long_term_blacklist_for_server_previews_,
-              passed_reasons);
-
-      if (status != PreviewsEligibilityReason::ALLOWED) {
-        if (type == PreviewsType::LITE_PAGE) {
-          previews_data->set_black_listed_for_lite_page(true);
-        }
-        return status;
-      }
-    }
-  }
-
   // Check the network quality for client previews that don't have optimization
   // hints. This defers checking ECT for server previews because the server will
   // perform its own ECT check and for previews with hints because the hints may
@@ -357,9 +320,48 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
     }
   }
 
+  // Skip blacklist checks if the blacklist is ignored or defer check until
+  // commit time if preview type is to be decided at commit time.
+  if (!blacklist_ignored_ && !IsCommitTimePreview(type)) {
+    PreviewsEligibilityReason status =
+        CheckLocalBlacklist(url, type, is_drp_server_preview, passed_reasons);
+    if (status != PreviewsEligibilityReason::ALLOWED) {
+      if (type == PreviewsType::LITE_PAGE) {
+        previews_data->set_black_listed_for_lite_page(true);
+      }
+      return status;
+    }
+  }
+
   return PreviewsEligibilityReason::ALLOWED;
 }
 
+PreviewsEligibilityReason PreviewsDeciderImpl::CheckLocalBlacklist(
+    const GURL& url,
+    PreviewsType type,
+    bool is_drp_server_preview,
+    std::vector<PreviewsEligibilityReason>* passed_reasons) const {
+  if (!previews_black_list_)
+    return PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE;
+  passed_reasons->push_back(PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE);
+
+  // Trigger the USER_RECENTLY_OPTED_OUT rule when a reload on a preview has
+  // occurred recently. No need to push_back the eligibility reason as it will
+  // be added in IsLoadedAndAllowed as the first check.
+  if (recent_preview_reload_time_ &&
+      recent_preview_reload_time_.value() + params::SingleOptOutDuration() >
+          clock_->Now()) {
+    return PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT;
+  }
+
+  // The blacklist will disallow certain hosts for periods of time based on
+  // user's opting out of the preview.
+  return previews_black_list_->IsLoadedAndAllowed(
+      url, type,
+      is_drp_server_preview && ignore_long_term_blacklist_for_server_previews_,
+      passed_reasons);
+}
+
 bool PreviewsDeciderImpl::LoadPageHints(
     content::NavigationHandle* navigation_handle) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -402,18 +404,12 @@ bool PreviewsDeciderImpl::ShouldCommitPreview(
 
   const GURL committed_url = navigation_handle->GetURL();
 
-  if (!blacklist_ignored_) {
-    if (!previews_black_list_) {
-      LogPreviewDecisionMade(PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE,
-                             committed_url, clock_->Now(), type, {},
-                             previews_data);
-      return false;
-    }
+  // Re-check server optimization hints (if provided) on this commit-time URL.
+  if (ShouldCheckOptimizationHints(type) &&
+      optimization_guide::features::IsOptimizationHintsEnabled()) {
     std::vector<PreviewsEligibilityReason> passed_reasons;
-    // The blacklist will disallow certain hosts for periods of time based on
-    // user's opting out of the preview.
-    PreviewsEligibilityReason status = previews_black_list_->IsLoadedAndAllowed(
-        committed_url, type, false, &passed_reasons);
+    PreviewsEligibilityReason status = ShouldCommitPreviewPerOptimizationHints(
+        previews_data, navigation_handle, type, &passed_reasons);
     if (status != PreviewsEligibilityReason::ALLOWED) {
       LogPreviewDecisionMade(status, committed_url, clock_->Now(), type,
                              std::move(passed_reasons), previews_data);
@@ -421,12 +417,11 @@ bool PreviewsDeciderImpl::ShouldCommitPreview(
     }
   }
 
-  // Re-check server optimization hints (if provided) on this commit-time URL.
-  if (ShouldCheckOptimizationHints(type) &&
-      optimization_guide::features::IsOptimizationHintsEnabled()) {
+  // Check local blacklist for commit-time preview (if blacklist not ignored).
+  if (!blacklist_ignored_ && IsCommitTimePreview(type)) {
     std::vector<PreviewsEligibilityReason> passed_reasons;
-    PreviewsEligibilityReason status = ShouldCommitPreviewPerOptimizationHints(
-        previews_data, navigation_handle, type, &passed_reasons);
+    PreviewsEligibilityReason status =
+        CheckLocalBlacklist(committed_url, type, false, &passed_reasons);
     if (status != PreviewsEligibilityReason::ALLOWED) {
       LogPreviewDecisionMade(status, committed_url, clock_->Now(), type,
                              std::move(passed_reasons), previews_data);

components/previews/content/previews_decider_impl.h

@@ -168,6 +168,13 @@ class PreviewsDeciderImpl : public PreviewsDecider,
       bool is_drp_server_preview,
       std::vector<PreviewsEligibilityReason>* passed_reasons) const;
 
+  // Returns previews eligibility with respect to the local blacklist.
+  PreviewsEligibilityReason CheckLocalBlacklist(
+      const GURL& url,
+      PreviewsType type,
+      bool is_drp_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

components/previews/content/previews_decider_impl_unittest.cc

@@ -2124,11 +2124,6 @@ TEST_F(PreviewsDeciderImplTest, LogDecisionMadeNetworkNotSlow) {
   std::vector<PreviewsEligibilityReason> checked_decisions = {
       PreviewsEligibilityReason::URL_HAS_BASIC_AUTH,
       PreviewsEligibilityReason::EXCLUDED_BY_MEDIA_SUFFIX,
-      PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE,
-      PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED,
-      PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT,
-      PreviewsEligibilityReason::USER_BLACKLISTED,
-      PreviewsEligibilityReason::HOST_BLACKLISTED,
       PreviewsEligibilityReason::NETWORK_QUALITY_UNAVAILABLE,
       PreviewsEligibilityReason::DEVICE_OFFLINE,
   };
@@ -2172,11 +2167,6 @@ TEST_F(PreviewsDeciderImplTest, LogDecisionMadeReloadDisallowed) {
   std::vector<PreviewsEligibilityReason> checked_decisions = {
       PreviewsEligibilityReason::URL_HAS_BASIC_AUTH,
       PreviewsEligibilityReason::EXCLUDED_BY_MEDIA_SUFFIX,
-      PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE,
-      PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED,
-      PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT,
-      PreviewsEligibilityReason::USER_BLACKLISTED,
-      PreviewsEligibilityReason::HOST_BLACKLISTED,
       PreviewsEligibilityReason::NETWORK_QUALITY_UNAVAILABLE,
       PreviewsEligibilityReason::DEVICE_OFFLINE,
       PreviewsEligibilityReason::NETWORK_NOT_SLOW,
@@ -2249,16 +2239,16 @@ TEST_F(PreviewsDeciderImplTest, LogDecisionMadeAllowClientPreviewsWithECT) {
   std::vector<PreviewsEligibilityReason> checked_decisions = {
       PreviewsEligibilityReason::URL_HAS_BASIC_AUTH,
       PreviewsEligibilityReason::EXCLUDED_BY_MEDIA_SUFFIX,
-      PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE,
-      PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED,
-      PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT,
-      PreviewsEligibilityReason::USER_BLACKLISTED,
-      PreviewsEligibilityReason::HOST_BLACKLISTED,
       PreviewsEligibilityReason::NETWORK_QUALITY_UNAVAILABLE,
       PreviewsEligibilityReason::DEVICE_OFFLINE,
       PreviewsEligibilityReason::NETWORK_NOT_SLOW,
       PreviewsEligibilityReason::NETWORK_NOT_SLOW_FOR_SESSION,
       PreviewsEligibilityReason::RELOAD_DISALLOWED,
+      PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE,
+      PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED,
+      PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT,
+      PreviewsEligibilityReason::USER_BLACKLISTED,
+      PreviewsEligibilityReason::HOST_BLACKLISTED,
   };
   PreviewsUserData user_data(kDefaultPageId);
   content::MockNavigationHandle navigation_handle;