[Downloads] Ask DownloadHistory if a download was from history.

chrome/browser/download/download_ui_controller.h

Index: chrome/browser/download/download_ui_controller.h
diff --git a/chrome/browser/download/download_ui_controller.h b/chrome/browser/download/download_ui_controller.h
index f4f8de8a7e6a49fac408f70904d7de7fd249db15..9d0d6ab8b8990de12dfc431cd120b8d3a49dad89 100644
--- a/chrome/browser/download/download_ui_controller.h
+++ b/chrome/browser/download/download_ui_controller.h
@@ -7,32 +7,49 @@
 
 #include <set>
 
+#include "base/callback_forward.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/download/all_download_item_notifier.h"
 
 class Profile;
+class DownloadHistory;
 
 namespace content {
 class WebContents;
 }
 
-// This class handles the task of observing for download notifications and
-// notifying the UI when a new download should be displayed in the UI.
+// This class handles the task of observing a single DownloadManager for
+// download notifications and notifying the UI when a new download should be
+// displayed in the UI.
 //
-// This class monitors each IN_PROGRESS download that is created (for which
-// OnDownloadCreated is called) until:
-// - it is assigned a target path or
-// - is interrupted.
-// Then the NotifyDownloadStarting() method of the Delegate is invoked.
+// This class monitors each new download that is created (for which
+// OnDownloadCreated is called) until it is assigned a target path. "New"
+// downloads are distinguished from old downloads using a DownloadFilter (see
+// comments on constructor below). Once a target path is available, the download
+// is sent to the UI via the NotifyDownloadStarting() method of the Delegate.
 class DownloadUIController : public AllDownloadItemNotifier::Observer {
  public:
   // The delegate is responsible for figuring out how to notify the UI.
   class Delegate {
    public:
     virtual ~Delegate();
+
+    // This method is invoked to notify the UI of the new download |item|. Note
+    // that |item| may be in any state by the time this method is invoked.
     virtual void NotifyDownloadStarting(content::DownloadItem* item) = 0;
   };
 
+  // Predicate for determining whether a given download should be considered a
+  // new download. Only new downloads are considered for display in the UI.
+  typedef base::Callback<bool(const content::DownloadItem*)> DownloadFilter;
+
+  // Construct a DownloadFilter based on a DownloadHistory instance. The
+  // returned predicate assumes that if a given DownloadItem is persisted in
+  // history, then it is not new. The lifetime of the |download_history| object
+  // should larger than the returned predicate.
+  static DownloadFilter NewDownloadFilterFromDownloadHistory(
+      DownloadHistory* download_history);

[Randy Smith (Not in Mondays), 2014/04/10 18:03:23]

I dislike cluttering up class interfaces for testing purposes (which is what I
think this is; is that right?).  Given that the interface needs to "know" about
DownloadHistory, I'd think it would make more sense to just pass a
DownloadHistory argument to the constructor and skip the extra mechanism.  Did
you explore how hard it would be to mock DownloadHistory to allow tests to
control this bit?

I'm willing to go with your preference on this--the test code will be more
complicated and messy with my suggested change, and arguably all the interface
wants is a way to tell whether or not the incoming download is one it needs to
pay attention to.  But the interface would be simpler without the filter, and if
we take the "all the interface wants is a way to tell whether or not the
incoming download is one it needs to pay attention to" we should replace the
DownloadManager with some kind of download item source that has the filter
embedded in it.  Which seems like a lot of work for just this use case.

[asanka, 2014/04/17 21:16:10]

I refactored this. PTAL.

+
   // |manager| is the download manager to observe for new downloads. If
   // |delegate.get()| is NULL, then the default delegate is constructed.
   //
@@ -40,7 +57,11 @@ class DownloadUIController : public AllDownloadItemNotifier::Observer {
   // other platforms the target of the notification is a Browser object.
   //
   // Currently explicit delegates are only used for testing.
+  //
+  // |new_download_filter| is a DownloadFilter that should return true if a
+  // newly created download should be displayed in the UI.
   DownloadUIController(content::DownloadManager* manager,
+                       const DownloadFilter& new_download_filter,
                        scoped_ptr<Delegate> delegate);
 
   virtual ~DownloadUIController();
@@ -53,6 +74,7 @@ class DownloadUIController : public AllDownloadItemNotifier::Observer {
 
   AllDownloadItemNotifier download_notifier_;
 
+  DownloadFilter new_download_filter_;
   scoped_ptr<Delegate> delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadUIController);