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(); }
 
+  // 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;
   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 43 matching lines @@
   };
 
   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();
 
   // 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.
   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_