Add UMA metrics to phishguard pings

chrome/browser/safe_browsing/chrome_password_protection_service.cc

Index: chrome/browser/safe_browsing/chrome_password_protection_service.cc
diff --git a/chrome/browser/safe_browsing/chrome_password_protection_service.cc b/chrome/browser/safe_browsing/chrome_password_protection_service.cc
index 9c4efee47e7016d45575efe9ec65b1143ebc84ef..10d2fe3382446040456607982308710167f9d5a4 100644
--- a/chrome/browser/safe_browsing/chrome_password_protection_service.cc
+++ b/chrome/browser/safe_browsing/chrome_password_protection_service.cc
@@ -23,9 +23,11 @@ using content::BrowserThread;
 namespace safe_browsing {
 
 namespace {
+
 // The number of user gestures we trace back for login event attribution.
 const int kPasswordEventAttributionUserGestureLimit = 2;
-}
+
+}  // namespace
 
 ChromePasswordProtectionService::ChromePasswordProtectionService(
     SafeBrowsingService* sb_service,
@@ -78,14 +80,31 @@ bool ChromePasswordProtectionService::IsIncognito() {
 
 bool ChromePasswordProtectionService::IsPingingEnabled(
     const base::Feature& feature) {
+  bool is_password_entry_ping =
+      feature.name == kProtectedPasswordEntryPinging.name;

[Nathan Parker, 2017/05/11 23:36:31]

Add a DCHECK() that feature is one of the two expected feature types, to ensure
we modify this code if we add another.

[Jialiu Lin, 2017/05/12 02:12:32]

Good point.

   if (!base::FeatureList::IsEnabled(feature)) {
+    if (is_password_entry_ping) {
+      UMA_HISTOGRAM_ENUMERATION(kPasswordEntryRequestOutcomeHistogramName,
+                                DISABLED_DUE_TO_FEATURE_DISABLED, MAX_OUTCOME);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION(kPasswordOnFocusRequestOutcomeHistogramName,
+                                DISABLED_DUE_TO_FEATURE_DISABLED, MAX_OUTCOME);
+    }
     return false;
   }
 
   bool allowed_incognito =
       base::GetFieldTrialParamByFeatureAsBool(feature, "incognito", false);
-  if (IsIncognito() && !allowed_incognito)
+  if (IsIncognito() && !allowed_incognito) {
+    if (is_password_entry_ping) {
+      UMA_HISTOGRAM_ENUMERATION(kPasswordEntryRequestOutcomeHistogramName,

[Nathan Parker, 2017/05/11 23:36:31]

An idea (you decide if this is any better): You could simplify this code to
return the DISABLED_DUE_TO_* value and have the caller (a wrapper function) put
that in the appropriate histogram.  If the value is SUCCESS (or whatever enum is
appropriate), then the caller would return "true".

[Jialiu Lin, 2017/05/12 02:12:32]

SG. code refactored.

+                                DISABLED_DUE_TO_INCOGNITO, MAX_OUTCOME);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION(kPasswordOnFocusRequestOutcomeHistogramName,
+                                DISABLED_DUE_TO_INCOGNITO, MAX_OUTCOME);
+    }
     return false;
+  }
 
   bool allowed_all_population =
       base::GetFieldTrialParamByFeatureAsBool(feature, "all_population", false);
@@ -100,6 +119,14 @@ bool ChromePasswordProtectionService::IsPingingEnabled(
         base::GetFieldTrialParamByFeatureAsBool(feature, "history_sync", false);
     if (IsHistorySyncEnabled() && allowed_history_sync)
       return true;
+
+    if (is_password_entry_ping) {
+      UMA_HISTOGRAM_ENUMERATION(kPasswordEntryRequestOutcomeHistogramName,
+                                DISABLED_DUE_TO_USER_POPULATION, MAX_OUTCOME);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION(kPasswordOnFocusRequestOutcomeHistogramName,
+                                DISABLED_DUE_TO_USER_POPULATION, MAX_OUTCOME);
+    }
   }
 
   return allowed_all_population;