PreviewsDecider cleanup - drops extra LoFi Finch blacklist support/cruft

Also drops extra/legacy server preview call for server LoFi (just use
use LitePage check for Flywheel CPAT based previews) and drops
is_server_preview parm from ShouldAllow* method.

Also ignores case of blacklist not being loaded if flagged to ignore
blacklist.

Change-Id: I5995a13180a06c592ad5d02a49e54e875faade31
Reviewed-on: https://chromium-review.googlesource.com/c/1331415Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607303}

chrome/browser/chrome_content_browser_client.cc

@@ -5065,10 +5065,7 @@ content::PreviewsState ChromeContentBrowserClient::DetermineAllowedPreviews(
   } else {
     if (previews_decider_impl->ShouldAllowPreviewAtNavigationStart(
             previews_data, current_navigation_url, is_reload,
-            previews::PreviewsType::LITE_PAGE, true /* is_server_preview */) &&
-        previews_decider_impl->ShouldAllowPreviewAtNavigationStart(
-            previews_data, current_navigation_url, is_reload,
-            previews::PreviewsType::LOFI, true /* is_server_preview */)) {
+            previews::PreviewsType::LITE_PAGE)) {
       previews_state |= server_previews_enabled_state;
     }
   }

components/previews/content/previews_content_util.cc

@@ -43,8 +43,8 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
     previews_state |=
         (previews_data->allowed_previews_state() & content::OFFLINE_PAGE_ON);
   } else if (previews_decider->ShouldAllowPreviewAtNavigationStart(
-                 previews_data, url, is_reload, previews::PreviewsType::OFFLINE,
-                 false /* is_server_preview */)) {
+                 previews_data, url, is_reload,
+                 previews::PreviewsType::OFFLINE)) {
     previews_state |= content::OFFLINE_PAGE_ON;
   }
 
@@ -54,7 +54,7 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
 
   if (previews_decider->ShouldAllowPreviewAtNavigationStart(
           previews_data, url, is_reload,
-          previews::PreviewsType::RESOURCE_LOADING_HINTS, false)) {
+          previews::PreviewsType::RESOURCE_LOADING_HINTS)) {
     previews_state |= content::RESOURCE_LOADING_HINTS_ON;
     // Initiate load of any applicable hint details.
     previews_decider->LoadResourceHints(url);
@@ -64,15 +64,13 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
   // 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->ShouldAllowPreviewAtNavigationStart(
-          previews_data, url, is_reload, previews::PreviewsType::NOSCRIPT,
-          false /* is_server_preview */)) {
+          previews_data, url, is_reload, previews::PreviewsType::NOSCRIPT)) {
     previews_state |= content::NOSCRIPT_ON;
   }
 
   if (previews::params::IsClientLoFiEnabled() &&
-      previews_decider->ShouldAllowClientPreviewWithFinchBlacklist(
-          previews_data, url, is_reload, previews::PreviewsType::LOFI,
-          previews::params::GetBlackListedHostsForClientLoFiFieldTrial())) {
+      previews_decider->ShouldAllowPreviewAtNavigationStart(
+          previews_data, url, is_reload, previews::PreviewsType::LOFI)) {
     previews_state |= content::CLIENT_LOFI_ON;
   }
 

components/previews/content/previews_content_util_unittest.cc

@@ -25,22 +25,10 @@ class PreviewEnabledPreviewsDecider : public PreviewsDecider {
   PreviewEnabledPreviewsDecider() {}
   ~PreviewEnabledPreviewsDecider() override {}
 
-  bool ShouldAllowPreviewAtNavigationStart(
-      PreviewsUserData* previews_data,
-      const GURL& url,
-      bool is_reload,
-      PreviewsType type,
-      bool is_server_preview) const override {
-    return IsEnabled(type);
-  }
-
-  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 ShouldAllowPreviewAtNavigationStart(PreviewsUserData* previews_data,
+                                           const GURL& url,
+                                           bool is_reload,
+                                           PreviewsType type) const override {
     return IsEnabled(type);
   }
 

components/previews/content/previews_decider_impl.cc

@@ -201,16 +201,17 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtNavigationStart(
     PreviewsUserData* previews_data,
     const GURL& url,
     bool is_reload,
-    PreviewsType type,
-    bool is_server_preview) const {
+    PreviewsType type) const {
   if (!ShouldConsiderPreview(type, url, previews_data)) {
     // Don't capture metrics since preview is either disabled or url is local.
     return false;
   }
 
+  bool is_drp_server_preview = (type == PreviewsType::LITE_PAGE);
   std::vector<PreviewsEligibilityReason> passed_reasons;
-  PreviewsEligibilityReason eligibility = DeterminePreviewEligibility(
-      previews_data, url, is_reload, type, is_server_preview, &passed_reasons);
+  PreviewsEligibilityReason eligibility =
+      DeterminePreviewEligibility(previews_data, url, is_reload, type,
+                                  is_drp_server_preview, &passed_reasons);
   LogPreviewDecisionMade(eligibility, url, clock_->Now(), type,
                          std::move(passed_reasons), previews_data->page_id());
   return eligibility == PreviewsEligibilityReason::ALLOWED ||
@@ -232,7 +233,7 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
     const GURL& url,
     bool is_reload,
     PreviewsType type,
-    bool is_server_preview,
+    bool is_drp_server_preview,
     std::vector<PreviewsEligibilityReason>* passed_reasons) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(previews::params::ArePreviewsAllowed());
@@ -243,11 +244,6 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
   // eligibility so that it will be available at commit time.
   previews_data->set_navigation_ect(effective_connection_type_);
 
-  if (!previews_black_list_) {
-    return PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE;
-  }
-  passed_reasons->push_back(PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE);
-
   // In the case that the user has chosen to ignore the normal blacklist rules
   // (flags or interventions-internals), a preview should still not be served
   // for 5 seconds after the last opt out. This allows "show original" to
@@ -259,12 +255,17 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
     }
     passed_reasons->push_back(
         PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT);
+  } else if (!previews_black_list_) {
+    return PreviewsEligibilityReason::BLACKLIST_UNAVAILABLE;
   } else {
+    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_server_preview && ignore_long_term_blacklist_for_server_previews_,
+        is_drp_server_preview &&
+            ignore_long_term_blacklist_for_server_previews_,
         passed_reasons);
 
     if (status != PreviewsEligibilityReason::ALLOWED) {
@@ -279,7 +280,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_server_preview && !ShouldCheckOptimizationHints(type)) {
+  if (!is_drp_server_preview && !ShouldCheckOptimizationHints(type)) {
     // Network quality estimator may sometimes return effective connection type
     // as offline when the Android APIs incorrectly return device connectivity
     // as null. See https://crbug.com/838969. So, we do not trigger previews
@@ -331,43 +332,6 @@ PreviewsEligibilityReason PreviewsDeciderImpl::DeterminePreviewEligibility(
   return PreviewsEligibilityReason::ALLOWED;
 }
 
-bool PreviewsDeciderImpl::ShouldAllowClientPreviewWithFinchBlacklist(
-    PreviewsUserData* previews_data,
-    const GURL& url,
-    bool is_reload,
-    PreviewsType type,
-    const std::vector<std::string>& host_blacklist_from_finch) const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(PreviewsType::LOFI == type);
-
-  if (!ShouldConsiderPreview(type, url, previews_data)) {
-    // Don't capture metrics since preview is either disabled or url is local.
-    return false;
-  }
-
-  std::vector<PreviewsEligibilityReason> passed_reasons;
-  uint64_t page_id = previews_data->page_id();
-
-  // Check Finch-provided blacklist, if any. This type of blacklist was added
-  // for Finch provided blacklist for Client LoFi.
-  if (base::ContainsValue(host_blacklist_from_finch, url.host_piece())) {
-    LogPreviewDecisionMade(
-        PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER, url,
-        clock_->Now(), type, std::move(passed_reasons), page_id);
-    return false;
-  }
-  passed_reasons.push_back(
-      PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER);
-
-  PreviewsEligibilityReason eligibility = DeterminePreviewEligibility(
-      previews_data, url, is_reload, type, false, &passed_reasons);
-  LogPreviewDecisionMade(eligibility, url, clock_->Now(), type,
-                         std::move(passed_reasons), page_id);
-  return eligibility == PreviewsEligibilityReason::ALLOWED ||
-         eligibility ==
-             PreviewsEligibilityReason::ALLOWED_WITHOUT_OPTIMIZATION_HINTS;
-}
-
 void PreviewsDeciderImpl::LoadResourceHints(const GURL& url) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   previews_opt_guide_->MaybeLoadOptimizationHints(

components/previews/content/previews_decider_impl.h

@@ -104,18 +104,10 @@ class PreviewsDeciderImpl : public PreviewsDecider,
   PreviewsBlackList* black_list() const { return previews_black_list_.get(); }
 
   // PreviewsDecider implementation:
-  bool ShouldAllowPreviewAtNavigationStart(
-      PreviewsUserData* previews_data,
-      const GURL& url,
-      bool is_reload,
-      PreviewsType type,
-      bool is_server_preview) 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 ShouldAllowPreviewAtNavigationStart(PreviewsUserData* previews_data,
+                                           const GURL& url,
+                                           bool is_reload,
+                                           PreviewsType type) const override;
   bool ShouldCommitPreview(PreviewsUserData* previews_data,
                            const GURL& committed_url,
                            PreviewsType type) const override;
@@ -159,7 +151,7 @@ class PreviewsDeciderImpl : public PreviewsDecider,
       const GURL& url,
       bool is_reload,
       PreviewsType type,
-      bool is_server_preview,
+      bool is_drp_server_preview,
       std::vector<PreviewsEligibilityReason>* passed_reasons) const;
 
   // Whether the preview |type| should be allowed to be considered for |url|

components/previews/core/previews_decider.h

@@ -25,27 +25,12 @@ class PreviewsDecider {
   // 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.
+  // perform a network quality check on the server.
   virtual bool ShouldAllowPreviewAtNavigationStart(
       PreviewsUserData* previews_data,
       const GURL& url,
       bool is_reload,
-      PreviewsType type,
-      bool is_server_preview) 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;
+      PreviewsType type) const = 0;
 
   // Whether the |committed_url| is allowed to show a preview of |type|.
   virtual bool ShouldCommitPreview(PreviewsUserData* previews_data,

components/previews/core/previews_experiments.cc

@@ -304,13 +304,6 @@ net::EffectiveConnectionType EffectiveConnectionTypeThresholdForClientLoFi() {
                                      net::EFFECTIVE_CONNECTION_TYPE_2G);
 }
 
-std::vector<std::string> GetBlackListedHostsForClientLoFiFieldTrial() {
-  return base::SplitString(base::GetFieldTrialParamValueByFeature(
-                               features::kClientLoFi, "short_host_blacklist"),
-                           ",", base::TRIM_WHITESPACE,
-                           base::SPLIT_WANT_NONEMPTY);
-}
-
 int NoScriptPreviewsInflationPercent() {
   // The default value was determined from lab experiment data of whitelisted
   // URLs. It may be improved once there is enough UKM live experiment data

components/previews/core/previews_experiments.h

@@ -153,9 +153,6 @@ bool IsOptimizationHintsEnabled();
 // should not be served.
 net::EffectiveConnectionType EffectiveConnectionTypeThresholdForClientLoFi();
 
-// Returns the hosts that are blacklisted by the Client Lo-Fi field trial.
-std::vector<std::string> GetBlackListedHostsForClientLoFiFieldTrial();
-
 // For estimating NoScript data savings, this is the percentage factor to
 // multiple by the network bytes for inflating the original_bytes count.
 int NoScriptPreviewsInflationPercent();

components/previews/core/previews_experiments_unittest.cc

@@ -135,24 +135,18 @@ TEST(PreviewsExperimentsTest, TestEnableClientLoFiWithDefaultParams) {
   EXPECT_EQ(0, params::ClientLoFiVersion());
   EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_2G,
             params::EffectiveConnectionTypeThresholdForClientLoFi());
-  EXPECT_EQ(std::vector<std::string>(),
-            params::GetBlackListedHostsForClientLoFiFieldTrial());
 }
 
 TEST(PreviewsExperimentsTest, TestEnableClientLoFiWithCustomParams) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeatureWithParameters(
       features::kClientLoFi,
-      {{"version", "10"},
-       {"max_allowed_effective_connection_type", "3G"},
-       {"short_host_blacklist", "some,hosts, to-blacklist ,,"}});
+      {{"version", "10"}, {"max_allowed_effective_connection_type", "3G"}});
 
   EXPECT_TRUE(params::IsClientLoFiEnabled());
   EXPECT_EQ(10, params::ClientLoFiVersion());
   EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_3G,
             params::EffectiveConnectionTypeThresholdForClientLoFi());
-  EXPECT_EQ(std::vector<std::string>({"some", "hosts", "to-blacklist"}),
-            params::GetBlackListedHostsForClientLoFiFieldTrial());
 }
 
 }  // namespace

components/previews/core/test_previews_decider.cc

@@ -15,17 +15,7 @@ bool TestPreviewsDecider::ShouldAllowPreviewAtNavigationStart(
     PreviewsUserData* previews_data,
     const GURL& url,
     bool is_reload,
-    PreviewsType type,
-    bool is_server_preview) const {
-  return allow_previews_;
-}
-
-bool TestPreviewsDecider::ShouldAllowClientPreviewWithFinchBlacklist(
-    PreviewsUserData* previews_data,
-    const GURL& url,
-    bool is_reload,
-    PreviewsType type,
-    const std::vector<std::string>& host_blacklist_from_finch) const {
+    PreviewsType type) const {
   return allow_previews_;
 }
 

components/previews/core/test_previews_decider.h

@@ -16,18 +16,10 @@ class TestPreviewsDecider : public previews::PreviewsDecider {
   ~TestPreviewsDecider() override;
 
   // previews::PreviewsDecider:
-  bool ShouldAllowPreviewAtNavigationStart(
-      PreviewsUserData* previews_data,
-      const GURL& url,
-      bool is_reload,
-      PreviewsType type,
-      bool is_server_preview) 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 ShouldAllowPreviewAtNavigationStart(PreviewsUserData* previews_data,
+                                           const GURL& url,
+                                           bool is_reload,
+                                           PreviewsType type) const override;
   bool ShouldCommitPreview(PreviewsUserData* previews_data,
                            const GURL& url,
                            PreviewsType type) const override;