Separate previews hints into a separate file

The hints are expected to get a bit more complicated, so
move them to a separate file instead of an inner class.

There is no loss in test coverage, but I am planning to
add separate unittests file in a subsequent CL.

Change-Id: I8c94ffc93de2e4b0e7e88c9c50344f499ab03cf3
Bug: 856243
Reviewed-on: https://chromium-review.googlesource.com/1116518Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570845}

components/optimization_guide/optimization_guide_service_observer.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_OPTIMIZATION_GUIDE_OPTIMIZATION_GUIDE_SERVICE_OBSERVER_H_
 #define COMPONENTS_OPTIMIZATION_GUIDE_OPTIMIZATION_GUIDE_SERVICE_OBSERVER_H_
 
+#include "base/version.h"
 #include "components/optimization_guide/proto/hints.pb.h"
 
 namespace optimization_guide {

components/previews/content/BUILD.gn

@@ -6,6 +6,8 @@ static_library("content") {
   sources = [
     "previews_content_util.cc",
     "previews_content_util.h",
+    "previews_hints.cc",
+    "previews_hints.h",
     "previews_io_data.cc",
     "previews_io_data.h",
     "previews_optimization_guide.cc",

components/previews/content/previews_hints.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 <memory>
+#include <string>
+
+#include "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "components/optimization_guide/optimization_guide_service_observer.h"
+
+namespace previews {
+
+namespace {
+
+// Name of sentinel file to guard potential crash loops while processing
+// the config into hints. It holds the version of the config that is/was
+// being processed into hints.
+const base::FilePath::CharType kSentinelFileName[] =
+    FILE_PATH_LITERAL("previews_config_sentinel.txt");
+
+// Creates the sentinel file (at |sentinel_path|) to persistently mark the
+// beginning of processing the configuration data for Previews hints. It
+// records the configuration version in the file. Returns true when the
+// sentinel file is successfully created and processing should continue.
+// Returns false if the processing should not continue because the
+// file exists with the same version (indicating that processing that version
+// failed previously (possibly crash or shutdown). Should be run in the
+// background (e.g., same task as Hints.CreateFromConfig).
+bool CreateSentinelFile(const base::FilePath& sentinel_path,
+                        const base::Version& version) {
+  DCHECK(version.IsValid());
+
+  if (base::PathExists(sentinel_path)) {
+    // Processing apparently did not complete previously, check its version.
+    std::string content;
+    if (!base::ReadFileToString(sentinel_path, &content)) {
+      DLOG(WARNING) << "Error reading previews config sentinel file";
+      // Attempt to delete sentinel for fresh start next time.
+      base::DeleteFile(sentinel_path, false /* recursive */);
+      return false;
+    }
+    base::Version previous_attempted_version(content);
+    if (!previous_attempted_version.IsValid()) {
+      DLOG(ERROR) << "Bad contents in previews config sentinel file";
+      // Attempt to delete sentinel for fresh start next time.
+      base::DeleteFile(sentinel_path, false /* recursive */);
+      return false;
+    }
+    if (previous_attempted_version.CompareTo(version) == 0) {
+      // Previously attempted same version without completion.
+      return false;
+    }
+  }
+
+  // Write config version in the sentinel file.
+  std::string new_sentinel_value = version.GetString();
+  if (base::WriteFile(sentinel_path, new_sentinel_value.data(),
+                      new_sentinel_value.length()) <= 0) {
+    DLOG(ERROR) << "Failed to create sentinel file " << sentinel_path;
+    return false;
+  }
+  return true;
+}
+
+// Enumerates the possible outcomes of processing previews hints. Used in UMA
+// histograms, so the order of enumerators should not be changed.
+//
+// Keep in sync with PreviewsProcessHintsResult in
+// tools/metrics/histograms/enums.xml.
+enum class PreviewsProcessHintsResult {
+  PROCESSED_NO_PREVIEWS_HINTS = 0,
+  PROCESSED_PREVIEWS_HINTS = 1,
+  FAILED_FINISH_PROCESSING = 2,
+
+  // Insert new values before this line.
+  MAX,
+};
+
+void RecordProcessHintsResult(PreviewsProcessHintsResult result) {
+  UMA_HISTOGRAM_ENUMERATION("Previews.ProcessHintsResult",
+                            static_cast<int>(result),
+                            static_cast<int>(PreviewsProcessHintsResult::MAX));
+}
+
+// Deletes the sentinel file. This should be done once processing the
+// configuration is complete and should be done in the background (e.g.,
+// same task as Hints.CreateFromConfig).
+void DeleteSentinelFile(const base::FilePath& sentinel_path) {
+  if (!base::DeleteFile(sentinel_path, false /* recursive */))
+    DLOG(ERROR) << "Error deleting sentinel file";
+}
+
+}  // namespace
+
+PreviewsHints::PreviewsHints() {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
+
+PreviewsHints::~PreviewsHints() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
+// static
+std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromConfig(
+    const optimization_guide::proto::Configuration& config,
+    const optimization_guide::ComponentInfo& info) {
+  base::FilePath sentinel_path(
+      info.hints_path.DirName().Append(kSentinelFileName));
+  if (!CreateSentinelFile(sentinel_path, info.hints_version)) {
+    std::unique_ptr<PreviewsHints> no_hints;
+    RecordProcessHintsResult(
+        PreviewsProcessHintsResult::FAILED_FINISH_PROCESSING);
+    return no_hints;
+  }
+
+  std::unique_ptr<PreviewsHints> hints(new PreviewsHints());
+
+  // The condition set ID is a simple increasing counter that matches the
+  // order of hints in the config (where earlier hints in the config take
+  // precendence over later hints in the config if there are multiple matches).
+  url_matcher::URLMatcherConditionSet::ID id = 0;
+  url_matcher::URLMatcherConditionFactory* condition_factory =
+      hints->url_matcher_.condition_factory();
+  url_matcher::URLMatcherConditionSet::Vector all_conditions;
+  std::set<std::string> seen_host_suffixes;
+
+  // Process hint configuration.
+  for (const auto hint : config.hints()) {
+    // We only support host suffixes at the moment. Skip anything else.
+    if (hint.key_representation() != optimization_guide::proto::HOST_SUFFIX)
+      continue;
+
+    // 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());
+
+    // Create whitelist condition set out of the optimizations that are
+    // whitelisted for the host suffix.
+    std::set<std::pair<PreviewsType, int>> whitelisted_optimizations;
+    for (const auto optimization : hint.whitelisted_optimizations()) {
+      if (optimization.optimization_type() ==
+          optimization_guide::proto::NOSCRIPT) {
+        whitelisted_optimizations.insert(std::make_pair(
+            PreviewsType::NOSCRIPT, 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++;
+  }
+  hints->url_matcher_.AddConditionSets(all_conditions);
+  // Completed processing hints data without crashing so clear sentinel.
+  DeleteSentinelFile(sentinel_path);
+  RecordProcessHintsResult(
+      all_conditions.empty()
+          ? PreviewsProcessHintsResult::PROCESSED_NO_PREVIEWS_HINTS
+          : PreviewsProcessHintsResult::PROCESSED_PREVIEWS_HINTS);
+  return hints;
+}
+
+bool PreviewsHints::IsWhitelisted(const GURL& url,
+                                  PreviewsType type,
+                                  int* out_inflation_percent) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  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.
+  const auto& first_match = matches.begin();
+  if (first_match == matches.end()) {
+    return false;
+  }
+
+  const auto whitelist_iter = whitelist_.find(*first_match);
+  if (whitelist_iter == whitelist_.end()) {
+    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;
+      return true;
+    }
+  }
+  return false;
+}
+
+}  // namespace previews
\ No newline at end of file

components/previews/content/previews_hints.h

+// 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.
+
+#ifndef COMPONENTS_PREVIEWS_CONTENT_PREVIEWS_HINTS_H_
+#define COMPONENTS_PREVIEWS_CONTENT_PREVIEWS_HINTS_H_
+
+#include <map>
+#include <set>
+
+#include "base/macros.h"
+#include "base/sequence_checker.h"
+#include "components/optimization_guide/proto/hints.pb.h"
+#include "components/previews/content/previews_hints.h"
+#include "components/previews/core/previews_user_data.h"
+#include "components/url_matcher/url_matcher.h"
+
+class GURL;
+
+namespace optimization_guide {
+struct ComponentInfo;
+}
+
+namespace previews {
+
+// Holds previews hints extracted from the configuration.
+class PreviewsHints {
+ public:
+  ~PreviewsHints();
+
+  // Creates a Hints instance from the provided configuration.
+  static std::unique_ptr<PreviewsHints> CreateFromConfig(
+      const optimization_guide::proto::Configuration& config,
+      const optimization_guide::ComponentInfo& info);
+
+  // Whether the URL is whitelisted for the given previews type. If so,
+  // |out_inflation_percent| will be populated if meta data available for it.
+  bool IsWhitelisted(const GURL& url,
+                     PreviewsType type,
+                     int* out_inflation_percent);
+
+ private:
+  PreviewsHints();
+
+  // The URLMatcher used to match whether a URL has any hints associated with
+  // it.
+  url_matcher::URLMatcher url_matcher_;
+
+  // A map from the condition set ID to associated whitelist Optimization
+  // details.
+  std::map<url_matcher::URLMatcherConditionSet::ID,
+           std::set<std::pair<PreviewsType, int>>>
+      whitelist_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(PreviewsHints);
+};
+
+}  // namespace previews
+
+#endif  // COMPONENTS_PREVIEWS_CONTENT_PREVIEWS_HINTS_H_
\ No newline at end of file

components/previews/content/previews_optimization_guide.cc

@@ -5,237 +5,17 @@
 #include "components/previews/content/previews_optimization_guide.h"
 
 #include "base/bind.h"
-#include "base/files/file.h"
-#include "base/files/file_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/task_runner_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "components/optimization_guide/proto/hints.pb.h"
+#include "components/previews/content/previews_hints.h"
 #include "components/previews/core/previews_user_data.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
 
 namespace previews {
 
-namespace {
-
-// Enumerates the possible outcomes of processing previews hints. Used in UMA
-// histograms, so the order of enumerators should not be changed.
-//
-// Keep in sync with PreviewsProcessHintsResult in
-// tools/metrics/histograms/enums.xml.
-enum class PreviewsProcessHintsResult {
-  PROCESSED_NO_PREVIEWS_HINTS = 0,
-  PROCESSED_PREVIEWS_HINTS = 1,
-  FAILED_FINISH_PROCESSING = 2,
-
-  // Insert new values before this line.
-  MAX,
-};
-
-void RecordProcessHintsResult(PreviewsProcessHintsResult result) {
-  UMA_HISTOGRAM_ENUMERATION("Previews.ProcessHintsResult",
-                            static_cast<int>(result),
-                            static_cast<int>(PreviewsProcessHintsResult::MAX));
-}
-
-// Name of sentinel file to guard potential crash loops while processing
-// the config into hints. It holds the version of the config that is/was
-// being processed into hints.
-const base::FilePath::CharType kSentinelFileName[] =
-    FILE_PATH_LITERAL("previews_config_sentinel.txt");
-
-// Creates the sentinel file (at |sentinel_path|) to persistently mark the
-// beginning of processing the configuration data for Previews hints. It
-// records the configuration version in the file. Returns true when the
-// sentinel file is successfully created and processing should continue.
-// Returns false if the processing should not continue because the
-// file exists with the same version (indicating that processing that version
-// failed previously (possibly crash or shutdown). Should be run in the
-// background (e.g., same task as Hints.CreateFromConfig).
-bool CreateSentinelFile(const base::FilePath& sentinel_path,
-                        const base::Version& version) {
-  DCHECK(version.IsValid());
-
-  if (base::PathExists(sentinel_path)) {
-    // Processing apparently did not complete previously, check its version.
-    std::string content;
-    if (!base::ReadFileToString(sentinel_path, &content)) {
-      DLOG(WARNING) << "Error reading previews config sentinel file";
-      // Attempt to delete sentinel for fresh start next time.
-      base::DeleteFile(sentinel_path, false /* recursive */);
-      return false;
-    }
-    base::Version previous_attempted_version(content);
-    if (!previous_attempted_version.IsValid()) {
-      DLOG(ERROR) << "Bad contents in previews config sentinel file";
-      // Attempt to delete sentinel for fresh start next time.
-      base::DeleteFile(sentinel_path, false /* recursive */);
-      return false;
-    }
-    if (previous_attempted_version.CompareTo(version) == 0) {
-      // Previously attempted same version without completion.
-      return false;
-    }
-  }
-
-  // Write config version in the sentinel file.
-  std::string new_sentinel_value = version.GetString();
-  if (base::WriteFile(sentinel_path, new_sentinel_value.data(),
-                      new_sentinel_value.length()) <= 0) {
-    DLOG(ERROR) << "Failed to create sentinel file " << sentinel_path;
-    return false;
-  }
-  return true;
-}
-
-// Deletes the sentinel file. This should be done once processing the
-// configuration is complete and should be done in the background (e.g.,
-// same task as Hints.CreateFromConfig).
-void DeleteSentinelFile(const base::FilePath& sentinel_path) {
-  if (!base::DeleteFile(sentinel_path, false /* recursive */))
-    DLOG(ERROR) << "Error deleting sentinel file";
-}
-
-}  // namespace
-
-// Holds previews hints extracted from the configuration sent by the
-// Optimization Guide Service.
-class PreviewsOptimizationGuide::Hints {
- public:
-  ~Hints();
-
-  // Creates a Hints instance from the provided configuration.
-  static std::unique_ptr<Hints> CreateFromConfig(
-      const optimization_guide::proto::Configuration& config,
-      const optimization_guide::ComponentInfo& info);
-
-  // Whether the URL is whitelisted for the given previews type. If so,
-  // |out_inflation_percent| will be populated if meta data available for it.
-  bool IsWhitelisted(const GURL& url,
-                     PreviewsType type,
-                     int* out_inflation_percent);
-
- private:
-  Hints();
-
-  // The URLMatcher used to match whether a URL has any hints associated with
-  // it.
-  url_matcher::URLMatcher url_matcher_;
-
-  // A map from the condition set ID to associated whitelist Optimization
-  // details.
-  std::map<url_matcher::URLMatcherConditionSet::ID,
-           std::set<std::pair<PreviewsType, int>>>
-      whitelist_;
-};
-
-PreviewsOptimizationGuide::Hints::Hints() {}
-
-PreviewsOptimizationGuide::Hints::~Hints() {}
-
-// static
-std::unique_ptr<PreviewsOptimizationGuide::Hints>
-PreviewsOptimizationGuide::Hints::CreateFromConfig(
-    const optimization_guide::proto::Configuration& config,
-    const optimization_guide::ComponentInfo& info) {
-  base::FilePath sentinel_path(
-      info.hints_path.DirName().Append(kSentinelFileName));
-  if (!CreateSentinelFile(sentinel_path, info.hints_version)) {
-    std::unique_ptr<Hints> no_hints;
-    RecordProcessHintsResult(
-        PreviewsProcessHintsResult::FAILED_FINISH_PROCESSING);
-    return no_hints;
-  }
-
-  std::unique_ptr<Hints> hints(new Hints());
-
-  // The condition set ID is a simple increasing counter that matches the
-  // order of hints in the config (where earlier hints in the config take
-  // precendence over later hints in the config if there are multiple matches).
-  url_matcher::URLMatcherConditionSet::ID id = 0;
-  url_matcher::URLMatcherConditionFactory* condition_factory =
-      hints->url_matcher_.condition_factory();
-  url_matcher::URLMatcherConditionSet::Vector all_conditions;
-  std::set<std::string> seen_host_suffixes;
-
-  // Process hint configuration.
-  for (const auto hint : config.hints()) {
-    // We only support host suffixes at the moment. Skip anything else.
-    if (hint.key_representation() != optimization_guide::proto::HOST_SUFFIX)
-      continue;
-
-    // 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());
-
-    // Create whitelist condition set out of the optimizations that are
-    // whitelisted for the host suffix.
-    std::set<std::pair<PreviewsType, int>> whitelisted_optimizations;
-    for (const auto optimization : hint.whitelisted_optimizations()) {
-      if (optimization.optimization_type() ==
-          optimization_guide::proto::NOSCRIPT) {
-        whitelisted_optimizations.insert(std::make_pair(
-            PreviewsType::NOSCRIPT, 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++;
-  }
-  hints->url_matcher_.AddConditionSets(all_conditions);
-  // Completed processing hints data without crashing so clear sentinel.
-  DeleteSentinelFile(sentinel_path);
-  RecordProcessHintsResult(
-      all_conditions.empty()
-          ? PreviewsProcessHintsResult::PROCESSED_NO_PREVIEWS_HINTS
-          : PreviewsProcessHintsResult::PROCESSED_PREVIEWS_HINTS);
-  return hints;
-}
-
-bool PreviewsOptimizationGuide::Hints::IsWhitelisted(
-    const GURL& url,
-    PreviewsType type,
-    int* out_inflation_percent) {
-  std::set<url_matcher::URLMatcherConditionSet::ID> matches =
-      url_matcher_.MatchURL(url);
-
-  // Only consider the first match in iteration order as it takes precendence
-  // if there are multiple matches.
-  const auto& first_match = matches.begin();
-  if (first_match == matches.end()) {
-    return false;
-  }
-
-  const auto whitelist_iter = whitelist_.find(*first_match);
-  if (whitelist_iter == whitelist_.end()) {
-    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;
-      return true;
-    }
-  }
-  return false;
-}
-
 PreviewsOptimizationGuide::PreviewsOptimizationGuide(
     optimization_guide::OptimizationGuideService* optimization_guide_service,
     const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
@@ -276,13 +56,13 @@ void PreviewsOptimizationGuide::OnHintsProcessed(
 
   base::PostTaskAndReplyWithResult(
       background_task_runner_.get(), FROM_HERE,
-      base::BindOnce(&PreviewsOptimizationGuide::Hints::CreateFromConfig,
-                     config, info),
+      base::BindOnce(&PreviewsHints::CreateFromConfig, config, info),
       base::BindOnce(&PreviewsOptimizationGuide::UpdateHints,
                      io_weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PreviewsOptimizationGuide::UpdateHints(std::unique_ptr<Hints> hints) {
+void PreviewsOptimizationGuide::UpdateHints(
+    std::unique_ptr<PreviewsHints> hints) {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
   hints_ = std::move(hints);
 }

components/previews/content/previews_optimization_guide.h

@@ -5,20 +5,15 @@
 #ifndef COMPONENTS_PREVIEWS_CONTENT_PREVIEWS_OPTIMIZATION_GUIDE_H_
 #define COMPONENTS_PREVIEWS_CONTENT_PREVIEWS_OPTIMIZATION_GUIDE_H_
 
-#include <map>
-#include <set>
-
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "components/optimization_guide/optimization_guide_service.h"
 #include "components/optimization_guide/optimization_guide_service_observer.h"
 #include "components/previews/content/previews_optimization_guide.h"
 #include "components/previews/core/previews_experiments.h"
-#include "components/url_matcher/url_matcher.h"
 
 namespace net {
 class URLRequest;
@@ -32,6 +27,8 @@ class Configuration;
 
 namespace previews {
 
+class PreviewsHints;
+
 // A Previews optimization guide that makes decisions guided by hints received
 // from the OptimizationGuideService.
 class PreviewsOptimizationGuide
@@ -55,10 +52,8 @@ class PreviewsOptimizationGuide
       const optimization_guide::ComponentInfo& component_info) override;
 
  private:
-  class Hints;
-
   // Updates the hints to the latest hints sent by the Component Updater.
-  void UpdateHints(std::unique_ptr<Hints> hints);
+  void UpdateHints(std::unique_ptr<PreviewsHints> hints);
 
   // The OptimizationGuideService that this guide is listening to. Not owned.
   optimization_guide::OptimizationGuideService* optimization_guide_service_;
@@ -70,7 +65,7 @@ class PreviewsOptimizationGuide
   scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
 
   // The current hints used for this optimization guide.
-  std::unique_ptr<Hints> hints_;
+  std::unique_ptr<PreviewsHints> hints_;
 
   // Used to get |weak_ptr_| to self on the IO thread.
   base::WeakPtrFactory<PreviewsOptimizationGuide> io_weak_ptr_factory_;