Adds HintCache checking to PreviewsHints::IsWhitelisted

Integrated PageHints whitelisted optimization checking to the
PreviewsHints::IsWhitelisted() which is used at commit time to
decide what PreviewsState bit to use.

Bug: 871795
Change-Id: Ifa0080a51f9df5ec9aefd4044b563259d0ce4978
TBR: dougarnett@chromiumn.org
Reviewed-on: https://chromium-review.googlesource.com/1176188Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583364}

components/previews/content/BUILD.gn

@@ -37,6 +37,7 @@ source_set("unit_tests") {
     "hint_cache_unittest.cc",
     "previews_content_util_unittest.cc",
     "previews_decider_impl_unittest.cc",
+    "previews_hints_unittest.cc",
     "previews_optimization_guide_unittest.cc",
     "previews_ui_service_unittest.cc",
   ]

components/previews/content/previews_hints.cc

@@ -231,6 +231,22 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromConfig(
   return hints;
 }
 
+// static
+const optimization_guide::proto::PageHint* PreviewsHints::FindPageHint(
+    const GURL& document_url,
+    const optimization_guide::proto::Hint& hint) {
+  std::string url = document_url.spec();
+  for (const auto& page_hint : hint.page_hints()) {
+    // TODO(dougarnett): Support wildcards. https://crbug.com/870039
+    if (!page_hint.page_pattern().empty() &&
+        url.find(page_hint.page_pattern()) != std::string::npos) {
+      // Return the first matching page hint.
+      return &page_hint;
+    }
+  }
+  return nullptr;
+}
+
 void PreviewsHints::Initialize() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!initial_hints_.empty()) {
@@ -246,33 +262,59 @@ bool PreviewsHints::IsWhitelisted(const GURL& url,
                                   int* out_inflation_percent) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  if (!url.has_host())
+    return false;
+
+  // First check for being whitelisted at top level for host suffix.
   std::set<url_matcher::URLMatcherConditionSet::ID> matches =
       url_matcher_.MatchURL(url);
 
   // Only consider the first match in iteration order as it takes precedence
-  // if there are multiple matches.
+  // if there are multiple matches for the top-level whitelist.
   const auto& first_match = matches.begin();
-  if (first_match == matches.end()) {
+  if (first_match != matches.end()) {
+    const auto whitelist_iter = whitelist_.find(*first_match);
+    if (whitelist_iter != whitelist_.end()) {
+      const auto& whitelisted_optimizations = whitelist_iter->second;
+      for (auto optimization_iter = whitelisted_optimizations.begin();
+           optimization_iter != whitelisted_optimizations.end();
+           ++optimization_iter) {
+        if (optimization_iter->first == type) {
+          *out_inflation_percent = optimization_iter->second;
+          // Whitelisted on top level whitelist.
+          return true;
+        }
+      }
+    }
+  }
+
+  if (!hint_cache_)
+    return false;
+
+  // Now check HintCache for a loaded entry with a PageHint that matches |url|
+  // that whitelists the optimization.
+  std::string host = url.host();
+  if (!hint_cache_->IsHintLoaded(host)) {
+    // TODO(dougarnett): Add UMA histogram counts for both cases of HasHint().
     return false;
   }
 
-  const auto whitelist_iter = whitelist_.find(*first_match);
-  if (whitelist_iter == whitelist_.end()) {
+  const optimization_guide::proto::Hint* hint = hint_cache_->GetHint(host);
+  const optimization_guide::proto::PageHint* matched_page_hint =
+      FindPageHint(url, *hint);
+  if (!matched_page_hint) {
     return false;
   }
 
-  const auto& whitelisted_optimizations = whitelist_iter->second;
-  for (auto optimization_iter = whitelisted_optimizations.begin();
-       optimization_iter != whitelisted_optimizations.end();
-       ++optimization_iter) {
-    if (optimization_iter->first == type) {
-      *out_inflation_percent = optimization_iter->second;
+  for (const auto& optimization :
+       matched_page_hint->whitelisted_optimizations()) {
+    if (ConvertProtoOptimizationTypeToPreviewsOptimizationType(
+            optimization.optimization_type()) == type) {
+      *out_inflation_percent = optimization.inflation_percent();
       return true;
     }
   }
 
-  // TODO(dougarnett): Check the hint cache for whitelisted optmizations too.
-
   return false;
 }
 

components/previews/content/previews_hints.h

@@ -35,12 +35,19 @@ class PreviewsHints {
       const optimization_guide::proto::Configuration& config,
       const optimization_guide::ComponentInfo& info);
 
+  // Returns the matching PageHint for |document_url| if found in |hint|.
+  // TODO(dougarnett): Consider moving to some hint_util file.
+  static const optimization_guide::proto::PageHint* FindPageHint(
+      const GURL& document_url,
+      const optimization_guide::proto::Hint& hint);
+
   void Initialize();
 
   // Whether the URL is whitelisted for the given previews type. If so,
   // |out_inflation_percent| will be populated if meta data available for it.
-  // Note: this is top-level whitelist check which does not include checking
-  // within PageHints.
+  // This first checks the top-level whitelist and, if not whitelisted there,
+  // it will check the HintCache for having a loaded, matching PageHint that
+  // whitelists it.
   bool IsWhitelisted(const GURL& url,
                      PreviewsType type,
                      int* out_inflation_percent);

components/previews/content/previews_hints_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/previews/content/previews_hints.h"
+
+#include "components/optimization_guide/proto/hints.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace previews {
+
+// NOTE: most of the PreviewsHints tests are still included in the tests for
+// PreviewsOptimizationGuide.
+
+TEST(PreviewsHintsTest, FindPageHintForSubstringPagePattern) {
+  optimization_guide::proto::Hint hint1;
+
+  // Page hint for "/one/"
+  optimization_guide::proto::PageHint* page_hint1 = hint1.add_page_hints();
+  page_hint1->set_page_pattern("/one/");
+
+  // Page hint for "two"
+  optimization_guide::proto::PageHint* page_hint2 = hint1.add_page_hints();
+  page_hint2->set_page_pattern("two");
+
+  // Page hint for "three.jpg"
+  optimization_guide::proto::PageHint* page_hint3 = hint1.add_page_hints();
+  page_hint3->set_page_pattern("three.jpg");
+
+  EXPECT_EQ(nullptr, PreviewsHints::FindPageHint(GURL(""), hint1));
+  EXPECT_EQ(nullptr,
+            PreviewsHints::FindPageHint(GURL("https://www.foo.org/"), hint1));
+  EXPECT_EQ(nullptr, PreviewsHints::FindPageHint(
+                         GURL("https://www.foo.org/one"), hint1));
+
+  EXPECT_EQ(page_hint1, PreviewsHints::FindPageHint(
+                            GURL("https://www.foo.org/one/"), hint1));
+  EXPECT_EQ(page_hint1, PreviewsHints::FindPageHint(
+                            GURL("https://www.foo.org/one/two"), hint1));
+  EXPECT_EQ(page_hint1,
+            PreviewsHints::FindPageHint(
+                GURL("https://www.foo.org/one/two/three.jpg"), hint1));
+
+  EXPECT_EQ(page_hint2,
+            PreviewsHints::FindPageHint(
+                GURL("https://www.foo.org/onetwo/three.jpg"), hint1));
+  EXPECT_EQ(page_hint2,
+            PreviewsHints::FindPageHint(GURL("https://one.two.org"), hint1));
+
+  EXPECT_EQ(page_hint3, PreviewsHints::FindPageHint(
+                            GURL("https://www.foo.org/bar/three.jpg"), hint1));
+}
+
+}  // namespace previews

components/previews/content/previews_optimization_guide.cc

@@ -55,30 +55,27 @@ void PreviewsOptimizationGuide::OnLoadedHint(
     const GURL& document_url,
     const optimization_guide::proto::Hint& loaded_hint) const {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
-  std::string url = document_url.spec();
+
+  const optimization_guide::proto::PageHint* matched_page_hint =
+      PreviewsHints::FindPageHint(document_url, loaded_hint);
+  if (!matched_page_hint)
+    return;
+
+  // Retrieve the resource patterns to be blocked from the page hint.
   std::vector<std::string> resource_patterns_to_block;
-  for (const auto& page_hint : loaded_hint.page_hints()) {
-    // TODO(dougarnett): Support wildcards. crbug.com/870039
-    if (!page_hint.page_pattern().empty() &&
-        url.find(page_hint.page_pattern()) != std::string::npos) {
-      // Matched the page pattern so now check for resource loading
-      // optimization.
-      for (const auto& optimization : page_hint.whitelisted_optimizations()) {
-        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());
-            }
-          }
+  for (const auto& optimization :
+       matched_page_hint->whitelisted_optimizations()) {
+    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());
         }
       }
-      // Only use this first matching page hint.
-      break;
     }
   }
   if (!resource_patterns_to_block.empty()) {

components/previews/content/previews_optimization_guide_unittest.cc

@@ -660,6 +660,19 @@ TEST_F(PreviewsOptimizationGuideTest, MaybeLoadOptimizationHints) {
             loaded_hints_document_gurl());
   EXPECT_EQ(1ul, loaded_hints_resource_patterns().size());
   EXPECT_EQ("news_cruft.js", loaded_hints_resource_patterns().front());
+
+  // Verify whitelisting from loaded page hints.
+  EXPECT_TRUE(guide()->IsWhitelisted(
+      *CreateRequestWithURL(
+          GURL("https://www.somedomain.org/news/weather/raininginseattle")),
+      PreviewsType::RESOURCE_LOADING_HINTS));
+  EXPECT_TRUE(guide()->IsWhitelisted(
+      *CreateRequestWithURL(
+          GURL("https://www.somedomain.org/football/seahawksrebuildingyear")),
+      PreviewsType::RESOURCE_LOADING_HINTS));
+  EXPECT_FALSE(guide()->IsWhitelisted(
+      *CreateRequestWithURL(GURL("https://www.somedomain.org/unhinted")),
+      PreviewsType::RESOURCE_LOADING_HINTS));
 }
 
 TEST_F(PreviewsOptimizationGuideTest,
@@ -674,6 +687,13 @@ TEST_F(PreviewsOptimizationGuideTest,
   EXPECT_FALSE(guide()->MaybeLoadOptimizationHints(
       *CreateRequestWithURL(GURL("https://www.somedomain.org")),
       base::DoNothing()));
+
+  RunUntilIdle();
+
+  EXPECT_FALSE(guide()->IsWhitelisted(
+      *CreateRequestWithURL(
+          GURL("https://www.somedomain.org/news/weather/raininginseattle")),
+      PreviewsType::RESOURCE_LOADING_HINTS));
 }
 
 TEST_F(PreviewsOptimizationGuideTest, RemoveObserverCalledAtDestruction) {