Strip disabled optimizations when parsing preview hints

The logic in PreviewsHints::CreateFromConfig() has been modified so that
it now strips out optimizations that are disabled and only allows one of
each optimization type within a PageHint. Duplicate types encountered
are also stripped. This makes it so that enabling an experimental
optimization will automatically disable a subsequent optimization of the
same optimization type in the same PageHint. If all optimizations for a
page hint are stripped, then the page hint is also stripped.

This fixes a bug related to disabled page hint optimizations being
usable, as the disabled optimizations will no longer exist in the
hint cache.

As a result of this change, when the total page hints with resource
hints exceeds the cap, the number of page hints added will always
exactly match the cap. This is due to the logic for stripping hints
allowing us to add hints that only include some of their page hints.

A DCHECK has been added verifying that non-resource loading hint
optimizations never contain resource loading hints.

Additionally, the description of |whitelisted_optimizations| for the
PageHint proto has been updated to match expectations that it will be
provided as an ordered list.

Unittests have been added to verify that the stripping is handled
properly and that the number of page hints allowed stops exactly at the
cap. Some issues with unittests in PreviewsOptimizationGuideUnittest
using InitializeMultipleResourceLoadingHints() have also been fixed.

Bug: 900725, 900733
Change-Id: Ic8d8ce2de98e4e2993cab994906c3b2e43c31a22
Reviewed-on: https://chromium-review.googlesource.com/c/1311796Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Jered Gray <jegray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604773}

components/optimization_guide/proto/hints.proto

@@ -120,8 +120,9 @@ message PageHint {
   // The maximum effective connection type threshold for triggering the
   // optimization associated with this hint.
   optional EffectiveConnectionType max_ect_trigger = 2;
-  // An unordered set of optimizations that should be whitelisted for this
-  // page pattern.
+  // An ordered list of optimizations that should be whitelisted for this page
+  // pattern. The client uses the first optimization in this list that is
+  // enabled for it.
   repeated Optimization whitelisted_optimizations = 3;
 }
 

components/previews/content/previews_hints.h

@@ -8,6 +8,8 @@
 #include <map>
 #include <memory>
 #include <set>
+#include <utility>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/sequence_checker.h"

components/previews/content/previews_hints_unittest.cc

@@ -295,6 +295,11 @@ TEST_F(PreviewsHintsTest, IsWhitelistedOutParams) {
   optimization_with_inflation_percent->set_inflation_percent(55);
   optimization_with_inflation_percent->set_optimization_type(
       optimization_guide::proto::RESOURCE_LOADING);
+  optimization_guide::proto::ResourceLoadingHint* resource_hint1 =
+      optimization_with_inflation_percent->add_resource_loading_hints();
+  resource_hint1->set_loading_optimization_type(
+      optimization_guide::proto::LOADING_BLOCK_RESOURCE);
+  resource_hint1->set_resource_pattern("default_resource.js");
   // Page hint for "/has_max_ect_trigger/"
   optimization_guide::proto::PageHint* page_hint2 = hint1->add_page_hints();
   page_hint2->set_page_pattern("/has_max_ect_trigger/");
@@ -306,6 +311,11 @@ TEST_F(PreviewsHintsTest, IsWhitelistedOutParams) {
           page_hint2->add_whitelisted_optimizations();
   optimization_without_inflation_percent->set_optimization_type(
       optimization_guide::proto::RESOURCE_LOADING);
+  optimization_guide::proto::ResourceLoadingHint* resource_hint2 =
+      optimization_without_inflation_percent->add_resource_loading_hints();
+  resource_hint2->set_loading_optimization_type(
+      optimization_guide::proto::LOADING_BLOCK_RESOURCE);
+  resource_hint2->set_resource_pattern("default_resource.js");
   ParseConfig(config);
 
   // Verify optimization providing inflation_percent.
@@ -333,6 +343,32 @@ TEST_F(PreviewsHintsTest, IsWhitelistedOutParams) {
             ect_threshold);
 }
 
+TEST_F(PreviewsHintsTest,
+       IsWhitelistedForNoScriptInPageHintsWithResourceLoadingHintsDisabled) {
+  optimization_guide::proto::Configuration config;
+
+  optimization_guide::proto::Hint* hint1 = config.add_hints();
+  hint1->set_key("somedomain.org");
+  hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+
+  // Page hint with NOSCRIPT optimization
+  optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
+  page_hint1->set_page_pattern("/has_noscript/");
+  optimization_guide::proto::Optimization* optimization_with_inflation_percent =
+      page_hint1->add_whitelisted_optimizations();
+  optimization_with_inflation_percent->set_optimization_type(
+      optimization_guide::proto::NOSCRIPT);
+
+  ParseConfig(config);
+
+  int inflation_percent = 0;
+  net::EffectiveConnectionType ect_threshold =
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+  EXPECT_TRUE(previews_hints()->IsWhitelisted(
+      GURL("https://www.somedomain.org/has_noscript/"), PreviewsType::NOSCRIPT,
+      &inflation_percent, &ect_threshold));
+}
+
 TEST_F(PreviewsHintsTest, IsWhitelistedForExperimentalPreview) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(features::kResourceLoadingHints);
@@ -371,28 +407,45 @@ TEST_F(PreviewsHintsTest, IsWhitelistedForExperimentalPreview) {
   default_resourcehint->set_loading_optimization_type(
       optimization_guide::proto::LOADING_BLOCK_RESOURCE);
   default_resourcehint->set_resource_pattern("experimental_resource.js");
-  ParseConfig(config);
 
-  // Verify default resource hint whitelisted (via inflation_percent).
-  int inflation_percent = 0;
-  net::EffectiveConnectionType ect_threshold =
-      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
-  EXPECT_TRUE(previews_hints()->IsWhitelisted(
-      GURL("https://www.somedomain.org/experimental_preview/"
-           "experimental_resource.js"),
-      PreviewsType::RESOURCE_LOADING_HINTS, &inflation_percent,
-      &ect_threshold));
-  EXPECT_EQ(33, inflation_percent);
-  EXPECT_EQ(net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G,
-            ect_threshold);
+  // Test with the experiment disabled.
+  {
+    base::HistogramTester histogram_tester;
+
+    ParseConfig(config);
+
+    // Verify default resource hint whitelisted (via inflation_percent).
+    int inflation_percent = 0;
+    net::EffectiveConnectionType ect_threshold =
+        net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+    EXPECT_TRUE(previews_hints()->IsWhitelisted(
+        GURL("https://www.somedomain.org/experimental_preview/"
+             "experimental_resource.js"),
+        PreviewsType::RESOURCE_LOADING_HINTS, &inflation_percent,
+        &ect_threshold));
+    EXPECT_EQ(33, inflation_percent);
+    EXPECT_EQ(net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G,
+              ect_threshold);
+
+    // Verify that the experimental optimization was not added when it was
+    // disabled.
+    histogram_tester.ExpectUniqueSample(
+        "ResourceLoadingHints.ResourceHints.TotalReceived", 1, 1);
+  }
 
   // Now enable the experiment and verify experimental resource hint chosen.
   {
+    base::HistogramTester histogram_tester;
+
     base::test::ScopedFeatureList scoped_list2;
     scoped_list2.InitAndEnableFeatureWithParameters(
         features::kOptimizationHintsExperiments,
         {{"experiment_name", "foo_experiment"}});
-    int inflation_percent;
+
+    // Parse the config again with the experiment enabled.
+    ParseConfig(config);
+
+    int inflation_percent = 0;
     net::EffectiveConnectionType ect_threshold =
         net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G;
     EXPECT_TRUE(previews_hints()->IsWhitelisted(
@@ -403,6 +456,11 @@ TEST_F(PreviewsHintsTest, IsWhitelistedForExperimentalPreview) {
     EXPECT_EQ(99, inflation_percent);
     EXPECT_EQ(net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G,
               ect_threshold);
+
+    // Verify that the second optimization was not added when the experimental
+    // optimization was enabled.
+    histogram_tester.ExpectUniqueSample(
+        "ResourceLoadingHints.ResourceHints.TotalReceived", 1, 1);
   }
 }
 

components/previews/content/previews_optimization_guide.cc

@@ -74,18 +74,23 @@ void PreviewsOptimizationGuide::OnLoadedHint(
   std::vector<std::string> resource_patterns_to_block;
   for (const auto& optimization :
        matched_page_hint->whitelisted_optimizations()) {
-    if (optimization.optimization_type() ==
+    if (optimization.optimization_type() !=
         optimization_guide::proto::RESOURCE_LOADING) {
-      for (const auto& resource_loading_hint :
-           optimization.resource_loading_hints()) {
-        if (!resource_loading_hint.resource_pattern().empty() &&
-            resource_loading_hint.loading_optimization_type() ==
-                optimization_guide::proto::LOADING_BLOCK_RESOURCE) {
-          resource_patterns_to_block.push_back(
-              resource_loading_hint.resource_pattern());
-        }
+      continue;
+    }
+
+    // TODO(jegray): When persistence is added for hints, address handling of
+    // disabled experimental optimizations.
+    for (const auto& resource_loading_hint :
+         optimization.resource_loading_hints()) {
+      if (!resource_loading_hint.resource_pattern().empty() &&
+          resource_loading_hint.loading_optimization_type() ==
+              optimization_guide::proto::LOADING_BLOCK_RESOURCE) {
+        resource_patterns_to_block.push_back(
+            resource_loading_hint.resource_pattern());
       }
     }
+    break;
   }
   if (!resource_patterns_to_block.empty()) {
     std::move(callback).Run(document_url, resource_patterns_to_block);