Move blacklist available check for commit-time previews to commit-time

This should reduce the page loads that have blacklist unavailable
eligibility reasons for commit-time previews.

Bug: 905088
Change-Id: I2350706637e2d0b37cc07d3ac9325137143164a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1831276Reviewed-by: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701292}

components/previews/content/previews_decider_impl.cc

@@ -70,9 +70,8 @@ bool ShouldCheckOptimizationHints(PreviewsType type) {
   return false;
 }
 
-// Returns true if ECT should be checked for |type| only at the commit time. If
-// true is returned, then ECT need not be checked at the navigation time.
-bool CheckECTOnlyAtCommitTime(PreviewsType type) {
+// Returns true if the decision to apply |type| can wait until commit time.
+bool IsCommitTimePreview(PreviewsType type) {
   switch (type) {
     case PreviewsType::NOSCRIPT:
     case PreviewsType::RESOURCE_LOADING_HINTS:
@@ -266,10 +265,6 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
 
   // Skip blacklist checks if the blacklist is ignored.
   if (!blacklist_ignored_) {
-    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.
@@ -279,19 +274,31 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
       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);
+    if (previews_black_list_) {
+      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;
       }
-      return status;
     }
   }
 
@@ -299,7 +306,7 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
   // 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
   // specify variable ECT thresholds for slow page hints.
-  if (!is_drp_server_preview && !CheckECTOnlyAtCommitTime(type)) {
+  if (!is_drp_server_preview && !IsCommitTimePreview(type)) {
     if (effective_connection_type_ == net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN) {
       return PreviewsEligibilityReason::NETWORK_QUALITY_UNAVAILABLE;
     }
@@ -393,7 +400,13 @@ bool PreviewsDeciderImpl::ShouldCommitPreview(
 
   const GURL committed_url = navigation_handle->GetURL();
 
-  if (previews_black_list_ && !blacklist_ignored_) {
+  if (!blacklist_ignored_) {
+    if (!previews_black_list_) {
+      LogPreviewDecisionMade(PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE,
+                             committed_url, clock_->Now(), type, {},
+                             previews_data);
+      return false;
+    }
     std::vector<PreviewsEligibilityReason> passed_reasons;
     // The blacklist will disallow certain hosts for periods of time based on
     // user's opting out of the preview.
@@ -507,7 +520,7 @@ PreviewsDeciderImpl::ShouldCommitPreviewPerOptimizationHints(
   // connectivity as null. See https://crbug.com/838969. So, we do not trigger
   // previews when |ect| is net::EFFECTIVE_CONNECTION_TYPE_OFFLINE.
   net::EffectiveConnectionType ect = previews_data->navigation_ect();
-  if (CheckECTOnlyAtCommitTime(type) &&
+  if (IsCommitTimePreview(type) &&
       ect == net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN) {
     // Update the |ect| to the current value.
     ect = effective_connection_type_;

components/previews/content/previews_decider_impl_unittest.cc

@@ -1159,6 +1159,23 @@ TEST_F(PreviewsDeciderImplTest, NoScriptCommitTimeWhitelistCheck) {
 
     histogram_tester.ExpectTotalCount("Previews.EligibilityReason.NoScript", 0);
   }
+
+  // Verify preview not allowed when blacklist is unavailable.
+  {
+    ReportEffectiveConnectionType(net::EFFECTIVE_CONNECTION_TYPE_2G);
+    base::HistogramTester histogram_tester;
+    previews_decider_impl()->InjectTestBlacklist(nullptr /* blacklist */);
+    PreviewsUserData user_data(kDefaultPageId);
+    user_data.set_navigation_ect(net::EFFECTIVE_CONNECTION_TYPE_2G);
+    content::MockNavigationHandle navigation_handle;
+    navigation_handle.set_url(GURL("https://whitelisted.example.com"));
+    EXPECT_FALSE(previews_decider_impl()->ShouldCommitPreview(
+        &user_data, &navigation_handle, PreviewsType::NOSCRIPT));
+
+    histogram_tester.ExpectUniqueSample(
+        "Previews.EligibilityReason.NoScript",
+        static_cast<int>(PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE), 1);
+  }
 }
 
 TEST_F(PreviewsDeciderImplTest,
@@ -1771,7 +1788,9 @@ TEST_F(PreviewsDeciderImplTest, LogPreviewDecisionMadePassInCorrectParams) {
   }
 }  // namespace
 
-TEST_F(PreviewsDeciderImplTest, LogDecisionMadeBlacklistNotAvailable) {
+TEST_F(
+    PreviewsDeciderImplTest,
+    LogDecisionMadeBlacklistUnavailableAtNavigationStartForNonCommitTimePreview) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(
       {features::kPreviews, features::kOfflinePreviews}, {});
@@ -1794,6 +1813,33 @@ TEST_F(PreviewsDeciderImplTest, LogDecisionMadeBlacklistNotAvailable) {
               ::testing::Contains(expected_type));
 }
 
+TEST_F(
+    PreviewsDeciderImplTest,
+    LogDecisionMadeBlacklistUnavailableAtNavigationStartForCommitTimePreview) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kPreviews, features::kNoScriptPreviews,
+       optimization_guide::features::kOptimizationHints},
+      {});
+
+  InitializeUIService();
+  auto expected_reason = PreviewsEligibilityReason::ALLOWED;
+  auto expected_type = PreviewsType::NOSCRIPT;
+
+  previews_decider_impl()->InjectTestBlacklist(nullptr /* blacklist */);
+  PreviewsUserData user_data(kDefaultPageId);
+  content::MockNavigationHandle navigation_handle;
+  navigation_handle.set_url(GURL("https://www.google.com"));
+  previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+      &user_data, &navigation_handle, false, expected_type);
+  base::RunLoop().RunUntilIdle();
+  // Testing correct log method is called.
+  EXPECT_THAT(ui_service()->decision_reasons(),
+              ::testing::Contains(expected_reason));
+  EXPECT_THAT(ui_service()->decision_types(),
+              ::testing::Contains(expected_type));
+}
+
 TEST_F(PreviewsDeciderImplTest, LogDecisionMadeBlacklistStatusesDefault) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(
@@ -2027,7 +2073,6 @@ TEST_F(PreviewsDeciderImplTest,
   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,
@@ -2255,7 +2300,6 @@ TEST_F(PreviewsDeciderImplTest, LogDecisionMadeAllowHintPreviewWithoutECT) {
   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,