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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // 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"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/constrained_window/constrained_window_views.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_danger_type.h"
 #include "content/public/browser/download_item.h"
 #include "ui/base/l10n/l10n_util.h"
@@ skipping 18 matching lines @@
 // formatting on the text in the body of the dialog.
 class DownloadDangerPromptViews : public DownloadDangerPrompt,
                                   public content::DownloadItem::Observer,
                                   public views::DialogDelegate {
  public:
   DownloadDangerPromptViews(content::DownloadItem* item,
                             bool show_context,
                             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;
   base::string16 GetWindowTitle() const override;
   void DeleteDelegate() override;
   ui::ModalType GetModalType() const override;
   bool Cancel() override;
   bool Accept() override;
   bool Close() override;
   views::View* GetInitiallyFocusedView() override;
@@ skipping 70 matching lines @@
   // ExperienceSampling: A malicious download warning is being shown to the
   // user, so we start a new SamplingEvent and track it.
   sampling_event_.reset(new ExperienceSamplingEvent(
       ExperienceSamplingEvent::kDownloadDangerPrompt,
       item->GetURL(),
       item->GetReferrerUrl(),
       item->GetBrowserContext()));
 }
 
 // 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();
       break;
 
     case CANCEL:
     case DISMISS:
       Cancel();
       break;
 
     default:
       NOTREACHED();
       break;
   }
+
+  return CreateSafeBrowsingDownloadRecoveryReport(action == ACCEPT,
+                                                  testing_url);
 }
 
 // views::DialogDelegate methods:
 base::string16 DownloadDangerPromptViews::GetDialogButtonLabel(
     ui::DialogButton button) const {
   switch (button) {
     case ui::DIALOG_BUTTON_OK:
       return GetAcceptButtonTitle();
 
     case ui::DIALOG_BUTTON_CANCEL:
@@ skipping 160 matching lines @@
       default: {
         return l10n_util::GetStringUTF16(
             IDS_PROMPT_CONFIRM_KEEP_DANGEROUS_DOWNLOAD);
       }
     }
   }
   NOTREACHED();
   return base::string16();
 }
 
 void DownloadDangerPromptViews::RunDone(Action action) {

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

Why does this have the same exact impl as DownloadDangerPromptImpl::RunDone?
That seems very strange to me...

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

Very hard to move this function to parent class, unfortunately.
Similar to the above reason, the Callback typed defined as OnDone in parent
class, but are actually different things in these two subclasses. Also parent
class does not implement content::DownloadItem::Observer, so cannot call
download_->RemoveObserver(this). 

   // Invoking the callback can cause the download item state to change or cause
   // the window to close, and |callback| refers to a member variable.
   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;
   }
   if (!done.is_null())
     done.Run(action);
 }
 
 }  // namespace
 
 DownloadDangerPrompt* DownloadDangerPrompt::Create(
     content::DownloadItem* item,
     content::WebContents* web_contents,
     bool show_context,
     const OnDone& done) {
   DownloadDangerPromptViews* download_danger_prompt =
       new DownloadDangerPromptViews(item, show_context, done);
   constrained_window::ShowWebModalDialogViews(download_danger_prompt,
                                               web_contents);
   return download_danger_prompt;
 }