Fix for PerformanceClassForURL returning PERFORMANCE_FAST for all links in some situations.

This would happen when the kPerformanceHintsTreatUnknownAsFast flag was
enabled but the user opted out of Make Browsing Better.

Also fix an issue where we wouldn't reset the value of hint_processed_,
which caused us to never log kHintNotReady after the first load.

Bug: 1040661
Change-Id: I32f0b8ea475d4d23a9af9e074482b9e009980719
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078908
Commit-Queue: Josh Simmons <jds@google.com>
Reviewed-by: Megan Jablonski <megjablon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746158}

chrome/browser/performance_hints/performance_hints_observer.cc

@@ -8,6 +8,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h"
+#include "chrome/browser/optimization_guide/optimization_guide_permissions_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/optimization_guide/optimization_guide_decider.h"
 #include "components/optimization_guide/url_pattern_with_wildcards.h"
@@ -27,18 +28,6 @@ using optimization_guide::proto::PerformanceHint;
 
 namespace {
 
-// These values are logged to UMA. Entries should not be renumbered and numeric
-// values should never be reused. Please keep in sync with
-// "PerformanceHintsObserverHintForURLResult" in
-// src/tools/metrics/histograms/enums.xml.
-enum class HintForURLResult {
-  kHintNotFound = 0,
-  kHintNotReady = 1,
-  kInvalidURL = 2,
-  kHintFound = 3,
-  kMaxValue = kHintFound,
-};
-
 // These values are logged to UMA. Entries should not be renumbered and numeric
 // values should never be reused. Please keep in sync with:
 //  - "PerformanceHintsPerformanceClass" in
@@ -115,22 +104,44 @@ PerformanceClass PerformanceHintsObserver::PerformanceClassForURLInternal(
     content::WebContents* web_contents,
     const GURL& url,
     bool record_metrics) {
-  PerformanceClass performance_class = [&]() {
-    if (!url.is_valid() || web_contents == nullptr) {
-      return PerformanceClass::PERFORMANCE_UNKNOWN;
-    }
+  if (web_contents == nullptr) {
+    return PerformanceClass::PERFORMANCE_UNKNOWN;
+  }
 
-    PerformanceHintsObserver* performance_hints_observer =
-        PerformanceHintsObserver::FromWebContents(web_contents);
-    if (performance_hints_observer == nullptr) {
-      return PerformanceClass::PERFORMANCE_UNKNOWN;
-    }
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  if (!profile || !IsUserPermittedToFetchFromRemoteOptimizationGuide(profile)) {
+    // We can't get performance hints if OptimizationGuide can't fetch them.
+    return PerformanceClass::PERFORMANCE_UNKNOWN;
+  }
+
+  PerformanceHintsObserver* performance_hints_observer =
+      PerformanceHintsObserver::FromWebContents(web_contents);
+  if (performance_hints_observer == nullptr) {
+    return PerformanceClass::PERFORMANCE_UNKNOWN;
+  }
+
+  HintForURLResult hint_result;
+  base::Optional<PerformanceHint> hint;
+  std::tie(hint_result, hint) = performance_hints_observer->HintForURL(url);
+  if (record_metrics) {
+    base::UmaHistogramEnumeration("PerformanceHints.Observer.HintForURLResult",
+                                  hint_result);
+  }
 
-    base::Optional<optimization_guide::proto::PerformanceHint> hint =
-        performance_hints_observer->HintForURL(record_metrics, url);
-    return hint ? hint->performance_class()
-                : PerformanceClass::PERFORMANCE_UNKNOWN;
-  }();
+  PerformanceClass performance_class;
+  switch (hint_result) {
+    case HintForURLResult::kHintFound:
+    case HintForURLResult::kHintNotFound:
+    case HintForURLResult::kHintNotReady:
+      performance_class = hint ? hint->performance_class()
+                               : PerformanceClass::PERFORMANCE_UNKNOWN;
+      break;
+    case HintForURLResult::kInvalidURL:
+    default:
+      // Error or unknown case. Don't allow the override.
+      return PerformanceClass::PERFORMANCE_UNKNOWN;
+  }
 
   if (record_metrics) {
     // Log to UMA before the override logic so we can determine how often the
@@ -140,8 +151,10 @@ PerformanceClass PerformanceHintsObserver::PerformanceClassForURLInternal(
         ToUmaPerformanceClass(performance_class));
   }
 
-  if (base::FeatureList::IsEnabled(kPerformanceHintsTreatUnknownAsFast) &&
-      performance_class == PerformanceClass::PERFORMANCE_UNKNOWN) {
+  if (performance_class == PerformanceClass::PERFORMANCE_UNKNOWN &&
+      base::FeatureList::IsEnabled(kPerformanceHintsTreatUnknownAsFast)) {
+    // If we couldn't get the hint or we didn't expect it on this page, give it
+    // the benefit of the doubt.
     return PerformanceClass::PERFORMANCE_FAST;
   }
 
@@ -156,9 +169,9 @@ void PerformanceHintsObserver::RecordPerformanceUMAForURL(
                                  /*record_metrics=*/true);
 }
 
-base::Optional<PerformanceHint> PerformanceHintsObserver::HintForURL(
-    bool record_metrics,
-    const GURL& url) const {
+std::tuple<PerformanceHintsObserver::HintForURLResult,
+           base::Optional<PerformanceHint>>
+PerformanceHintsObserver::HintForURL(const GURL& url) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   base::Optional<PerformanceHint> hint;
@@ -178,12 +191,7 @@ base::Optional<PerformanceHint> PerformanceHintsObserver::HintForURL(
     }
   }
 
-  if (record_metrics) {
-    base::UmaHistogramEnumeration("PerformanceHints.Observer.HintForURLResult",
-                                  hint_result);
-  }
-
-  return hint;
+  return {hint_result, hint};
 }
 
 void PerformanceHintsObserver::DidFinishNavigation(
@@ -200,6 +208,7 @@ void PerformanceHintsObserver::DidFinishNavigation(
   // We've navigated to a new page, so clear out any hints from the previous
   // page.
   hints_.clear();
+  hint_processed_ = false;
 
   if (!optimization_guide_decider_) {
     return;

chrome/browser/performance_hints/performance_hints_observer.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_PERFORMANCE_HINTS_PERFORMANCE_HINTS_OBSERVER_H_
 #define CHROME_BROWSER_PERFORMANCE_HINTS_PERFORMANCE_HINTS_OBSERVER_H_
 
+#include <tuple>
 #include <utility>
 #include <vector>
 
@@ -83,10 +84,30 @@ class PerformanceHintsObserver
       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(
-      bool record_metrics,
-      const GURL& url) const;
+  // These values are logged to UMA. Entries should not be renumbered and
+  // numeric values should never be reused. Please keep in sync with
+  // "PerformanceHintsObserverHintForURLResult" in
+  // src/tools/metrics/histograms/enums.xml.
+  enum class HintForURLResult {
+    // Hints for the current page have been processed and no hint for the URL
+    // was found.
+    kHintNotFound = 0,
+    // Hints have not yet been processed. The call may be attempted again.
+    kHintNotReady = 1,
+    // An invalid URL was passed.
+    kInvalidURL = 2,
+    // A matching hint was found and has been returned.
+    kHintFound = 3,
+    kMaxValue = kHintFound,
+  };
+
+  // Fetches a PerformanceHint for the given |url|.
+  //
+  // Returns an enum describing the fetch outcome and, if that outcome is
+  // kHintFound, the matching PerformanceHint. See HintForURLResult, above.
+  std::tuple<HintForURLResult,
+             base::Optional<optimization_guide::proto::PerformanceHint>>
+  HintForURL(const GURL& url) const;
 
   // Initialized in constructor. It may be null if !IsOptimizationHintsEnabled.
   optimization_guide::OptimizationGuideDecider* optimization_guide_decider_ =

chrome/browser/performance_hints/performance_hints_observer_unittest.cc

@@ -17,6 +17,7 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/optimization_guide/optimization_guide_features.h"
+#include "components/optimization_guide/optimization_guide_switches.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/test/mock_navigation_handle.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -68,6 +69,9 @@ class PerformanceHintsObserverTest : public ChromeRenderViewHostTestHarness {
          // nullptr.
          optimization_guide::features::kOptimizationHints},
         {});
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        optimization_guide::switches::
+            kDisableCheckingUserPermissionsForTesting);
 
     ChromeRenderViewHostTestHarness::SetUp();
     content::RenderFrameHostTester::For(main_rfh())
@@ -183,6 +187,29 @@ TEST_F(PerformanceHintsObserverTest, PerformanceHintsMetadataNotPresent) {
       "PerformanceHints.Observer.HintForURLResult", /*kHintNotFound*/ 0, 1);
 }
 
+TEST_F(PerformanceHintsObserverTest, InvalidURL) {
+  optimization_guide::OptimizationMetadata metadata;
+  EXPECT_CALL(*mock_optimization_guide_keyed_service_,
+              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());
+  CallDidFinishNavigation(web_contents());
+
+  base::HistogramTester histogram_tester;
+
+  EXPECT_THAT(PerformanceHintsObserver::PerformanceClassForURL(web_contents(),
+                                                               GURL("")),
+              Eq(optimization_guide::proto::PERFORMANCE_UNKNOWN));
+
+  histogram_tester.ExpectUniqueSample(
+      "PerformanceHints.Observer.HintForURLResult", /*kInvalidUrl*/ 2, 1);
+}
+
 TEST_F(PerformanceHintsObserverTest, NoHintsForPage) {
   optimization_guide::OptimizationMetadata metadata;
   metadata.set_performance_hints_metadata(
@@ -339,3 +366,86 @@ TEST_F(PerformanceHintsObserverTest, OverrideUnknownPerformanceToFast) {
   histogram_tester.ExpectBucketCount(
       "PerformanceHints.Observer.PerformanceClassForURL", /*kUnknown*/ 0, 2);
 }
+
+TEST_F(PerformanceHintsObserverTest, HintFetchingNotEnabled) {
+  base::CommandLine::ForCurrentProcess()->RemoveSwitch(
+      optimization_guide::switches::kDisableCheckingUserPermissionsForTesting);
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {kPerformanceHintsObserver,
+       optimization_guide::features::kOptimizationHints,
+       // Ensure PERFORMANCE_UNKNOWN is not overridden to FAST when fetching is
+       // disabled.
+       kPerformanceHintsTreatUnknownAsFast},
+      {});
+
+  ON_CALL(*mock_optimization_guide_keyed_service_,
+          CanApplyOptimizationAsync(testing::_, testing::_, testing::_))
+      .WillByDefault(base::test::RunOnceCallback<2>(
+          optimization_guide::OptimizationGuideDecision::kFalse,
+          optimization_guide::OptimizationMetadata{}));
+
+  PerformanceHintsObserver::CreateForWebContents(web_contents());
+  CallDidFinishNavigation(web_contents());
+
+  EXPECT_THAT(PerformanceHintsObserver::PerformanceClassForURL(
+                  web_contents(), GURL("http://www.test.com")),
+              Eq(optimization_guide::proto::PERFORMANCE_UNKNOWN));
+}
+
+TEST_F(PerformanceHintsObserverTest, ResetObserverForNextNavigation) {
+  optimization_guide::OptimizationGuideDecisionCallback finished_callback;
+
+  EXPECT_CALL(*mock_optimization_guide_keyed_service_,
+              CanApplyOptimizationAsync(
+                  testing::_, optimization_guide::proto::PERFORMANCE_HINTS,
+                  base::test::IsNotNullCallback()))
+      .Times(2)
+      .WillRepeatedly(testing::WithArgs<2>(testing::Invoke(
+          [&finished_callback](
+              optimization_guide::OptimizationGuideDecisionCallback callback) {
+            finished_callback = std::move(callback);
+          })));
+
+  PerformanceHintsObserver::CreateForWebContents(web_contents());
+  CallDidFinishNavigation(web_contents());
+
+  {
+    base::HistogramTester histogram_tester;
+    EXPECT_THAT(PerformanceHintsObserver::PerformanceClassForURL(
+                    web_contents(), GURL("https://www.nohint.com")),
+                Eq(optimization_guide::proto::PERFORMANCE_UNKNOWN));
+
+    histogram_tester.ExpectUniqueSample(
+        "PerformanceHints.Observer.HintForURLResult", /*kHintNotReady*/ 1, 1);
+  }
+
+  {
+    base::HistogramTester histogram_tester;
+
+    std::move(finished_callback)
+        .Run(optimization_guide::OptimizationGuideDecision::kTrue, {});
+    EXPECT_THAT(PerformanceHintsObserver::PerformanceClassForURL(
+                    web_contents(), GURL("https://www.nohint.com")),
+                Eq(optimization_guide::proto::PERFORMANCE_UNKNOWN));
+
+    histogram_tester.ExpectUniqueSample(
+        "PerformanceHints.Observer.HintForURLResult", /*kHintNotFound*/ 0, 1);
+  }
+
+  {
+    base::HistogramTester histogram_tester;
+
+    // Simulate navigation to another page.
+    CallDidFinishNavigation(web_contents());
+
+    EXPECT_THAT(PerformanceHintsObserver::PerformanceClassForURL(
+                    web_contents(), GURL("https://www.nohint.com")),
+                Eq(optimization_guide::proto::PERFORMANCE_UNKNOWN));
+
+    histogram_tester.ExpectUniqueSample(
+        "PerformanceHints.Observer.HintForURLResult",
+        /*kHintNotReady*/ 1, 1);
+  }
+}