Consolidate logic for processing hints into one function

Bug: 969558
Change-Id: Iff43356255c9e72dd85db61ce971c41851ada005
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1707130Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678504}

components/optimization_guide/hint_cache.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "components/optimization_guide/hint_update_data.h"
+#include "components/optimization_guide/hints_processing_util.h"
 #include "components/optimization_guide/optimization_guide_features.h"
 #include "url/gurl.h"
 
@@ -19,44 +20,6 @@ namespace {
 // is exceeded, the least recently used hint is purged from the cache.
 const size_t kDefaultMaxMemoryCacheHints = 20;
 
-// Verify |get_hints_response| and load the hints that have keys represented by
-// hosts suffix into UpdateData to be stored in the HintCache. Returns true if
-// there are applicable hints moved into UpdateData that can be stored.
-// TODO(crbug/969558): Consolidate this with ProcessConfigurationHints for
-// component hints.
-bool ProcessGetHintsResponse(proto::GetHintsResponse* get_hints_response,
-                             HintUpdateData* fetched_hints_update_data) {
-  std::unordered_set<std::string> seen_host_suffixes;
-
-  bool has_processed_hints = false;
-  // Process each hint in |get_hints_response|.
-  for (auto& hint : *(get_hints_response->mutable_hints())) {
-    // One |hint| applies to one host URL suffix.
-    if (hint.key_representation() != proto::HOST_SUFFIX) {
-      continue;
-    }
-    const std::string& hint_key = hint.key();
-
-    // Validate configuration keys.
-    DCHECK(!hint_key.empty());
-    if (hint_key.empty()) {
-      continue;
-    }
-
-    auto seen_host_suffixes_iter = seen_host_suffixes.find(hint_key);
-    DCHECK(seen_host_suffixes_iter == seen_host_suffixes.end());
-
-    seen_host_suffixes.insert(hint_key);
-
-    if (!hint.page_hints().empty()) {
-      // Move fetched hints into update data
-      fetched_hints_update_data->MoveHintIntoUpdateData(std::move(hint));
-      has_processed_hints = true;
-    }
-  }
-  return has_processed_hints;
-}
-
 }  // namespace
 
 HintCache::HintCache(
@@ -121,8 +84,8 @@ void HintCache::UpdateFetchedHints(
   }
   std::unique_ptr<HintUpdateData> fetched_hints_update_data =
       CreateUpdateDataForFetchedHints(update_time, expiry_time);
-  ProcessGetHintsResponse(get_hints_response.get(),
-                          fetched_hints_update_data.get());
+  ProcessHints(get_hints_response.get()->mutable_hints(),
+               fetched_hints_update_data.get());
   hint_store_->UpdateFetchedHints(std::move(fetched_hints_update_data),
                                   std::move(callback));
 }

components/optimization_guide/hints_processing_util.cc

@@ -8,8 +8,8 @@
 
 #include "base/metrics/field_trial_params.h"
 #include "base/strings/stringprintf.h"
+#include "components/optimization_guide/hint_update_data.h"
 #include "components/optimization_guide/optimization_guide_features.h"
-#include "components/optimization_guide/proto/hints.pb.h"
 #include "components/optimization_guide/url_pattern_with_wildcards.h"
 #include "url/gurl.h"
 
@@ -61,4 +61,54 @@ std::string HashHostForDictionary(const std::string& host) {
   return base::StringPrintf("%x", base::PersistentHash(host));
 }
 
+bool ProcessHints(
+    google::protobuf::RepeatedPtrField<optimization_guide::proto::Hint>* hints,
+    optimization_guide::HintUpdateData* hint_update_data) {
+  // If there's no update data, then there's nothing to do.
+  if (!hint_update_data)
+    return false;
+
+  std::unordered_set<std::string> seen_host_suffixes;
+
+  bool did_process_hints = false;
+  // Process each hint in the the hint configuration. The hints are mutable
+  // because once processing is completed on each individual hint, it is moved
+  // into the component update data. This eliminates the need to make any
+  // additional copies of the hints.
+  for (auto& hint : *hints) {
+    // We only support host suffixes at the moment. Skip anything else.
+    // One |hint| applies to one host URL suffix.
+    if (hint.key_representation() != optimization_guide::proto::HOST_SUFFIX) {
+      continue;
+    }
+
+    const std::string& hint_key = hint.key();
+
+    // Validate configuration keys.
+    DCHECK(!hint_key.empty());
+    if (hint_key.empty()) {
+      continue;
+    }
+
+    auto seen_host_suffixes_iter = seen_host_suffixes.find(hint_key);
+    DCHECK(seen_host_suffixes_iter == seen_host_suffixes.end());
+    if (seen_host_suffixes_iter != seen_host_suffixes.end()) {
+      DLOG(WARNING) << "Received config with duplicate key";
+      continue;
+    }
+    seen_host_suffixes.insert(hint_key);
+
+    if (!hint.page_hints().empty()) {
+      // Now that processing is finished on |hint|, move it into the update
+      // data.
+      // WARNING: Do not use |hint| after this call. Its contents will no
+      // longer be valid.
+      hint_update_data->MoveHintIntoUpdateData(std::move(hint));
+      did_process_hints = true;
+    }
+  }
+
+  return did_process_hints;
+}
+
 }  // namespace optimization_guide

components/optimization_guide/hints_processing_util.h

@@ -7,13 +7,12 @@
 
 #include <string>
 
+#include "components/optimization_guide/proto/hints.pb.h"
+
 class GURL;
+
 namespace optimization_guide {
-namespace proto {
-class Hint;
-class Optimization;
-class PageHint;
-}  // namespace proto
+class HintUpdateData;
 
 // Returns whether |optimization| is disabled subject to it being part of
 // an optimization hint experiment. |optimization| could be disabled either
@@ -37,6 +36,13 @@ const proto::PageHint* FindPageHintForURL(const GURL& gurl,
 // practically zero.
 std::string HashHostForDictionary(const std::string& host);
 
+// Verifies and processes |hints| and places the ones it supports into
+// |hint_update_data|.
+//
+// Returns true if there was at least one hint moved into |hint_update_data|.
+bool ProcessHints(google::protobuf::RepeatedPtrField<proto::Hint>* hints,
+                  HintUpdateData* hint_update_data);
+
 }  // namespace optimization_guide
 
 #endif  // COMPONENTS_OPTIMIZATION_GUIDE_HINTS_PROCESSING_UTIL_H_

components/optimization_guide/hints_processing_util_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "components/optimization_guide/hints_processing_util.h"
 
+#include "components/optimization_guide/hint_update_data.h"
+#include "components/optimization_guide/proto/hint_cache.pb.h"
 #include "components/optimization_guide/proto/hints.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -62,4 +64,61 @@ TEST_F(HintsProcessingUtilTest, FindPageHintForSubstringPagePattern) {
                             GURL("https://www.foo.org/bar/three.jpg"), &hint1));
 }
 
+TEST_F(HintsProcessingUtilTest, ProcessHintsNoUpdateData) {
+  proto::Hint hint;
+  hint.set_key("whatever.com");
+  hint.set_key_representation(proto::HOST_SUFFIX);
+  proto::PageHint* page_hint = hint.add_page_hints();
+  page_hint->set_page_pattern("foo.org/*/one/");
+
+  google::protobuf::RepeatedPtrField<proto::Hint> hints;
+  *(hints.Add()) = hint;
+
+  EXPECT_FALSE(ProcessHints(&hints, nullptr));
+}
+
+TEST_F(HintsProcessingUtilTest, ProcessHintsWithNoPageHintsAndUpdateData) {
+  proto::Hint hint;
+  hint.set_key("whatever.com");
+  hint.set_key_representation(proto::HOST_SUFFIX);
+
+  google::protobuf::RepeatedPtrField<proto::Hint> hints;
+  *(hints.Add()) = hint;
+
+  std::unique_ptr<HintUpdateData> update_data =
+      HintUpdateData::CreateComponentHintUpdateData(base::Version("1.0.0"));
+  EXPECT_FALSE(ProcessHints(&hints, update_data.get()));
+  // Verify there is 1 store entries: 1 for the metadata entry.
+  EXPECT_EQ(1ul, update_data->TakeUpdateEntries()->size());
+}
+
+TEST_F(HintsProcessingUtilTest, ProcessHintsWithPageHintsAndUpdateData) {
+  google::protobuf::RepeatedPtrField<proto::Hint> hints;
+
+  proto::Hint hint;
+  hint.set_key("foo.org");
+  hint.set_key_representation(proto::HOST_SUFFIX);
+  proto::PageHint* page_hint = hint.add_page_hints();
+  page_hint->set_page_pattern("foo.org/*/one/");
+  *(hints.Add()) = hint;
+
+  proto::Hint no_host_suffix_hint;
+  no_host_suffix_hint.set_key("foo2.org");
+  proto::PageHint* page_hint3 = no_host_suffix_hint.add_page_hints();
+  page_hint3->set_page_pattern("foo2.org/blahh");
+  *(hints.Add()) = no_host_suffix_hint;
+
+  proto::Hint no_page_hints_hint;
+  no_page_hints_hint.set_key("whatever.com");
+  no_page_hints_hint.set_key_representation(proto::HOST_SUFFIX);
+  *(hints.Add()) = no_page_hints_hint;
+
+  std::unique_ptr<HintUpdateData> update_data =
+      HintUpdateData::CreateComponentHintUpdateData(base::Version("1.0.0"));
+  EXPECT_TRUE(ProcessHints(&hints, update_data.get()));
+  // Verify there are 2 store entries: 1 for the metadata entry plus
+  // the 1 added hint entries.
+  EXPECT_EQ(2ul, update_data->TakeUpdateEntries()->size());
+}
+
 }  // namespace optimization_guide

components/previews/content/previews_hints.cc

@@ -174,63 +174,6 @@ net::EffectiveConnectionType ConvertProtoEffectiveConnectionType(
   }
 }
 
-PreviewsProcessHintsResult ProcessConfigurationHints(
-    optimization_guide::proto::Configuration* config,
-    optimization_guide::HintUpdateData* component_update_data) {
-  DCHECK(config);
-  // If there's no component update data, then there's nothing to do. This
-  // component is not newer than the one contained within the hint cache.
-  if (!component_update_data) {
-    return PreviewsProcessHintsResult::kSkippedProcessingPreviewsHints;
-  }
-
-  std::unordered_set<std::string> seen_host_suffixes;
-
-  size_t total_processed_hints_with_page_hints = 0;
-
-  // Process each hint in the the hint configuration. The hints are mutable
-  // because once processing is completed on each individual hint, it is moved
-  // into the component update data. This eliminates the need to make any
-  // additional copies of the hints.
-  for (auto& hint : *(config->mutable_hints())) {
-    // We only support host suffixes at the moment. Skip anything else.
-    // One |hint| applies to one host URL suffix.
-    if (hint.key_representation() != optimization_guide::proto::HOST_SUFFIX) {
-      continue;
-    }
-
-    const std::string& hint_key = hint.key();
-
-    // Validate configuration keys.
-    DCHECK(!hint_key.empty());
-    if (hint_key.empty()) {
-      continue;
-    }
-
-    auto seen_host_suffixes_iter = seen_host_suffixes.find(hint_key);
-    DCHECK(seen_host_suffixes_iter == seen_host_suffixes.end());
-    if (seen_host_suffixes_iter != seen_host_suffixes.end()) {
-      DLOG(WARNING) << "Received config with duplicate key";
-      continue;
-    }
-    seen_host_suffixes.insert(hint_key);
-
-    if (!hint.page_hints().empty()) {
-      ++total_processed_hints_with_page_hints;
-
-      // Now that processing is finished on |hint|, move it into the component
-      // data.
-      // WARNING: Do not use |hint| after this call. Its contents will no
-      // longer be valid.
-      component_update_data->MoveHintIntoUpdateData(std::move(hint));
-    }
-  }
-
-  return total_processed_hints_with_page_hints > 0
-             ? PreviewsProcessHintsResult::kProcessedPreviewsHints
-             : PreviewsProcessHintsResult::kProcessedNoPreviewsHints;
-}
-
 void RecordProcessHintsResult(PreviewsProcessHintsResult result) {
   base::UmaHistogramEnumeration("Previews.ProcessHintsResult", result);
 }
@@ -285,10 +228,18 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromHintsComponent(
 std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromHintsConfiguration(
     std::unique_ptr<optimization_guide::proto::Configuration> config,
     std::unique_ptr<optimization_guide::HintUpdateData> component_update_data) {
-  // Process the hints within the configuration. This will move the hints from
-  // |config| into |component_update_data|.
   PreviewsProcessHintsResult process_hints_result =
-      ProcessConfigurationHints(config.get(), component_update_data.get());
+      PreviewsProcessHintsResult::kSkippedProcessingPreviewsHints;
+  if (component_update_data) {
+    // Process the hints within the configuration. This will move the hints from
+    // |config| into |component_update_data|.
+    bool did_process_hints = ProcessHints(config.get()->mutable_hints(),
+                                          component_update_data.get());
+    process_hints_result =
+        did_process_hints
+            ? PreviewsProcessHintsResult::kProcessedPreviewsHints
+            : PreviewsProcessHintsResult::kProcessedNoPreviewsHints;
+  }
 
   // Construct the PrevewsHints object with |component_update_data|, which
   // will later be used to update the HintCache's component data during