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..d81d2f557dfb183e9f392ab8c9c405b0e8bf1154 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,12 @@ class DownloadItemView : public views::InkDropHostView,
   // Returns the DownloadItem model object belonging to this item.
   content::DownloadItem* download() { return model_.download(); }
 
+  // Submit download to download feedback service if the user has approved and

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

Nit: Verbs in function comments should be descriptive ("Submits") rather than
imperative ("Submit"); see
http://google.github.io/styleguide/cppguide.html#Function_Comments .  (Several
places)

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

Done.

+  // the download is suitable for submission, then apply |download_command|.
+  // If user hasn't seen SBER opt-in text before, show SBER opt-in dialog first.

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

Does this function delete |this|?  If so it should probably say so.  Note that I
don't think this does this in all cases, see comments in .cc file.

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

Agree, I don't think it deletes |this|. Will fix comment in .cc file.

+  void MaybeSubmitDownloadToFeedbackService(
+      DownloadCommands::Command download_command);
+
   // DownloadItem::Observer methods
   void OnDownloadUpdated(content::DownloadItem* download) override;
   void OnDownloadOpened(content::DownloadItem* download) override;
@@ -142,13 +149,16 @@ class DownloadItemView : public views::InkDropHostView,
   void OpenDownload();
 
   // Submits the downloaded file to the safebrowsing download feedback service.
-  // Returns whether submission was successful. On successful submission,
-  // |this| and the DownloadItem will have been deleted.
-  bool SubmitDownloadToFeedbackService();
+  // Returns whether submission was successful. On successful submission, apply
+  // |download_command|, and |this| will have been deleted.

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

This comment doesn't seem to accurately describe what the code does.  It looks
like the code attempts to submit the download for feedback, and if that fails,
it applies the download command.

I think the code and comments here are all more confusing than they need to be
because of the way that trying to submit the download for feedback "takes over"
the download command.  If submitting the download for feedback is a purely async
process that makes a temporary copy of the downloaded file (only when it's small
enough for submission) and always deletes that temporary copy when it's done,
then basically feedback submission becomes decoupled from handling the download
completion at all.

Is that change feasible to make?  Because it would avoid a lot of hassling about
precise comments and architecture for achieving the current design.

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

I agree. If the "make a temp copy" strategy works out, it is much cleaner to use
it for both DISCARD and KEEP. 
However, at this point, I am leaning towards only applying this strategy on
"KEEP" (7% of all download feedback cases). Since a number of things can go
wrong with the temp copy, e.g. temporary file location runs out of space,
download file and the temp file may end up on different storage devices and may
be subject to different size restrictions, etc.
I added a UMA metrics to track the failing rate in this CL too (per asanka@'s
request). So if this strategy looks fine in M56, I'll do a refactor to use temp
copy for both KEEP and DISCARD. 

+  bool SubmitDownloadToFeedbackService(
+      DownloadCommands::Command download_command);
 
   // If the user has |enabled| uploading, calls SubmitDownloadToFeedbackService.
-  // Otherwise, it simply removes the DownloadItem without uploading.
-  void PossiblySubmitDownloadToFeedbackService(bool enabled);
+  // Otherwise, apply |download_command|.
+  void SubmitDownloadWhenFeedbackServiceEnabled(
+      DownloadCommands::Command download_command,
+      bool feedback_enabled);
 
   // This function calculates the vertical coordinate to draw the file name text
   // relative to local bounds.