Persist the minimum site engagement score threshold in pref

In hints fetcher, persist the minimum site engagement score
threshold in a pref. The threshold is determined at the time the
blocklist is computed. Next, at the time of fetching the hints,
the score of a host is checked against the threshold score
in the pref.

For most of the users where the blocklist size is sufficient
to store all the hosts, the threshold would be set to -1.0.
This would ensure that we fetch hints for all hosts
(that are not blocked), including the ones with low engagement
score.

Change-Id: I2424c675e920ce7cac4b40933155529488830bd6
Bug: 1003826
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1802968Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696447}

chrome/browser/data_saver/data_saver_top_host_provider.cc

@@ -110,16 +110,34 @@ void DataSaverTopHostProvider::InitializeHintsFetcherTopHostBlacklist() {
       optimization_guide::prefs::kTimeBlacklistLastInitialized,
       time_clock_->Now().ToDeltaSinceWindowsEpoch().InSecondsF());
 
+  // Set the minimum engagement score to -1.0f. This ensures that in the default
+  // case (where the blacklist size is enough to accommodate all hosts from the
+  // site engagement service), a threshold on the minimum site engagement score
+  // does not disqualify |this| from requesting hints for any host.
+  pref_service_->SetDouble(
+      optimization_guide::prefs::
+          kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore,
+      -1.0f);
+
   for (const auto& detail : engagement_details) {
+    if (!detail.origin.SchemeIsHTTPOrHTTPS())
+      continue;
     if (top_host_blacklist->size() >=
         optimization_guide::features::MaxHintsFetcherTopHostBlacklistSize()) {
+      // Set the minimum engagement score to the score of the host that
+      // could not be added to  |top_host_blacklist|. Add a small epsilon value
+      // to the threshold so that any host with score equal to or less than
+      // the threshold is not included in the hints fetcher request.
+      pref_service_->SetDouble(
+          optimization_guide::prefs::
+              kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore,
+          std::min(detail.total_score + 0.001f,
+                   optimization_guide::features::
+                       MinTopHostEngagementScoreThreshold()));
       break;
     }
-    if (detail.origin.SchemeIsHTTPOrHTTPS()) {
-      top_host_blacklist->SetBoolKey(
-          optimization_guide::HashHostForDictionary(detail.origin.host()),
-          true);
-    }
+    top_host_blacklist->SetBoolKey(
+        optimization_guide::HashHostForDictionary(detail.origin.host()), true);
   }
 
   UMA_HISTOGRAM_COUNTS_1000(
@@ -312,8 +330,10 @@ std::vector<std::string> DataSaverTopHostProvider::GetTopHosts(
     if (duration_since_blacklist_initialized <=
             optimization_guide::features::
                 DurationApplyLowEngagementScoreThreshold() &&
-        detail.total_score < optimization_guide::features::
-                                 MinTopHostEngagementScoreThreshold()) {
+        detail.total_score <
+            pref_service_->GetDouble(
+                optimization_guide::prefs::
+                    kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore)) {
       return top_hosts;
     }
     if (!IsHostBlacklisted(top_host_blacklist, detail.origin.host())) {

chrome/browser/data_saver/data_saver_top_host_provider_unittest.cc

@@ -64,11 +64,18 @@ class DataSaverTopHostProviderTest : public ChromeRenderViewHostTestHarness {
     }
   }
 
+  void AddEngagedHostsWithPoints(size_t num_hosts, int num_points) {
+    for (size_t i = 1; i <= num_hosts; i++) {
+      AddEngagedHost(GURL(base::StringPrintf("https://domain%zu.com", i)),
+                     num_points);
+    }
+  }
+
   void AddEngagedHost(GURL url, int num_points) {
     service_->AddPointsForTesting(url, num_points);
   }
 
-  bool IsHostBlacklisted(const std::string& host) {
+  bool IsHostBlacklisted(const std::string& host) const {
     const base::DictionaryValue* top_host_blacklist =
         pref_service_->GetDictionary(
             optimization_guide::prefs::kHintsFetcherDataSaverTopHostBlacklist);
@@ -76,6 +83,13 @@ class DataSaverTopHostProviderTest : public ChromeRenderViewHostTestHarness {
         optimization_guide::HashHostForDictionary(host));
   }
 
+  double GetHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore()
+      const {
+    return pref_service_->GetDouble(
+        optimization_guide::prefs::
+            kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore);
+  }
+
   void PopulateTopHostBlacklist(size_t num_hosts) {
     std::unique_ptr<base::DictionaryValue> top_host_filter =
         pref_service_
@@ -422,10 +436,7 @@ TEST_F(DataSaverTopHostProviderTest, TopHostsFilteredByEngagementThreshold) {
   EXPECT_EQ(hosts.size(), 0u);
 
   hosts = top_host_provider()->GetTopHosts(kMaxHostsRequested);
-  EXPECT_EQ(
-      hosts.size(),
-      engaged_hosts -
-          optimization_guide::features::MaxHintsFetcherTopHostBlacklistSize());
+  EXPECT_EQ(1u, hosts.size());
   EXPECT_EQ(GetCurrentTopHostBlacklistState(),
             optimization_guide::prefs::HintsFetcherTopHostBlacklistState::
                 kInitialized);
@@ -466,3 +477,80 @@ TEST_F(DataSaverTopHostProviderTest, TopHostsFilteredByEngagementThreshold) {
   EXPECT_NE(std::find(hosts.begin(), hosts.end(), "lowengagement4.com"),
             hosts.end());
 }
+
+TEST_F(DataSaverTopHostProviderTest,
+       TopHostsFilteredByEngagementThreshold_NumPoints) {
+  size_t engaged_hosts =
+      optimization_guide::features::MaxHintsFetcherTopHostBlacklistSize() + 1;
+
+  // Set the count of maximum hosts requested to a large number so that the
+  // count itself does not prevent top_host_provider() from returning hosts that
+  // it would have otherwise returned.
+  static const size_t kMaxHostsRequested = INT16_MAX;
+
+  AddEngagedHostsWithPoints(engaged_hosts, 15);
+  // Add two hosts with engagement scores less than 15. These hosts should not
+  // be returned by the top host provider because the minimum engagement score
+  // threshold is set to a value larger than 5.
+  AddEngagedHost(GURL("https://lowengagement1.com"), 5);
+  AddEngagedHost(GURL("https://lowengagement2.com"), 5);
+
+  // Before the blacklist is populated, the threshold should have a default
+  // value.
+  EXPECT_EQ(3,
+            GetHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore());
+
+  // Blacklist should be populated on the first request.
+  std::vector<std::string> hosts =
+      top_host_provider()->GetTopHosts(kMaxHostsRequested);
+  EXPECT_EQ(hosts.size(), 0u);
+
+  hosts = top_host_provider()->GetTopHosts(kMaxHostsRequested);
+  EXPECT_NEAR(GetHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore(),
+              GetHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore(),
+              1);
+  EXPECT_EQ(3u, hosts.size());
+  EXPECT_EQ(GetCurrentTopHostBlacklistState(),
+            optimization_guide::prefs::HintsFetcherTopHostBlacklistState::
+                kInitialized);
+  EXPECT_NE(std::find(hosts.begin(), hosts.end(), "lowengagement1.com"),
+            hosts.end());
+  EXPECT_NE(std::find(hosts.begin(), hosts.end(), "lowengagement2.com"),
+            hosts.end());
+}
+
+TEST_F(DataSaverTopHostProviderTest,
+       TopHostsFilteredByEngagementThreshold_LowScore) {
+  size_t engaged_hosts =
+      optimization_guide::features::MaxHintsFetcherTopHostBlacklistSize() - 2;
+
+  // Set the count of maximum hosts requested to a large number so that the
+  // count itself does not prevent top_host_provider() from returning hosts that
+  // it would have otherwise returned.
+  static const size_t kMaxHostsRequested = INT16_MAX;
+
+  AddEngagedHostsWithPoints(engaged_hosts, 2);
+
+  // Blacklist should be populated on the first request. Set the count of
+  // desired
+  std::vector<std::string> hosts =
+      top_host_provider()->GetTopHosts(kMaxHostsRequested);
+  EXPECT_EQ(hosts.size(), 0u);
+
+  // Add two hosts with very low engagement scores. These hosts should be
+  // returned by top_host_provider() even with low score.
+  EXPECT_EQ(-1,
+            GetHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore());
+  AddEngagedHost(GURL("https://lowengagement1.com"), 1);
+  AddEngagedHost(GURL("https://lowengagement2.com"), 1);
+
+  hosts = top_host_provider()->GetTopHosts(kMaxHostsRequested);
+  EXPECT_EQ(2u, hosts.size());
+  EXPECT_EQ(GetCurrentTopHostBlacklistState(),
+            optimization_guide::prefs::HintsFetcherTopHostBlacklistState::
+                kInitialized);
+  EXPECT_NE(std::find(hosts.begin(), hosts.end(), "lowengagement1.com"),
+            hosts.end());
+  EXPECT_NE(std::find(hosts.begin(), hosts.end(), "lowengagement2.com"),
+            hosts.end());
+}

components/optimization_guide/optimization_guide_prefs.cc

@@ -4,6 +4,7 @@
 
 #include "components/optimization_guide/optimization_guide_prefs.h"
 
+#include "components/optimization_guide/optimization_guide_features.h"
 #include "components/prefs/pref_registry_simple.h"
 
 namespace optimization_guide {
@@ -33,6 +34,11 @@ const char kHintsFetcherDataSaverTopHostBlacklistState[] =
 const char kTimeBlacklistLastInitialized[] =
     "optimization_guide.hintsfetcher.time_blacklist_last_initialized";
 
+// If a host has site engagement score less than the value stored in this pref,
+// then hints fetcher may not fetch hints for that host.
+const char kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore[] =
+    "optimization_guide.hintsfetcher.top_host_blacklist_min_engagement_score";
+
 // A dictionary pref that stores hosts that have had hints successfully fetched
 // from the remote Optimization Guide Server. The entry for each host contains
 // the time that the fetch that covered this host expires, i.e., any hints
@@ -62,6 +68,17 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) {
       PrefRegistry::LOSSY_PREF);
   registry->RegisterDoublePref(kTimeBlacklistLastInitialized, 0,
                                PrefRegistry::LOSSY_PREF);
+
+  // Use a default value of MinTopHostEngagementScoreThreshold() for the
+  // threshold. This ensures that the users for which this pref can't be
+  // computed (possibly because they had the blacklist initialized before this
+  // pref was added to the code) use the default value for the site engagement
+  // threshold.
+  registry->RegisterDoublePref(
+      kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore,
+      optimization_guide::features::MinTopHostEngagementScoreThreshold(),
+      PrefRegistry::LOSSY_PREF);
+
   registry->RegisterStringPref(kPendingHintsProcessingVersion, "",
                                PrefRegistry::LOSSY_PREF);
 }

components/optimization_guide/optimization_guide_prefs.h

@@ -16,6 +16,8 @@ extern const char kHintsFetcherLastFetchAttempt[];
 extern const char kHintsFetcherDataSaverTopHostBlacklist[];
 extern const char kHintsFetcherDataSaverTopHostBlacklistState[];
 extern const char kTimeBlacklistLastInitialized[];
+extern const char
+    kHintsFetcherDataSaverTopHostBlacklistMinimumEngagementScore[];
 extern const char kHintsFetcherHostsSuccessfullyFetched[];
 extern const char kPendingHintsProcessingVersion[];