Rewrite download manager unit to be actual unit tests.

content/browser/download/download_manager_impl.h

 // Copyright (c) 2012 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 <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/message_loop_helpers.h"
 #include "base/observer_list.h"
 #include "base/synchronization/lock.h"
 #include "content/browser/download/download_item_impl.h"
 #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.
   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;
@@ skipping 65 matching lines @@
   virtual void DownloadOpened(
       content::DownloadItem* download) OVERRIDE;
   virtual void DownloadRemoved(content::DownloadItem* download) OVERRIDE;
   virtual void DownloadRenamedToIntermediateName(
       content::DownloadItem* download) OVERRIDE;
   virtual void DownloadRenamedToFinalName(
       content::DownloadItem* download) OVERRIDE;
   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;
 
   // For testing.
   friend class DownloadManagerTest;
   friend class DownloadTest;
 
   friend class base::RefCountedThreadSafe<DownloadManagerImpl>;
 
@@ skipping 47 matching lines @@
 
   // Remove from internal maps.
   int RemoveDownloadItems(const DownloadVector& pending_deletes);
 
   // Called when a download entry is committed to the persistent store.
   void OnDownloadItemAddedToPersistentStore(int32 download_id, int64 db_handle);
 
   // 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
   // "save page as" downloads.  All other DownloadItem containers in
   // the DownloadManager are maps; they do not own the DownloadItems.
   // Note that this is the only place (with any functional implications;
   // see save_page_downloads_ below) that "save page as" downloads are
   // kept, as the DownloadManager's only job is to hold onto those
   // until destruction.
   //
@@ skipping 41 matching lines @@
 
   // Allows an embedder to control behavior. Guaranteed to outlive this object.
   content::DownloadManagerDelegate* delegate_;
 
   net::NetLog* net_log_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerImpl);
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_