Rewrite download manager unit to be actual 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 ad1a0278e6fba3a3fd7bf624b66c5ac6fafdd40f..11a67cab19e92ce0c606aa204cca906058bae1fb 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -21,11 +21,42 @@
 #include "content/common/content_export.h"
 #include "content/public/browser/download_manager.h"
 
+class DownloadFileManager;
+
 class CONTENT_EXPORT DownloadManagerImpl
     : public content::DownloadManager,
       public DownloadItemImpl::Delegate {
  public:
+  class DownloadItemFactory {

[jam, 2012/06/07 03:21:45]

nit: can this be put in new h/cc files for readability? 

[Randy Smith (Not in Mondays), 2012/06/07 23:27:21]

I moved the declaration out into its own .h file.  The default implementation is
specific to the download manager and in the anonymous namespace currently, so it
didn't feel appropriate to me to move that out--let me know if you disagree.

+   public:
+    virtual ~DownloadItemFactory() {}
+
+    virtual content::DownloadItem* CreatePersistedItem(
+        DownloadItemImpl::Delegate* delegate,
+        content::DownloadId download_id,
+        const content::DownloadPersistentStoreInfo& info,
+        const net::BoundNetLog& bound_net_log) = 0;
+
+    virtual content::DownloadItem* CreateActiveItem(
+        DownloadItemImpl::Delegate* delegate,
+        const DownloadCreateInfo& info,
+        DownloadRequestHandleInterface* request_handle,
+        bool is_otr,
+        const net::BoundNetLog& bound_net_log) = 0;
+
+    virtual content::DownloadItem* CreateSavePageItem(
+        DownloadItemImpl::Delegate* delegate,
+        const FilePath& path,
+        const GURL& url,
+        bool is_otr,
+        content::DownloadId download_id,
+        const std::string& mime_type,
+        const net::BoundNetLog& bound_net_log) = 0;
+  };
+
   DownloadManagerImpl(content::DownloadManagerDelegate* delegate,

[asanka, 2012/06/07 17:04:53]

Nit: Document ownership semantics?

[Randy Smith (Not in Mondays), 2012/06/07 23:27:21]

Delegate argument gone, file_manager (lack of) ownership documented, and factory
changed over to scoped_ptr<> so self-documenting.  Let me know what you think.

+                      DownloadFileManager* file_manager,
+                      DownloadItemFactory* factory,
                       net::NetLog* net_log);
 
   // content::DownloadManager functions.
@@ -111,9 +142,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual void AssertStateConsistent(
       content::DownloadItem* download) const OVERRIDE;
 
-  // For unit tests only.
-  void SetFileManagerForTesting(DownloadFileManager* file_manager);
-
  private:
   typedef std::set<content::DownloadItem*> DownloadSet;
   typedef base::hash_map<int64, content::DownloadItem*> DownloadMap;
@@ -181,6 +209,9 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Called when Save Page As entry is committed to the persistent store.
   void OnSavePageItemAddedToPersistentStore(int32 download_id, int64 db_handle);
 
+  // Factory for creation of downloads items.
+  scoped_ptr<DownloadItemFactory> factory_;
+
   // |downloads_| is the owning set for all downloads known to the
   // DownloadManager.  This includes downloads started by the user in
   // this session, downloads initialized from the history system, and