DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

content/browser/download/download_manager_impl.h

 // Copyright (c) 2011 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.
 //
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_
 #pragma once
 
 #include "content/browser/download/download_manager.h"
 
 #include <map>
 #include <set>
 
 #include "base/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #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"
 
 class DownloadIdFactory;
 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,
                   DownloadIdFactory* id_factory,
                   DownloadStatusUpdater* status_updater);
 
   // DownloadManager functions.
   virtual void Shutdown() OVERRIDE;
   virtual void GetTemporaryDownloads(const FilePath& dir_path,
                                      DownloadVector* result) OVERRIDE;
   virtual void GetAllDownloads(const FilePath& dir_path,
                                DownloadVector* result) OVERRIDE;
   virtual void SearchDownloads(const string16& query,
                                DownloadVector* result) OVERRIDE;
   virtual bool Init(content::BrowserContext* browser_context) OVERRIDE;
   virtual void StartDownload(int32 id) OVERRIDE;
   virtual void UpdateDownload(int32 download_id, int64 size) OVERRIDE;
   virtual void OnResponseCompleted(int32 download_id, int64 size,
                                    const std::string& hash) OVERRIDE;
   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,
                                             int uniquifier) OVERRIDE;
   virtual int RemoveDownloadsBetween(const base::Time remove_begin,
                                      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,
                            TabContents* tab_contents) OVERRIDE;
   virtual void DownloadUrlToFile(const GURL& url,
                                  const GURL& referrer,
                                  const std::string& referrer_encoding,
                                  const DownloadSaveInfo& save_info,
                                  TabContents* tab_contents) OVERRIDE;
   virtual void AddObserver(Observer* observer) OVERRIDE;
   virtual void RemoveObserver(Observer* observer) OVERRIDE;
   virtual void OnPersistentStoreQueryComplete(
       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;
   virtual int64 GetInProgressDownloadCount() const OVERRIDE;
   virtual int64 GetReceivedDownloadBytes() const OVERRIDE;
   virtual int64 GetTotalDownloadBytes() const OVERRIDE;
 
  private:
   typedef std::set<DownloadItem*> DownloadSet;
   typedef base::hash_map<int64, DownloadItem*> DownloadMap;
 
   // For testing.
   friend class DownloadManagerTest;
   friend class DownloadTest;
-  friend class MockDownloadManager;
 
   friend class base::RefCountedThreadSafe<
       DownloadManagerImpl, content::BrowserThread::DeleteOnUIThread>;
   friend struct content::BrowserThread::DeleteOnThread<
       content::BrowserThread::UI>;
   friend class DeleteTask<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);
 
   // Called on the UI thread if the FILE thread detects the removal of
   // the downloaded file. The UI thread updates the state of the file
   // and then notifies this update to the file's observer.
   void OnFileRemovalDetected(int64 db_handle);
 
   // Called back after a target path for the file to be downloaded to has been
   // determined, either automatically based on the suggested file name, or by
@@ skipping 104 matching lines @@
   DownloadIdFactory* id_factory_;
 
   // TODO(rdsmith): Remove when http://crbug.com/85408 is fixed.
   // For debugging only.
   int64 largest_db_handle_in_history_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerImpl);
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_