Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads

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

Index: chrome/browser/ui/views/download/download_danger_prompt_views.cc
diff --git a/chrome/browser/ui/views/download/download_danger_prompt_views.cc b/chrome/browser/ui/views/download/download_danger_prompt_views.cc
index ef7bbfbb06f7c2abbb1c223fa115c6cfe3418899..70e729bcc926b2760caabfb4d685584312351ebb 100644
--- a/chrome/browser/ui/views/download/download_danger_prompt_views.cc
+++ b/chrome/browser/ui/views/download/download_danger_prompt_views.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/compiler_specific.h"
+

[msw, 2015/11/13 19:01:28]

Remove this blank line or just put download_danger_prompt.h up top.

[Jialiu Lin, 2015/11/13 23:47:01]

oops, sorry

 #include "chrome/browser/download/download_danger_prompt.h"
 #include "chrome/browser/download/download_stats.h"
 #include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h"
@@ -41,7 +42,8 @@ class DownloadDangerPromptViews : public DownloadDangerPrompt,
                             const OnDone& done);
 
   // DownloadDangerPrompt methods:
-  void InvokeActionForTesting(Action action) override;
+  std::string InvokeActionForTesting(Action action,
+                                     const GURL& testing_url) override;

[msw, 2015/11/13 19:01:28]

nit: use |url| here and below to match the base class' identifier.

[Jialiu Lin, 2015/11/13 23:47:01]

Done.

 
   // views::DialogDelegate methods:
   base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
@@ -132,7 +134,9 @@ DownloadDangerPromptViews::DownloadDangerPromptViews(
 }
 
 // DownloadDangerPrompt methods:
-void DownloadDangerPromptViews::InvokeActionForTesting(Action action) {
+std::string DownloadDangerPromptViews::InvokeActionForTesting(

[msw, 2015/11/13 19:01:28]

This looks like a near-copy of DownloadDangerPromptImpl::InvokeActionForTesting,
with different handling of dismiss... I think this code needs a cleanup pass.

[Jialiu Lin, 2015/11/13 23:47:00]

They looks like the same, however, the Accept() in DownloadDangerPromptViews is
actually calling views::DialogDelegate::Accept()
And the Accept() in DownloadDangerPromptImpt is calling
TabModalConfirmDialogDelegate::Accept()

Similarly, OnDownloadUpdated() though look exactly the same, they are actually
calling different things. 

[msw, 2015/11/17 01:17:37]

This is troubling... someone more familiar here should review this CL and audit
this area; there ought to be a better pattern that doesn't 'duplicate' so much
code. That said, this CL doesn't cause that mess; deferring cleanup may be okay.

[msw, 2015/11/17 18:54:40]

My approval is gated on someone familiar with this area reviewing this CL.

[Jialiu Lin, 2015/11/17 19:13:53]

asanka (already in reviewer list) would be the expert who is very familiar with
this part of code. 

+    Action action,
+    const GURL& testing_url) {
   switch (action) {
     case ACCEPT:
       Accept();
@@ -147,6 +151,9 @@ void DownloadDangerPromptViews::InvokeActionForTesting(Action action) {
       NOTREACHED();
       break;
   }
+
+  return CreateSafeBrowsingDownloadRecoveryReport(action == ACCEPT,
+                                                  testing_url);
 }
 
 // views::DialogDelegate methods:
@@ -333,6 +340,12 @@ void DownloadDangerPromptViews::RunDone(Action action) {
   OnDone done = done_;
   done_.Reset();
   if (download_ != NULL) {
+    if (download_->GetURL() != GURL::EmptyGURL()) {

[msw, 2015/11/13 19:01:28]

nit: use GURL::is_empty()

[Jialiu Lin, 2015/11/13 23:47:00]

Done.

+      std::string report = CreateSafeBrowsingDownloadRecoveryReport(
+          action == DownloadDangerPrompt::ACCEPT, download_->GetURL());
+      if (!report.empty())
+        SendSerializedReport(report);
+    }
     download_->RemoveObserver(this);
     download_ = NULL;
   }