Rewrite download manager unit to be actual unit tests.

content/browser/download/download_manager_impl.cc

 // 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.
 
 #include "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 118 matching lines @@
 
 void EnsureNoPendingDownloadJobsOnIO(bool* result) {
   scoped_refptr<DownloadFileManager> download_file_manager =
       ResourceDispatcherHostImpl::Get()->download_file_manager();
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&EnsureNoPendingDownloadsOnFile,
                  download_file_manager, result));
 }
 
+class DownloadItemFactoryImpl
+    : public DownloadManagerImpl::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(
+    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 {
 
 // static
 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);

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

Why not create the DownloadItemFactoryImpl instance here?

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

Fair question, and I'll try to implement the spirt of it, but actually
implementation is dependent on how I resolve the unit-test targeted constructor
vs. externally targeted constructor pattern (see top level question to John).

 }
 
 bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   bool result = true;
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&EnsureNoPendingDownloadJobsOnIO, &result));
   MessageLoop::current()->Run();
   return result;
 }
 
 }  // namespace content
 
 DownloadManagerImpl::DownloadManagerImpl(
     content::DownloadManagerDelegate* delegate,
+    DownloadFileManager* file_manager,
+    DownloadManagerImpl::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() {
   DCHECK(!shutdown_needed_);
 }
 
 DownloadId DownloadManagerImpl::GetNextId() {
   return delegate_->GetNextId();
 }
 
 bool DownloadManagerImpl::ShouldOpenDownload(DownloadItem* item) {
   return delegate_->ShouldOpenDownload(item);
 }
 
 bool DownloadManagerImpl::ShouldOpenFileBasedOnExtension(const FilePath& path) {
   return delegate_->ShouldOpenFileBasedOnExtension(path);
 }
 
 void DownloadManagerImpl::Shutdown() {
   VLOG(20) << __FUNCTION__ << "()"
            << " shutdown_needed_ = " << shutdown_needed_;
   if (!shutdown_needed_)
     return;
   shutdown_needed_ = false;
 
   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();
 
   // Go through all downloads in downloads_.  Dangerous ones we need to
   // remove on disk, and in progress ones we need to cancel.
   for (DownloadSet::iterator it = downloads_.begin(); it != downloads_.end();) {
     DownloadItem* download = *it;
 
     // Save iterator from potential erases in this set done by called code.
     // Iterators after an erasure point are still valid for lists and
@@ skipping 80 matching lines @@
     // The Incognito Downloads page will get the list of non-Incognito downloads
     // from its parent profile.
     if (browser_context_->IsOffTheRecord() != download_item->IsOtr())
       continue;
 
     if (download_item->MatchesQuery(query_lower))
       result->push_back(download_item);
   }
 }
 
-// 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.";
   shutdown_needed_ = true;
 
   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;
 }
 
 // We have received a message from DownloadFileManager about a new download.
 void DownloadManagerImpl::StartDownload(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (delegate_->ShouldStartDownload(download_id))
     RestartDownload(download_id);
 }
@@ skipping 71 matching lines @@
 FilePath DownloadManagerImpl::LastDownloadPath() {
   return last_download_path_;
 }
 
 net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
     DownloadCreateInfo* info, const DownloadRequestHandle& request_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   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();
 
   DCHECK(!ContainsKey(active_downloads_, download_id));
   downloads_.insert(download);
   active_downloads_[download_id] = download;
 
   return bound_net_log;
 }
 
 DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
     const FilePath& main_file_path,
     const GURL& page_url,
     bool is_otr,
     const std::string& mime_type,
     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,
       is_otr,
       GetNextId(),
       mime_type,
       bound_net_log);
 
   download->AddObserver(observer);
 
@@ skipping 204 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
   RemoveFromActiveList(download);
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
   AssertStateConsistent(download);
 
-  if (file_manager_)
-    download->OffThreadCancel(file_manager_);
+  DCHECK(file_manager_);
+  download->OffThreadCancel(file_manager_);
 }
 
 void DownloadManagerImpl::OnDownloadInterrupted(
     int32 download_id,
     int64 size,
     const std::string& hash_state,
     content::DownloadInterruptReason reason) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = GetActiveDownload(download_id);
@@ skipping 184 matching lines @@
 // 'DownloadPersistentStoreInfo's in sorted order (by ascending start_time).
 void DownloadManagerImpl::OnPersistentStoreQueryComplete(
     std::vector<DownloadPersistentStoreInfo>* entries) {
   for (size_t i = 0; i < entries->size(); ++i) {
     int64 db_handle = entries->at(i).db_handle;
     base::debug::Alias(&db_handle);
     DCHECK(!ContainsKey(history_downloads_, db_handle));
 
     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;
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
   }
   NotifyModelChanged();
   CheckForHistoryFilesRemoval();
 }
 
@@ skipping 243 matching lines @@
 }
 
 void DownloadManagerImpl::DownloadRenamedToFinalName(
     DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // If the rename failed, we receive an OnDownloadInterrupted() call before we
   // receive the DownloadRenamedToFinalName() call.
   delegate_->UpdatePathForItemInPersistentStore(download,
                                                 download->GetFullPath());
 }
-
-void DownloadManagerImpl::SetFileManagerForTesting(
-      DownloadFileManager* file_manager) {
-  file_manager_ = file_manager;
-}