Remove redundancy in security interstitial UMA logic

chrome/browser/ssl/ssl_blocking_page.cc

Index: chrome/browser/ssl/ssl_blocking_page.cc
diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc
index 1f052e0d716e0c4dbc01aa1596f9e09fe75516f3..644f7668e4db972d33ec6d1742745aedaee8a8b3 100644
--- a/chrome/browser/ssl/ssl_blocking_page.cc
+++ b/chrome/browser/ssl/ssl_blocking_page.cc
@@ -20,7 +20,6 @@
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_preferences_util.h"
 #include "chrome/browser/ssl/ssl_error_classification.h"
@@ -46,10 +45,6 @@
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
-#if defined(ENABLE_EXTENSIONS)
-#include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h"
-#endif
-
 #if defined(OS_WIN)
 #include "base/base_paths_win.h"
 #include "base/path_service.h"
@@ -73,10 +68,6 @@ using content::InterstitialPage;
 using content::NavigationController;
 using content::NavigationEntry;
 
-#if defined(ENABLE_EXTENSIONS)
-using extensions::ExperienceSamplingEvent;
-#endif
-
 namespace {
 
 // URL for help page.
@@ -90,38 +81,6 @@ const char kEventOverridable[] = "overridable_";
 #endif
 
 // Events for UMA. Do not reorder or change!
-enum SSLBlockingPageEvent {
-  SHOW_ALL,
-  SHOW_OVERRIDABLE,
-  PROCEED_OVERRIDABLE,
-  PROCEED_NAME,
-  PROCEED_DATE,
-  PROCEED_AUTHORITY,
-  DONT_PROCEED_OVERRIDABLE,
-  DONT_PROCEED_NAME,
-  DONT_PROCEED_DATE,
-  DONT_PROCEED_AUTHORITY,
-  MORE,
-  SHOW_UNDERSTAND,  // Used by the summer 2013 Finch trial. Deprecated.
-  SHOW_INTERNAL_HOSTNAME,
-  PROCEED_INTERNAL_HOSTNAME,
-  SHOW_NEW_SITE,
-  PROCEED_NEW_SITE,
-  PROCEED_MANUAL_NONOVERRIDABLE,
-  // Captive Portal errors moved to ssl_error_classification.
-  DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED,
-  DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE,
-  DEPRECATED_CAPTIVE_PORTAL_PROBE_COMPLETED,
-  DEPRECATED_CAPTIVE_PORTAL_PROBE_COMPLETED_OVERRIDABLE,
-  DEPRECATED_CAPTIVE_PORTAL_NO_RESPONSE,
-  DEPRECATED_CAPTIVE_PORTAL_NO_RESPONSE_OVERRIDABLE,
-  DEPRECATED_CAPTIVE_PORTAL_DETECTED,
-  DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE,
-  DISPLAYED_CLOCK_INTERSTITIAL,
-  UNUSED_BLOCKING_PAGE_EVENT,
-};
-
-// Events for UMA. Do not reorder or change!
 enum SSLExpirationAndDecision {
   EXPIRED_AND_PROCEED,
   EXPIRED_AND_DO_NOT_PROCEED,
@@ -130,12 +89,6 @@ enum SSLExpirationAndDecision {
   END_OF_SSL_EXPIRATION_AND_DECISION,
 };
 
-void RecordSSLBlockingPageEventStats(SSLBlockingPageEvent event) {
-  UMA_HISTOGRAM_ENUMERATION("interstitial.ssl",
-                            event,
-                            UNUSED_BLOCKING_PAGE_EVENT);
-}
-
 void RecordSSLExpirationPageEventState(bool expired_but_previously_allowed,
                                        bool proceed,
                                        bool overridable) {
@@ -162,64 +115,6 @@ void RecordSSLExpirationPageEventState(bool expired_but_previously_allowed,
   }
 }
 
-void RecordSSLBlockingPageDetailedStats(bool proceed,
-                                        int cert_error,
-                                        bool overridable,
-                                        bool internal,
-                                        int num_visits,
-                                        bool expired_but_previously_allowed) {
-  UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_type",
-      SSLErrorInfo::NetErrorToErrorType(cert_error), SSLErrorInfo::END_OF_ENUM);
-  RecordSSLExpirationPageEventState(
-      expired_but_previously_allowed, proceed, overridable);
-  if (!overridable) {
-    if (proceed) {
-      RecordSSLBlockingPageEventStats(PROCEED_MANUAL_NONOVERRIDABLE);
-    }
-    // Overridable is false if the user didn't have any option except to turn
-    // back. If that's the case, don't record some of the metrics.
-    return;
-  }
-  if (num_visits == 0)
-    RecordSSLBlockingPageEventStats(SHOW_NEW_SITE);
-  if (proceed) {
-    RecordSSLBlockingPageEventStats(PROCEED_OVERRIDABLE);
-    if (internal)
-      RecordSSLBlockingPageEventStats(PROCEED_INTERNAL_HOSTNAME);
-    if (num_visits == 0)
-      RecordSSLBlockingPageEventStats(PROCEED_NEW_SITE);
-  } else if (!proceed) {
-    RecordSSLBlockingPageEventStats(DONT_PROCEED_OVERRIDABLE);
-  }
-  SSLErrorInfo::ErrorType type = SSLErrorInfo::NetErrorToErrorType(cert_error);
-  switch (type) {
-    case SSLErrorInfo::CERT_COMMON_NAME_INVALID: {
-      if (proceed)
-        RecordSSLBlockingPageEventStats(PROCEED_NAME);
-      else
-        RecordSSLBlockingPageEventStats(DONT_PROCEED_NAME);
-      break;
-    }
-    case SSLErrorInfo::CERT_DATE_INVALID: {
-      if (proceed)
-        RecordSSLBlockingPageEventStats(PROCEED_DATE);
-      else
-        RecordSSLBlockingPageEventStats(DONT_PROCEED_DATE);
-      break;
-    }
-    case SSLErrorInfo::CERT_AUTHORITY_INVALID: {
-      if (proceed)
-        RecordSSLBlockingPageEventStats(PROCEED_AUTHORITY);
-      else
-        RecordSSLBlockingPageEventStats(DONT_PROCEED_AUTHORITY);
-      break;
-    }
-    default: {
-      break;
-    }
-  }
-}
-
 void LaunchDateAndTimeSettings() {
   // The code for each OS is completely separate, in order to avoid bugs like
   // https://crbug.com/430877 .
@@ -334,31 +229,17 @@ SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents,
       overridable_(IsOptionsOverridable(options_mask)),
       danger_overridable_(true),
       strict_enforcement_((options_mask & STRICT_ENFORCEMENT) != 0),
-      internal_(false),
-      num_visits_(-1),
       expired_but_previously_allowed_(
           (options_mask & EXPIRED_BUT_PREVIOUSLY_ALLOWED) != 0) {
-  Profile* profile = Profile::FromBrowserContext(
-      web_contents->GetBrowserContext());
-  // For UMA stats.
-  if (SSLErrorClassification::IsHostnameNonUniqueOrDotless(
-          request_url.HostNoBrackets()))
-    internal_ = true;
-  RecordSSLBlockingPageEventStats(SHOW_ALL);
-  if (overridable_) {
-    RecordSSLBlockingPageEventStats(SHOW_OVERRIDABLE);
-    if (internal_)
-      RecordSSLBlockingPageEventStats(SHOW_INTERNAL_HOSTNAME);
-    HistoryService* history_service = HistoryServiceFactory::GetForProfile(
-        profile, ServiceAccessType::EXPLICIT_ACCESS);
-    if (history_service) {
-      history_service->GetVisibleVisitCountToHost(
-          request_url,
-          base::Bind(&SSLBlockingPage::OnGotHistoryCount,
-                     base::Unretained(this)),
-          &request_tracker_);
-    }
-  }
+  interstitial_reason_ =
+      IsErrorDueToBadClock(base::Time::NowFromSystemTime(), cert_error_) ?
+      SSL_REASON_BAD_CLOCK : SSL_REASON_SSL;
+
+  uma_helper_.reset(new SecurityInterstitialUmaHelper(

[Alexei Svitkine (slow), 2015/01/14 22:12:33]

Why is it a scoped_ptr and not a member? Same for the other one?

[felt, 2015/01/15 06:47:22]

It needs to be done after calculating |interstitial_reason_|. I've added a
comment to that effect.

+      web_contents, request_url, GetHistogramPrefix(), GetSamplingEventName()));
+  uma_helper_->RecordUserDecision(SecurityInterstitialUmaHelper::SHOW);
+  uma_helper_->RecordUserInteraction(
+      SecurityInterstitialUmaHelper::TOTAL_VISITS);
 
   ssl_error_classification_.reset(new SSLErrorClassification(
       web_contents,
@@ -367,26 +248,10 @@ SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents,
       cert_error_,
       *ssl_info_.cert.get()));
   ssl_error_classification_->RecordUMAStatistics(overridable_);
-
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
   ssl_error_classification_->RecordCaptivePortalUMAStatistics(overridable_);
 #endif
 
-#if defined(ENABLE_EXTENSIONS)
-  // ExperienceSampling: Set up new sampling event for this interstitial.
-  std::string event_name(kEventNameBase);
-  if (overridable_ && !strict_enforcement_)
-    event_name.append(kEventOverridable);
-  else
-    event_name.append(kEventNotOverridable);
-  event_name.append(net::ErrorToString(cert_error_));
-  sampling_event_.reset(new ExperienceSamplingEvent(
-      event_name,
-      request_url,
-      web_contents->GetLastCommittedURL(),
-      web_contents->GetBrowserContext()));
-#endif
-
   // Creating an interstitial without showing (e.g. from chrome://interstitials)
   // it leaks memory, so don't create it here.
 }
@@ -419,14 +284,12 @@ SSLBlockingPage::~SSLBlockingPage() {
       break;
   }
   if (!callback_.is_null()) {
-    RecordSSLBlockingPageDetailedStats(false,
-                                       cert_error_,
-                                       overridable_,
-                                       internal_,
-                                       num_visits_,
-                                       expired_but_previously_allowed_);
     // The page is closed without the user having chosen what to do, default to
     // deny.
+    uma_helper_->RecordUserDecision(
+        SecurityInterstitialUmaHelper::DONT_PROCEED);
+    RecordSSLExpirationPageEventState(
+        expired_but_previously_allowed_, false, overridable_);
     NotifyDenyCertificate();
   }
 }
@@ -440,8 +303,6 @@ void SSLBlockingPage::PopulateInterstitialStrings(
 
   // Shared UI configuration for all SSL interstitials.
   base::Time now = base::Time::NowFromSystemTime();
-  bool bad_clock = IsErrorDueToBadClock(now, cert_error_);
-
   load_time_data->SetString("errorCode", net::ErrorToString(cert_error_));
   load_time_data->SetString(
       "openDetails",
@@ -451,9 +312,7 @@ void SSLBlockingPage::PopulateInterstitialStrings(
       l10n_util::GetStringUTF16(IDS_SSL_V2_CLOSE_DETAILS_BUTTON));
 
   // Conditional UI configuration.
-  if (bad_clock) {
-    RecordSSLBlockingPageEventStats(DISPLAYED_CLOCK_INTERSTITIAL);
-
+  if (interstitial_reason_ == SSL_REASON_BAD_CLOCK) {
     load_time_data->SetBoolean("bad_clock", true);
     load_time_data->SetBoolean("overridable", false);
 
@@ -611,30 +470,29 @@ void SSLBlockingPage::CommandReceived(const std::string& command) {
       break;
     }
     case CMD_MORE: {
-      RecordSSLBlockingPageEventStats(MORE);
-#if defined(ENABLE_EXTENSIONS)
-      if (sampling_event_.get())
-        sampling_event_->set_has_viewed_details(true);
-#endif
+      uma_helper_->RecordUserInteraction(
+          SecurityInterstitialUmaHelper::SHOW_ADVANCED);
       break;
     }
     case CMD_RELOAD: {
+      uma_helper_->RecordUserInteraction(
+          SecurityInterstitialUmaHelper::RELOAD);
       // The interstitial can't refresh itself.
       web_contents()->GetController().Reload(true);
       break;
     }
     case CMD_HELP: {
+      uma_helper_->RecordUserInteraction(
+          SecurityInterstitialUmaHelper::SHOW_LEARN_MORE);
       content::NavigationController::LoadURLParams help_page_params(
           google_util::AppendGoogleLocaleParam(
               GURL(kHelpURL), g_browser_process->GetApplicationLocale()));
-#if defined(ENABLE_EXTENSIONS)
-      if (sampling_event_.get())
-        sampling_event_->set_has_viewed_learn_more(true);
-#endif
       web_contents()->GetController().LoadURLWithParams(help_page_params);
       break;
     }
     case CMD_CLOCK: {
+      uma_helper_->RecordUserInteraction(
+          SecurityInterstitialUmaHelper::OPEN_TIME_SETTINGS);
       LaunchDateAndTimeSettings();
       break;
     }
@@ -653,35 +511,17 @@ void SSLBlockingPage::OverrideRendererPrefs(
 }
 
 void SSLBlockingPage::OnProceed() {
-  RecordSSLBlockingPageDetailedStats(true,
-                                     cert_error_,
-                                     overridable_,
-                                     internal_,
-                                     num_visits_,
-                                     expired_but_previously_allowed_);
-#if defined(ENABLE_EXTENSIONS)
-  // ExperienceSampling: Notify that user decided to proceed.
-  if (sampling_event_.get())
-    sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
-#endif
-
+  uma_helper_->RecordUserDecision(SecurityInterstitialUmaHelper::PROCEED);
+  RecordSSLExpirationPageEventState(
+      expired_but_previously_allowed_, true, overridable_);
   // Accepting the certificate resumes the loading of the page.
   NotifyAllowCertificate();
 }
 
 void SSLBlockingPage::OnDontProceed() {
-  RecordSSLBlockingPageDetailedStats(false,
-                                     cert_error_,
-                                     overridable_,
-                                     internal_,
-                                     num_visits_,
-                                     expired_but_previously_allowed_);
-#if defined(ENABLE_EXTENSIONS)
-  // ExperienceSampling: Notify that user decided to not proceed.
-  // This also occurs if the user navigates away or closes the tab.
-  if (sampling_event_.get())
-    sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kDeny);
-#endif
+  uma_helper_->RecordUserDecision(SecurityInterstitialUmaHelper::DONT_PROCEED);
+  RecordSSLExpirationPageEventState(
+      expired_but_previously_allowed_, false, overridable_);
   NotifyDenyCertificate();
 }
 
@@ -703,21 +543,27 @@ void SSLBlockingPage::NotifyAllowCertificate() {
   callback_.Reset();
 }
 
-// static
-void SSLBlockingPage::SetExtraInfo(
-    base::DictionaryValue* strings,
-    const std::vector<base::string16>& extra_info) {
-  DCHECK_LT(extra_info.size(), 5U);  // We allow 5 paragraphs max.
-  const char* keys[5] = {
-      "moreInfo1", "moreInfo2", "moreInfo3", "moreInfo4", "moreInfo5"
-  };
-  int i;
-  for (i = 0; i < static_cast<int>(extra_info.size()); i++) {

[Alexei Svitkine (slow), 2015/01/14 22:12:33]

Why not use size_t for i?

[felt, 2015/01/15 06:47:22]

This code is being deleted

-    strings->SetString(keys[i], extra_info[i]);
-  }
-  for (; i < 5; i++) {

[Alexei Svitkine (slow), 2015/01/14 22:12:33]

Use arraysize() macro.

[felt, 2015/01/15 06:47:22]

This code is being deleted

-    strings->SetString(keys[i], std::string());
+std::string SSLBlockingPage::GetHistogramPrefix() const {
+  switch (interstitial_reason_) {
+    case SSL_REASON_SSL:
+      if (overridable_)
+        return "ssl_overridable";
+      else
+        return "ssl_nonoverridable";
+    case SSL_REASON_BAD_CLOCK:
+      return "bad_clock";
   }
+  NOTREACHED();
+}
+
+std::string SSLBlockingPage::GetSamplingEventName() const {
+  std::string event_name(kEventNameBase);
+  if (overridable_)
+    event_name.append(kEventOverridable);
+  else
+    event_name.append(kEventNotOverridable);
+  event_name.append(net::ErrorToString(cert_error_));
+  return event_name;
 }
 
 // static
@@ -725,9 +571,3 @@ bool SSLBlockingPage::IsOptionsOverridable(int options_mask) {
   return (options_mask & SSLBlockingPage::OVERRIDABLE) &&
          !(options_mask & SSLBlockingPage::STRICT_ENFORCEMENT);
 }
-
-void SSLBlockingPage::OnGotHistoryCount(bool success,
-                                        int num_visits,
-                                        base::Time first_visit) {
-  num_visits_ = num_visits;
-}