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 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 {
 
 // 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);
 }
 
 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,
+    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() {
   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;
-}