Separate out ShouldTargetNavigation and CanApplyOptimization methods

This also leaves a ShouldTargetNavigationAndCanApplyOptimization that
replaced the existing CanApplyOptimization that did both. This is
required for supporting the dev incentives project since they do not
have an optimization target.

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

chrome/browser/optimization_guide/optimization_guide_keyed_service.h

@@ -78,7 +78,16 @@ class OptimizationGuideKeyedService
           optimization_types,
       const std::vector<optimization_guide::proto::OptimizationTarget>&
           optimization_targets) override;
+  optimization_guide::OptimizationGuideDecision ShouldTargetNavigation(
+      content::NavigationHandle* navigation_handle,
+      optimization_guide::proto::OptimizationTarget optimization_target)
+      override;
   optimization_guide::OptimizationGuideDecision CanApplyOptimization(
+      content::NavigationHandle* navigation_handle,
+      optimization_guide::proto::OptimizationType optimization_type,
+      optimization_guide::OptimizationMetadata* optimization_metadata) override;
+  optimization_guide::OptimizationGuideDecision
+  ShouldTargetNavigationAndCanApplyOptimization(
       content::NavigationHandle* navigation_handle,
       optimization_guide::proto::OptimizationTarget optimization_target,
       optimization_guide::proto::OptimizationType optimization_type,

chrome/browser/optimization_guide/optimization_guide_keyed_service_browsertest.cc

@@ -83,22 +83,52 @@ class OptimizationGuideConsumerWebContentsObserver
     OptimizationGuideKeyedService* service =
         OptimizationGuideKeyedServiceFactory::GetForProfile(
             Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
-    last_optimization_guide_decision_ = service->CanApplyOptimization(
+    last_should_target_navigation_decision_ = service->ShouldTargetNavigation(
         navigation_handle,
-        optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD,
-        optimization_guide::proto::NOSCRIPT, /*optimization_metadata=*/nullptr);
+        optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD);
+    last_can_apply_optimization_decision_ = service->CanApplyOptimization(
+        navigation_handle, optimization_guide::proto::NOSCRIPT,
+        /*optimization_metadata=*/nullptr);
+    last_optimization_guide_decision_ =
+        service->ShouldTargetNavigationAndCanApplyOptimization(
+            navigation_handle,
+            optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD,
+            optimization_guide::proto::NOSCRIPT,
+            /*optimization_metadata=*/nullptr);
   }
 
   // Returns the last optimization guide decision that was returned by the
-  // OptimizationGuideKeyedService.
+  // OptimizationGuideKeyedService's
+  // ShouldTargetNavigationAndCanApplyOptimization() method.
   optimization_guide::OptimizationGuideDecision
   last_optimization_guide_decision() const {
     return last_optimization_guide_decision_;
   }
 
+  // Returns the last optimization guide decision that was returned by the
+  // OptimizationGuideKeyedService's ShouldTargetNavigation() method.
+  optimization_guide::OptimizationGuideDecision
+  last_should_target_navigation_decision() {
+    return last_should_target_navigation_decision_;
+  }
+
+  // Returns the last optimization guide decision that was returned by the
+  // OptimizationGuideKeyedService's CanApplyOptimization() method.
+  optimization_guide::OptimizationGuideDecision
+  last_can_apply_optimization_decision() {
+    return last_can_apply_optimization_decision_;
+  }
+
  private:
   optimization_guide::OptimizationGuideDecision
-      last_optimization_guide_decision_;
+      last_optimization_guide_decision_ =
+          optimization_guide::OptimizationGuideDecision::kUnknown;
+  optimization_guide::OptimizationGuideDecision
+      last_should_target_navigation_decision_ =
+          optimization_guide::OptimizationGuideDecision::kUnknown;
+  optimization_guide::OptimizationGuideDecision
+      last_can_apply_optimization_decision_ =
+          optimization_guide::OptimizationGuideDecision::kUnknown;
 };
 
 }  // namespace
@@ -217,8 +247,22 @@ class OptimizationGuideKeyedServiceBrowserTest
         ->ReportEffectiveConnectionTypeForTesting(effective_connection_type);
   }
 
-  // Returns the last decision seen by the consumer of the
-  // OptimizationGuideKeyedService.
+  // Returns the last decision from the CanApplyOptimization() method seen by
+  // the consumer of the OptimizationGuideKeyedService.
+  optimization_guide::OptimizationGuideDecision
+  last_should_target_navigation_decision() {
+    return consumer_->last_should_target_navigation_decision();
+  }
+
+  // Returns the last decision from the CanApplyOptimization() method seen by
+  // the consumer of the OptimizationGuideKeyedService.
+  optimization_guide::OptimizationGuideDecision
+  last_can_apply_optimization_decision() {
+    return consumer_->last_can_apply_optimization_decision();
+  }
+
+  // Returns the last decision from the ShouldTargetAndCanApplyOptimization()
+  // method seen by the consumer of the OptimizationGuideKeyedService.
   optimization_guide::OptimizationGuideDecision last_consumer_decision() {
     return consumer_->last_optimization_guide_decision();
   }
@@ -275,6 +319,10 @@ IN_PROC_BROWSER_TEST_F(
 
   histogram_tester.ExpectTotalCount("OptimizationGuide.LoadedHint.Result", 0);
   // There is a hint that matches this URL but it wasn't loaded.
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kUnknown,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kUnknown,
             last_consumer_decision());
 }
@@ -308,7 +356,11 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
   histogram_tester.ExpectUniqueSample(
       "OptimizationGuide.HintCache.PageMatch.AtCommit", true, 1);
 
-  // We had a hint but it wasn't loaded and it was painful enough.
+  // We had a hint and it was loaded and it was painful enough.
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
             last_consumer_decision());
   // Expect that the optimization guide UKM was recorded.
@@ -347,6 +399,10 @@ IN_PROC_BROWSER_TEST_F(
                                       true, 1);
 
   // We had a matching hint but the page load was not painful.
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
             last_consumer_decision());
   histogram_tester.ExpectUniqueSample(
@@ -394,6 +450,10 @@ IN_PROC_BROWSER_TEST_F(
   histogram_tester.ExpectBucketCount("OptimizationGuide.LoadedHint.Result",
                                      true, 1);
   // Hint is still applicable so we expect it to be allowed to be applied.
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
             last_consumer_decision());
   // Make sure hint cache match UMA was logged.
@@ -438,6 +498,10 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
   // attempt to load a hint but still fail.
   histogram_tester.ExpectUniqueSample("OptimizationGuide.LoadedHint.Result",
                                       false, 1);
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
             last_consumer_decision());
   histogram_tester.ExpectUniqueSample(
@@ -484,6 +548,10 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
   // There should be a hint that matches this URL.
   histogram_tester.ExpectUniqueSample("OptimizationGuide.LoadedHint.Result",
                                       true, 1);
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
             last_consumer_decision());
   histogram_tester.ExpectUniqueSample(
@@ -529,6 +597,10 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
   // There should be a hint that matches this URL.
   histogram_tester.ExpectUniqueSample("OptimizationGuide.LoadedHint.Result",
                                       true, 1);
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
             last_consumer_decision());
   histogram_tester.ExpectUniqueSample(
@@ -730,6 +802,10 @@ IN_PROC_BROWSER_TEST_F(
   // There should be a hint that matches this URL.
   histogram_tester.ExpectUniqueSample("OptimizationGuide.LoadedHint.Result",
                                       true, 1);
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
             last_consumer_decision());
   histogram_tester.ExpectUniqueSample(
@@ -780,6 +856,10 @@ IN_PROC_BROWSER_TEST_F(
   // There should be a hint that matches this URL.
   histogram_tester.ExpectUniqueSample("OptimizationGuide.LoadedHint.Result",
                                       true, 1);
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            last_should_target_navigation_decision());
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
+            last_can_apply_optimization_decision());
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
             last_consumer_decision());
   histogram_tester.ExpectUniqueSample(

components/optimization_guide/optimization_guide_decider.h

@@ -46,10 +46,23 @@ class OptimizationGuideDecider {
       const std::vector<proto::OptimizationType>& optimization_types,
       const std::vector<proto::OptimizationTarget>& optimization_targets) = 0;
 
+  // Returns whether the current conditions match |optimization_target|.
+  virtual OptimizationGuideDecision ShouldTargetNavigation(
+      content::NavigationHandle* navigation_handle,
+      proto::OptimizationTarget optimization_target) = 0;
+
+  // Returns whether |optimization_type| can be applied for the URL associated
+  // with |navigation_handle|.
+  virtual OptimizationGuideDecision CanApplyOptimization(
+      content::NavigationHandle* navigation_handle,
+      proto::OptimizationType optimization_type,
+      OptimizationMetadata* optimization_metadata) = 0;
+
   // Returns whether the current conditions match |optimization_target| and
   // |optimization_type| can be applied for the URL associated with
   // |navigation_handle|.
-  virtual OptimizationGuideDecision CanApplyOptimization(
+  virtual OptimizationGuideDecision
+  ShouldTargetNavigationAndCanApplyOptimization(
       content::NavigationHandle* navigation_handle,
       proto::OptimizationTarget optimization_target,
       proto::OptimizationType optimization_type,

components/previews/content/previews_optimization_guide_decider.cc

@@ -125,10 +125,11 @@ bool PreviewsOptimizationGuideDecider::CanApplyPreview(
   // conditions match a painful page load as a prerequisite for returning true.
   optimization_guide::OptimizationMetadata optimization_metadata;
   optimization_guide::OptimizationGuideDecision decision =
-      optimization_guide_decider_->CanApplyOptimization(
-          navigation_handle,
-          optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD,
-          *optimization_type, &optimization_metadata);
+      optimization_guide_decider_
+          ->ShouldTargetNavigationAndCanApplyOptimization(
+              navigation_handle,
+              optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD,
+              *optimization_type, &optimization_metadata);
 
   // Return false if we are even unsure if we can apply the optimization (i.e.
   // hint not loaded yet or just not applicable).
@@ -174,7 +175,6 @@ bool PreviewsOptimizationGuideDecider::MaybeLoadOptimizationHints(
 
     if (optimization_guide_decider_->CanApplyOptimization(
             navigation_handle,
-            optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD,
             optimization_type,
             /*optimization_metadata=*/nullptr) !=
         optimization_guide::OptimizationGuideDecision::kFalse) {

components/previews/content/previews_optimization_guide_decider_unittest.cc

@@ -57,7 +57,29 @@ class TestOptimizationGuideDecider
     return registered_optimization_targets_;
   }
 
+  optimization_guide::OptimizationGuideDecision ShouldTargetNavigation(
+      content::NavigationHandle* navigation_handle,
+      optimization_guide::proto::OptimizationTarget optimization_target)
+      override {
+    // Should not be called.
+    EXPECT_TRUE(false);
+    return optimization_guide::OptimizationGuideDecision::kFalse;
+  }
+
   optimization_guide::OptimizationGuideDecision CanApplyOptimization(
+      content::NavigationHandle* navigation_handle,
+      optimization_guide::proto::OptimizationType optimization_type,
+      optimization_guide::OptimizationMetadata* optimization_metadata)
+      override {
+    auto response_iter = responses_.find(
+        std::make_tuple(navigation_handle->GetURL(), optimization_type));
+    if (response_iter == responses_.end())
+      return optimization_guide::OptimizationGuideDecision::kFalse;
+    return std::get<0>(response_iter->second);
+  }
+
+  optimization_guide::OptimizationGuideDecision
+  ShouldTargetNavigationAndCanApplyOptimization(
       content::NavigationHandle* navigation_handle,
       optimization_guide::proto::OptimizationTarget optimization_target,
       optimization_guide::proto::OptimizationType optimization_type,