Output whether the classification result is cached in debug UI

This makes it much easier to reason about what's happening in various
testing scenarios, by using the "Filtering Results" box in the
chrome://family-link-user-internals/ page.

See screenshot in linked bug.

Low-Coverage-Reason: TRIVIAL_CHANGE The family_link_user_internals/family_link_user_internals_message_handler.cc change has only small log text changes in a debug UI, with no existing test coverage
Bug: b:362907992
Change-Id: I378b660b1b2ac3e9ede4e60ac258d7f1e99d242f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824007
Commit-Queue: James Lee <ljjlee@google.com>
Reviewed-by: Anqing Zhao <anqing@chromium.org>
Reviewed-by: Nohemi Fernandez <fernandex@chromium.org>
Reviewed-by: Sean Topping <seantopping@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1350631}

chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc

@@ -658,8 +658,7 @@ class MockSupervisedUserURLFilterObserver
               OnURLChecked,
               (const GURL& url,
                supervised_user::FilteringBehavior behavior,
-               supervised_user::FilteringBehaviorReason reason,
-               bool uncertain),
+               supervised_user::FilteringBehaviorDetails details),
               (override));
 
  private:

chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.cc

@@ -96,6 +96,23 @@ std::string FilteringBehaviorToString(
   return result;
 }
 
+std::string FilteringBehaviorDetailsToString(
+    supervised_user::FilteringBehaviorDetails details) {
+  switch (details.reason) {
+    case supervised_user::FilteringBehaviorReason::DEFAULT:
+      return "Default";
+    case supervised_user::FilteringBehaviorReason::ASYNC_CHECKER:
+      if (details.classification_details.reason ==
+          safe_search_api::ClassificationDetails::Reason::kCachedResponse) {
+        return "AsyncChecker (Cached)";
+      }
+      return "AsyncChecker";
+    case supervised_user::FilteringBehaviorReason::MANUAL:
+      return "Manual";
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 FamilyLinkUserInternalsMessageHandler::FamilyLinkUserInternalsMessageHandler() =
@@ -262,12 +279,15 @@ void FamilyLinkUserInternalsMessageHandler::OnTryURLResult(
 void FamilyLinkUserInternalsMessageHandler::OnURLChecked(
     const GURL& url,
     supervised_user::FilteringBehavior behavior,
-    supervised_user::FilteringBehaviorReason reason,
-    bool uncertain) {
+    supervised_user::FilteringBehaviorDetails details) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::Value::Dict result;
   result.Set("url", url.possibly_invalid_spec());
-  result.Set("result", FilteringBehaviorToString(behavior, uncertain));
-  result.Set("reason", FilteringBehaviorReasonToString(reason));
+  result.Set("result",
+             FilteringBehaviorToString(
+                 behavior, details.classification_details.reason ==
+                               safe_search_api::ClassificationDetails::Reason::
+                                   kFailedUseDefault));
+  result.Set("reason", FilteringBehaviorDetailsToString(details));
   FireWebUIListener("filtering-result-received", result);
 }

chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.h

@@ -54,8 +54,7 @@ class FamilyLinkUserInternalsMessageHandler
 
   void OnURLChecked(const GURL& url,
                     supervised_user::FilteringBehavior behavior,
-                    supervised_user::FilteringBehaviorReason reason,
-                    bool uncertain) override;
+                    supervised_user::FilteringBehaviorDetails details) override;
 
   base::CallbackListSubscription user_settings_subscription_;
 

chromecast/browser/general_audience_browsing_service.cc

@@ -22,7 +22,7 @@ void CheckURLCallbackWrapper(
     GeneralAudienceBrowsingService::CheckURLCallback callback,
     const GURL& /* url */,
     safe_search_api::Classification classification,
-    bool /* uncertain */) {
+    safe_search_api::ClassificationDetails details) {
   std::move(callback).Run(classification ==
                           safe_search_api::Classification::SAFE);
 }

components/policy/content/safe_search_service.cc

@@ -22,7 +22,7 @@ namespace {
 void OnCheckURLDone(SafeSearchService::CheckSafeSearchCallback callback,
                     const GURL& /* url */,
                     safe_search_api::Classification classification,
-                    bool /* uncertain */) {
+                    safe_search_api::ClassificationDetails details) {
   std::move(callback).Run(classification ==
                           safe_search_api::Classification::SAFE);
 }

components/safe_search_api/url_checker.cc

@@ -40,8 +40,7 @@ URLChecker::Check::Check(const GURL& url, CheckCallback callback) : url(url) {
 URLChecker::Check::~Check() = default;
 
 URLChecker::CheckResult::CheckResult(Classification classification)
-    : classification(classification),
-      timestamp(base::TimeTicks::Now()) {}
+    : classification(classification), timestamp(base::TimeTicks::Now()) {}
 
 URLChecker::URLChecker(std::unique_ptr<URLCheckerClient> async_checker)
     : URLChecker(std::move(async_checker), kDefaultCacheSize) {}
@@ -82,7 +81,10 @@ bool URLChecker::CheckURL(const GURL& url, CheckCallback callback) {
       DVLOG(1) << "Cache hit! " << url.spec() << " is "
                << (result.classification == Classification::UNSAFE ? "NOT" : "")
                << " safe";
-      std::move(callback).Run(url, result.classification, /*uncertain=*/false);
+      std::move(callback).Run(
+          url, result.classification,
+          ClassificationDetails{
+              .reason = ClassificationDetails::Reason::kCachedResponse});
 
       base::UmaHistogramEnumeration(kCacheHitMetricKey,
                                     CacheAccessStatus::kHit);
@@ -122,7 +124,13 @@ void URLChecker::OnAsyncCheckComplete(CheckList::iterator it,
   }
 
   for (CheckCallback& callback : callbacks) {
-    std::move(callback).Run(url, classification, uncertain);
+    std::move(callback).Run(
+        url, classification,
+        ClassificationDetails{
+            .reason =
+                uncertain
+                    ? ClassificationDetails::Reason::kFailedUseDefault
+                    : ClassificationDetails::Reason::kFreshServerResponse});
   }
 }
 

components/safe_search_api/url_checker.h

@@ -20,6 +20,24 @@ namespace safe_search_api {
 // The SafeSearch API classification of a URL.
 enum class Classification { SAFE, UNSAFE };
 
+// Additional details regarding how the `Classification` result was determined.
+struct ClassificationDetails {
+  enum class Reason {
+    // Chrome sent a fresh external request to the server, and received a valid
+    // response.
+    kFreshServerResponse,
+
+    // Chrome used a cached response, stored from a previous successful request
+    // to the server.
+    kCachedResponse,
+
+    // Chrome tried but failed to query the server, and defaulted to
+    // `Classification::SAFE`.
+    kFailedUseDefault,
+  };
+  Reason reason;
+};
+
 // These values are sent to Uma to understand Cache utilization. Must not be
 // renumbered.
 enum class CacheAccessStatus {
@@ -40,8 +58,9 @@ enum class CacheAccessStatus {
 class URLChecker {
  public:
   // Used to report whether |url| should be blocked. Called from CheckURL.
-  using CheckCallback = base::OnceCallback<
-      void(const GURL&, Classification classification, bool /* uncertain */)>;
+  using CheckCallback = base::OnceCallback<void(const GURL&,
+                                                Classification classification,
+                                                ClassificationDetails details)>;
 
   explicit URLChecker(std::unique_ptr<URLCheckerClient> async_checker);
 

components/safe_search_api/url_checker_unittest.cc

@@ -67,6 +67,12 @@ auto Recorded(const std::map<CacheAccessStatus, int>& expected) {
   return base::BucketsInclude(buckets_array);
 }
 
+// A matcher which checks that the provided |ClassificationDetails| has the
+// expected |reason| value.
+MATCHER_P(ReasonEq, reason, "") {
+  return arg.reason == reason;
+}
+
 }  // namespace
 
 class SafeSearchURLCheckerTest : public testing::Test {
@@ -81,7 +87,7 @@ class SafeSearchURLCheckerTest : public testing::Test {
   MOCK_METHOD3(OnCheckDone,
                void(const GURL& url,
                     Classification classification,
-                    bool uncertain));
+                    ClassificationDetails details));
 
  protected:
   GURL GetNewURL() {
@@ -121,21 +127,29 @@ class SafeSearchURLCheckerTest : public testing::Test {
 TEST_F(SafeSearchURLCheckerTest, Simple) {
   {
     GURL url(GetNewURL());
-    EXPECT_CALL(*this,
-                OnCheckDone(url, Classification::SAFE, /*uncertain=*/false));
+    EXPECT_CALL(
+        *this,
+        OnCheckDone(
+            url, Classification::SAFE,
+            ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
     ASSERT_FALSE(SendResponse(url, Classification::SAFE, /*uncertain=*/false));
   }
   {
     GURL url(GetNewURL());
-    EXPECT_CALL(*this,
-                OnCheckDone(url, Classification::UNSAFE, /*uncertain=*/false));
+    EXPECT_CALL(
+        *this,
+        OnCheckDone(
+            url, Classification::UNSAFE,
+            ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
     ASSERT_FALSE(
         SendResponse(url, Classification::UNSAFE, /*uncertain=*/false));
   }
   {
     GURL url(GetNewURL());
-    EXPECT_CALL(*this,
-                OnCheckDone(url, Classification::SAFE, /*uncertain=*/true));
+    EXPECT_CALL(
+        *this, OnCheckDone(
+                   url, Classification::SAFE,
+                   ReasonEq(ClassificationDetails::Reason::kFailedUseDefault)));
     ASSERT_FALSE(SendResponse(url, Classification::SAFE, /*uncertain=*/true));
   }
 
@@ -151,28 +165,44 @@ TEST_F(SafeSearchURLCheckerTest, Cache) {
   GURL url3(GetNewURL());
 
   // Populate the cache.
-  EXPECT_CALL(*this,
-              OnCheckDone(url1, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(
+          url1, Classification::SAFE,
+          ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
   ASSERT_FALSE(SendResponse(url1, Classification::SAFE, /*uncertain=*/false));
-  EXPECT_CALL(*this,
-              OnCheckDone(url2, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(
+          url2, Classification::SAFE,
+          ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
   ASSERT_FALSE(SendResponse(url2, Classification::SAFE, /*uncertain=*/false));
 
   // Now we should get results synchronously, without a request to the api.
-  EXPECT_CALL(*this,
-              OnCheckDone(url2, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(url2, Classification::SAFE,
+                  ReasonEq(ClassificationDetails::Reason::kCachedResponse)));
   ASSERT_TRUE(CheckURL(url2));
-  EXPECT_CALL(*this,
-              OnCheckDone(url1, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(url1, Classification::SAFE,
+                  ReasonEq(ClassificationDetails::Reason::kCachedResponse)));
   ASSERT_TRUE(CheckURL(url1));
 
   // Now |url2| is the LRU and should be evicted on the next check.
-  EXPECT_CALL(*this,
-              OnCheckDone(url3, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(
+          url3, Classification::SAFE,
+          ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
   ASSERT_FALSE(SendResponse(url3, Classification::SAFE, /*uncertain=*/false));
 
-  EXPECT_CALL(*this,
-              OnCheckDone(url2, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(
+          url2, Classification::SAFE,
+          ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
   ASSERT_FALSE(SendResponse(url2, Classification::SAFE, /*uncertain=*/false));
 
   EXPECT_THAT(CacheHitMetric(), Recorded({{CacheAccessStatus::kHit, 2},
@@ -185,7 +215,11 @@ TEST_F(SafeSearchURLCheckerTest, CoalesceRequestsToSameURL) {
   ASSERT_FALSE(CheckURL(url));
   ASSERT_FALSE(CheckURL(url));
   // A single response should answer both of those checks
-  EXPECT_CALL(*this, OnCheckDone(url, Classification::SAFE, false)).Times(2);
+  EXPECT_CALL(
+      *this, OnCheckDone(
+                 url, Classification::SAFE,
+                 ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)))
+      .Times(2);
   fake_client_->RunCallback(ToAPIClassification(Classification::SAFE, false));
 
   EXPECT_THAT(CacheHitMetric(), Recorded({{CacheAccessStatus::kHit, 0},
@@ -197,14 +231,20 @@ TEST_F(SafeSearchURLCheckerTest, CacheTimeout) {
 
   checker_->SetCacheTimeoutForTesting(base::Seconds(0));
 
-  EXPECT_CALL(*this,
-              OnCheckDone(url, Classification::SAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(
+          url, Classification::SAFE,
+          ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
   ASSERT_FALSE(SendResponse(url, Classification::SAFE, /*uncertain=*/false));
 
   // Since the cache timeout is zero, the cache entry should be invalidated
   // immediately.
-  EXPECT_CALL(*this,
-              OnCheckDone(url, Classification::UNSAFE, /*uncertain=*/false));
+  EXPECT_CALL(
+      *this,
+      OnCheckDone(
+          url, Classification::UNSAFE,
+          ReasonEq(ClassificationDetails::Reason::kFreshServerResponse)));
   ASSERT_FALSE(SendResponse(url, Classification::UNSAFE, /*uncertain=*/false));
 
   EXPECT_THAT(CacheHitMetric(), Recorded({{CacheAccessStatus::kHit, 0},

components/supervised_user/core/browser/supervised_user_error_page_unittest.cc

@@ -32,7 +32,7 @@ TEST_P(SupervisedUserErrorPageTest_GetBlockMessageID, GetBlockMessageID) {
   BlockMessageIDTestParameter param = GetParam();
   EXPECT_EQ(param.expected_result,
             GetBlockMessageID(param.reason, param.single_parent))
-      << "reason = " << FilteringBehaviorReasonToString(param.reason)
+      << "reason = " << int(param.reason)
       << " single parent = " << param.single_parent;
 }
 

components/supervised_user/core/browser/supervised_user_url_filter.cc

@@ -20,6 +20,7 @@
 #include "base/notreached.h"
 #include "base/strings/string_util.h"
 #include "base/task/thread_pool.h"
+#include "components/safe_search_api/url_checker.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/supervised_user/core/browser/kids_chrome_management_url_checker_client.h"
 #include "components/supervised_user/core/browser/supervised_user_capabilities.h"
@@ -567,10 +568,12 @@ bool SupervisedUserURLFilter::GetFilteringBehaviorForURLWithAsyncChecks(
       reason != supervised_user::FilteringBehaviorReason::DEFAULT) {
     std::move(callback).Run(behavior, reason, false);
     for (Observer& observer : observers_) {
-      observer.OnURLChecked(url, behavior, reason, false);
+      observer.OnURLChecked(
+          url, behavior,
+          FilteringBehaviorDetails{.reason = reason});
     }
     return true;
-  }
+    }
 
   if (!skip_manual_parent_filter) {
     // Any non-default reason trumps the async checker.
@@ -579,7 +582,9 @@ bool SupervisedUserURLFilter::GetFilteringBehaviorForURLWithAsyncChecks(
         behavior == FilteringBehavior::kBlock) {
       std::move(callback).Run(behavior, reason, false);
       for (Observer& observer : observers_) {
-        observer.OnURLChecked(url, behavior, reason, false);
+        observer.OnURLChecked(
+            url, behavior,
+            FilteringBehaviorDetails{.reason = reason});
       }
       return true;
     }
@@ -601,7 +606,9 @@ bool SupervisedUserURLFilter::GetFilteringBehaviorForSubFrameURLWithAsyncChecks(
   if (reason != supervised_user::FilteringBehaviorReason::DEFAULT) {
     std::move(callback).Run(behavior, reason, false);
     for (Observer& observer : observers_) {
-      observer.OnURLChecked(url, behavior, reason, false);
+      observer.OnURLChecked(
+          url, behavior,
+          FilteringBehaviorDetails{.reason = reason});
     }
     return true;
   }
@@ -613,7 +620,9 @@ bool SupervisedUserURLFilter::GetFilteringBehaviorForSubFrameURLWithAsyncChecks(
     // It is not in the same domain and is blocked.
     std::move(callback).Run(behavior, reason, false);
     for (Observer& observer : observers_) {
-      observer.OnURLChecked(url, behavior, reason, false);
+      observer.OnURLChecked(
+          url, behavior,
+          FilteringBehaviorDetails{.reason = reason});
     }
     return true;
   }
@@ -782,16 +791,19 @@ void SupervisedUserURLFilter::CheckCallback(
     FilteringBehaviorCallback callback,
     const GURL& url,
     safe_search_api::Classification classification,
-    bool uncertain) const {
+    safe_search_api::ClassificationDetails details) const {
   FilteringBehavior behavior =
       GetBehaviorFromSafeSearchClassification(classification);
   std::move(callback).Run(
       behavior, supervised_user::FilteringBehaviorReason::ASYNC_CHECKER,
-      uncertain);
+      details.reason ==
+          safe_search_api::ClassificationDetails::Reason::kFailedUseDefault);
   for (Observer& observer : observers_) {
     observer.OnURLChecked(
-        url, behavior, supervised_user::FilteringBehaviorReason::ASYNC_CHECKER,
-        uncertain);
+        url, behavior,
+        supervised_user::FilteringBehaviorDetails{
+            .reason = supervised_user::FilteringBehaviorReason::ASYNC_CHECKER,
+            .classification_details = details});
   }
 }
 

components/supervised_user/core/browser/supervised_user_url_filter.h

@@ -103,10 +103,10 @@ class SupervisedUserURLFilter {
    public:
     // Called whenever a check started via
     // GetFilteringBehaviorForURLWithAsyncChecks completes.
-    virtual void OnURLChecked(const GURL& url,
-                              FilteringBehavior behavior,
-                              supervised_user::FilteringBehaviorReason reason,
-                              bool uncertain) {}
+    virtual void OnURLChecked(
+        const GURL& url,
+        FilteringBehavior behavior,
+        supervised_user::FilteringBehaviorDetails details) {}
   };
 
   SupervisedUserURLFilter(PrefService& user_prefs,
@@ -252,7 +252,7 @@ class SupervisedUserURLFilter {
   void CheckCallback(FilteringBehaviorCallback callback,
                      const GURL& url,
                      safe_search_api::Classification classification,
-                     bool uncertain) const;
+                     safe_search_api::ClassificationDetails details) const;
 
   base::ObserverList<Observer>::Unchecked observers_;
 

components/supervised_user/core/browser/supervised_user_url_filter_unittest.cc

@@ -41,10 +41,9 @@ class SupervisedUserURLFilterTest : public ::testing::Test,
   // SupervisedUserURLFilter::Observer:
   void OnURLChecked(const GURL& url,
                     supervised_user::FilteringBehavior behavior,
-                    supervised_user::FilteringBehaviorReason reason,
-                    bool uncertain) override {
+                    supervised_user::FilteringBehaviorDetails details) override {
     behavior_ = behavior;
-    reason_ = reason;
+    reason_ = details.reason;
   }
 
  protected:

components/supervised_user/core/browser/supervised_user_utils.cc

@@ -107,18 +107,6 @@ std::string FamilyRoleToString(kidsmanagement::FamilyRole role) {
   }
 }
 
-std::string FilteringBehaviorReasonToString(FilteringBehaviorReason reason) {
-  switch (reason) {
-    case FilteringBehaviorReason::DEFAULT:
-      return "Default";
-    case FilteringBehaviorReason::ASYNC_CHECKER:
-      return "AsyncChecker";
-    case FilteringBehaviorReason::MANUAL:
-      return "Manual";
-  }
-  return "Unknown";
-}
-
 GURL NormalizeUrl(const GURL& url) {
   GURL effective_url = url_matcher::util::GetEmbeddedURL(url);
   if (!effective_url.is_valid()) {

components/supervised_user/core/browser/supervised_user_utils.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/memory/raw_ref.h"
+#include "components/safe_search_api/url_checker.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/supervised_user/core/browser/family_link_user_log_record.h"
 #include "components/supervised_user/core/browser/proto/families_common.pb.h"
@@ -25,6 +26,14 @@ enum class FilteringBehaviorReason {
   MANUAL = 2,
 };
 
+// Details degarding how a particular filtering classification was arrived at.
+struct FilteringBehaviorDetails {
+  FilteringBehaviorReason reason;
+
+  // The following field only applies if `reason` is `ASYNC_CHECKER`.
+  safe_search_api::ClassificationDetails classification_details;
+};
+
 // A Java counterpart will be generated for this enum.
 // Values are stored in prefs under kDefaultSupervisedUserFilteringBehavior.
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.superviseduser
@@ -61,9 +70,6 @@ enum class LocallyParentApprovedExtensionsMigrationState : int {
 // Converts FamilyRole enum to string format.
 std::string FamilyRoleToString(kidsmanagement::FamilyRole role);
 
-// Converts FilteringBehaviorReason enum to string format.
-std::string FilteringBehaviorReasonToString(FilteringBehaviorReason reason);
-
 // Strips user-specific tokens in a URL to generalize it.
 GURL NormalizeUrl(const GURL& url);