Rewrite download manager unit to be actual unit tests.

content/browser/download/download_manager_impl_unittest.cc

Index: content/browser/download/download_manager_impl_unittest.cc
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index 1be540f99e2aa8087d7f7d17320a5ce4cf02870f..cfa98bec8c3640c256ace0b6146c22eadc6fcc60 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -8,62 +8,42 @@
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/message_loop.h"
 #include "base/memory/scoped_ptr.h"

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

Duplicate

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

Whoops; done.

+#include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
 #include "base/stl_util.h"
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
-#include "content/browser/download/download_file_impl.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/mock_download_file.h"
-#include "content/browser/power_save_blocker.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager_delegate.h"
-#include "content/public/test/mock_download_manager.h"
+#include "content/public/test/mock_download_item.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread.h"
-#include "net/base/io_buffer.h"
 #include "net/base/net_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(USE_AURA) && defined(OS_WIN)
-// http://crbug.com/105200
-#define MAYBE_StartDownload DISABLED_StartDownload
-#define MAYBE_DownloadOverwriteTest DISABLED_DownloadOverwriteTest
-#define MAYBE_DownloadRemoveTest DISABLED_DownloadRemoveTest
-#define MAYBE_DownloadFileErrorTest DownloadFileErrorTest
-#elif defined(OS_LINUX)
-// http://crbug.com/110886 for Linux
-#define MAYBE_StartDownload DISABLED_StartDownload
-#define MAYBE_DownloadOverwriteTest DISABLED_DownloadOverwriteTest
-#define MAYBE_DownloadRemoveTest DISABLED_DownloadRemoveTest
-#define MAYBE_DownloadFileErrorTest DISABLED_DownloadFileErrorTest
-#else
-#define MAYBE_StartDownload StartDownload
-#define MAYBE_DownloadOverwriteTest DownloadOverwriteTest
-#define MAYBE_DownloadRemoveTest DownloadRemoveTest
-#define MAYBE_DownloadFileErrorTest DownloadFileErrorTest
-#endif
-
-using content::BrowserContext;
-using content::BrowserThread;
-using content::DownloadFile;
-using content::DownloadId;
+using ::testing::DoAll;
+using ::testing::Ref;
+using ::testing::Return;
+using ::testing::ReturnRef;
+using ::testing::SetArgPointee;
+using ::testing::StrictMock;
+using ::testing::_;
 using content::DownloadItem;
 using content::DownloadManager;
 using content::WebContents;
-using ::testing::NiceMock;
-using ::testing::ReturnRef;
-using ::testing::Return;
 
 namespace content {
 class ByteStreamReader;
@@ -71,1213 +51,530 @@ class ByteStreamReader;
 
 namespace {
 
-FilePath GetTempDownloadPath(const FilePath& suggested_path) {
-  return FilePath(suggested_path.value() + FILE_PATH_LITERAL(".temp"));
-}
-
-class MockDownloadFileFactory
-    : public DownloadFileManager::DownloadFileFactory {
+class MockDownloadManagerDelegate : public content::DownloadManagerDelegate {
  public:
-  MockDownloadFileFactory() {}
-
-  virtual DownloadFile* CreateFile(
-      DownloadCreateInfo* info,
-      scoped_ptr<content::ByteStreamReader> stream,
-      const DownloadRequestHandle& request_handle,
-      DownloadManager* download_manager,
-      bool calculate_hash,
-      const net::BoundNetLog& bound_net_log) OVERRIDE;
+  MockDownloadManagerDelegate();
+  virtual ~MockDownloadManagerDelegate();
+
+  MOCK_METHOD0(Shutdown, void());
+  MOCK_METHOD0(GetNextId, content::DownloadId());
+  MOCK_METHOD1(ShouldStartDownload, bool(int32));
+  MOCK_METHOD3(ChooseDownloadPath, void(WebContents*, const FilePath&, int32));
+  MOCK_METHOD2(GetIntermediatePath, FilePath(const DownloadItem&, bool*));
+  MOCK_METHOD0(GetAlternativeWebContentsToNotifyForDownload, WebContents*());
+  MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath&));
+  MOCK_METHOD2(ShouldCompleteDownload, bool(
+      DownloadItem*, const base::Closure&));
+  MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItem*));
+  MOCK_METHOD0(GenerateFileHash, bool());
+  MOCK_METHOD1(AddItemToPersistentStore, void(DownloadItem*));
+  MOCK_METHOD1(UpdateItemInPersistentStore, void(DownloadItem*));
+  MOCK_METHOD2(UpdatePathForItemInPersistentStore,
+               void(DownloadItem*, const FilePath&));
+  MOCK_METHOD1(RemoveItemFromPersistentStore, void(DownloadItem*));
+  MOCK_METHOD2(RemoveItemsFromPersistentStoreBetween, void(
+      base::Time remove_begin, base::Time remove_end));
+  MOCK_METHOD3(GetSaveDir, void(WebContents*, FilePath*, FilePath*));
+  MOCK_METHOD5(ChooseSavePath, void(
+      WebContents*, const FilePath&, const FilePath::StringType&,
+      bool, const content::SavePackagePathPickedCallback&));
 };
 
-DownloadFile* MockDownloadFileFactory::CreateFile(
-    DownloadCreateInfo* info,
-    scoped_ptr<content::ByteStreamReader> stream,
-    const DownloadRequestHandle& request_handle,
-    DownloadManager* download_manager,
-    bool calculate_hash,
-    const net::BoundNetLog& bound_net_log) {
-  NOTREACHED();
-  return NULL;
-}
+MockDownloadManagerDelegate::MockDownloadManagerDelegate() { }
 
-DownloadId::Domain kValidIdDomain = "valid DownloadId::Domain";
+MockDownloadManagerDelegate::~MockDownloadManagerDelegate() { }
 
-class TestDownloadManagerDelegate : public content::DownloadManagerDelegate {
+class MockDownloadFileManager : public DownloadFileManager {
  public:
-  TestDownloadManagerDelegate()
-      : mark_content_dangerous_(false),
-        prompt_user_for_save_location_(false),
-        should_complete_download_(true),
-        download_manager_(NULL) {
-  }
-
-  void set_download_directory(const FilePath& path) {
-    download_directory_ = path;
-  }
-
-  void set_download_manager(content::DownloadManager* dm) {
-    download_manager_ = dm;
-  }
+  MockDownloadFileManager();
+
+  MOCK_METHOD0(Shutdown, void());
+  MOCK_METHOD3(MockStartDownload, void(DownloadCreateInfo*,
+                                       content::ByteStreamReader* stream,
+                                       const DownloadRequestHandle&));
+
+  virtual void StartDownload(scoped_ptr<DownloadCreateInfo> info,
+                             scoped_ptr<content::ByteStreamReader> stream,
+                             const DownloadRequestHandle& request_handle) {

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

Nit: OVERRIDE

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

Done.

+    MockStartDownload(info.get(), stream.get(), request_handle);
+  }
+
+  MOCK_METHOD1(CancelDownload, void(content::DownloadId));
+  MOCK_METHOD1(CompleteDownload, void(content::DownloadId));
+  MOCK_METHOD1(OnDownloadManagerShutdown, void(content::DownloadManager*));
+  MOCK_METHOD4(RenameInProgressDownloadFile,
+               void(content::DownloadId,
+                    const FilePath&,
+                    bool,
+                    const RenameCompletionCallback&));
+  MOCK_METHOD4(RenameCompletingDownloadFile,
+               void(content::DownloadId,
+                    const FilePath&,
+                    bool,
+                    const RenameCompletionCallback&));
+  MOCK_CONST_METHOD0(NumberOfActiveDownloads, int());
+ protected:
+  virtual ~MockDownloadFileManager();
+};
 
-  void set_prompt_user_for_save_location(bool value) {
-    prompt_user_for_save_location_ = value;
-  }
+MockDownloadFileManager::MockDownloadFileManager()
+    : DownloadFileManager(NULL) { }
 
-  virtual bool ShouldStartDownload(int32 download_id) OVERRIDE {
-    DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
-    FilePath path = net::GenerateFileName(item->GetURL(),
-                                          item->GetContentDisposition(),
-                                          item->GetReferrerCharset(),
-                                          item->GetSuggestedFilename(),
-                                          item->GetMimeType(),
-                                          std::string());
-    DownloadItem::TargetDisposition disposition = item->GetTargetDisposition();
-    if (!ShouldOpenFileBasedOnExtension(path) && prompt_user_for_save_location_)
-      disposition = DownloadItem::TARGET_DISPOSITION_PROMPT;
-    item->OnTargetPathDetermined(download_directory_.Append(path),
-                                 disposition,
-                                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
-    return true;
-  }
+MockDownloadFileManager::~MockDownloadFileManager() { }
 
-  virtual FilePath GetIntermediatePath(const DownloadItem& item,
-                                       bool* ok_to_overwrite) OVERRIDE {
-    if (intermediate_path_.empty()) {
-      *ok_to_overwrite = true;
-      return GetTempDownloadPath(item.GetTargetFilePath());
-    } else {
-      *ok_to_overwrite = overwrite_intermediate_path_;
-      return intermediate_path_;
-    }
-  }
+class MockDownloadItemFactory
+     : public DownloadManagerImpl::DownloadItemFactory {
+ public:
+  class Observer {

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

Nit: Would it be cleaner if MockDownloadItemFactory maintained and exposed a
download item map of items it has created?

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

Good idea.  I'm not certain about the best way to manage the exposure of the
map; we could just make it a public data member, but that seems a bit yucky. 
I'm tempted to publicly inherit from std::map<>, which is slightly less yucky,
but still makes me twitch a bit.  And going to the hassle of actually doing a
proper iterator interface seems like overkill.  So I just exposed the
information needed in accessors.  Let me know what you think.

+   public:
+    virtual ~Observer() = 0;
+    virtual void PersistedItemCreated(
+        content::MockDownloadItem* item,
+        const content::DownloadPersistentStoreInfo& info) = 0;
+    virtual void ActiveItemCreated(
+        content::MockDownloadItem* item,
+        const DownloadCreateInfo& info,
+        DownloadRequestHandleInterface* request_handle,
+        bool is_otr) = 0;
+    virtual void SavePageItemCreated(
+        content::MockDownloadItem* item,
+        const FilePath& path,
+        const GURL& url,
+        bool is_otr) = 0;
+  };
+
+  // |observer| must outlive the factory.
+  explicit MockDownloadItemFactory(Observer* observer);
+  virtual ~MockDownloadItemFactory();
+
+  virtual content::DownloadItem* CreatePersistedItem(
+      DownloadItemImpl::Delegate* delegate,
+      content::DownloadId download_id,
+      const content::DownloadPersistentStoreInfo& info,
+      const net::BoundNetLog& bound_net_log) OVERRIDE;
 
-  virtual void ChooseDownloadPath(WebContents* web_contents,
-                                  const FilePath& suggested_path,
-                                  int32 download_id) OVERRIDE {
-    if (!expected_suggested_path_.empty()) {
-      EXPECT_STREQ(expected_suggested_path_.value().c_str(),
-                   suggested_path.value().c_str());
-    }
-    if (file_selection_response_.empty()) {
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          base::Bind(&DownloadManager::FileSelectionCanceled,
-                     download_manager_,
-                     download_id));
-    } else {
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          base::Bind(&DownloadManager::FileSelected,
-                     download_manager_,
-                     file_selection_response_,
-                     download_id));
-    }
-    expected_suggested_path_.clear();
-    file_selection_response_.clear();
-  }
+  virtual content::DownloadItem* CreateActiveItem(
+      DownloadItemImpl::Delegate* delegate,
+      const DownloadCreateInfo& info,
+      DownloadRequestHandleInterface* request_handle,
+      bool is_otr,
+      const net::BoundNetLog& bound_net_log) OVERRIDE;
 
-  virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) OVERRIDE {
-    return path.Extension() == FilePath::StringType(FILE_PATH_LITERAL(".pdf"));
-  }
+  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;
 
-  virtual void AddItemToPersistentStore(DownloadItem* item) OVERRIDE {
-    static int64 db_handle = DownloadItem::kUninitializedHandle;
-    download_manager_->OnItemAddedToPersistentStore(item->GetId(), --db_handle);
-  }
+ private:
+  Observer* observer_;
 
-  void SetFileSelectionExpectation(const FilePath& suggested_path,
-                                   const FilePath& response) {
-    expected_suggested_path_ = suggested_path;
-    file_selection_response_ = response;
-  }
+  DISALLOW_COPY_AND_ASSIGN(MockDownloadItemFactory);
+};
 
-  void SetMarkContentsDangerous(bool dangerous) {
-    mark_content_dangerous_ = dangerous;
-  }
+MockDownloadItemFactory::Observer::~Observer() { }
 
-  void SetIntermediatePath(const FilePath& intermediate_path,
-                           bool overwrite_intermediate_path) {
-    intermediate_path_ = intermediate_path;
-    overwrite_intermediate_path_ = overwrite_intermediate_path;
-  }
+MockDownloadItemFactory::MockDownloadItemFactory(
+    MockDownloadItemFactory::Observer* observer) : observer_(observer) { }
 
-  void SetShouldCompleteDownload(bool value) {
-    should_complete_download_ = value;
-  }
+MockDownloadItemFactory::~MockDownloadItemFactory() { }
 
-  void InvokeDownloadCompletionCallback() {
-    EXPECT_FALSE(download_completion_callback_.is_null());
-    download_completion_callback_.Run();
-    download_completion_callback_.Reset();
-  }
+content::DownloadItem* MockDownloadItemFactory::CreatePersistedItem(
+    DownloadItemImpl::Delegate* delegate,
+    content::DownloadId download_id,
+    const content::DownloadPersistentStoreInfo& info,
+    const net::BoundNetLog& bound_net_log) {
+  content::MockDownloadItem* result =
+      new StrictMock<content::MockDownloadItem>;
+  EXPECT_CALL(*result, GetId())
+      .WillRepeatedly(Return(download_id.local()));
+  observer_->PersistedItemCreated(result, info);
+  return result;
+}
 
-  virtual bool ShouldCompleteDownload(
-      DownloadItem* item,
-      const base::Closure& complete_callback) OVERRIDE {
-    download_completion_callback_ = complete_callback;
-    if (mark_content_dangerous_) {
-      CHECK(!complete_callback.is_null());
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          base::Bind(&TestDownloadManagerDelegate::MarkContentDangerous,
-                     base::Unretained(this), item->GetId()));
-      mark_content_dangerous_ = false;
-      return false;
-    } else {
-      return should_complete_download_;
-    }
-  }
+content::DownloadItem* MockDownloadItemFactory::CreateActiveItem(
+    DownloadItemImpl::Delegate* delegate,
+    const DownloadCreateInfo& info,
+    DownloadRequestHandleInterface* request_handle,
+    bool is_otr,
+    const net::BoundNetLog& bound_net_log) {
+  content::MockDownloadItem* result =
+      new StrictMock<content::MockDownloadItem>;
+  EXPECT_CALL(*result, GetId())
+      .WillRepeatedly(Return(info.download_id.local()));
+  observer_->ActiveItemCreated(result, info, request_handle, is_otr);
+  return result;
+}
 
- private:
-  void MarkContentDangerous(
-      int32 download_id) {
-    DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
-    if (!item)
-      return;
-    item->OnContentCheckCompleted(
-        content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT);
-    InvokeDownloadCompletionCallback();
-  }
+content::DownloadItem* MockDownloadItemFactory::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) {
+  content::MockDownloadItem* result =
+      new StrictMock<content::MockDownloadItem>;
+  EXPECT_CALL(*result, GetId())
+      .WillRepeatedly(Return(download_id.local()));
+  observer_->SavePageItemCreated(result, path, url, is_otr);
+  return result;
+}
 
-  FilePath download_directory_;
-  FilePath expected_suggested_path_;
-  FilePath file_selection_response_;
-  FilePath intermediate_path_;
-  bool overwrite_intermediate_path_;
-  bool mark_content_dangerous_;
-  bool prompt_user_for_save_location_;
-  bool should_complete_download_;
-  DownloadManager* download_manager_;
-  base::Closure download_completion_callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestDownloadManagerDelegate);
+class MockBrowserContext : public content::BrowserContext {
+ public:
+  MockBrowserContext() { }
+  ~MockBrowserContext() { }
+
+  MOCK_METHOD0(GetPath, FilePath());
+  MOCK_CONST_METHOD0(IsOffTheRecord, bool());
+  MOCK_METHOD0(GetDownloadManager, content::DownloadManager*());
+  MOCK_METHOD0(GetRequestContext, net::URLRequestContextGetter*());
+  MOCK_METHOD1(GetRequestContextForRenderProcess,
+               net::URLRequestContextGetter*(int renderer_child_id));
+  MOCK_METHOD0(GetRequestContextForMedia, net::URLRequestContextGetter*());
+  MOCK_METHOD0(GetResourceContext, content::ResourceContext*());
+  MOCK_METHOD0(GetGeolocationPermissionContext,
+               content::GeolocationPermissionContext* ());
+  MOCK_METHOD0(GetSpeechRecognitionPreferences,
+               content::SpeechRecognitionPreferences* ());
+  MOCK_METHOD0(DidLastSessionExitCleanly, bool());
+  MOCK_METHOD0(GetSpecialStoragePolicy, quota::SpecialStoragePolicy*());
 };
 
 } // namespace
 
-class DownloadManagerTest : public testing::Test {
+class DownloadManagerTest
+    : public testing::Test, MockDownloadItemFactory::Observer {
  public:
   static const char* kTestData;
   static const size_t kTestDataLen;
 
   DownloadManagerTest()
-      : browser_context(new content::TestBrowserContext()),
-        download_manager_delegate_(new TestDownloadManagerDelegate()),
-        download_manager_(new DownloadManagerImpl(
-            download_manager_delegate_.get(), NULL)),
-        ui_thread_(BrowserThread::UI, &message_loop_),
-        file_thread_(BrowserThread::FILE, &message_loop_) {
-    download_manager_->Init(browser_context.get());
-    download_manager_delegate_->set_download_manager(download_manager_);
-  }
+      : ui_thread_(content::BrowserThread::UI, &message_loop_),
+        file_thread_(content::BrowserThread::FILE, &message_loop_),
+        next_download_id_(0) {

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

Nit: Initialize scalars (mock_download_item_factory_)

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

Done.

+  }
+
+  // We tear down everything in TearDown().
+  ~DownloadManagerTest() {}
+
+  // *** New helper functions targeted at new mock classes.
+
+  // Create a MockDownloadItemFactory, MockDownloadManagerDelegate,
+  // and MockDownloadFileManager, then create a DownloadManager that points
+  // at all of those.
+  virtual void SetUp() {
+    DCHECK(!download_manager_.get());
+
+    mock_download_item_factory_ = new MockDownloadItemFactory(this);
+    mock_download_manager_delegate_.reset(
+        new StrictMock<MockDownloadManagerDelegate>);
+    EXPECT_CALL(*mock_download_manager_delegate_.get(), Shutdown())
+        .WillOnce(Return());
+    mock_download_file_manager_ = new StrictMock<MockDownloadFileManager>;
+    EXPECT_CALL(*mock_download_file_manager_.get(),
+                OnDownloadManagerShutdown(_));
+    mock_browser_context_.reset(new StrictMock<MockBrowserContext>);
+    EXPECT_CALL(*mock_browser_context_.get(), IsOffTheRecord())
+        .WillRepeatedly(Return(false));
+
+    download_manager_ = new DownloadManagerImpl(
+        mock_download_manager_delegate_.get(),
+        mock_download_file_manager_.get(),
+        mock_download_item_factory_,
+        NULL);
+    download_manager_->Init(mock_browser_context_.get());
+  }
+
+  virtual void TearDown() {
+    // Setup all download items for Shutdown.
+    for (std::map<int32, content::MockDownloadItem*>::iterator it =
+             items_.begin(); it != items_.end(); ++it) {
+      EXPECT_CALL(*(it->second), GetSafetyState())
+          .WillOnce(Return(content::DownloadItem::SAFE));
+      EXPECT_CALL(*(it->second), IsPartialDownload())
+          .WillOnce(Return(false));
+    }
+    items_.clear();
 
-  ~DownloadManagerTest() {
+    mock_download_item_factory_ = NULL; // Owned by DownloadManager.

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

Could we test that it gets deleted? Perhaps stuff in a WeakPtr?

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

Done.  That also makes me feel more comfortable about keeping a pointer to an
unowned object that I'm expecting to be deleted while I'm holding the pointer
:-}.

     download_manager_->Shutdown();
-    // browser_context must outlive download_manager_, so we explicitly delete
-    // download_manager_ first.
     download_manager_ = NULL;
-    download_manager_delegate_.reset();
-    browser_context.reset(NULL);
     message_loop_.RunAllPending();
+    mock_download_manager_delegate_.reset();
+    mock_download_file_manager_ = NULL;
+    mock_browser_context_.reset();
   }
 
-  // Create a temporary directory as the downloads directory.
-  bool CreateTempDownloadsDirectory() {
-    if (!scoped_download_dir_.CreateUniqueTempDir())
-      return false;
-    download_manager_delegate_->set_download_directory(
-        scoped_download_dir_.path());
-    return true;
-  }
-
-  void AddDownloadToFileManager(int id, DownloadFile* download_file) {
-    file_manager()->downloads_[DownloadId(kValidIdDomain, id)] =
-      download_file;
-  }
+  // Returns download id.
+  int AddItemToManager() {

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

Why not return a MockDownloadItem& directly?

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

Done.

+    DownloadCreateInfo info;
+    DownloadRequestHandle handle;
 
-  void AddMockDownloadToFileManager(int id, MockDownloadFile* download_file) {
-    AddDownloadToFileManager(id, download_file);
-    EXPECT_CALL(*download_file, GetDownloadManager())
-        .WillRepeatedly(Return(download_manager_));
+    // Args are ignored except for download id, so everything else can be
+    // null.
+    int id = next_download_id_;
+    ++next_download_id_;
+    info.download_id = content::DownloadId(NULL, id);
+    download_manager_->CreateDownloadItem(&info, handle);
+    DCHECK(items_[id]);
+    ON_CALL(*items_[id], GetId())
+        .WillByDefault(Return(id));
+    return id;
   }
 
-  void OnResponseCompleted(int32 download_id, int64 size,
-                           const std::string& hash) {
-    download_manager_->OnResponseCompleted(download_id, size, hash);
+  content::MockDownloadItem& GetMockDownloadItem(int id) {
+    DCHECK(items_[id]);
+    return *items_[id];
   }
 
-  void FileSelected(const FilePath& path, int32 download_id) {
-    download_manager_->FileSelected(path, download_id);
+  void RemoveMockDownloadItem(int id) {
+    // Owned by DownloadManager; should be deleted there.
+    DCHECK(items_[id]);
+    items_.erase(id);
   }
 
-  void ContinueDownloadWithPath(DownloadItem* download, const FilePath& path) {
-    download->OnTargetPathDetermined(
-        path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-        content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
-    download_manager_->OnTargetPathAvailable(download);
+  MockDownloadManagerDelegate& GetMockDownloadManagerDelegate() {
+    return *mock_download_manager_delegate_;
   }
 
-  void OnDownloadInterrupted(int32 download_id, int64 size,
-                             const std::string& hash_state,
-                             content::DownloadInterruptReason reason) {
-    download_manager_->OnDownloadInterrupted(download_id, size,
-                                             hash_state, reason);
+  MockDownloadFileManager& GetMockDownloadFileManager() {
+    return *mock_download_file_manager_;
   }
 
-  // Get the download item with ID |id|.
+  // Probe at private internals.
   DownloadItem* GetActiveDownloadItem(int32 id) {
     return download_manager_->GetActiveDownload(id);
   }
 
-  FilePath GetPathInDownloadsDir(const FilePath::StringType& fragment) {
-    DCHECK(scoped_download_dir_.IsValid());
-    FilePath full_path(scoped_download_dir_.path().Append(fragment));
-    return full_path.NormalizePathSeparators();
+  // From MockDownloadItemFactory::Observer
+  virtual void PersistedItemCreated(
+      content::MockDownloadItem* item,
+      const content::DownloadPersistentStoreInfo& info) OVERRIDE {
+    int32 id = item->GetId();
+    DCHECK(items_.find(id) == items_.end());
+    items_[id] = item;
   }
 
- protected:
-  scoped_ptr<content::TestBrowserContext> browser_context;
-  scoped_ptr<TestDownloadManagerDelegate> download_manager_delegate_;
-  scoped_refptr<DownloadManagerImpl> download_manager_;
-  scoped_refptr<DownloadFileManager> file_manager_;
-  MessageLoopForUI message_loop_;
-  content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread file_thread_;
-  ScopedTempDir scoped_download_dir_;
-
-  DownloadFileManager* file_manager() {
-    if (!file_manager_) {
-      file_manager_ = new DownloadFileManager(new MockDownloadFileFactory);
-      download_manager_->SetFileManagerForTesting(file_manager_);
-    }
-    return file_manager_;
+  virtual void ActiveItemCreated(
+      content::MockDownloadItem* item,
+      const DownloadCreateInfo& info,
+      DownloadRequestHandleInterface* request_handle,
+      bool is_otr) OVERRIDE {
+    int32 id = item->GetId();
+    DCHECK(items_.find(id) == items_.end());
+    items_[id] = item;
   }
 
-  DISALLOW_COPY_AND_ASSIGN(DownloadManagerTest);
-};
-
-const char* DownloadManagerTest::kTestData = "a;sdlfalsdfjalsdkfjad";
-const size_t DownloadManagerTest::kTestDataLen =
-    strlen(DownloadManagerTest::kTestData);
-
-// A DownloadFile that we can inject errors into.
-class DownloadFileWithErrors : public DownloadFileImpl {
- public:
-  DownloadFileWithErrors(DownloadCreateInfo* info,
-                         scoped_ptr<content::ByteStreamReader> stream,
-                         DownloadManager* manager,
-                         bool calculate_hash);
-  virtual ~DownloadFileWithErrors() {}
-
-  // BaseFile delegated functions.
-  virtual net::Error Initialize() OVERRIDE;
-  virtual net::Error AppendDataToFile(const char* data,
-                                      size_t data_len) OVERRIDE;
-  virtual net::Error Rename(const FilePath& full_path) OVERRIDE;
-
-  void set_forced_error(net::Error error) { forced_error_ = error; }
-  void clear_forced_error() { forced_error_ = net::OK; }
-  net::Error forced_error() const { return forced_error_; }
-
- private:
-  net::Error ReturnError(net::Error function_error) {
-    if (forced_error_ != net::OK) {
-      net::Error ret = forced_error_;
-      clear_forced_error();
-      return ret;
-    }
-
-    return function_error;
+  virtual void SavePageItemCreated(
+      content::MockDownloadItem* item,
+      const FilePath& path,
+      const GURL& url,
+      bool is_otr) OVERRIDE {
+    int32 id = item->GetId();
+    DCHECK(items_.find(id) == items_.end());
+    items_[id] = item;
   }
 
-  net::Error forced_error_;
-};
-
-DownloadFileWithErrors::DownloadFileWithErrors(
-    DownloadCreateInfo* info,
-    scoped_ptr<content::ByteStreamReader> stream,
-    DownloadManager* manager,
-    bool calculate_hash)
-    : DownloadFileImpl(info,
-                       stream.Pass(),
-                       new DownloadRequestHandle(),
-                       manager,
-                       calculate_hash,
-                       scoped_ptr<PowerSaveBlocker>(NULL).Pass(),
-                       net::BoundNetLog()),
-      forced_error_(net::OK) {
-}
+  void AddItemToHistory(int download_id, int64 db_handle) {
+    content::MockDownloadItem& item(GetMockDownloadItem(download_id));
 
-net::Error DownloadFileWithErrors::Initialize() {
-  return ReturnError(DownloadFileImpl::Initialize());
-}
+    // For DCHECK in AddDownloadItemToHistory.  Don't want to use
+    // WillRepeatedly as it may have to return true after this.
+    if (DCHECK_IS_ON())
+      EXPECT_CALL(item, IsPersisted())
+          .WillOnce(Return(false));
 
-net::Error DownloadFileWithErrors::AppendDataToFile(const char* data,
-                                                    size_t data_len) {
-  return ReturnError(DownloadFileImpl::AppendDataToFile(data, data_len));
-}
+    EXPECT_CALL(item, SetDbHandle(db_handle));
+    EXPECT_CALL(item, SetIsPersisted());
+    EXPECT_CALL(item, GetDbHandle())
+        .WillRepeatedly(Return(db_handle));
 
-net::Error DownloadFileWithErrors::Rename(const FilePath& full_path) {
-  return ReturnError(DownloadFileImpl::Rename(full_path));
-}
+    // Null out ShowDownloadInBrowser
+    EXPECT_CALL(item, GetWebContents())
+        .WillOnce(Return(static_cast<WebContents*>(NULL)));
+    EXPECT_CALL(GetMockDownloadManagerDelegate(),
+                GetAlternativeWebContentsToNotifyForDownload())
+        .WillOnce(Return(static_cast<WebContents*>(NULL)));
 
-namespace {
+    EXPECT_CALL(item, IsInProgress())
+        .WillOnce(Return(true));
 
-const struct {
-  const char* url;
-  const char* mime_type;
-  bool save_as;
-  bool prompt_for_download;
-  bool expected_save_as;
-} kStartDownloadCases[] = {
-  { "http://www.foo.com/dont-open.html",
-    "text/html",
-    false,
-    false,
-    false, },
-  { "http://www.foo.com/save-as.html",
-    "text/html",
-    true,
-    false,
-    true, },
-  { "http://www.foo.com/always-prompt.html",
-    "text/html",
-    false,
-    true,
-    true, },
-  { "http://www.foo.com/user-script-text-html-mimetype.user.js",
-    "text/html",
-    false,
-    false,
-    false, },
-  { "http://www.foo.com/extensionless-extension",
-    "application/x-chrome-extension",
-    true,
-    false,
-    true, },
-  { "http://www.foo.com/save-as.pdf",
-    "application/pdf",
-    true,
-    false,
-    true, },
-  { "http://www.foo.com/sometimes_prompt.pdf",
-    "application/pdf",
-    false,
-    true,
-    false, },
-  { "http://www.foo.com/always_prompt.jar",
-    "application/jar",
-    false,
-    true,
-    true, },
-};
+    // Null out MaybeCompleteDownload
+    EXPECT_CALL(item, AllDataSaved())
+        .WillOnce(Return(false));
 
-// This is an observer that records what download IDs have opened a select
-// file dialog.
-class SelectFileObserver : public DownloadManager::Observer {
- public:
-  explicit SelectFileObserver(DownloadManager* download_manager)
-      : download_manager_(download_manager) {
-    DCHECK(download_manager_.get());
-    download_manager_->AddObserver(this);
+    download_manager_->OnItemAddedToPersistentStore(download_id, db_handle);
   }
 
-  ~SelectFileObserver() {
-    download_manager_->RemoveObserver(this);
-  }
-
-  // Downloadmanager::Observer functions.
-  virtual void ModelChanged(DownloadManager* manager) OVERRIDE {}
-  virtual void ManagerGoingDown(DownloadManager* manager) OVERRIDE {}
-  virtual void SelectFileDialogDisplayed(
-      DownloadManager* manager, int32 id) OVERRIDE {
-    file_dialog_ids_.insert(id);
-  }
-
-  bool ShowedFileDialogForId(int32 id) {
-    return file_dialog_ids_.find(id) != file_dialog_ids_.end();
-  }
-
- private:
-  std::set<int32> file_dialog_ids_;
-  scoped_refptr<DownloadManager> download_manager_;
-};
-
-// This observer tracks the progress of |DownloadItem|s.
-class ItemObserver : public DownloadItem::Observer {
- public:
-  explicit ItemObserver(DownloadItem* tracked)
-      : tracked_(tracked), states_hit_(0),
-        was_updated_(false), was_opened_(false) {
-    DCHECK(tracked_);
-    tracked_->AddObserver(this);
-    // Record the initial state.
-    OnDownloadUpdated(tracked_);
-  }
-  ~ItemObserver() {
-    tracked_->RemoveObserver(this);
-  }
-
-  bool hit_state(int state) const {
-    return (1 << state) & states_hit_;
-  }
-  bool was_updated() const { return was_updated_; }
-  bool was_opened() const { return was_opened_; }
+ protected:
+  // Key test variable; we'll keep it available to sub-classes.
+  scoped_refptr<DownloadManagerImpl> download_manager_;
 
  private:
-  // DownloadItem::Observer methods
-  virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE {
-    DCHECK_EQ(tracked_, download);
-    states_hit_ |= (1 << download->GetState());
-    was_updated_ = true;
-  }
-  virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE{
-    DCHECK_EQ(tracked_, download);
-    states_hit_ |= (1 << download->GetState());
-    was_opened_ = true;
-  }
-
-  DownloadItem* tracked_;
-  int states_hit_;
-  bool was_updated_;
-  bool was_opened_;
-};
-
-}  // namespace
-
-TEST_F(DownloadManagerTest, MAYBE_StartDownload) {
-  content::TestBrowserThread io_thread(BrowserThread::IO, &message_loop_);
-  ASSERT_TRUE(CreateTempDownloadsDirectory());
-  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) {
-    DVLOG(1) << "Test case " << i;
-    download_manager_delegate_->set_prompt_user_for_save_location(
-        kStartDownloadCases[i].prompt_for_download);
-
-    SelectFileObserver observer(download_manager_);
-    // Normally, the download system takes ownership of info, and is
-    // responsible for deleting it.  In these unit tests, however, we
-    // don't call the function that deletes it, so we do so ourselves.
-    scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-    const DownloadId id = DownloadId(kValidIdDomain, static_cast<int>(i));
-    info->download_id = id;
-    info->prompt_user_for_save_location = kStartDownloadCases[i].save_as;
-    info->url_chain.push_back(GURL(kStartDownloadCases[i].url));
-    info->mime_type = kStartDownloadCases[i].mime_type;
-    download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-
-  scoped_ptr<content::ByteStreamWriter> stream_writer;
-  scoped_ptr<content::ByteStreamReader> stream_reader;
-  content::CreateByteStream(message_loop_.message_loop_proxy(),
-                            message_loop_.message_loop_proxy(),
-                            kTestDataLen, &stream_writer, &stream_reader);
-
-    DownloadFile* download_file(
-        new DownloadFileImpl(info.get(),
-                             stream_reader.Pass(),
-                             new DownloadRequestHandle(),
-                             download_manager_, false,
-                             scoped_ptr<PowerSaveBlocker>(NULL).Pass(),
-                             net::BoundNetLog()));
-    AddDownloadToFileManager(info->download_id.local(), download_file);
-    download_file->Initialize();
-    download_manager_->StartDownload(info->download_id.local());
-    message_loop_.RunAllPending();
-
-    // SelectFileObserver will have recorded any attempt to open the
-    // select file dialog.
-    // Note that DownloadManager::FileSelectionCanceled() is never called.
-    EXPECT_EQ(kStartDownloadCases[i].expected_save_as,
-              observer.ShowedFileDialogForId(i));
-
-    file_manager()->CancelDownload(id);
-  }
-}
-
-namespace {
-
-enum OverwriteDownloadPath {
-  DONT_OVERWRITE,
-  OVERWRITE
-};
-
-enum ResponseCompletionTime {
-  COMPLETES_BEFORE_RENAME,
-  COMPLETES_AFTER_RENAME
-};
+  MessageLoopForUI message_loop_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread file_thread_;
+  MockDownloadItemFactory* mock_download_item_factory_;
+  scoped_ptr<MockDownloadManagerDelegate> mock_download_manager_delegate_;
+  scoped_refptr<MockDownloadFileManager> mock_download_file_manager_;
+  scoped_ptr<MockBrowserContext> mock_browser_context_;
+  std::map<int32, content::MockDownloadItem*> items_;
+  int next_download_id_;
 
-// Test cases to be used with DownloadFilenameTest.  The paths are relative to
-// the temporary downloads directory used for testing.
-const struct DownloadFilenameTestCase {
-  // DownloadItem properties prior to calling RestartDownload().
-  const FilePath::CharType*       target_path;
-  DownloadItem::TargetDisposition target_disposition;
-
-  // If we receive a ChooseDownloadPath() call to prompt the user for a download
-  // location, |expected_prompt_path| is the expected prompt path. The
-  // TestDownloadManagerDelegate will respond with |chosen_path|. If
-  // |chosen_path| is empty, then the file choose dialog be cancelled.
-  const FilePath::CharType*       expected_prompt_path;
-  const FilePath::CharType*       chosen_path;
-
-  // Response to GetIntermediatePath(). If |intermediate_path| is empty, then a
-  // temporary path is constructed with
-  // GetTempDownloadPath(item->GetTargetFilePath()).
-  const FilePath::CharType*       intermediate_path;
-  OverwriteDownloadPath           overwrite_intermediate_path;
-
-  // Determines when OnResponseCompleted() is called. If this is
-  // COMPLETES_BEFORE_RENAME, then the response completes before the filename is
-  // determines.
-  ResponseCompletionTime          completion;
-
-  // The expected intermediate path for the download.
-  const FilePath::CharType*       expected_intermediate_path;
-
-  // The expected final path for the download.
-  const FilePath::CharType*       expected_final_path;
-} kDownloadFilenameTestCases[] = {
-
-#define TARGET                FILE_PATH_LITERAL
-#define INTERMEDIATE          FILE_PATH_LITERAL
-#define EXPECTED_PROMPT       FILE_PATH_LITERAL
-#define PROMPT_RESPONSE       FILE_PATH_LITERAL
-#define EXPECTED_INTERMEDIATE FILE_PATH_LITERAL
-#define EXPECTED_FINAL        FILE_PATH_LITERAL
-
-  {
-    // 0: No prompting.
-    TARGET("foo.txt"),
-    DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-
-    EXPECTED_PROMPT(""),
-    PROMPT_RESPONSE(""),
-
-    INTERMEDIATE("foo.txt.intermediate"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("foo.txt.intermediate"),
-    EXPECTED_FINAL("foo.txt"),
-  },
-  {
-    // 1: With prompting. No rename.
-    TARGET("foo.txt"),
-    DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-
-    EXPECTED_PROMPT("foo.txt"),
-    PROMPT_RESPONSE("foo.txt"),
-
-    INTERMEDIATE("foo.txt.intermediate"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("foo.txt.intermediate"),
-    EXPECTED_FINAL("foo.txt"),
-  },
-  {
-    // 2: With prompting. Download is renamed.
-    TARGET("foo.txt"),
-    DownloadItem::TARGET_DISPOSITION_PROMPT,
-
-    EXPECTED_PROMPT("foo.txt"),
-    PROMPT_RESPONSE("bar.txt"),
-
-    INTERMEDIATE("bar.txt.intermediate"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("bar.txt.intermediate"),
-    EXPECTED_FINAL("bar.txt"),
-  },
-  {
-    // 3: With prompting. Download path is changed.
-    TARGET("foo.txt"),
-    DownloadItem::TARGET_DISPOSITION_PROMPT,
-
-    EXPECTED_PROMPT("foo.txt"),
-    PROMPT_RESPONSE("Foo/bar.txt"),
-
-    INTERMEDIATE("Foo/bar.txt.intermediate"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("Foo/bar.txt.intermediate"),
-    EXPECTED_FINAL("Foo/bar.txt"),
-  },
-  {
-    // 4: No prompting. Intermediate path exists. Doesn't overwrite
-    // intermediate path.
-    TARGET("exists.png"),
-    DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-
-    EXPECTED_PROMPT(""),
-    PROMPT_RESPONSE(""),
-
-    INTERMEDIATE("exists.png.temp"),
-    DONT_OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("exists.png (1).temp"),
-    EXPECTED_FINAL("exists.png"),
-  },
-  {
-    // 5: No prompting. Intermediate path exists. Overwrites.
-    TARGET("exists.png"),
-    DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-
-    EXPECTED_PROMPT(""),
-    PROMPT_RESPONSE(""),
-
-    INTERMEDIATE("exists.png.temp"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("exists.png.temp"),
-    EXPECTED_FINAL("exists.png"),
-  },
-  {
-    // 6: No prompting. Final path exists. Doesn't overwrite.
-    TARGET("exists.txt"),
-    DownloadItem::TARGET_DISPOSITION_UNIQUIFY,
-
-    EXPECTED_PROMPT(""),
-    PROMPT_RESPONSE(""),
-
-    INTERMEDIATE("exists.txt.temp"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("exists.txt.temp"),
-    EXPECTED_FINAL("exists (1).txt"),
-  },
-  {
-    // 7: No prompting. Final path exists. Overwrites.
-    TARGET("exists.txt"),
-    DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-
-    EXPECTED_PROMPT(""),
-    PROMPT_RESPONSE(""),
-
-    INTERMEDIATE("exists.txt.temp"),
-    OVERWRITE,
-
-    COMPLETES_AFTER_RENAME,
-    EXPECTED_INTERMEDIATE("exists.txt.temp"),
-    EXPECTED_FINAL("exists.txt"),
-  },
-  {
-    // 8: No prompting. Response completes before filename determination.
-    TARGET("foo.txt"),
-    DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-
-    EXPECTED_PROMPT(""),
-    PROMPT_RESPONSE(""),
-
-    INTERMEDIATE("foo.txt.intermediate"),
-    OVERWRITE,
-
-    COMPLETES_BEFORE_RENAME,
-    EXPECTED_INTERMEDIATE("foo.txt.intermediate"),
-    EXPECTED_FINAL("foo.txt"),
-  },
-  {
-    // 9: With prompting. Download path is changed. Response completes before
-    // filename determination.
-    TARGET("foo.txt"),
-    DownloadItem::TARGET_DISPOSITION_PROMPT,
-
-    EXPECTED_PROMPT("foo.txt"),
-    PROMPT_RESPONSE("Foo/bar.txt"),
-
-    INTERMEDIATE("Foo/bar.txt.intermediate"),
-    OVERWRITE,
-
-    COMPLETES_BEFORE_RENAME,
-    EXPECTED_INTERMEDIATE("Foo/bar.txt.intermediate"),
-    EXPECTED_FINAL("Foo/bar.txt"),
-  },
-
-#undef TARGET
-#undef INTERMEDIATE
-#undef EXPECTED_PROMPT
-#undef PROMPT_RESPONSE
-#undef EXPECTED_INTERMEDIATE
-#undef EXPECTED_FINAL
+  DISALLOW_COPY_AND_ASSIGN(DownloadManagerTest);
 };
 
-} // namespace
-
-TEST_F(DownloadManagerTest, DownloadFilenameTest) {
-  ASSERT_TRUE(CreateTempDownloadsDirectory());
-
-  // We create a known file to test file uniquification.
-  ASSERT_TRUE(file_util::WriteFile(
-      GetPathInDownloadsDir(FILE_PATH_LITERAL("exists.txt")), "", 0) == 0);
-  ASSERT_TRUE(file_util::WriteFile(
-      GetPathInDownloadsDir(FILE_PATH_LITERAL("exists.png.temp")), "", 0) == 0);
-
-  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDownloadFilenameTestCases); ++i) {
-    const DownloadFilenameTestCase& test_case = kDownloadFilenameTestCases[i];
-    scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-
-    SCOPED_TRACE(testing::Message() << "Running test case " << i);
-    info->download_id = DownloadId(kValidIdDomain, i);
-    info->url_chain.push_back(GURL());
-
-    MockDownloadFile* download_file(
-        new ::testing::StrictMock<MockDownloadFile>());
-    FilePath target_path = GetPathInDownloadsDir(test_case.target_path);
-    FilePath expected_prompt_path =
-        GetPathInDownloadsDir(test_case.expected_prompt_path);
-    FilePath chosen_path = GetPathInDownloadsDir(test_case.chosen_path);
-    FilePath intermediate_path =
-        GetPathInDownloadsDir(test_case.intermediate_path);
-    FilePath expected_intermediate_path =
-        GetPathInDownloadsDir(test_case.expected_intermediate_path);
-    FilePath expected_final_path =
-        GetPathInDownloadsDir(test_case.expected_final_path);
-
-    AddMockDownloadToFileManager(info->download_id.local(), download_file);
-    EXPECT_CALL(*download_file, Rename(expected_intermediate_path))
-        .WillOnce(Return(net::OK));
-    EXPECT_CALL(*download_file, Rename(expected_final_path))
-        .WillOnce(Return(net::OK));
-#if defined(OS_MACOSX)
-    EXPECT_CALL(*download_file, AnnotateWithSourceInformation());
-#endif
-    EXPECT_CALL(*download_file, Detach());
-
-    download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-    DownloadItem* download = GetActiveDownloadItem(i);
-    ASSERT_TRUE(download != NULL);
-
-    if (test_case.completion == COMPLETES_BEFORE_RENAME)
-      OnResponseCompleted(i, 1024, std::string("fake_hash"));
-
-    download->OnTargetPathDetermined(
-        target_path, test_case.target_disposition,
-        content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
-    download_manager_delegate_->SetFileSelectionExpectation(
-        expected_prompt_path, chosen_path);
-    download_manager_delegate_->SetIntermediatePath(
-        intermediate_path, test_case.overwrite_intermediate_path);
-    download_manager_delegate_->SetShouldCompleteDownload(false);
-    download_manager_->RestartDownload(i);
-    message_loop_.RunAllPending();
-
-    if (test_case.completion == COMPLETES_AFTER_RENAME) {
-      OnResponseCompleted(i, 1024, std::string("fake_hash"));
-      message_loop_.RunAllPending();
-    }
-
-    EXPECT_EQ(expected_intermediate_path.value(),
-              download->GetFullPath().value());
-    download_manager_delegate_->SetShouldCompleteDownload(true);
-    download_manager_delegate_->InvokeDownloadCompletionCallback();
-    message_loop_.RunAllPending();
-    EXPECT_EQ(expected_final_path.value(),
-              download->GetTargetFilePath().value());
-  }
-}
-
-void WriteCopyToStream(const char *source,
-                     size_t len, content::ByteStreamWriter* stream) {
-  scoped_refptr<net::IOBuffer> copy(new net::IOBuffer(len));
-  memcpy(copy.get()->data(), source, len);
-  stream->Write(copy, len);
+// Does the DownloadManager prompt when requested?
+TEST_F(DownloadManagerTest, RestartDownload) {
+  // Put a mock we have a handle to on the download manager.
+  int download_id = AddItemToManager();
+  content::MockDownloadItem& item(GetMockDownloadItem(download_id));
+
+  ScopedTempDir download_dir;
+  ASSERT_TRUE(download_dir.CreateUniqueTempDir());
+  FilePath expected_path(download_dir.path().Append(
+      FILE_PATH_LITERAL("location")));
+
+  EXPECT_CALL(item, GetTargetDisposition())
+      .WillOnce(Return(DownloadItem::TARGET_DISPOSITION_PROMPT));
+  EXPECT_CALL(item, GetWebContents())
+      .WillOnce(Return(reinterpret_cast<WebContents*>(0x44)));
+  EXPECT_CALL(item, GetTargetFilePath())
+      .WillOnce(ReturnRef(expected_path));
+  EXPECT_CALL(GetMockDownloadManagerDelegate(), ChooseDownloadPath(
+      reinterpret_cast<WebContents*>(0x44), expected_path, download_id));
+  download_manager_->RestartDownload(download_id);
+
+  // The alternative pathway goes straight to OnTargetPathAvailable,
+  // so it more naturally belongs below.
 }
 
-TEST_F(DownloadManagerTest, DownloadInterruptTest) {
-  using ::testing::_;
-  using ::testing::CreateFunctor;
-  using ::testing::Invoke;
-  using ::testing::Return;
-
-  // Normally, the download system takes ownership of info, and is
-  // responsible for deleting it.  In these unit tests, however, we
-  // don't call the function that deletes it, so we do so ourselves.
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = DownloadId(kValidIdDomain, 0);
-  info->prompt_user_for_save_location = false;
-  info->url_chain.push_back(GURL());
-  info->total_bytes = static_cast<int64>(kTestDataLen);
-  const FilePath new_path(FILE_PATH_LITERAL("foo.zip"));
-  const FilePath cr_path(GetTempDownloadPath(new_path));
-
-  MockDownloadFile* download_file(new NiceMock<MockDownloadFile>());
-
-  // |download_file| is owned by DownloadFileManager.
-  AddMockDownloadToFileManager(info->download_id.local(), download_file);
-
-  EXPECT_CALL(*download_file, Rename(cr_path))
-      .WillOnce(Return(net::OK));
-  EXPECT_CALL(*download_file, Cancel());
-
-  download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-
-  DownloadItem* download = GetActiveDownloadItem(0);
-  ASSERT_TRUE(download != NULL);
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
-
-  ContinueDownloadWithPath(download, new_path);
-  message_loop_.RunAllPending();
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
-
-  int64 error_size = 3;
-  OnDownloadInterrupted(0, error_size, "",
-                        content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
-  message_loop_.RunAllPending();
-
-  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_FALSE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
-
-  download->Cancel(true);
-
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_FALSE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
-  EXPECT_EQ(download->GetReceivedBytes(), error_size);
-  EXPECT_EQ(download->GetTotalBytes(), static_cast<int64>(kTestDataLen));
+// Do the results of GetIntermediatePath get passed through to the
+// download?  Note that this path is tested from RestartDownload
+// to test the non-prompting path in RestartDownload as well.
+TEST_F(DownloadManagerTest, OnTargetPathAvailable) {
+  // Put a mock we have a handle to on the download manager.
+  int download_id = AddItemToManager();
+  content::MockDownloadItem& item(GetMockDownloadItem(download_id));
+
+  ScopedTempDir download_dir;
+  ASSERT_TRUE(download_dir.CreateUniqueTempDir());
+  FilePath target_path(download_dir.path().Append(
+      FILE_PATH_LITERAL("location")));
+  FilePath intermediate_path(download_dir.path().Append(
+      FILE_PATH_LITERAL("location.crdownload")));
+
+  EXPECT_CALL(item, GetTargetDisposition())
+      .WillOnce(Return(DownloadItem::TARGET_DISPOSITION_UNIQUIFY));
+  EXPECT_CALL(GetMockDownloadManagerDelegate(),
+              GetIntermediatePath(Ref(item), _))
+      .WillOnce(DoAll(SetArgPointee<1>(true), Return(intermediate_path)));
+  // Finesse DCHECK with WillRepeatedly.
+  EXPECT_CALL(item, GetTargetFilePath())
+      .WillRepeatedly(ReturnRef(target_path));
+  EXPECT_CALL(item, OnIntermediatePathDetermined(
+      &GetMockDownloadFileManager(), intermediate_path, true));
+  download_manager_->RestartDownload(download_id);
 }
 
-// Test the behavior of DownloadFileManager and DownloadManager in the event
-// of a file error while writing the download to disk.
-TEST_F(DownloadManagerTest, MAYBE_DownloadFileErrorTest) {
-  // Create a temporary file and a mock stream.
-  FilePath path;
-  ASSERT_TRUE(file_util::CreateTemporaryFile(&path));
-
-  // This file stream will be used, until the first rename occurs.
-  net::FileStream* stream = new net::FileStream(NULL);
-  ASSERT_EQ(0, stream->OpenSync(
-      path,
-      base::PLATFORM_FILE_OPEN_ALWAYS | base::PLATFORM_FILE_WRITE));
-
-  // Make sure the DFM exists; we'll need it later.
-  // TODO(rdsmith): This is ugly and should be rewritten, but a large
-  // rewrite of this test is in progress in a different CL, so doing it
-  // the hacky way for now.
-  (void) file_manager();
-
-  // Normally, the download system takes ownership of info, and is
-  // responsible for deleting it.  In these unit tests, however, we
-  // don't call the function that deletes it, so we do so ourselves.
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  static const int32 local_id = 0;
-  info->download_id = DownloadId(kValidIdDomain, local_id);
-  info->prompt_user_for_save_location = false;
-  info->url_chain.push_back(GURL());
-  info->total_bytes = static_cast<int64>(kTestDataLen * 3);
-  info->save_info.file_path = path;
-  info->save_info.file_stream.reset(stream);
-  scoped_ptr<content::ByteStreamWriter> stream_input;
-  scoped_ptr<content::ByteStreamReader> stream_output;
-  content::CreateByteStream(message_loop_.message_loop_proxy(),
-                            message_loop_.message_loop_proxy(),
-                            kTestDataLen, &stream_input, &stream_output);
-
-  // Create a download file that we can insert errors into.
-  scoped_ptr<DownloadFileWithErrors> download_file(new DownloadFileWithErrors(
-      info.get(), stream_output.Pass(), download_manager_, false));
-  download_file->Initialize();
-
-  download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-
-  DownloadItem* download = GetActiveDownloadItem(0);
-  ASSERT_TRUE(download != NULL);
-
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
-
-  // Add some data before finalizing the file name.
-  WriteCopyToStream(kTestData, kTestDataLen, stream_input.get());
-
-  // Finalize the file name.
-  ContinueDownloadWithPath(download, path);
-  message_loop_.RunAllPending();
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
-
-  // Add more data.
-  WriteCopyToStream(kTestData, kTestDataLen, stream_input.get());
-
-  // Add more data, but an error occurs.
-  download_file->set_forced_error(net::ERR_FAILED);
-  WriteCopyToStream(kTestData, kTestDataLen, stream_input.get());
-  message_loop_.RunAllPending();
-
-  // Check the state.  The download should have been interrupted.
-  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_FALSE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
-
-  // Clean up.
-  download->Cancel(true);
-  message_loop_.RunAllPending();
+// Do the results of an OnDownloadInterrupted get passed through properly
+// to the DownloadItem?  This test tests non-persisted downloads.
+TEST_F(DownloadManagerTest, OnDownloadInterrupted_NonPersisted) {
+  // Put a mock we have a handle to on the download manager.
+  int download_id = AddItemToManager();
+  content::MockDownloadItem& item(GetMockDownloadItem(download_id));
+
+  EXPECT_EQ(&item, GetActiveDownloadItem(download_id));
+
+  int64 size = 0xdeadbeef;
+  const std::string hash_state("Undead beef");
+  content::DownloadInterruptReason reason(
+      content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
+
+  EXPECT_CALL(item, Interrupted(size, hash_state, reason));
+  EXPECT_CALL(item, OffThreadCancel(&GetMockDownloadFileManager()));
+  EXPECT_CALL(item, IsPersisted())
+      .WillOnce(Return(false));
+  download_manager_->OnDownloadInterrupted(
+      download_id, size, hash_state, reason);
+  EXPECT_EQ(&item, GetActiveDownloadItem(download_id));
 }
 
-TEST_F(DownloadManagerTest, DownloadCancelTest) {
-  using ::testing::_;
-  using ::testing::CreateFunctor;
-  using ::testing::Invoke;
-  using ::testing::Return;
-
-  // Normally, the download system takes ownership of info, and is
-  // responsible for deleting it.  In these unit tests, however, we
-  // don't call the function that deletes it, so we do so ourselves.
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  DownloadId id = DownloadId(kValidIdDomain, 0);
-  info->download_id = id;
-  info->prompt_user_for_save_location = false;
-  info->url_chain.push_back(GURL());
-  const FilePath new_path(FILE_PATH_LITERAL("foo.zip"));
-  const FilePath cr_path(GetTempDownloadPath(new_path));
-
-  MockDownloadFile* download_file(new NiceMock<MockDownloadFile>());
-  AddMockDownloadToFileManager(info->download_id.local(), download_file);
-
-  // |download_file| is owned by DownloadFileManager.
-  EXPECT_CALL(*download_file, Rename(cr_path))
-      .WillOnce(Return(net::OK));
-  EXPECT_CALL(*download_file, Cancel());
-
-  download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-
-  DownloadItem* download = GetActiveDownloadItem(0);
-  ASSERT_TRUE(download != NULL);
-
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
-
-  ContinueDownloadWithPath(download, new_path);
-  message_loop_.RunAllPending();
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
-
-  download->Cancel(false);
-  message_loop_.RunAllPending();
-
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_FALSE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::CANCELLED, download->GetState());
-
-  file_manager()->CancelDownload(id);
-
-  EXPECT_FALSE(file_util::PathExists(new_path));
-  EXPECT_FALSE(file_util::PathExists(cr_path));
+// Do the results of an OnDownloadInterrupted get passed through properly
+// to the DownloadItem?  This test tests persisted downloads.
+TEST_F(DownloadManagerTest, OnDownloadInterrupted_Persisted) {
+  // Put a mock we have a handle to on the download manager.
+  int download_id = AddItemToManager();
+  content::MockDownloadItem& item(GetMockDownloadItem(download_id));
+  int64 db_handle = 0x7;
+  AddItemToHistory(download_id, db_handle);
+
+  EXPECT_EQ(&item, GetActiveDownloadItem(download_id));
+
+  int64 size = 0xdeadbeef;
+  const std::string hash_state("Undead beef");
+  content::DownloadInterruptReason reason(
+      content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
+
+  EXPECT_CALL(item, Interrupted(size, hash_state, reason));
+  EXPECT_CALL(item, OffThreadCancel(&GetMockDownloadFileManager()));
+  EXPECT_CALL(item, IsPersisted())
+      .WillOnce(Return(true));
+  EXPECT_CALL(GetMockDownloadManagerDelegate(),
+              UpdateItemInPersistentStore(&item));
+  download_manager_->OnDownloadInterrupted(
+      download_id, size, hash_state, reason);
+  EXPECT_EQ(NULL, GetActiveDownloadItem(download_id));
+
+  // Remove so we don't get errors on shutdown.
+  EXPECT_CALL(GetMockDownloadManagerDelegate(),
+              RemoveItemFromPersistentStore(&item));
+  download_manager_->DownloadRemoved(&item);
+  RemoveMockDownloadItem(download_id);
 }
 
-TEST_F(DownloadManagerTest, MAYBE_DownloadOverwriteTest) {
-  using ::testing::_;
-  using ::testing::CreateFunctor;
-  using ::testing::Invoke;
-  using ::testing::Return;
-
-  ASSERT_TRUE(CreateTempDownloadsDirectory());
-  // File names we're using.
-  const FilePath new_path(GetPathInDownloadsDir(FILE_PATH_LITERAL("foo.txt")));
-  const FilePath cr_path(GetTempDownloadPath(new_path));
-  EXPECT_FALSE(file_util::PathExists(new_path));
-
-  // Create the file that we will overwrite.  Will be automatically cleaned
-  // up when temp_dir_ is destroyed.
-  FILE* fp = file_util::OpenFile(new_path, "w");
-  file_util::CloseFile(fp);
-  EXPECT_TRUE(file_util::PathExists(new_path));
-
-  // Construct the unique file name that normally would be created, but
-  // which we will override.
-  int uniquifier =
-      file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL(""));
-  FilePath unique_new_path = new_path;
-  EXPECT_NE(0, uniquifier);
-  unique_new_path = unique_new_path.InsertBeforeExtensionASCII(
-            StringPrintf(" (%d)", uniquifier));
-
-  // Normally, the download system takes ownership of info, and is
-  // responsible for deleting it.  In these unit tests, however, we
-  // don't call the function that deletes it, so we do so ourselves.
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = DownloadId(kValidIdDomain, 0);
-  info->prompt_user_for_save_location = true;
-  info->url_chain.push_back(GURL());
-  scoped_ptr<content::ByteStreamWriter> stream_input;
-  scoped_ptr<content::ByteStreamReader> stream_output;
-  content::CreateByteStream(message_loop_.message_loop_proxy(),
-                            message_loop_.message_loop_proxy(),
-                            kTestDataLen, &stream_input, &stream_output);
-
-  download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-
-  DownloadItem* download = GetActiveDownloadItem(0);
-  ASSERT_TRUE(download != NULL);
-
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
-
-  // Create and initialize the download file.  We're bypassing the first part
-  // of the download process and skipping to the part after the final file
-  // name has been chosen, so we need to initialize the download file
-  // properly.
-  DownloadFile* download_file(
-      new DownloadFileImpl(info.get(), stream_output.Pass(),
-                           new DownloadRequestHandle(),
-                           download_manager_, false,
-                           scoped_ptr<PowerSaveBlocker>(NULL).Pass(),
-                           net::BoundNetLog()));
-  download_file->Rename(cr_path);
-  // This creates the .temp version of the file.
-  download_file->Initialize();
-  // |download_file| is owned by DownloadFileManager.
-  AddDownloadToFileManager(info->download_id.local(), download_file);
-
-  ContinueDownloadWithPath(download, new_path);
-  message_loop_.RunAllPending();
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
-
-  WriteCopyToStream(kTestData, kTestDataLen, stream_input.get());
-
-  // Finish the download.
-  OnResponseCompleted(0, kTestDataLen, "");
-  message_loop_.RunAllPending();
-
-  // Download is complete.
-  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_FALSE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::COMPLETE, download->GetState());
-
-  EXPECT_TRUE(file_util::PathExists(new_path));
-  EXPECT_FALSE(file_util::PathExists(cr_path));
-  EXPECT_FALSE(file_util::PathExists(unique_new_path));
-  std::string file_contents;
-  EXPECT_TRUE(file_util::ReadFileToString(new_path, &file_contents));
-  EXPECT_EQ(std::string(kTestData), file_contents);
+// Does DownloadCancelled remove Download from appropriate queues?
+// This test tests non-persisted downloads.
+TEST_F(DownloadManagerTest, OnDownloadCancelled_NonPersisted) {
+  // Put a mock we have a handle to on the download manager.
+  int download_id = AddItemToManager();
+  content::MockDownloadItem& item(GetMockDownloadItem(download_id));
+
+  EXPECT_CALL(item, IsPersisted())
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(item, GetState())
+      .WillRepeatedly(Return(DownloadItem::CANCELLED));
+  EXPECT_CALL(item, GetDbHandle())
+      .WillRepeatedly(Return(DownloadItem::kUninitializedHandle));
+
+  EXPECT_CALL(item, OffThreadCancel(&GetMockDownloadFileManager()));
+  download_manager_->DownloadCancelled(&item);

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

Check GetActiveDownloadItem() afterwards?

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

The current behavior (which is that non-persisted downloads stick around as
active even when cancelled, because they're waiting to rendezvous with the
callback from the history system) is one that I'd like to change, and that I
don't think external consumers of the DownloadManager should be relying on, so
I'm not inclined to test GetActiveDownloadItem().  I've put in a TODO about
changing this when we change the download manager behavior.  WDYT?

 }
 
-TEST_F(DownloadManagerTest, MAYBE_DownloadRemoveTest) {
-  using ::testing::_;
-  using ::testing::CreateFunctor;
-  using ::testing::Invoke;
-  using ::testing::Return;
-
-  ASSERT_TRUE(CreateTempDownloadsDirectory());
-
-  // File names we're using.
-  const FilePath new_path(GetPathInDownloadsDir(FILE_PATH_LITERAL("foo.txt")));
-  const FilePath cr_path(GetTempDownloadPath(new_path));
-  EXPECT_FALSE(file_util::PathExists(new_path));
-
-  // Normally, the download system takes ownership of info, and is
-  // responsible for deleting it.  In these unit tests, however, we
-  // don't call the function that deletes it, so we do so ourselves.
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = DownloadId(kValidIdDomain, 0);
-  info->prompt_user_for_save_location = true;
-  info->url_chain.push_back(GURL());
-  scoped_ptr<content::ByteStreamWriter> stream_input;
-  scoped_ptr<content::ByteStreamReader> stream_output;
-  content::CreateByteStream(message_loop_.message_loop_proxy(),
-                            message_loop_.message_loop_proxy(),
-                            kTestDataLen, &stream_input, &stream_output);
-
-  download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
-
-  DownloadItem* download = GetActiveDownloadItem(0);
-  ASSERT_TRUE(download != NULL);
-
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
-
-  // Create and initialize the download file.  We're bypassing the first part
-  // of the download process and skipping to the part after the final file
-  // name has been chosen, so we need to initialize the download file
-  // properly.
-  DownloadFile* download_file(
-      new DownloadFileImpl(info.get(), stream_output.Pass(),
-                           new DownloadRequestHandle(),
-                           download_manager_, false,
-                           scoped_ptr<PowerSaveBlocker>(NULL).Pass(),
-                           net::BoundNetLog()));
-  download_file->Rename(cr_path);
-  // This creates the .temp version of the file.
-  download_file->Initialize();
-  // |download_file| is owned by DownloadFileManager.
-  AddDownloadToFileManager(info->download_id.local(), download_file);
-
-  ContinueDownloadWithPath(download, new_path);
-  message_loop_.RunAllPending();
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
-
-  WriteCopyToStream(kTestData, kTestDataLen, stream_input.get());
-
-  // Finish the download.
-  OnResponseCompleted(0, kTestDataLen, "");
-  message_loop_.RunAllPending();
-
-  // Download is complete.
-  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_FALSE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::COMPLETE, download->GetState());
-
-  EXPECT_TRUE(file_util::PathExists(new_path));
-  EXPECT_FALSE(file_util::PathExists(cr_path));
-
-  // Remove the downloaded file.
-  ASSERT_TRUE(file_util::Delete(new_path, false));
-  download->OnDownloadedFileRemoved();
-  message_loop_.RunAllPending();
-
-  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
-  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
-  EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
-  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
-  EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_opened());
-  EXPECT_TRUE(download->GetFileExternallyRemoved());
-  EXPECT_EQ(DownloadItem::COMPLETE, download->GetState());
-
-  EXPECT_FALSE(file_util::PathExists(new_path));
+// Does DownloadCancelled remove Download from appropriate queues?
+// This test tests persisted downloads.
+TEST_F(DownloadManagerTest, OnDownloadCancelled_Persisted) {
+  // Put a mock we have a handle to on the download manager.
+  int download_id = AddItemToManager();
+  content::MockDownloadItem& item(GetMockDownloadItem(download_id));
+  int64 db_handle = 0x7;
+  AddItemToHistory(download_id, db_handle);
+
+  EXPECT_CALL(item, IsPersisted())
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(GetMockDownloadManagerDelegate(),
+              UpdateItemInPersistentStore(&item));
+  EXPECT_CALL(item, GetState())
+      .WillRepeatedly(Return(DownloadItem::CANCELLED));
+  EXPECT_CALL(item, GetDbHandle())
+      .WillRepeatedly(Return(db_handle));
+
+  EXPECT_CALL(item, OffThreadCancel(&GetMockDownloadFileManager()));
+  download_manager_->DownloadCancelled(&item);

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

Check GetActiveDownloadItem() afterwards?

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

Done.

 }