Add Rappor metrics for Safe Browsing and SSL interstitials.

chrome/browser/interstitials/security_interstitial_uma_helper.cc

Index: chrome/browser/interstitials/security_interstitial_uma_helper.cc
diff --git a/chrome/browser/interstitials/security_interstitial_uma_helper.cc b/chrome/browser/interstitials/security_interstitial_uma_helper.cc
index aade7ff796f99560eac7d2bc1e126bfab6720f75..cba936fd41db970578209d34fd04bd9e2e3fdf06 100644
--- a/chrome/browser/interstitials/security_interstitial_uma_helper.cc
+++ b/chrome/browser/interstitials/security_interstitial_uma_helper.cc
@@ -2,14 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+
 #include "chrome/browser/interstitials/security_interstitial_uma_helper.h"
 
+#include <string>
+
 #include "base/metrics/histogram.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
+#include "components/rappor/rappor_service.h"
 #include "content/public/browser/web_contents.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h"
@@ -18,11 +24,13 @@
 SecurityInterstitialUmaHelper::SecurityInterstitialUmaHelper(
     content::WebContents* web_contents,
     const GURL& request_url,
-    const std::string& histogram_prefix,
+    const std::string& uma_prefix,
+    const std::string& rappor_prefix,
     const std::string& sampling_event_name)
     : web_contents_(web_contents),
       request_url_(request_url),
-      histogram_prefix_(histogram_prefix),
+      uma_prefix_(uma_prefix),
+      rappor_prefix_(rappor_prefix),
       sampling_event_name_(sampling_event_name),
       num_visits_(-1) {
   HistoryService* history_service = HistoryServiceFactory::GetForProfile(
@@ -40,18 +48,38 @@ SecurityInterstitialUmaHelper::SecurityInterstitialUmaHelper(
 SecurityInterstitialUmaHelper::~SecurityInterstitialUmaHelper() {
 }
 
-// Directly adds to the histograms, using the same properties as
+// Directly adds to the UMA histograms, using the same properties as
 // UMA_HISTOGRAM_ENUMERATION, because the macro doesn't allow non-constant
-// histogram names.
+// histogram names.  Reports to Rappor for certain decisions.
 void SecurityInterstitialUmaHelper::RecordUserDecision(
     SecurityInterstitialDecision decision) {
+  // UMA
   std::string decision_histogram_name(
-      "interstitial." + histogram_prefix_ + ".decision");
+      "interstitial." + uma_prefix_ + ".decision");
   base::HistogramBase* decision_histogram = base::LinearHistogram::FactoryGet(
       decision_histogram_name, 1, MAX_DECISION, MAX_DECISION + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   decision_histogram->Add(decision);
 
+  // Rappor
+  rappor::RapporService* rappor_service = g_browser_process->rappor_service();
+  if (rappor_service && !rappor_prefix_.empty()) {
+    if (decision == SHOW) {

[felt, 2015/01/28 19:19:17]

nit: can you make this a single if-statement until the TODO below is resolved,
at which point you can add a switch statement?

[Nathan Parker, 2015/01/29 01:36:02]

Done.

+      const std::string domain =
+          net::registry_controlled_domains::GetDomainAndRegistry(
+              request_url_,
+              net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+
+      // e.g. "Interstitial.Malware.Domain"
+      const std::string metric_name =
+          "Interstitial." + rappor_prefix_ + ".Domain";

[felt, 2015/01/28 19:19:17]

Do we really need to have both uma_prefix and rappor_prefix?

The way uma is put together is by concatenating "interstitial." + "phishing".
Couldn't you just use the same here?

[Nathan Parker, 2015/01/29 01:36:02]

Unfortunately they have different capitalization.  We could toupper(prefix[0]),
but  that wouldn't properly camel-case any future prefixes w/ multiple words. 
I'm ok with punting on that problem for now if you like, to simplify the code.

[Steven Holte, 2015/01/29 02:46:12]

I would lean towards being consistent with the related histograms over being
consistent with other Rappor metrics.

[Nathan Parker, 2015/01/30 00:39:51]

In that case I'll user lowercase for both.  I'll update it in rappor.xml as
well.

Talking with felt@ and lgarron@, we agreed that SSL interstitials would be very
useful as well (e.g. if we see spike on dropbox.com, that's info we and they
don't have now).  I enabled that in this CL, and it additionaly requires
separating UMA/Rappor prefixes since we don't want Rappor metrics named
"ssl_overridable" and "ssl_nonoverridable" (that'd leak info).  So we can't keep
UMA and Rappor namespaces identical, but they're close.

+      rappor_service->RecordSample(metric_name, rappor::COARSE_RAPPOR_TYPE,
+                                   domain);
+    }
+    // TODO(nparker): Add Rappor reporting at PROCEED and DONT_PROCEED once
+    // crbug.com/451647 is fixed.
+  }
+
 #if defined(ENABLE_EXTENSIONS)
   if (!sampling_event_.get()) {
     sampling_event_.reset(new extensions::ExperienceSamplingEvent(
@@ -81,7 +109,7 @@ void SecurityInterstitialUmaHelper::RecordUserDecision(
   if (num_visits_ < 1 || (decision != PROCEED && decision != DONT_PROCEED))
     return;
   std::string history_histogram_name(
-      "interstitial." + histogram_prefix_ + ".decision.repeat_visit");
+      "interstitial." + uma_prefix_ + ".decision.repeat_visit");
   base::HistogramBase* history_histogram = base::LinearHistogram::FactoryGet(
       history_histogram_name, 1, MAX_DECISION, MAX_DECISION + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
@@ -92,7 +120,7 @@ void SecurityInterstitialUmaHelper::RecordUserDecision(
 void SecurityInterstitialUmaHelper::RecordUserInteraction(
     SecurityInterstitialInteraction interaction) {
   std::string interaction_histogram_name(
-      "interstitial." + histogram_prefix_ + ".interaction");
+      "interstitial." + uma_prefix_ + ".interaction");
   base::HistogramBase* interaction_histogram =
       base::LinearHistogram::FactoryGet(
           interaction_histogram_name, 1, MAX_INTERACTION, MAX_INTERACTION + 1,