Rewrite download manager unit to be actual unit tests.

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 3296daae4373e441bcea7127d96816d3ece80e58..1e6f993cb5d210db65f7fe7aaa72a6feac80d548 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -136,6 +136,65 @@ void EnsureNoPendingDownloadJobsOnIO(bool* result) {
                  download_file_manager, result));
 }
 
+class DownloadItemFactoryImpl : public content::DownloadItemFactory {
+ public:
+    DownloadItemFactoryImpl() {}
+    virtual ~DownloadItemFactoryImpl() {}
+
+    virtual content::DownloadItem* CreatePersistedItem(
+        DownloadItemImpl::Delegate* delegate,
+        content::DownloadId download_id,
+        const content::DownloadPersistentStoreInfo& info,
+        const net::BoundNetLog& bound_net_log) OVERRIDE;
+
+    virtual content::DownloadItem* CreateActiveItem(
+        DownloadItemImpl::Delegate* delegate,
+        const DownloadCreateInfo& info,
+        DownloadRequestHandleInterface* request_handle,
+        bool is_otr,
+        const net::BoundNetLog& bound_net_log) OVERRIDE;
+
+    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) OVERRIDE;
+};
+
+
+content::DownloadItem* DownloadItemFactoryImpl::CreatePersistedItem(

[jam, 2012/06/07 21:35:41]

this is personal style so feel free to ignore, but in cases like this I prefer
to put the implementation inline. one knows what the functions are from the
interface header, which is where the function documentation is anyways. so
splitting it makes the code take longer to scan

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

Done.  I have a strong preference for fewer lines of code (all other things
being equal) which I have to stomp on sometimes for cleanliness.  So I'm happy
to unstomp :-}.

+    DownloadItemImpl::Delegate* delegate,
+    content::DownloadId download_id,
+    const content::DownloadPersistentStoreInfo& info,
+    const net::BoundNetLog& bound_net_log) {
+  return new DownloadItemImpl(delegate, download_id, info, bound_net_log);
+}
+
+content::DownloadItem* DownloadItemFactoryImpl::CreateActiveItem(
+    DownloadItemImpl::Delegate* delegate,
+    const DownloadCreateInfo& info,
+    DownloadRequestHandleInterface* request_handle,
+    bool is_otr,
+    const net::BoundNetLog& bound_net_log) {
+  return new DownloadItemImpl(delegate, info, request_handle, is_otr,
+                              bound_net_log);
+}
+
+content::DownloadItem* DownloadItemFactoryImpl::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) {
+  return new DownloadItemImpl(delegate, path, url, is_otr, download_id,
+                              mime_type, bound_net_log);
+}
+
 }  // namespace
 
 namespace content {
@@ -144,7 +203,17 @@ namespace content {
 DownloadManager* DownloadManager::Create(
     content::DownloadManagerDelegate* delegate,
     net::NetLog* net_log) {
-  return new DownloadManagerImpl(delegate, net_log);
+  // Find the DownloadFileManager for use when creating the DownloadManager.
+  // Note that unit tests should construct based on the underlying
+  // constructor, so ResourceDispatcherHostImpl & DownloadFileManager
+  // should always be present when using this code.
+  ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
+  DownloadFileManager* file_manager = NULL;
+  DCHECK(rdh);
+  file_manager = rdh->download_file_manager();
+  DCHECK(file_manager);
+
+  return new DownloadManagerImpl(delegate, file_manager, NULL, net_log);
 }
 
 bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
@@ -161,12 +230,17 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
 
 DownloadManagerImpl::DownloadManagerImpl(
     content::DownloadManagerDelegate* delegate,
+    DownloadFileManager* file_manager,
+    content::DownloadItemFactory *factory,
     net::NetLog* net_log)
-        : shutdown_needed_(false),
-          browser_context_(NULL),
-          file_manager_(NULL),
-          delegate_(delegate),
-          net_log_(net_log) {
+    : factory_(factory),
+      shutdown_needed_(false),
+      browser_context_(NULL),
+      file_manager_(file_manager),
+      delegate_(delegate),
+      net_log_(net_log) {
+  if (!factory_.get())
+    factory_.reset(new DownloadItemFactoryImpl());
 }
 
 DownloadManagerImpl::~DownloadManagerImpl() {
@@ -195,11 +269,11 @@ void DownloadManagerImpl::Shutdown() {
   FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this));
   // TODO(benjhayden): Consider clearing observers_.
 
-  if (file_manager_) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::OnDownloadManagerShutdown,
-                   file_manager_, make_scoped_refptr(this)));
-  }
+  DCHECK(file_manager_);
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::OnDownloadManagerShutdown,
+                 file_manager_, make_scoped_refptr(this)));
 
   AssertContainersConsistent();
 
@@ -300,7 +374,6 @@ void DownloadManagerImpl::SearchDownloads(const string16& query,
   }
 }
 
-// Query the history service for information about all persisted downloads.
 bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
   DCHECK(browser_context);
   DCHECK(!shutdown_needed_)  << "DownloadManager already initialized.";
@@ -308,15 +381,6 @@ bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
 
   browser_context_ = browser_context;
 
-  // In test mode, there may be no ResourceDispatcherHostImpl.  In this case
-  // it's safe to avoid setting |file_manager_| because we only call a small
-  // set of functions, none of which need it.
-  ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
-  if (rdh) {
-    file_manager_ = rdh->download_file_manager();
-    DCHECK(file_manager_);
-  }
-
   return true;
 }
 
@@ -408,7 +472,7 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
 
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-  DownloadItem* download = new DownloadItemImpl(
+  DownloadItem* download = factory_->CreateActiveItem(
       this, *info, new DownloadRequestHandle(request_handle),
       browser_context_->IsOffTheRecord(), bound_net_log);
   int32 download_id = info->download_id.local();
@@ -428,7 +492,7 @@ DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
     DownloadItem::Observer* observer) {
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-  DownloadItem* download = new DownloadItemImpl(
+  DownloadItem* download = factory_->CreateSavePageItem(
       this,
       main_file_path,
       page_url,
@@ -653,8 +717,8 @@ void DownloadManagerImpl::DownloadCancelled(DownloadItem* download) {
   // should already have been updated.
   AssertStateConsistent(download);
 
-  if (file_manager_)
-    download->OffThreadCancel(file_manager_);
+  DCHECK(file_manager_);
+  download->OffThreadCancel(file_manager_);
 }
 
 void DownloadManagerImpl::OnDownloadInterrupted(
@@ -859,7 +923,7 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
 
     net::BoundNetLog bound_net_log =
         net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-    DownloadItem* download = new DownloadItemImpl(
+    DownloadItem* download = factory_->CreatePersistedItem(
         this, GetNextId(), entries->at(i), bound_net_log);
     downloads_.insert(download);
     history_downloads_[download->GetDbHandle()] = download;
@@ -1123,8 +1187,3 @@ void DownloadManagerImpl::DownloadRenamedToFinalName(
   delegate_->UpdatePathForItemInPersistentStore(download,
                                                 download->GetFullPath());
 }
-
-void DownloadManagerImpl::SetFileManagerForTesting(
-      DownloadFileManager* file_manager) {
-  file_manager_ = file_manager;
-}