Download Feedback Service should upload all eligible downloads

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

Index: chrome/browser/ui/views/download/download_item_view.h
diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h
index df9dd8708f42d29a4c420d203be0860f2cbbb2da..f3586461303ec2911050862ce61dac6883f4a660 100644
--- a/chrome/browser/ui/views/download/download_item_view.h
+++ b/chrome/browser/ui/views/download/download_item_view.h
@@ -25,6 +25,7 @@
 #include "base/task/cancelable_task_tracker.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "chrome/browser/download/download_commands.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/icon_manager.h"
 #include "content/public/browser/download_item.h"
@@ -84,6 +85,17 @@ class DownloadItemView : public views::InkDropHostView,
   // Returns the DownloadItem model object belonging to this item.
   content::DownloadItem* download() { return model_.download(); }
 
+  // If download is bad && download feedback feature is available &&
+  // not incognito.
+  bool ShouldAllowDownloadFeedback();

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

Nit: I suggest a comment that states the purpose of the code instead of its
function, and a function name more parallel to the others you use:

  // Returns whether the current download should be sent to the feedback service
  // (if the user approves).
  bool ShouldSubmitDownloadToFeedbackService();

However, note that both callers of this method just want to gate calls to
MaybeSubmitDownloadToFeedbackService() on it.  So consider just rolling it into
that function, and updating the comment I suggest below to "...if the user has
approved and the download is suitable for submission,..."

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

Function merged into MaybeSubmitDownloadToFeedbackService().

+
+  // Submit download to download feedback service if user opts in safe browsing
+  // extended reporting (SBER) service, then perform the specified
+  // |download_command|. If user hasn't seen SBER opt-in text before,
+  // show SBER opt-in dialog first.
+  void MaybeSubmitDownloadToFeedbackService(

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

It's very confusing that you have both MaybeSubmitDownloadToFeedbackService()
and PossiblySubmitDownloadToFeedbackService().

Below I suggest a name change for the latter.  I would also change the comments
on here to soemthing like the following:

  // Sends the current download to the feedback service if the user has
  // approved, then performs the specified |download_command|.
  //
  // User approval comes in the form of a positive response to the
  // DownloadFeedbackDialogView, which this function will first show if it has
  // not been shown before.

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

Good suggestion! I have renamed the second function accordingly.

+      DownloadCommands::Command download_command);
+
   // DownloadItem::Observer methods
   void OnDownloadUpdated(content::DownloadItem* download) override;
   void OnDownloadOpened(content::DownloadItem* download) override;
@@ -147,8 +159,10 @@ class DownloadItemView : public views::InkDropHostView,
   bool SubmitDownloadToFeedbackService();
 
   // If the user has |enabled| uploading, calls SubmitDownloadToFeedbackService.
-  // Otherwise, it simply removes the DownloadItem without uploading.
-  void PossiblySubmitDownloadToFeedbackService(bool enabled);
+  // Otherwise, apply the download command (DISCARD or KEEP) without uploading.
+  void PossiblySubmitDownloadToFeedbackService(

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

Nit: Call this function something more precise.  I suggest
SubmitDownloadWhenFeedbackServiceEnabled().

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

Done.

+      DownloadCommands::Command download_command_id,
+      bool enabled);

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

Nit: I would rename this |feedback_enabled| and put it before |command_id|.

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

Renamed |enabled| to |feedback_enabled|, though I have to keep
|feedback_enabled| at the end of the variable list, since it will take in user
decision callback values from DownloadFeedbackDialogView.

 
   // This function calculates the vertical coordinate to draw the file name text
   // relative to local bounds.