DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

content/browser/download/download_manager_impl.h

Index: content/browser/download/download_manager_impl.h
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index d020c553a886a3c8a528f48c6070e261ae72d385..50a9be888bc6b4ad9cb9197d51c249ca0b7fc46d 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -18,6 +18,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/synchronization/lock.h"
+#include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_status_updater_delegate.h"
 #include "content/common/content_export.h"
 
@@ -26,6 +27,7 @@ class DownloadStatusUpdater;
 
 class CONTENT_EXPORT DownloadManagerImpl
     : public DownloadManager,
+      public DownloadItemImpl::Delegate,

[cbentzel, 2011/11/29 21:30:44]

I think this is additional argument that the Delegate should be nested in
DownloadItem rather than DownloadItemImpl. It might be nice eventually to have
the DownloadManagerImpl work with mock DownloadItem's, as well as for
DownloadItemImpl to work with mock delegates.

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

As per our offline conversation and my comments elsewhere, leaving it on
DownloadItemImpl.

       public DownloadStatusUpdaterDelegate {
  public:
   DownloadManagerImpl(content::DownloadManagerDelegate* delegate,
@@ -48,9 +50,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual void CancelDownload(int32 download_id) OVERRIDE;
   virtual void OnDownloadInterrupted(int32 download_id, int64 size,
                                      InterruptReason reason) OVERRIDE;
-  virtual void DownloadCancelledInternal(DownloadItem* download) OVERRIDE;
-  virtual void RemoveDownload(int64 download_handle) OVERRIDE;
-  virtual bool IsDownloadReadyForCompletion(DownloadItem* download) OVERRIDE;
   virtual void MaybeCompleteDownload(DownloadItem* download) OVERRIDE;
   virtual void OnDownloadRenamedToFinalName(int download_id,
                                             const FilePath& full_path,
@@ -59,7 +58,6 @@ class CONTENT_EXPORT DownloadManagerImpl
                                      const base::Time remove_end) OVERRIDE;
   virtual int RemoveDownloads(const base::Time remove_begin) OVERRIDE;
   virtual int RemoveAllDownloads() OVERRIDE;
-  virtual void DownloadCompleted(int32 download_id) OVERRIDE;
   virtual void DownloadUrl(const GURL& url,
                            const GURL& referrer,
                            const std::string& referrer_encoding,
@@ -75,29 +73,41 @@ class CONTENT_EXPORT DownloadManagerImpl
       std::vector<DownloadPersistentStoreInfo>* entries) OVERRIDE;
   virtual void OnItemAddedToPersistentStore(int32 download_id,
                                             int64 db_handle) OVERRIDE;
-  virtual void ShowDownloadInBrowser(DownloadItem* download) OVERRIDE;
   virtual int InProgressCount() const OVERRIDE;
-  virtual content::BrowserContext* BrowserContext() OVERRIDE;
+  virtual content::BrowserContext* BrowserContext() const OVERRIDE;
   virtual FilePath LastDownloadPath() OVERRIDE;
   virtual void CreateDownloadItem(
       DownloadCreateInfo* info,
       const DownloadRequestHandle& request_handle) OVERRIDE;
+  virtual DownloadItem* CreateSavePackageDownloadItem(
+      const FilePath& main_file_path,
+      const GURL& page_url,
+      bool is_otr,
+      DownloadItem::Observer* observer) OVERRIDE;
   virtual void ClearLastDownloadPath() OVERRIDE;
   virtual void FileSelected(const FilePath& path, void* params) OVERRIDE;
   virtual void FileSelectionCanceled(void* params) OVERRIDE;
   virtual void RestartDownload(int32 download_id) OVERRIDE;
-  virtual void MarkDownloadOpened(DownloadItem* download) OVERRIDE;
   virtual void CheckForHistoryFilesRemoval() OVERRIDE;
-  virtual void CheckForFileRemoval(DownloadItem* download_item) OVERRIDE;
-  virtual void AssertQueueStateConsistent(DownloadItem* download) OVERRIDE;
   virtual DownloadItem* GetDownloadItem(int id) OVERRIDE;
-  virtual void SavePageDownloadStarted(DownloadItem* download) OVERRIDE;
   virtual void SavePageDownloadFinished(DownloadItem* download) OVERRIDE;
   virtual DownloadItem* GetActiveDownloadItem(int id) OVERRIDE;
   virtual content::DownloadManagerDelegate* delegate() const OVERRIDE;
   virtual void SetDownloadManagerDelegate(
       content::DownloadManagerDelegate* delegate) OVERRIDE;
-  virtual DownloadId GetNextId() OVERRIDE;
+
+  // Overridden from DownloadItemImpl::Delegate
+  // (Note that |MaybeCompleteDownload| and |BrowserContext| are
+  // present in both interfaces.)

[cbentzel, 2011/11/29 21:30:44]

Ooh. Should that change? I suppose both will go to the same implementation. 

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

I agree with you it's not ideal, but I couldn't see a way to change
BrowserContext().  I *do* see a way to change MaybeCompleteDownload (shift the
interface over to being on the DownloadItemImpl only, so that the
DownloadManager doesn't need to export the thing).  I'll try to do that.  

BrowserContext() is needed in the delegate interface so that the DownloadItem
can figure out what BrowserContext it's on (which it needs as a substitute for
figuring out what DownloadManager it's on, which I no longer want it to know
about).  BrowserContext() is needed in the DownloadManager interface for some
operations in the DownloadsDOMHandler around file icons and translating back and
forth between incognito and regular profiles.  I'm sure there are better ways to
handle all those things (or at least the incognito/regular translation ...
stuff) but they aren't part of this CL.

+  virtual bool ShouldOpenDownload(DownloadItem* item) OVERRIDE;
+  virtual bool ShouldOpenFileBasedOnExtension(
+      const FilePath& path) OVERRIDE;
+  virtual void CheckForFileRemoval(DownloadItem* download_item) OVERRIDE;
+  virtual void DownloadCancelled(DownloadItem* download) OVERRIDE;
+  virtual void DownloadCompleted(DownloadItem* download) OVERRIDE;
+  virtual void DownloadOpened(DownloadItem* download) OVERRIDE;
+  virtual void DownloadRemoved(DownloadItem* download) OVERRIDE;
+  virtual void AssertStateConsistent(DownloadItem* download) const OVERRIDE;
 
   // Overridden from DownloadStatusUpdaterDelegate:
   virtual bool IsDownloadProgressKnown() const OVERRIDE;
@@ -112,7 +122,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   // For testing.
   friend class DownloadManagerTest;
   friend class DownloadTest;
-  friend class MockDownloadManager;
 
   friend class base::RefCountedThreadSafe<
       DownloadManagerImpl, content::BrowserThread::DeleteOnUIThread>;
@@ -122,6 +131,17 @@ class CONTENT_EXPORT DownloadManagerImpl
 
   virtual ~DownloadManagerImpl();
 
+  // Determine if the download is ready for completion, i.e. has had
+  // all data saved, and completed the filename determination and
+  // history insertion.
+  bool IsDownloadReadyForCompletion(DownloadItem* download);
+
+  // Show the download in the browser.
+  void ShowDownloadInBrowser(DownloadItem* download);
+
+  // Get next download id from factory.
+  DownloadId GetNextId();
+
   // Called on the FILE thread to check the existence of a downloaded file.
   void CheckForFileRemovalOnFileThread(int64 db_handle, const FilePath& path);