Download Feedback Service should upload all eligible downloads

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 f22135871709b79f713d6468c86e07f52fbd6b5f..8b9ff51d8dd1b76baa364096ffbd414b70720c39 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -257,6 +257,29 @@ void DownloadItemView::OnExtractIconComplete(gfx::Image* icon_bitmap) {
     shelf_->SchedulePaint();
 }
 
+void DownloadItemView::MaybeSubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command) {
+  PrefService* prefs = shelf_->browser()->profile()->GetPrefs();
+  if (model_.MightBeMalicious() && model_.ShouldAllowDownloadFeedback() &&
+      !shelf_->browser()->profile()->IsOffTheRecord()) {
+    if (safe_browsing::ExtendedReportingPrefExists(*prefs)) {
+      SubmitDownloadWhenFeedbackServiceEnabled(
+          download_command, safe_browsing::IsExtendedReportingEnabled(*prefs));
+    } else {
+      // Show dialog, because the dialog hasn't been shown before.
+      DownloadFeedbackDialogView::Show(
+          shelf_->get_parent()->GetNativeWindow(), shelf_->browser()->profile(),
+          shelf_->GetNavigator(),
+          base::Bind(
+              &DownloadItemView::SubmitDownloadWhenFeedbackServiceEnabled,
+              weak_ptr_factory_.GetWeakPtr(), download_command));
+    }
+  } else {
+    DownloadCommands command(download());
+    command.ExecuteCommand(download_command);
+  }
+}
+
 // DownloadObserver interface.
 
 // Update the progress graphic on the icon and our text status label
@@ -572,23 +595,7 @@ void DownloadItemView::ButtonPressed(views::Button* sender,
   // WARNING: all end states after this point delete |this|.

[Peter Kasting, 2016/11/01 23:48:32]

I think this lies; when MaybeSubmit...() calls
DownloadFeedbackDialogView::Show(), that doesn't delete |this|, does it?

In either case I'd move this to be at the end of the function like the comment
in SubmitDownloadWhenFeedbackServiceEnabled(), but be careful that the wording
is accurate.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

   DCHECK_EQ(discard_button_, sender);
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", warning_duration);
-  Profile* profile = shelf_->browser()->profile();
-  if (!model_.IsMalicious() && model_.ShouldAllowDownloadFeedback() &&
-      !profile->IsOffTheRecord()) {
-    if (!safe_browsing::ExtendedReportingPrefExists(*profile->GetPrefs())) {
-      // Show dialog, because the dialog hasn't been shown before.
-      DownloadFeedbackDialogView::Show(
-          shelf_->get_parent()->GetNativeWindow(), profile,
-          shelf_->GetNavigator(),
-          base::Bind(&DownloadItemView::PossiblySubmitDownloadToFeedbackService,
-                     weak_ptr_factory_.GetWeakPtr()));
-    } else {
-      PossiblySubmitDownloadToFeedbackService(
-          safe_browsing::IsExtendedReportingEnabled(*profile->GetPrefs()));
-    }
-    return;
-  }
-  download()->Remove();
+  MaybeSubmitDownloadToFeedbackService(DownloadCommands::DISCARD);
 }
 
 SkColor DownloadItemView::GetVectorIconBaseColor() const {
@@ -753,7 +760,8 @@ void DownloadItemView::OpenDownload() {
   download()->OpenDownload();
 }
 
-bool DownloadItemView::SubmitDownloadToFeedbackService() {
+bool DownloadItemView::SubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command) {
 #if defined(FULL_SAFE_BROWSING)
   safe_browsing::SafeBrowsingService* sb_service =
       g_browser_process->safe_browsing_service();
@@ -764,7 +772,7 @@ bool DownloadItemView::SubmitDownloadToFeedbackService() {
   if (!download_protection_service)
     return false;
   download_protection_service->feedback_service()->BeginFeedbackForDownload(
-      download());
+      download(), download_command);
   // WARNING: we are deleted at this point.  Don't access 'this'.
   return true;
 #else
@@ -773,9 +781,14 @@ bool DownloadItemView::SubmitDownloadToFeedbackService() {
 #endif
 }
 
-void DownloadItemView::PossiblySubmitDownloadToFeedbackService(bool enabled) {
-  if (!enabled || !SubmitDownloadToFeedbackService())
-    download()->Remove();
+void DownloadItemView::SubmitDownloadWhenFeedbackServiceEnabled(
+    DownloadCommands::Command download_command,
+    bool feedback_enabled) {
+  if (feedback_enabled && SubmitDownloadToFeedbackService(download_command))
+    return;
+
+  DownloadCommands command(download());
+  command.ExecuteCommand(download_command);
   // WARNING: 'this' is deleted at this point. Don't access 'this'.
 }
 
@@ -825,7 +838,7 @@ void DownloadItemView::ShowContextMenuImpl(const gfx::Rect& rect,
       ->SetMouseHandler(nullptr);
 
   if (!context_menu_.get())
-    context_menu_.reset(new DownloadShelfContextMenuView(download()));
+    context_menu_.reset(new DownloadShelfContextMenuView(this));
   context_menu_->Run(GetWidget()->GetTopLevelWidget(), rect, source_type,
                      base::Bind(&DownloadItemView::ReleaseDropdown,
                                 weak_ptr_factory_.GetWeakPtr()));