Updating PerformanceHintsObserver to use the new async

OptimizationGuideDecider API.

Bug: 1030813
Change-Id: I2d0e73e22ec52f1c09e218e50fe11cd737525d48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2029228Reviewed-by: Megan Jablonski <megjablon@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Josh Simmons <jds@google.com>
Cr-Commit-Position: refs/heads/master@{#736959}

chrome/browser/performance_hints/performance_hints_observer.cc

@@ -32,10 +32,13 @@ PerformanceHintsObserver::PerformanceHintsObserver(
   }
 }
 
-PerformanceHintsObserver::~PerformanceHintsObserver() = default;
+PerformanceHintsObserver::~PerformanceHintsObserver() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 base::Optional<PerformanceHint> PerformanceHintsObserver::HintForURL(
     const GURL& url) const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!url.is_valid()) {
     return base::nullopt;
   }
@@ -50,6 +53,7 @@ base::Optional<PerformanceHint> PerformanceHintsObserver::HintForURL(
 
 void PerformanceHintsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(navigation_handle);
   if (!navigation_handle->IsInMainFrame() ||
       navigation_handle->IsSameDocument() ||
@@ -70,25 +74,24 @@ void PerformanceHintsObserver::DidFinishNavigation(
     return;
   }
 
-  optimization_guide::OptimizationMetadata optimization_metadata;
-  OptimizationGuideDecision decision =
-      optimization_guide_decider_->CanApplyOptimization(
-          navigation_handle, optimization_guide::proto::PERFORMANCE_HINTS,
-          &optimization_metadata);
+  optimization_guide_decider_->CanApplyOptimizationAsync(
+      navigation_handle, optimization_guide::proto::PERFORMANCE_HINTS,
+      base::BindOnce(&PerformanceHintsObserver::ProcessPerformanceHint,
+                     weak_factory_.GetWeakPtr()));
+}
 
+void PerformanceHintsObserver::ProcessPerformanceHint(
+    OptimizationGuideDecision decision,
+    const optimization_guide::OptimizationMetadata& optimization_metadata) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (decision != OptimizationGuideDecision::kTrue) {
     // Apply results are counted under
     // OptimizationGuide.ApplyDecision.PerformanceHints.
     return;
   }
 
-  if (optimization_metadata.performance_hints_metadata.performance_hints()
-          .size() <= 0) {
-    return;
-  }
-
-  for (PerformanceHint& hint : *optimization_metadata.performance_hints_metadata
-                                    .mutable_performance_hints()) {
+  for (const PerformanceHint& hint :
+       optimization_metadata.performance_hints_metadata.performance_hints()) {
     hints_.emplace_back(URLPatternWithWildcards(hint.wildcard_pattern()), hint);
   }
 }

chrome/browser/performance_hints/performance_hints_observer.h

@@ -10,7 +10,10 @@
 
 #include "base/feature_list.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/sequence_checker.h"
+#include "components/optimization_guide/optimization_guide_decider.h"
 #include "components/optimization_guide/proto/hints.pb.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -21,7 +24,6 @@ class WebContents;
 }  // namespace content
 
 namespace optimization_guide {
-class OptimizationGuideDecider;
 class URLPatternWithWildcards;
 namespace proto {
 class PerformanceHint;
@@ -44,6 +46,13 @@ class PerformanceHintsObserver
   PerformanceHintsObserver(const PerformanceHintsObserver&) = delete;
   PerformanceHintsObserver& operator=(const PerformanceHintsObserver&) = delete;
 
+  // This callback populates |hints_| with performance information for links on
+  // the current page and is called by |optimization_guide_decider_| when a
+  // definite decision has been reached.
+  void ProcessPerformanceHint(
+      optimization_guide::OptimizationGuideDecision decision,
+      const optimization_guide::OptimizationMetadata& optimization_metadata);
+
   // Returns a PerformanceHint for a link to |url|, if one exists.
   base::Optional<optimization_guide::proto::PerformanceHint> HintForURL(
       const GURL& url) const;
@@ -66,6 +75,10 @@ class PerformanceHintsObserver
                         optimization_guide::proto::PerformanceHint>>
       hints_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  base::WeakPtrFactory<PerformanceHintsObserver> weak_factory_{this};
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };
 

chrome/browser/performance_hints/performance_hints_observer_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/feature_list.h"
+#include "base/test/gmock_callback_support.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h"
@@ -48,6 +49,10 @@ class MockOptimizationGuideKeyedService : public OptimizationGuideKeyedService {
                    content::NavigationHandle*,
                    optimization_guide::proto::OptimizationType,
                    optimization_guide::OptimizationMetadata*));
+  MOCK_METHOD3(CanApplyOptimizationAsync,
+               void(content::NavigationHandle*,
+                    optimization_guide::proto::OptimizationType,
+                    optimization_guide::OptimizationGuideDecisionCallback));
 };
 
 class PerformanceHintsObserverTest : public ChromeRenderViewHostTestHarness {
@@ -107,22 +112,20 @@ TEST_F(PerformanceHintsObserverTest, HasPerformanceHints) {
                       optimization_guide::proto::PERFORMANCE_HINTS),
                   testing::IsEmpty()));
 
-  optimization_guide::OptimizationMetadata output;
-  auto* hint = output.performance_hints_metadata.add_performance_hints();
+  optimization_guide::OptimizationMetadata metadata;
+  auto* hint = metadata.performance_hints_metadata.add_performance_hints();
   hint->set_wildcard_pattern("test.com");
   hint->set_performance_class(optimization_guide::proto::PERFORMANCE_SLOW);
-  hint = output.performance_hints_metadata.add_performance_hints();
+  hint = metadata.performance_hints_metadata.add_performance_hints();
   hint->set_wildcard_pattern("othersite.net");
   hint->set_performance_class(optimization_guide::proto::PERFORMANCE_FAST);
   EXPECT_CALL(*mock_optimization_guide_keyed_service_,
-              CanApplyOptimization(testing::_,
-                                   optimization_guide::proto::PERFORMANCE_HINTS,
-                                   testing::NotNull()))
-      .WillOnce(testing::WithArgs<2>(testing::Invoke(
-          [&output](optimization_guide::OptimizationMetadata* metadata) {
-            *metadata = output;
-            return optimization_guide::OptimizationGuideDecision::kTrue;
-          })));
+              CanApplyOptimizationAsync(
+                  testing::_, optimization_guide::proto::PERFORMANCE_HINTS,
+                  base::test::IsNotNullCallback()))
+      .WillOnce(base::test::RunOnceCallback<2>(
+          optimization_guide::OptimizationGuideDecision::kTrue,
+          testing::ByRef(metadata)));
 
   PerformanceHintsObserver::CreateForWebContents(web_contents());
   PerformanceHintsObserver* observer =
@@ -148,12 +151,31 @@ TEST_F(PerformanceHintsObserverTest, HasPerformanceHints) {
 }
 
 TEST_F(PerformanceHintsObserverTest, NoHintsForPage) {
+  optimization_guide::OptimizationMetadata metadata;
   EXPECT_CALL(*mock_optimization_guide_keyed_service_,
-              CanApplyOptimization(testing::_,
-                                   optimization_guide::proto::PERFORMANCE_HINTS,
-                                   testing::NotNull()))
-      .WillOnce(testing::Return(
-          optimization_guide::OptimizationGuideDecision::kFalse));
+              CanApplyOptimizationAsync(
+                  testing::_, optimization_guide::proto::PERFORMANCE_HINTS,
+                  base::test::IsNotNullCallback()))
+      .WillOnce(base::test::RunOnceCallback<2>(
+          optimization_guide::OptimizationGuideDecision::kFalse,
+          testing::ByRef(metadata)));
+
+  PerformanceHintsObserver::CreateForWebContents(web_contents());
+  PerformanceHintsObserver* observer =
+      PerformanceHintsObserver::FromWebContents(web_contents());
+  ASSERT_NE(observer, nullptr);
+  CallDidFinishNavigation(observer);
+
+  EXPECT_THAT(observer->HintForURL(GURL("https://www.nohint.com")),
+              Eq(base::nullopt));
+}
+
+TEST_F(PerformanceHintsObserverTest, PerformanceInfoRequestedBeforeCallback) {
+  optimization_guide::OptimizationMetadata metadata;
+  EXPECT_CALL(*mock_optimization_guide_keyed_service_,
+              CanApplyOptimizationAsync(
+                  testing::_, optimization_guide::proto::PERFORMANCE_HINTS,
+                  base::test::IsNotNullCallback()));
 
   PerformanceHintsObserver::CreateForWebContents(web_contents());
   PerformanceHintsObserver* observer =
@@ -184,7 +206,7 @@ TEST_F(PerformanceHintsObserverTest, NoErrorPageHints) {
   test_handle_->set_is_error_page(true);
 
   EXPECT_CALL(*mock_optimization_guide_keyed_service_,
-              CanApplyOptimization(testing::_, testing::_, testing::_))
+              CanApplyOptimizationAsync(testing::_, testing::_, testing::_))
       .Times(0);
 
   PerformanceHintsObserver::CreateForWebContents(web_contents());
@@ -209,7 +231,7 @@ TEST_F(PerformanceHintsObserverTest, DontFetchForSubframe) {
   test_handle_->set_is_error_page(false);
 
   EXPECT_CALL(*mock_optimization_guide_keyed_service_,
-              CanApplyOptimization(testing::_, testing::_, testing::_))
+              CanApplyOptimizationAsync(testing::_, testing::_, testing::_))
       .Times(0);
 
   PerformanceHintsObserver::CreateForWebContents(web_contents());