[Previews] Stops processing top level cacao hints

Actually puts top level hint processing and evaluation behind a new
Feature that is disabled by default (in case we need to revert hints
due to memory footprint or such).

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

components/previews/content/previews_hints.cc

@@ -342,35 +342,38 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromConfig(
     }
     seen_host_suffixes.insert(hint_key);
 
-    // Create whitelist condition set out of the optimizations that are
-    // whitelisted for the host suffix at the top level (i.e., not within
-    // PageHints).
-    std::set<std::pair<PreviewsType, int>> whitelisted_optimizations;
-    for (const auto& optimization : hint.whitelisted_optimizations()) {
-      if (IsDisabledExperimentalOptimization(optimization)) {
-        continue;
-      }
-      base::Optional<PreviewsType> previews_type =
-          ConvertProtoOptimizationTypeToPreviewsType(
-              optimization.optimization_type());
-      if (!previews_type.has_value()) {
-        continue;
+    // Only process legacy top level hints if specifically enabled.
+    if (previews::params::NoScriptPreviewsUsesTopLevelHints()) {
+      // Create whitelist condition set out of the optimizations that are
+      // whitelisted for the host suffix at the top level (i.e., not within
+      // PageHints).
+      std::set<std::pair<PreviewsType, int>> whitelisted_optimizations;
+      for (const auto& optimization : hint.whitelisted_optimizations()) {
+        if (IsDisabledExperimentalOptimization(optimization)) {
+          continue;
+        }
+        base::Optional<PreviewsType> previews_type =
+            ConvertProtoOptimizationTypeToPreviewsType(
+                optimization.optimization_type());
+        if (!previews_type.has_value()) {
+          continue;
+        }
+        // Resource loading hints should always be page hints; if they appear as
+        // top-level whitelisted optimizations, then it indicates a bug.
+        DCHECK(previews_type != PreviewsType::RESOURCE_LOADING_HINTS);
+
+        whitelisted_optimizations.insert(std::make_pair(
+            previews_type.value(), optimization.inflation_percent()));
       }
-      // Resource loading hints should always be page hints; if they appear as
-      // top-level whitelisted optimizations, then it indicates a bug.
-      DCHECK(previews_type != PreviewsType::RESOURCE_LOADING_HINTS);
 
-      whitelisted_optimizations.insert(std::make_pair(
-          previews_type.value(), optimization.inflation_percent()));
+      url_matcher::URLMatcherCondition condition =
+          condition_factory->CreateHostSuffixCondition(hint_key);
+      all_conditions.push_back(new url_matcher::URLMatcherConditionSet(
+          id, std::set<url_matcher::URLMatcherCondition>{condition}));
+      hints->whitelist_[id] = whitelisted_optimizations;
+      id++;
     }
 
-    url_matcher::URLMatcherCondition condition =
-        condition_factory->CreateHostSuffixCondition(hint_key);
-    all_conditions.push_back(new url_matcher::URLMatcherConditionSet(
-        id, std::set<url_matcher::URLMatcherCondition>{condition}));
-    hints->whitelist_[id] = whitelisted_optimizations;
-    id++;
-
     // If this hint contains page hints, then add a pared down version of the
     // hint to the initial hints that are used to populate the hint cache,
     // removing all of the hint's top-level optimizations and only retaining the
@@ -400,7 +403,7 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromConfig(
   // Completed processing hints data without crashing so clear sentinel.
   DeleteSentinelFile(sentinel_path);
   RecordProcessHintsResult(
-      all_conditions.empty()
+      hints->initial_hints_.empty() && all_conditions.empty()
           ? PreviewsProcessHintsResult::kProcessedNoPreviewsHints
           : PreviewsProcessHintsResult::kProcessedPreviewsHints);
   return hints;
@@ -523,6 +526,12 @@ bool PreviewsHints::IsWhitelistedAtTopLevel(const GURL& url,
                                             int* out_inflation_percent) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  // Top level hints are deprecated so only check here if specifically enabled
+  // for NoScript.
+  if (!previews::params::NoScriptPreviewsUsesTopLevelHints()) {
+    return false;
+  }
+
   // Resource loading hints are not processed in the top-level whitelist.
   if (type == PreviewsType::RESOURCE_LOADING_HINTS) {
     return false;

components/previews/content/previews_optimization_guide_unittest.cc

@@ -298,7 +298,11 @@ TEST_F(PreviewsOptimizationGuideTest, IsWhitelistedWithoutHints) {
 }
 
 TEST_F(PreviewsOptimizationGuideTest,
-       ProcessHintsWhitelistForNoScriptTopLevelHintsPopulatedCorrectly) {
+       ProcessHintsWhitelistForNoScriptTopLevelHintsWithTopLevelHintsEnabled) {
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(
+      features::kNoScriptPreviewsUsesTopLevelHints);
+
   optimization_guide::proto::Configuration config;
   optimization_guide::proto::Hint* hint1 = config.add_hints();
   hint1->set_key("facebook.com");
@@ -334,6 +338,46 @@ TEST_F(PreviewsOptimizationGuideTest,
                                       PreviewsType::NOSCRIPT, &ect_threshold));
 }
 
+TEST_F(PreviewsOptimizationGuideTest,
+       ProcessHintsWhitelistForNoScriptTopLevelHintsWithTopLevelHintsDisabled) {
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndDisableFeature(
+      features::kNoScriptPreviewsUsesTopLevelHints);
+
+  optimization_guide::proto::Configuration config;
+  optimization_guide::proto::Hint* hint1 = config.add_hints();
+  hint1->set_key("facebook.com");
+  hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::Optimization* optimization1 =
+      hint1->add_whitelisted_optimizations();
+  optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
+  // Add a second optimization to ensure that the applicable optimizations are
+  // still whitelisted.
+  optimization_guide::proto::Optimization* optimization2 =
+      hint1->add_whitelisted_optimizations();
+  optimization2->set_optimization_type(
+      optimization_guide::proto::TYPE_UNSPECIFIED);
+  // Add a second hint.
+  optimization_guide::proto::Hint* hint2 = config.add_hints();
+  hint2->set_key("twitter.com");
+  hint2->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::Optimization* optimization3 =
+      hint2->add_whitelisted_optimizations();
+  optimization3->set_optimization_type(optimization_guide::proto::NOSCRIPT);
+  ProcessHints(config, "2.0.0");
+
+  RunUntilIdle();
+  PreviewsUserData user_data(kDefaultPageId);
+  net::EffectiveConnectionType ect_threshold;
+  // Not whitelisted since NoScriptPreviewsUsesTopLevelHints disabled.
+  EXPECT_FALSE(guide()->IsWhitelisted(&user_data,
+                                      GURL("https://m.facebook.com"),
+                                      PreviewsType::NOSCRIPT, &ect_threshold));
+  EXPECT_FALSE(guide()->IsWhitelisted(&user_data,
+                                      GURL("https://m.twitter.com/example"),
+                                      PreviewsType::NOSCRIPT, &ect_threshold));
+}
+
 TEST_F(PreviewsOptimizationGuideTest,
        ProcessHintsForNoScriptPageHintsPopulatedCorrectly) {
   optimization_guide::proto::Configuration config;
@@ -396,7 +440,7 @@ TEST_F(PreviewsOptimizationGuideTest,
 
 // Test when resource loading hints are enabled.
 TEST_F(PreviewsOptimizationGuideTest,
-       ProcessHintsWhitelistForResourceLoadingHintsPopulatedCorrectly) {
+       ProcessHintsForResourceLoadingHintsPopulatedCorrectly) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(features::kResourceLoadingHints);
 
@@ -475,8 +519,10 @@ TEST_F(
   optimization_guide::proto::Hint* hint1 = config.add_hints();
   hint1->set_key("facebook.com");
   hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
+  page_hint1->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization1 =
-      hint1->add_whitelisted_optimizations();
+      page_hint1->add_whitelisted_optimizations();
   optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
 
   // Add a second optimization to ensure that the applicable optimizations are
@@ -490,10 +536,10 @@ TEST_F(
   optimization_guide::proto::Hint* hint2 = config.add_hints();
   hint2->set_key("twitter.com");
   hint2->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
-  optimization_guide::proto::PageHint* page_hint1 = hint2->add_page_hints();
-  page_hint1->set_page_pattern("*");
+  optimization_guide::proto::PageHint* page_hint2 = hint2->add_page_hints();
+  page_hint2->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization3 =
-      page_hint1->add_whitelisted_optimizations();
+      page_hint2->add_whitelisted_optimizations();
   optimization3->set_optimization_type(
       optimization_guide::proto::RESOURCE_LOADING);
   optimization_guide::proto::ResourceLoadingHint* resource_hint1 =
@@ -522,7 +568,7 @@ TEST_F(
 }
 
 TEST_F(PreviewsOptimizationGuideTest,
-       ProcessHintsWhitelistForResourceLoadingHintsWithSlowPageTriggering) {
+       ProcessHintsForResourceLoadingHintsWithSlowPageTriggering) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(features::kResourceLoadingHints);
 
@@ -604,8 +650,10 @@ void PreviewsOptimizationGuideTest::DoExperimentFlagTest(
   optimization_guide::proto::Hint* hint1 = config.add_hints();
   hint1->set_key("facebook.com");
   hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
+  page_hint1->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization1 =
-      hint1->add_whitelisted_optimizations();
+      page_hint1->add_whitelisted_optimizations();
   // NOSCRIPT is the optimization under test and may be marked experimental.
   if (experiment_name.has_value()) {
     optimization1->set_experiment_name(experiment_name.value());
@@ -613,8 +661,8 @@ void PreviewsOptimizationGuideTest::DoExperimentFlagTest(
   optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
 
   // RESOURCE_LOADING is not marked experimental.
-  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
-  page_hint1->set_page_pattern("*");
+  optimization_guide::proto::PageHint* page_hint2 = hint1->add_page_hints();
+  page_hint2->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization2 =
       page_hint1->add_whitelisted_optimizations();
   optimization2->set_optimization_type(
@@ -629,8 +677,10 @@ void PreviewsOptimizationGuideTest::DoExperimentFlagTest(
   optimization_guide::proto::Hint* hint2 = config.add_hints();
   hint2->set_key("twitter.com");
   hint2->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::PageHint* page_hint3 = hint2->add_page_hints();
+  page_hint3->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization3 =
-      hint2->add_whitelisted_optimizations();
+      page_hint3->add_whitelisted_optimizations();
   optimization3->set_optimization_type(optimization_guide::proto::NOSCRIPT);
   ProcessHints(config, "2.0.0");
 
@@ -725,8 +775,10 @@ TEST_F(PreviewsOptimizationGuideTest, ProcessHintsUnsupportedKeyRepIsIgnored) {
   hint->set_key("facebook.com");
   hint->set_key_representation(
       optimization_guide::proto::REPRESENTATION_UNSPECIFIED);
+  optimization_guide::proto::PageHint* page_hint = hint->add_page_hints();
+  page_hint->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization =
-      hint->add_whitelisted_optimizations();
+      page_hint->add_whitelisted_optimizations();
   optimization->set_optimization_type(optimization_guide::proto::NOSCRIPT);
   ProcessHints(config, "2.0.0");
 
@@ -768,8 +820,10 @@ TEST_F(PreviewsOptimizationGuideTest, ProcessHintsWithExistingSentinel) {
   optimization_guide::proto::Hint* hint1 = config.add_hints();
   hint1->set_key("facebook.com");
   hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
+  page_hint1->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization1 =
-      hint1->add_whitelisted_optimizations();
+      page_hint1->add_whitelisted_optimizations();
   optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
 
   // Create sentinel file for version 2.0.0.
@@ -808,8 +862,10 @@ TEST_F(PreviewsOptimizationGuideTest, ProcessHintsWithInvalidSentinelFile) {
   optimization_guide::proto::Hint* hint1 = config.add_hints();
   hint1->set_key("facebook.com");
   hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
+  page_hint1->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization1 =
-      hint1->add_whitelisted_optimizations();
+      page_hint1->add_whitelisted_optimizations();
   optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
 
   // Create sentinel file with invalid contents.
@@ -860,8 +916,10 @@ TEST_F(PreviewsOptimizationGuideTest,
   optimization_guide::proto::Hint* hint1 = config.add_hints();
   hint1->set_key("facebook.com");
   hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
+  page_hint1->set_page_pattern("*");
   optimization_guide::proto::Optimization* optimization1 =
-      hint1->add_whitelisted_optimizations();
+      page_hint1->add_whitelisted_optimizations();
   optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
   optimization_guide::proto::Hint* hint2 = config.add_hints();
   hint2->set_key("facebook.com");
@@ -876,7 +934,12 @@ TEST_F(PreviewsOptimizationGuideTest,
   });
 }
 
-TEST_F(PreviewsOptimizationGuideTest, IsWhitelistedWithMultipleHintMatches) {
+TEST_F(PreviewsOptimizationGuideTest,
+       IsWhitelistedWithMultipleTopLevelHintMatches) {
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(
+      features::kNoScriptPreviewsUsesTopLevelHints);
+
   optimization_guide::proto::Configuration config;
 
   // Whitelist NoScript for indoor.sports.yahoo.com:

components/previews/core/previews_experiments.cc

@@ -317,6 +317,11 @@ int NoScriptPreviewsInflationBytes() {
                                           kNoScriptInflationBytes, 0);
 }
 
+bool NoScriptPreviewsUsesTopLevelHints() {
+  return base::FeatureList::IsEnabled(
+      features::kNoScriptPreviewsUsesTopLevelHints);
+}
+
 int ResourceLoadingHintsPreviewsInflationPercent() {
   return GetFieldTrialParamByFeatureAsInt(features::kResourceLoadingHints,
                                           kResourceLoadingHintsInflationPercent,

components/previews/core/previews_experiments.h

@@ -161,6 +161,11 @@ int NoScriptPreviewsInflationPercent();
 // for inflating the original_bytes count.
 int NoScriptPreviewsInflationBytes();
 
+// Whether to use top level optimization hints for NoScript instead of
+// page hints. This is to allow for reverting to original behavior until
+// page hints for NoScript is successfully launched.
+bool NoScriptPreviewsUsesTopLevelHints();
+
 // For estimating ResourceLoadingHints data savings, this is the percentage
 // factor to multiple by the network bytes for inflating the original_bytes
 // count.

components/previews/core/previews_features.cc

@@ -46,6 +46,10 @@ const base::Feature kNoScriptPreviews {
 #endif  // defined(OS_ANDROID)
 };
 
+// Enables using (legacy) top level optimization hints to whitelist NoScript.
+const base::Feature kNoScriptPreviewsUsesTopLevelHints{
+    "NoScriptPreviewsUsesTopLevelHints", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Enables the Stale Previews timestamp on Previews infobars.
 const base::Feature kStalePreviewsTimestamp{"StalePreviewsTimestamp",
                                             base::FEATURE_ENABLED_BY_DEFAULT};

components/previews/core/previews_features.h

@@ -14,6 +14,7 @@ extern const base::Feature kPreviews;
 extern const base::Feature kOfflinePreviews;
 extern const base::Feature kClientLoFi;
 extern const base::Feature kNoScriptPreviews;
+extern const base::Feature kNoScriptPreviewsUsesTopLevelHints;
 extern const base::Feature kStalePreviewsTimestamp;
 extern const base::Feature kOptimizationHints;
 extern const base::Feature kOptimizationHintsExperiments;