Experience sampling instrumentation for dangerous downloads warnings

chrome/browser/ui/views/download/download_item_view.cc

Index: chrome/browser/ui/views/download/download_item_view.cc
diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc
index d1f91a1e62d79c50c789a8be8cc63a18a74b932b..d62f89ec6cdb59fe9151c7834bec7dae080a1524 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_stats.h"
 #include "chrome/browser/download/drag_download_item.h"
+#include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/download_feedback_service.h"
 #include "chrome/browser/safe_browsing/download_protection_service.h"
@@ -54,6 +55,7 @@
 #include "ui/views/widget/widget.h"
 
 using content::DownloadItem;
+using extensions::ExperienceSamplingEvent;
 
 // TODO(paulg): These may need to be adjusted when download progress
 //              animation is added, and also possibly to take into account
@@ -92,6 +94,10 @@ static const int kDisabledOnOpenDuration = 3000;
 // light-on-dark themes.
 static const double kDownloadItemLuminanceMod = 0.8;
 
+// Constants for the Experience Sampling instrumentation.
+const char kEventNameMalicious[] = "download_warning_malicious";

[msw, 2014/08/13 20:11:37]

These string constants should be shared between Cocoa and Views.
Are we doing the same thing for Android and iOS? Ditto there then.

[Chris Thompson, 2014/08/13 21:18:00]

Done. Moved these to experience_sampling.h.

+const char kEventNameDangerous[] = "download_warning_dangerous";
+
 namespace {
 
 // Callback for DownloadShelf paint functions to mirror the progress animation
@@ -230,6 +236,12 @@ DownloadItemView::DownloadItemView(DownloadItem* download_item,
 DownloadItemView::~DownloadItemView() {
   StopDownloadProgress();
   download()->RemoveObserver(this);
+
+  // Experience Sampling: If the user took no action to remove the warning

[msw, 2014/08/13 20:11:37]

What's the use for this info? Wouldn't allow/deny/ignore be more informative?

[Chris Thompson, 2014/08/13 21:18:00]

Changed this to "ignore".

+  // before it disappeared, then the user effectively dismissed the download
+  // without keeping it.
+  if (mode_ == DANGEROUS_MODE || mode_ == MALICIOUS_MODE)
+    sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kDeny);
 }
 
 // Progress animation handlers.
@@ -1127,6 +1139,9 @@ void DownloadItemView::ClearWarningDialog() {
   body_state_ = NORMAL;
   drop_down_state_ = NORMAL;
 
+  // ExperienceSampling: User proceeded through the warning.

[msw, 2014/08/13 20:11:37]

Use "Experience Sampling" or "ExperienceSampling" consistently everywhere.

[Chris Thompson, 2014/08/13 21:18:00]

Done. s/Experience Sampling/ExperienceSampling/g

+  sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
+
   // Remove the views used by the warning dialog.
   if (save_button_) {
     RemoveChildView(save_button_);
@@ -1170,6 +1185,17 @@ void DownloadItemView::ShowWarningDialog() {
 #endif
   mode_ = model_.MightBeMalicious() ? MALICIOUS_MODE : DANGEROUS_MODE;
 
+  // ExperienceSampling: Dangerous or malicious download warning is being shown
+  // to the user, so we start a new SamplingEvent and track it.
+  std::string event_name = model_.MightBeMalicious() ?
+      kEventNameMalicious
+      : kEventNameDangerous;

[msw, 2014/08/13 20:11:37]

This formatting is incorrect, use "git cl format"
https://code.google.com/p/chromium/wiki/ClangFormat
I'd probably just combine this line with the one above.

[Chris Thompson, 2014/08/13 21:18:00]

Done. Missed this in an edit that shortened the line.

+  sampling_event_.reset(new ExperienceSamplingEvent(

[msw, 2014/08/13 20:11:37]

Yikes, I really hope this has undergone security review...

[Chris Thompson, 2014/08/13 21:18:00]

Do you have particular concerns about this use of scoped_ptr here?

[msw, 2014/08/14 01:48:33]

No, my point was more about reporting url, referrer, and browser context info to
some extension. You whitelisting explanation addressed my security concerns.

+      event_name,
+      download()->GetURL(),
+      download()->GetReferrerUrl(),
+      download()->GetBrowserContext()));
+
   body_state_ = NORMAL;
   drop_down_state_ = NORMAL;
   if (mode_ == DANGEROUS_MODE) {