Don't report cached verdicts in SBClientPhishing.ServerModelDetectsPhishing

Responses served from the cache follow a very similar code path to
responses from the server, but we probably don't want to treat them the
same in metrics, so this CL breaks the metric in two.

Change-Id: I80fc9ebbfa8afc39237dc57727f13f5be68cabcb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438889Reviewed-by: Bettina Dea <bdea@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812895}

chrome/browser/safe_browsing/client_side_detection_host.cc

@@ -251,7 +251,8 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
       base::UmaHistogramBoolean("SBClientPhishing.RequestSatisfiedFromCache",
                                 true);
       // Since we are already on the UI thread, this is safe.
-      host_->MaybeShowPhishingWarning(url_, is_phishing);
+      host_->MaybeShowPhishingWarning(/*is_from_cache=*/true, url_,
+                                      is_phishing);
       DontClassifyForPhishing(NO_CLASSIFY_RESULT_FROM_CACHE);
     }
 
@@ -522,11 +523,18 @@ void ClientSideDetectionHost::PhishingDetectionDone(
   }
 }
 
-void ClientSideDetectionHost::MaybeShowPhishingWarning(GURL phishing_url,
+void ClientSideDetectionHost::MaybeShowPhishingWarning(bool is_from_cache,
+                                                       GURL phishing_url,
                                                        bool is_phishing) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (is_from_cache) {
+    base::UmaHistogramBoolean("SBClientPhishing.CacheDetectsPhishing",
+                              is_phishing);
+  } else {
     base::UmaHistogramBoolean("SBClientPhishing.ServerModelDetectsPhishing",
                               is_phishing);
+  }
+
   if (is_phishing) {
     DCHECK(web_contents());
     if (ui_manager_.get()) {
@@ -563,7 +571,7 @@ void ClientSideDetectionHost::FeatureExtractionDone(
   // response because we aren't going to display a warning.
   if (request->is_phishing()) {
     callback = base::Bind(&ClientSideDetectionHost::MaybeShowPhishingWarning,
-                          weak_factory_.GetWeakPtr());
+                          weak_factory_.GetWeakPtr(), /*is_from_cache=*/false);
   }
   Profile* profile =
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());

chrome/browser/safe_browsing/client_side_detection_host.h

@@ -91,8 +91,12 @@ class ClientSideDetectionHost : public content::WebContentsObserver,
 
   // Callback that is called when the server ping back is
   // done. Display an interstitial if |is_phishing| is true.
-  // Otherwise, we do nothing.  Called in UI thread.
-  void MaybeShowPhishingWarning(GURL phishing_url, bool is_phishing);
+  // Otherwise, we do nothing. Called in UI thread. |is_from_cache| indicates
+  // whether the warning is being shown due to a cached verdict or from an
+  // actual server ping.
+  void MaybeShowPhishingWarning(bool is_from_cache,
+                                GURL phishing_url,
+                                bool is_phishing);
 
   // Callback that is called when the browser feature extractor is done.
   // This method is responsible for deleting the request object.  Called on

tools/metrics/histograms/histograms_xml/sb_client/histograms.xml

@@ -250,6 +250,16 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   <summary>The counts for malware verdicts given by server side model.</summary>
 </histogram>
 
+<histogram name="SBClientPhishing.CacheDetectsPhishing"
+    enum="BooleanIsPhishing" expires_after="2021-09-17">
+  <owner>drubery@chromium.org</owner>
+  <owner>chrome-safebrowsing-alerts@google.com</owner>
+  <summary>
+    When a result is served from cache, this histogram records whether the
+    result was phishing or not.
+  </summary>
+</histogram>
+
 <histogram name="SBClientPhishing.CancelClassificationReason"
     enum="SBClientPhishingCancelClassificationReason"
     expires_after="2021-01-27">