Update the Safe Browsing interstitial histograms

chrome/browser/safe_browsing/safe_browsing_blocking_page.cc

Index: chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
index b859f7343d24e682fb2807aaf19a8e5772a9faf5..8d5d9a294c91f146bb8ac59ab19a864dd89f24cb 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -123,26 +123,6 @@ const char kEventNameOther[] = "safebrowsing_other_interstitial_";
 base::LazyInstance<SafeBrowsingBlockingPage::UnsafeResourceMap>
     g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;
 
-// This enum is used for a histogram.  Don't reorder, delete, or insert
-// elements.  New elements should be added before MAX_ACTION only.
-enum DetailedDecision {
-  MALWARE_SHOW_NEW_SITE = 0,
-  MALWARE_PROCEED_NEW_SITE,
-  MALWARE_SHOW_CROSS_SITE,
-  MALWARE_PROCEED_CROSS_SITE,
-  PHISHING_SHOW_NEW_SITE,
-  PHISHING_PROCEED_NEW_SITE,
-  PHISHING_SHOW_CROSS_SITE,
-  PHISHING_PROCEED_CROSS_SITE,
-  MAX_DETAILED_ACTION
-};
-
-void RecordDetailedUserAction(DetailedDecision decision) {
-  UMA_HISTOGRAM_ENUMERATION("SB2.InterstitialActionDetails",
-                            decision,
-                            MAX_DETAILED_ACTION);
-}
-
 }  // namespace
 
 // static
@@ -214,7 +194,10 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
   else
     interstitial_type_ = TYPE_PHISHING;
 
-  RecordUserAction(SHOW);
+  RecordUserDecision(SHOW);
+  if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled))
+    RecordUserDecision(PROCEEDING_DISABLED);
+
   HistoryService* history_service = HistoryServiceFactory::GetForProfile(
           Profile::FromBrowserContext(web_contents->GetBrowserContext()),
           Profile::EXPLICIT_ACCESS);
@@ -301,10 +284,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
 
   if (command == kLearnMoreCommand) {
     // User pressed "Learn more".
-#if defined(ENABLE_EXTENSIONS)
-    if (sampling_event_.get())
-      sampling_event_->set_has_viewed_learn_more(true);
-#endif
+    RecordUserInteraction(SHOW_LEARN_MORE);
     GURL learn_more_url(interstitial_type_ == TYPE_PHISHING ?
                         kLearnMorePhishingUrlV2 : kLearnMoreMalwareUrlV2);
     learn_more_url = google_util::AppendGoogleLocaleParam(
@@ -320,6 +300,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
 
   if (command == kShowPrivacyCommand) {
     // User pressed "Safe Browsing privacy policy".
+    RecordUserInteraction(SHOW_PRIVACY_POLICY);
     GURL privacy_url(
         l10n_util::GetStringUTF8(IDS_SAFE_BROWSING_PRIVACY_POLICY_URL));
     privacy_url = google_util::AppendGoogleLocaleParam(
@@ -338,6 +319,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
       proceed_blocked = true;
     } else {
+      RecordUserDecision(PROCEED);
       interstitial_page_->Proceed();
       // |this| has been deleted after Proceed() returns.
       return;
@@ -345,6 +327,8 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
   }
 
   if (command == kTakeMeBackCommand || proceed_blocked) {
+    if (!proceed_blocked)
+      RecordUserDecision(DONT_PROCEED);

[mattm, 2014/09/16 07:48:20]

I still don't think this will be recorded if the user hits the back button or
navigates to a different URL.

[felt, 2014/09/16 15:03:32]

Oh, hmm, good point. Fixed.

     if (is_main_frame_load_blocked_) {
       // If the load is blocked, we want to close the interstitial and discard
       // the pending entry.
@@ -410,6 +394,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
 
   if (command == kShowDiagnosticCommand) {
     // We're going to take the user to Google's SafeBrowsing diagnostic page.
+    RecordUserInteraction(SHOW_DIAGNOSTIC);
     std::string diagnostic =
         base::StringPrintf(kSbDiagnosticUrl,
             net::EscapeQueryParamValue(bad_url_spec, true).c_str());
@@ -428,15 +413,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
   }
 
   if (command == kExpandedSeeMoreCommand) {
-    // User expanded the "see more info" section of the page.  We don't actually
-    // do any action based on this, it's just so that RecordUserReactionTime can
-    // track it.
-
-#if defined(ENABLE_EXTENSIONS)
-    // ExperienceSampling: We track that the user expanded the details.
-    if (sampling_event_.get())
-      sampling_event_->set_has_viewed_details(true);
-#endif
+    RecordUserInteraction(SHOW_ADVANCED);
     return;
   }
 
@@ -460,7 +437,6 @@ void SafeBrowsingBlockingPage::SetReportingPreference(bool report) {
 
 void SafeBrowsingBlockingPage::OnProceed() {
   proceeded_ = true;
-  RecordUserAction(PROCEED);
   // Send the malware details, if we opted to.
   FinishMalwareDetails(malware_details_proceed_delay_ms_);
 
@@ -480,12 +456,6 @@ void SafeBrowsingBlockingPage::OnProceed() {
     unsafe_resource_map->erase(iter);
   }
 
-#if defined(ENABLE_EXTENSIONS)
-  // ExperienceSampling: Notify that user decided to proceed.
-  if (sampling_event_.get())
-    sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
-#endif
-
   // Now that this interstitial is gone, we can show the new one.
   if (blocking_page)
     blocking_page->Show();
@@ -512,7 +482,6 @@ void SafeBrowsingBlockingPage::OnDontProceed() {
   if (proceeded_)
     return;
 
-  RecordUserAction(DONT_PROCEED);
   // Send the malware details, if we opted to.
   FinishMalwareDetails(0);  // No delay
 
@@ -541,13 +510,6 @@ void SafeBrowsingBlockingPage::OnDontProceed() {
         navigation_entry_index_to_remove_));
     navigation_entry_index_to_remove_ = -1;
   }
-
-#if defined(ENABLE_EXTENSIONS)
-  // ExperienceSampling: Notify that user decided to go back.
-  // This also occurs if the user navigates away or closes the tab.
-  if (sampling_event_.get())
-    sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kDeny);
-#endif
 }
 
 void SafeBrowsingBlockingPage::OnGotHistoryCount(bool success,
@@ -557,124 +519,87 @@ void SafeBrowsingBlockingPage::OnGotHistoryCount(bool success,
     num_visits_ = num_visits;
 }
 
-void SafeBrowsingBlockingPage::RecordUserAction(BlockingPageEvent event) {
-  // This enum is used for a histogram.  Don't reorder, delete, or insert
-  // elements.  New elements should be added before MAX_ACTION only.
-  enum {
-    MALWARE_SHOW = 0,
-    MALWARE_DONT_PROCEED,
-    MALWARE_FORCED_DONT_PROCEED,
-    MALWARE_PROCEED,
-    MULTIPLE_SHOW,
-    MULTIPLE_DONT_PROCEED,
-    MULTIPLE_FORCED_DONT_PROCEED,
-    MULTIPLE_PROCEED,
-    PHISHING_SHOW,
-    PHISHING_DONT_PROCEED,
-    PHISHING_FORCED_DONT_PROCEED,
-    PHISHING_PROCEED,
-    MALWARE_SHOW_ADVANCED,
-    MULTIPLE_SHOW_ADVANCED,
-    PHISHING_SHOW_ADVANCED,
-    MAX_ACTION
-  } histogram_action = MAX_ACTION;
-
-  switch (event) {
-    case SHOW:
-      switch (interstitial_type_) {
-        case TYPE_MALWARE_AND_PHISHING:
-          histogram_action = MULTIPLE_SHOW;
-          break;
-        case TYPE_MALWARE:
-          histogram_action = MALWARE_SHOW;
-          break;
-        case TYPE_PHISHING:
-          histogram_action = PHISHING_SHOW;
-          break;
-      }
+void SafeBrowsingBlockingPage::RecordUserDecision(Decision decision) {
+  switch (interstitial_type_) {
+    case TYPE_MALWARE:
+      UMA_HISTOGRAM_ENUMERATION("interstitial.malware.decision",
+                                decision,
+                                MAX_DECISION);
       break;
-    case PROCEED:
-      switch (interstitial_type_) {
-        case TYPE_MALWARE_AND_PHISHING:
-          histogram_action = MULTIPLE_PROCEED;
-          break;
-        case TYPE_MALWARE:
-          histogram_action = MALWARE_PROCEED;
-          break;
-        case TYPE_PHISHING:
-          histogram_action = PHISHING_PROCEED;
-          break;
-      }
+    case TYPE_PHISHING:
+      UMA_HISTOGRAM_ENUMERATION("interstitial.phishing.decision",
+                                decision,
+                                MAX_DECISION);
       break;
-    case DONT_PROCEED:
-      if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
-        switch (interstitial_type_) {
-          case TYPE_MALWARE_AND_PHISHING:
-            histogram_action = MULTIPLE_FORCED_DONT_PROCEED;
-            break;
-          case TYPE_MALWARE:
-            histogram_action = MALWARE_FORCED_DONT_PROCEED;
-            break;
-          case TYPE_PHISHING:
-            histogram_action = PHISHING_FORCED_DONT_PROCEED;
-            break;
-        }
-      } else {
-        switch (interstitial_type_) {
-          case TYPE_MALWARE_AND_PHISHING:
-            histogram_action = MULTIPLE_DONT_PROCEED;
-            break;
-          case TYPE_MALWARE:
-            histogram_action = MALWARE_DONT_PROCEED;
-            break;
-          case TYPE_PHISHING:
-            histogram_action = PHISHING_DONT_PROCEED;
-            break;
-        }
-      }
+    default:
+      NOTREACHED();
+  }
+
+#if defined(ENABLE_EXTENSIONS)
+  if (sampling_event_.get()) {
+    switch (decision) {
+      case PROCEED:
+        sampling_event_->CreateUserDecisionEvent(
+            ExperienceSamplingEvent::kProceed);
+        break;
+      case DONT_PROCEED:
+        sampling_event_->CreateUserDecisionEvent(
+            ExperienceSamplingEvent::kDeny);
+        break;
+      case SHOW:
+      case PROCEEDING_DISABLED:
+      case MAX_DECISION:
+        break;
+    }
+  }
+#endif
+
+  // Record additional information about malware sites that users have
+  // visited before.
+  if (num_visits_ < 1 || interstitial_type_ != TYPE_MALWARE)
+    return;
+  if (decision == PROCEED || decision == DONT_PROCEED) {
+    UMA_HISTOGRAM_ENUMERATION("interstitial.malware.repeat_visit_decision",
+                              SHOW,
+                              MAX_DECISION);
+    UMA_HISTOGRAM_ENUMERATION("interstitial.malware.repeat_visit_decision",
+                              decision,
+                              MAX_DECISION);
+  }
+}
+
+void SafeBrowsingBlockingPage::RecordUserInteraction(Interaction interaction) {
+  switch (interstitial_type_) {
+    case TYPE_MALWARE:
+      UMA_HISTOGRAM_ENUMERATION("interstitial.malware.interaction",
+                                interaction,
+                                MAX_INTERACTION);
       break;
-    case SHOW_ADVANCED:
-      switch (interstitial_type_) {
-        case TYPE_MALWARE_AND_PHISHING:
-          histogram_action = MULTIPLE_SHOW_ADVANCED;
-          break;
-        case TYPE_MALWARE:
-          histogram_action = MALWARE_SHOW_ADVANCED;
-          break;
-        case TYPE_PHISHING:
-          histogram_action = PHISHING_SHOW_ADVANCED;
-          break;
-      }
+    case TYPE_PHISHING:
+      UMA_HISTOGRAM_ENUMERATION("interstitial.phishing.interaction",
+                                interaction,
+                                MAX_INTERACTION);
       break;
     default:
-      NOTREACHED() << "Unexpected event: " << event;
+      NOTREACHED();
   }
-  if (histogram_action == MAX_ACTION) {
-    NOTREACHED();
-  } else {
-    UMA_HISTOGRAM_ENUMERATION("SB2.InterstitialAction", histogram_action,
-                              MAX_ACTION);
-  }
-
-  if (event == PROCEED || event == DONT_PROCEED) {
-    if (num_visits_ == 0 && interstitial_type_ != TYPE_MALWARE_AND_PHISHING) {
-      RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ?
-                               MALWARE_SHOW_NEW_SITE : PHISHING_SHOW_NEW_SITE);
-      if (event == PROCEED) {
-        RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ?
-            MALWARE_PROCEED_NEW_SITE : PHISHING_PROCEED_NEW_SITE);
-      }
-    }
-    if (unsafe_resources_[0].is_subresource &&
-        interstitial_type_ != TYPE_MALWARE_AND_PHISHING) {
-      RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ?
-          MALWARE_SHOW_CROSS_SITE : PHISHING_SHOW_CROSS_SITE);
-      if (event == PROCEED) {
-        RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ?
-            MALWARE_PROCEED_CROSS_SITE : PHISHING_PROCEED_CROSS_SITE);
-      }
-    }
+
+#if defined(ENABLE_EXTENSIONS)
+  if (!sampling_event_.get())
+    return;
+  switch (interaction) {
+    case SHOW_LEARN_MORE:
+      sampling_event_->set_has_viewed_learn_more(true);
+      break;
+    case SHOW_ADVANCED:
+      sampling_event_->set_has_viewed_details(true);
+      break;
+    case SHOW_PRIVACY_POLICY:
+    case SHOW_DIAGNOSTIC:
+    case MAX_INTERACTION:
+      break;
   }
+#endif
 }
 
 void SafeBrowsingBlockingPage::RecordUserReactionTime(
@@ -711,7 +636,6 @@ void SafeBrowsingBlockingPage::RecordUserReactionTime(
       // interstitial.
       if (has_expanded_see_more_section_)
         return;
-      RecordUserAction(SHOW_ADVANCED);
       UMA_HISTOGRAM_MEDIUM_TIMES("SB2.MalwareInterstitialTimeExpandedSeeMore",
                                  dt);
       has_expanded_see_more_section_ = true;
@@ -738,7 +662,6 @@ void SafeBrowsingBlockingPage::RecordUserReactionTime(
       // interstitial.
       if (has_expanded_see_more_section_)
         return;
-      RecordUserAction(SHOW_ADVANCED);
       UMA_HISTOGRAM_MEDIUM_TIMES("SB2.PhishingInterstitialTimeExpandedSeeMore",
                                  dt);
       has_expanded_see_more_section_ = true;