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..06c9e2f3637d4e01eb300edd1748a3dbf4051e38 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -257,6 +257,28 @@ void DownloadItemView::OnExtractIconComplete(gfx::Image* icon_bitmap) {
     shelf_->SchedulePaint();
 }
 
+bool DownloadItemView::ShouldAllowDownloadFeedback() {
+  DCHECK(shelf_);
+  return model_.MightBeMalicious() && model_.ShouldAllowDownloadFeedback() &&
+         !shelf_->browser()->profile()->IsOffTheRecord();
+}
+
+void DownloadItemView::MaybeSubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command) {
+  PrefService* prefs = shelf_->browser()->profile()->GetPrefs();
+  if (!safe_browsing::ExtendedReportingPrefExists(*prefs)) {

[Peter Kasting, 2016/10/24 21:39:07]

Nit: Remove ! and reverse conditional arms, so the "else" doesn't mentally read
like a double-negative.

[Jialiu Lin, 2016/10/25 21:07:09]

Done.

+    // Show dialog, because the dialog hasn't been shown before.
+    DownloadFeedbackDialogView::Show(
+        shelf_->get_parent()->GetNativeWindow(), shelf_->browser()->profile(),
+        shelf_->GetNavigator(),
+        base::Bind(&DownloadItemView::PossiblySubmitDownloadToFeedbackService,
+                   weak_ptr_factory_.GetWeakPtr(), download_command));
+  } else {
+    PossiblySubmitDownloadToFeedbackService(
+        download_command, safe_browsing::IsExtendedReportingEnabled(*prefs));
+  }
+}
+
 // DownloadObserver interface.
 
 // Update the progress graphic on the icon and our text status label
@@ -572,23 +594,11 @@ void DownloadItemView::ButtonPressed(views::Button* sender,
   // WARNING: all end states after this point delete |this|.
   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;
+  if (ShouldAllowDownloadFeedback()) {

[Peter Kasting, 2016/10/24 21:39:07]

Nit: No {}

[Jialiu Lin, 2016/10/25 21:07:09]

Done.

+    MaybeSubmitDownloadToFeedbackService(DownloadCommands::DISCARD);
+  } else {
+    download()->Remove();
   }
-  download()->Remove();
 }
 
 SkColor DownloadItemView::GetVectorIconBaseColor() const {
@@ -773,8 +783,11 @@ bool DownloadItemView::SubmitDownloadToFeedbackService() {
 #endif
 }
 
-void DownloadItemView::PossiblySubmitDownloadToFeedbackService(bool enabled) {
-  if (!enabled || !SubmitDownloadToFeedbackService())
+void DownloadItemView::PossiblySubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command_id,
+    bool enabled) {
+  if ((!enabled || !SubmitDownloadToFeedbackService()) &&

[Peter Kasting, 2016/10/24 21:39:07]

Doesn't this result in deleting the download when submission succeeds, even if
the command was KEEP?  Is that a problem?

[asanka, 2016/10/25 20:51:13]

Yes. The StealDangerousDownload logic is meant to acquire a dangerous download
after the user has chosen to discard it.

Even if we didn't delete the file, we can't run the upload logic after accepting
a dangerous download. :-( We expose the file to external consumers at that
point. The upload logic may interfere with applications that are trying to
access the download file and prevent the user from opening the file
successfully. 

[Jialiu Lin, 2016/10/25 21:07:09]

oops, Thanks for catching this. Seems changes need to be made to more files.

+      download_command_id == DownloadCommands::DISCARD)

[Peter Kasting, 2016/10/24 21:39:07]

Nit: This might be clearer if written like:

  if (feedback_enabled && SubmitDownloadToFeedbackService())
    return;  // WARNING: |this| is now deleted.

  if (download_command_id == DownloadCommands::DISCARD)
    download()->Remove();

[Jialiu Lin, 2016/10/25 21:07:09]

Done.

     download()->Remove();
   // 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()));