Download Feedback Service should upload all eligible downloads

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

 // Copyright 2015 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.
 //
 // A view that implements one download on the Download shelf.
 // Each DownloadItemView contains an application icon, a text label
 // indicating the download's file name, a text label indicating the
 // download's status (such as the number of bytes downloaded so far)
 // and a button for canceling an in progress download, or opening
 // the completed download.
 //
 // The DownloadItemView lives in the Browser, and has a corresponding
 // DownloadController that receives / writes data which lives in the
 // Renderer.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_ITEM_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_ITEM_VIEW_H_
 
 #include <memory>
 #include <string>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string_util.h"
 #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"
 #include "content/public/browser/download_manager.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/font_list.h"
 #include "ui/views/animation/ink_drop_host_view.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/controls/button/vector_icon_button_delegate.h"
 
@@ skipping 39 matching lines @@
 
   // Returns the base color for text on this download item, based on |theme|.
   static SkColor GetTextColorForThemeProvider(const ui::ThemeProvider* theme);
 
   // IconManager::Client interface.
   void OnExtractIconComplete(gfx::Image* icon);
 
   // 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;
   void OnDownloadDestroyed(content::DownloadItem* download) override;
 
   // Overridden from views::View:
   void Layout() override;
   gfx::Size GetPreferredSize() const override;
   bool OnMousePressed(const ui::MouseEvent& event) override;
   bool OnMouseDragged(const ui::MouseEvent& event) override;
@@ skipping 38 matching lines @@
 
   enum Mode {
     NORMAL_MODE = 0,  // Showing download item.
     DANGEROUS_MODE,   // Displaying the dangerous download warning.
     MALICIOUS_MODE    // Displaying the malicious download warning.
   };
 
   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.
   int GetYForFilenameText() const;
 
   // Painting of various download item bits.
   void DrawStatusText(gfx::Canvas* canvas);
   void DrawFilename(gfx::Canvas* canvas);
   void DrawIcon(gfx::Canvas* canvas);
 
@@ skipping 158 matching lines @@
   std::unique_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
 
   // Method factory used to delay reenabling of the item when opening the
   // downloaded file.
   base::WeakPtrFactory<DownloadItemView> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItemView);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_ITEM_VIEW_H_