Experience sampling insturmentation for SSL and Safe Browsing interstitials

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 ca97c2b60a0a90009235bd3716fc0b3b657f2126..84a276feb573fdb8805aff7f0ff88b5e595b7972 100644
--- a/chrome/browser/ssl/ssl_blocking_page.cc
+++ b/chrome/browser/ssl/ssl_blocking_page.cc
@@ -17,6 +17,7 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_preferences_util.h"
@@ -59,6 +60,7 @@ using base::TimeTicks;
 using content::InterstitialPage;
 using content::NavigationController;
 using content::NavigationEntry;
+using extensions::SamplingEvent;
 
 namespace {
 
@@ -278,6 +280,19 @@ SSLBlockingPage::SSLBlockingPage(
                  content::Source<Profile>(profile));
 #endif
 
+  // ExperienceSampling: Set up new sampling event for this interstitial.
+  std::string event_name("ssl_interstitial_");

[felt, 2014/07/18 21:13:59]

these should be constants too

you might even want to add these constants to the experience sampling API class
and then refer to them here, so that there is a single place with all constants

[Chris Thompson, 2014/07/23 22:45:38]

Done. I've put the context-specific strings at the top of each file/context, and
the general ones (such as "proceed", "deny") into the SamplingEvent class.

+  if (overridable_)
+    event_name.append("nonblocking_");

[felt, 2014/07/18 21:13:59]

use "overridable" here to match the boolean; also, you need to check

overridable_ && !strict_enforcement_

(I know, this is messy, I need to refactor at some point to create a single
boolean.)

[Chris Thompson, 2014/07/23 22:45:38]

Done.

+  else
+    event_name.append("blocking_");
+  event_name.append(net::ErrorToString(cert_error_));
+  sampling_event_ = make_scoped_ptr(new SamplingEvent(
+      event_name,
+      request_url_.possibly_invalid_spec(),
+      web_contents_->GetLastCommittedURL().possibly_invalid_spec(),
+      web_contents_->GetBrowserContext()));
+
   interstitial_page_ = InterstitialPage::Create(
       web_contents_, true, request_url, this);
   interstitial_page_->Show();
@@ -294,6 +309,7 @@ SSLBlockingPage::~SSLBlockingPage() {
                                        captive_portal_probe_completed_,
                                        captive_portal_no_response_,
                                        captive_portal_detected_);
+

[felt, 2014/07/18 21:13:59]

don't add spurious blank lines please

[Chris Thompson, 2014/07/23 22:45:38]

Done. Accidentally left this in from previous version of the instrumentation.

     // The page is closed without the user having chosen what to do, default to
     // deny.
     NotifyDenyCertificate();
@@ -639,6 +655,8 @@ void SSLBlockingPage::OnProceed() {
                                      captive_portal_probe_completed_,
                                      captive_portal_no_response_,
                                      captive_portal_detected_);
+  // ExperienceSampling: Notify that user decided to proceed.
+  sampling_event_->CreateUserDecisionEvent("proceed");
   // Accepting the certificate resumes the loading of the page.
   NotifyAllowCertificate();
 }
@@ -653,6 +671,10 @@ void SSLBlockingPage::OnDontProceed() {
                                      captive_portal_probe_completed_,
                                      captive_portal_no_response_,
                                      captive_portal_detected_);
+  // ExperienceSampling: Notify that user decided to not proceed.
+  // This also occurs if the user navigates away or closes the tab.
+  sampling_event_->CreateUserDecisionEvent("deny");
+
   NotifyDenyCertificate();
 }