Separate out max ECT trigger from hint in hints manager CanApply logic

This makes it easier to refactor CanApply for the URL-keyed hints.
The new ShouldTargetNavigation in hints manager is temporary and will
be removed once prediction manager is fully live.

Bug: 1036490
Change-Id: I5855abc507ff1f12f9c392d234b9bfb71d9acb8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986250
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728276}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -728,55 +728,64 @@ bool OptimizationGuideHintsManager::HasLoadedOptimizationFilter(
          blacklist_optimization_filters_.end();
 }
 
-void OptimizationGuideHintsManager::CanApplyOptimization(
+optimization_guide::OptimizationTargetDecision
+OptimizationGuideHintsManager::ShouldTargetNavigation(
+    content::NavigationHandle* navigation_handle,
+    optimization_guide::proto::OptimizationTarget optimization_target) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (optimization_target !=
+      optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD) {
+    return optimization_guide::OptimizationTargetDecision::
+        kModelNotAvailableOnClient;
+  }
+
+  net::EffectiveConnectionType max_ect_trigger =
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G;
+
+  const auto& url = navigation_handle->GetURL();
+  if (url.has_host()) {
+    const auto& host = url.host();
+    // Check if we have a hint already loaded for this navigation.
+    const optimization_guide::proto::Hint* loaded_hint =
+        hint_cache_->GetHintIfLoaded(host);
+    const optimization_guide::proto::PageHint* matched_page_hint =
+        loaded_hint ? GetPageHintForNavigation(navigation_handle, loaded_hint)
+                    : nullptr;
+
+    if (matched_page_hint && matched_page_hint->has_max_ect_trigger()) {
+      max_ect_trigger = optimization_guide::ConvertProtoEffectiveConnectionType(
+          matched_page_hint->max_ect_trigger());
+    }
+  }
+
+  if (current_effective_connection_type_ !=
+          net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_UNKNOWN &&
+      current_effective_connection_type_ <= max_ect_trigger) {
+    return optimization_guide::OptimizationTargetDecision::kPageLoadMatches;
+  }
+
+  return optimization_guide::OptimizationTargetDecision::kPageLoadDoesNotMatch;
+}
+
+optimization_guide::OptimizationTypeDecision
+OptimizationGuideHintsManager::CanApplyOptimization(
     content::NavigationHandle* navigation_handle,
-    optimization_guide::proto::OptimizationTarget optimization_target,
     optimization_guide::proto::OptimizationType optimization_type,
-    optimization_guide::OptimizationTargetDecision*
-        optimization_target_decision,
-    optimization_guide::OptimizationTypeDecision* optimization_type_decision,
     optimization_guide::OptimizationMetadata* optimization_metadata) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(optimization_target_decision);
-  DCHECK(optimization_type_decision);
 
   // Clear out optimization metadata if provided.
   if (optimization_metadata)
     (*optimization_metadata).previews_metadata.Clear();
 
-  *optimization_target_decision =
-      optimization_guide::OptimizationTargetDecision::kUnknown;
-  *optimization_type_decision =
-      optimization_guide::OptimizationTypeDecision::kUnknown;
-
-  bool should_update_optimization_target_decision = true;
-  if (optimization_target !=
-      optimization_guide::proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD) {
-    *optimization_target_decision = optimization_guide::
-        OptimizationTargetDecision::kModelNotAvailableOnClient;
-    should_update_optimization_target_decision = false;
-  }
-
   const auto& url = navigation_handle->GetURL();
   // If the URL doesn't have a host, we cannot query the hint for it, so just
   // return early.
-  if (!url.has_host()) {
-    if (should_update_optimization_target_decision) {
-      *optimization_target_decision =
-          optimization_guide::OptimizationTargetDecision::kPageLoadDoesNotMatch;
-    }
-    *optimization_type_decision =
-        optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
-    return;
-  }
+  if (!url.has_host())
+    return optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
   const auto& host = url.host();
 
-  net::EffectiveConnectionType max_ect_trigger =
-      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G;
-
-  // TODO(sophiechang): Maybe cache the page hint for a navigation ID so we
-  // don't have to iterate through all page hints every time this is called.
-
   // Check if we have a hint already loaded for this navigation.
   const optimization_guide::proto::Hint* loaded_hint =
       hint_cache_->GetHintIfLoaded(host);
@@ -792,27 +801,9 @@ void OptimizationGuideHintsManager::CanApplyOptimization(
   if (navigation_data) {
     navigation_data->set_has_hint_after_commit(has_hint_in_cache);
 
-    if (loaded_hint)
+    if (loaded_hint) {
       navigation_data->set_serialized_hint_version_string(
           loaded_hint->version());
-  }
-
-  if (matched_page_hint && matched_page_hint->has_max_ect_trigger()) {
-    max_ect_trigger = optimization_guide::ConvertProtoEffectiveConnectionType(
-        matched_page_hint->max_ect_trigger());
-  }
-
-  if (should_update_optimization_target_decision) {
-    if (current_effective_connection_type_ ==
-            net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_UNKNOWN ||
-        current_effective_connection_type_ > max_ect_trigger) {
-      // The current network is not slow enough, so this navigation is likely
-      // not going to be painful.
-      *optimization_target_decision =
-          optimization_guide::OptimizationTargetDecision::kPageLoadDoesNotMatch;
-    } else {
-      *optimization_target_decision =
-          optimization_guide::OptimizationTargetDecision::kPageLoadMatches;
     }
   }
 
@@ -825,22 +816,19 @@ void OptimizationGuideHintsManager::CanApplyOptimization(
     // if the URL matches anything in the filter.
     if (blacklist_optimization_filters_.find(optimization_type) !=
         blacklist_optimization_filters_.end()) {
-      *optimization_type_decision =
-          blacklist_optimization_filters_[optimization_type]->Matches(url)
-              ? optimization_guide::OptimizationTypeDecision::
-                    kNotAllowedByOptimizationFilter
-              : optimization_guide::OptimizationTypeDecision::
-                    kAllowedByOptimizationFilter;
-      return;
+      return blacklist_optimization_filters_[optimization_type]->Matches(url)
+                 ? optimization_guide::OptimizationTypeDecision::
+                       kNotAllowedByOptimizationFilter
+                 : optimization_guide::OptimizationTypeDecision::
+                       kAllowedByOptimizationFilter;
     }
 
     // Check if we had an optimization filter for it, but it was not loaded into
     // memory.
     if (optimization_types_with_filter_.find(optimization_type) !=
         optimization_types_with_filter_.end()) {
-      *optimization_type_decision = optimization_guide::
-          OptimizationTypeDecision::kHadOptimizationFilterButNotLoadedInTime;
-      return;
+      return optimization_guide::OptimizationTypeDecision::
+          kHadOptimizationFilterButNotLoadedInTime;
     }
   }
 
@@ -849,21 +837,18 @@ void OptimizationGuideHintsManager::CanApplyOptimization(
     // cache, we do not know what to do with the URL so just return.
     // Otherwise, we do have information, but we just do not know it yet.
     if (has_hint_in_cache) {
-      *optimization_type_decision = optimization_guide::
-          OptimizationTypeDecision::kHadHintButNotLoadedInTime;
-    } else if (IsHintBeingFetched(url.host())) {
-      *optimization_type_decision = optimization_guide::
-          OptimizationTypeDecision::kHintFetchStartedButNotAvailableInTime;
-    } else {
-      *optimization_type_decision =
-          optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
+      return optimization_guide::OptimizationTypeDecision::
+          kHadHintButNotLoadedInTime;
     }
-    return;
+    if (IsHintBeingFetched(url.host())) {
+      return optimization_guide::OptimizationTypeDecision::
+          kHintFetchStartedButNotAvailableInTime;
+    }
+    return optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
   }
+
   if (!matched_page_hint) {
-    *optimization_type_decision =
-        optimization_guide::OptimizationTypeDecision::kNoMatchingPageHint;
-    return;
+    return optimization_guide::OptimizationTypeDecision::kNoMatchingPageHint;
   }
 
   // Now check if we have any optimizations for it.
@@ -883,14 +868,11 @@ void OptimizationGuideHintsManager::CanApplyOptimization(
       (*optimization_metadata).previews_metadata =
           optimization.previews_metadata();
     }
-    *optimization_type_decision =
-        optimization_guide::OptimizationTypeDecision::kAllowedByHint;
-    return;
+    return optimization_guide::OptimizationTypeDecision::kAllowedByHint;
   }
 
   // We didn't find anything, so it's not allowed by the hint.
-  *optimization_type_decision =
-      optimization_guide::OptimizationTypeDecision::kNotAllowedByHint;
+  return optimization_guide::OptimizationTypeDecision::kNotAllowedByHint;
 }
 
 void OptimizationGuideHintsManager::OnEffectiveConnectionTypeChanged(

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -104,15 +104,18 @@ class OptimizationGuideHintsManager
   bool HasLoadedOptimizationFilter(
       optimization_guide::proto::OptimizationType optimization_type);
 
-  // Populates |optimization_target_decision| and |optimization_type_decision|
-  // for whether the page load matches the given parameters.
-  void CanApplyOptimization(
+  // Returns the OptimizationTargetDecision based on the given parameters.
+  // TODO(crbug/1021364): Remove this method once the hints have nothing to do
+  // with predicting navigations.
+  optimization_guide::OptimizationTargetDecision ShouldTargetNavigation(
+      content::NavigationHandle* navigation_handle,
+      optimization_guide::proto::OptimizationTarget optimization_target);
+
+  // Returns the OptimizationTypeDecision based on the given parameters.
+  // |optimization_metadata| will be populated, if applicable.
+  optimization_guide::OptimizationTypeDecision CanApplyOptimization(
       content::NavigationHandle* navigation_handle,
-      optimization_guide::proto::OptimizationTarget optimization_target,
       optimization_guide::proto::OptimizationType optimization_type,
-      optimization_guide::OptimizationTargetDecision*
-          optimization_target_decision,
-      optimization_guide::OptimizationTypeDecision* optimization_type_decision,
       optimization_guide::OptimizationMetadata* optimization_metadata);
 
   // Clears fetched hints from |hint_cache_|.

chrome/browser/optimization_guide/optimization_guide_keyed_service.cc

@@ -210,13 +210,8 @@ OptimizationGuideKeyedService::ShouldTargetNavigation(
         navigation_handle, optimization_target);
   } else {
     DCHECK(hints_manager_);
-    optimization_guide::OptimizationTypeDecision
-        unused_optimization_type_decision;
-    hints_manager_->CanApplyOptimization(
-        navigation_handle, optimization_target,
-        optimization_guide::proto::OPTIMIZATION_NONE,
-        &optimization_target_decision, &unused_optimization_type_decision,
-        /*optimization_metadata=*/nullptr);
+    optimization_target_decision = hints_manager_->ShouldTargetNavigation(
+        navigation_handle, optimization_target);
   }
 
   LogOptimizationTargetDecision(navigation_handle, optimization_target,
@@ -232,13 +227,9 @@ OptimizationGuideKeyedService::CanApplyOptimization(
     optimization_guide::OptimizationMetadata* optimization_metadata) {
   DCHECK(hints_manager_);
 
-  optimization_guide::OptimizationTargetDecision
-      unused_optimization_target_decision;
-  optimization_guide::OptimizationTypeDecision optimization_type_decision;
-  hints_manager_->CanApplyOptimization(
-      navigation_handle, optimization_guide::proto::OPTIMIZATION_TARGET_UNKNOWN,
-      optimization_type, &unused_optimization_target_decision,
-      &optimization_type_decision, optimization_metadata);
+  optimization_guide::OptimizationTypeDecision optimization_type_decision =
+      hints_manager_->CanApplyOptimization(navigation_handle, optimization_type,
+                                           optimization_metadata);
 
   LogOptimizationTypeDecision(navigation_handle, optimization_type,
                               optimization_type_decision);