Rewrite download manager unit to be actual unit tests.

Note that this isn't intended to be complete coverage; it's an attempt to preserve coverage from the old tests while making these "real" unit tests, i.e. with every class except for the main one being tested mocked.  Thorough unit tests are intended for the future after we're more completely done with refactoring.  

BUG=107264
BUG=105200
BUG=110886

Review URL: https://chromiumcodereview.appspot.com/10344024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@141674 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/browser_context.cc

@@ -6,9 +6,11 @@
 
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/dom_storage/dom_storage_context_impl.h"
+#include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/fileapi/browser_file_system_helper.h"
 #include "content/browser/in_process_webkit/indexed_db_context_impl.h"
+#include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
 #include "content/browser/resource_context_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
@@ -75,7 +77,7 @@ void CreateQuotaManagerAndClients(BrowserContext* context) {
       context->GetPath(), context->IsOffTheRecord(),
       context->GetSpecialStoragePolicy(), quota_manager->proxy(),
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
-  context->SetUserData(kDatabaseTrackerKeyName, 
+  context->SetUserData(kDatabaseTrackerKeyName,
                        new UserDataAdapter<DatabaseTracker>(db_tracker));
 
   FilePath path = context->IsOffTheRecord() ? FilePath() : context->GetPath();
@@ -142,8 +144,16 @@ DOMStorageContextImpl* GetDOMStorageContextImpl(BrowserContext* context) {
 DownloadManager* BrowserContext::GetDownloadManager(
     BrowserContext* context) {
   if (!context->GetUserData(kDownloadManagerKeyName)) {
-    scoped_refptr<DownloadManager> download_manager = new DownloadManagerImpl(
-        GetContentClient()->browser()->GetNetLog());
+    ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
+    DCHECK(rdh);
+    DownloadFileManager* file_manager = rdh->download_file_manager();
+    DCHECK(file_manager);
+    scoped_refptr<DownloadManager> download_manager =
+        new DownloadManagerImpl(
+            file_manager,
+            scoped_ptr<DownloadItemFactory>(),
+            GetContentClient()->browser()->GetNetLog());
+
     context->SetUserData(
         kDownloadManagerKeyName,
         new UserDataAdapter<DownloadManager>(download_manager));

content/browser/download/download_item_factory.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.
+//
+// The DownloadItemFactory is used to produce different DownloadItems.
+// It is separate from the DownloadManager to allow download manager
+// unit tests to control the items produced.
+
+#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_FACTORY_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_FACTORY_H_
+#pragma once
+
+#include "content/browser/download/download_item_impl.h"
+
+#include <string>
+
+struct DownloadCreateInfo;
+class DownloadRequestHandleInterface;
+class FilePath;
+class GURL;
+
+namespace content {
+class DownloadId;
+class DownloadItem;
+struct DownloadPersistentStoreInfo;
+}
+
+namespace net {
+class BoundNetLog;
+}
+
+namespace content {
+
+class DownloadItemFactory {
+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;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_FACTORY_H_

content/browser/download/download_item_impl_unittest.cc

@@ -551,6 +551,47 @@ TEST_F(DownloadItemTest, CallbackAfterRenameToIntermediateName) {
   ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
 }
 
+TEST_F(DownloadItemTest, Interrupted) {
+  DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+
+  int64 size = 1022;
+  const std::string hash_state("Live beef");
+  const content::DownloadInterruptReason reason(
+      content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
+
+  // Confirm interrupt sets state properly.
+  item->Interrupted(size, hash_state, reason);
+  EXPECT_EQ(size, item->GetReceivedBytes());
+  EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
+  EXPECT_EQ(hash_state, item->GetHashState());
+  EXPECT_EQ(reason, item->GetLastReason());
+
+  // Cancel should result in no change.
+  item->Cancel(true);
+  EXPECT_EQ(size, item->GetReceivedBytes());
+  EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
+  EXPECT_EQ(hash_state, item->GetHashState());
+  EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED,
+            item->GetLastReason());
+}
+
+TEST_F(DownloadItemTest, Canceled) {
+  DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+
+  // Confirm cancel sets state properly.
+  EXPECT_CALL(*mock_delegate(), DownloadCancelled(item));
+  item->Cancel(true);
+  EXPECT_EQ(DownloadItem::CANCELLED, item->GetState());
+}
+
+TEST_F(DownloadItemTest, FileRemoved) {
+  DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+
+  EXPECT_EQ(false, item->GetFileExternallyRemoved());
+  item->OnDownloadedFileRemoved();
+  EXPECT_EQ(true, item->GetFileExternallyRemoved());
+}
+
 TEST(MockDownloadItem, Compiles) {
   MockDownloadItem mock_item;
 }

content/browser/download/download_manager_impl.cc

@@ -136,6 +136,42 @@ 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 {
+      return new DownloadItemImpl(delegate, download_id, info, bound_net_log);
+    }
+
+    virtual content::DownloadItem* CreateActiveItem(
+        DownloadItemImpl::Delegate* delegate,
+        const DownloadCreateInfo& info,
+        DownloadRequestHandleInterface* request_handle,
+        bool is_otr,
+        const net::BoundNetLog& bound_net_log) OVERRIDE {
+      return new DownloadItemImpl(delegate, info, request_handle, is_otr,
+                                  bound_net_log);
+    }
+
+    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 {
+      return new DownloadItemImpl(delegate, path, url, is_otr, download_id,
+                                  mime_type, bound_net_log);
+    }
+};
+
 }  // namespace
 
 namespace content {
@@ -152,12 +188,18 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
 
 }  // namespace content
 
-DownloadManagerImpl::DownloadManagerImpl(net::NetLog* net_log)
-    : shutdown_needed_(false),
+DownloadManagerImpl::DownloadManagerImpl(
+    DownloadFileManager* file_manager,
+    scoped_ptr<content::DownloadItemFactory> factory,
+    net::NetLog* net_log)
+    : factory_(factory.Pass()),
+      shutdown_needed_(false),
       browser_context_(NULL),
-      file_manager_(NULL),
-      delegate_(NULL),
+      file_manager_(file_manager),
       net_log_(net_log) {
+  DCHECK(file_manager);
+  if (!factory_.get())
+    factory_.reset(new DownloadItemFactoryImpl());
 }
 
 DownloadManagerImpl::~DownloadManagerImpl() {
@@ -209,11 +251,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();
 
@@ -316,7 +358,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.";
@@ -324,15 +365,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;
 }
 
@@ -425,8 +457,9 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
 
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-  info->download_id = GetNextId();
-  DownloadItem* download = new DownloadItemImpl(
+  if (!info->download_id.IsValid())
+    info->download_id = GetNextId();
+  DownloadItem* download = factory_->CreateActiveItem(
       this, *info, new DownloadRequestHandle(request_handle),
       browser_context_->IsOffTheRecord(), bound_net_log);
   int32 download_id = info->download_id.local();
@@ -446,7 +479,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,
@@ -680,8 +713,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(
@@ -880,7 +913,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;
@@ -1150,8 +1183,3 @@ void DownloadManagerImpl::DownloadRenamedToFinalName(
         download, download->GetFullPath());
   }
 }
-
-void DownloadManagerImpl::SetFileManagerForTesting(
-      DownloadFileManager* file_manager) {
-  file_manager_ = file_manager;
-}

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_
@@ -17,15 +16,25 @@
 #include "base/message_loop_helpers.h"
 #include "base/observer_list.h"
 #include "base/synchronization/lock.h"
+#include "content/browser/download/download_item_factory.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:
-  explicit DownloadManagerImpl(net::NetLog* net_log);
+  // Caller guarantees that |file_manager| and |net_log| will remain valid
+  // for the lifetime of DownloadManagerImpl (until Shutdown() is called).
+  // |factory| may be a default constructed (null) scoped_ptr; if so,
+  // the DownloadManagerImpl creates and takes ownership of the
+  // default DownloadItemFactory.
+  DownloadManagerImpl(DownloadFileManager* file_manager,
+                      scoped_ptr<content::DownloadItemFactory> factory,
+                      net::NetLog* net_log);
 
   // content::DownloadManager functions.
   virtual void SetDelegate(content::DownloadManagerDelegate* delegate) OVERRIDE;
@@ -109,9 +118,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;
@@ -179,6 +185,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<content::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

content/content_browser.gypi

@@ -276,6 +276,7 @@
     'browser/download/download_file_impl.h',
     'browser/download/download_file_manager.cc',
     'browser/download/download_file_manager.h',
+    'browser/download/download_item_factory.h',
     'browser/download/download_item_impl.cc',
     'browser/download/download_item_impl.h',
     'browser/download/download_manager_impl.cc',