Make DownloadHistory observe manager, items

chrome/browser/download/download_history_unittest.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 <set>
+#include <vector>
+
+#include "base/rand_util.h"
+#include "base/stl_util.h"
+#include "chrome/browser/download/download_history.h"
+#include "chrome/browser/history/download_database.h"
+#include "chrome/browser/history/download_persistent_store_info.h"
+#include "chrome/browser/history/history.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/test/mock_download_item.h"
+#include "content/public/test/mock_download_manager.h"
+#include "content/public/test/test_utils.h"
+
+using testing::DoAll;
+using testing::Invoke;
+using testing::Return;
+using testing::ReturnRef;
+using testing::SetArgPointee;
+using testing::WithArg;
+using testing::_;
+
+namespace {
+
+void CheckInfoEqual(const DownloadPersistentStoreInfo& left,
+                    const DownloadPersistentStoreInfo& right) {
+  CHECK_EQ(left.path.value(), right.path.value());

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Why CHECK instead of EXPECT?  That'll give you a crash as opposed to a failed
test.

[benjhayden, 2012/11/12 18:44:16]

Done.

+  CHECK_EQ(left.url.spec(), right.url.spec());
+  CHECK_EQ(left.referrer_url.spec(), right.referrer_url.spec());
+  CHECK_EQ(left.start_time.ToTimeT(), right.start_time.ToTimeT());
+  CHECK_EQ(left.end_time.ToTimeT(), right.end_time.ToTimeT());
+  CHECK_EQ(left.received_bytes, right.received_bytes);
+  CHECK_EQ(left.total_bytes, right.total_bytes);
+  CHECK_EQ(left.state, right.state);
+  CHECK_EQ(left.db_handle, right.db_handle);
+  CHECK_EQ(left.opened, right.opened);
+}
+
+typedef std::set<int64> HandleSet;
+typedef std::vector<DownloadPersistentStoreInfo> InfoVector;
+typedef testing::NiceMock<content::MockDownloadItem> NiceMockDownloadItem;
+
+class FakeHistoryService : public DownloadHistory::HistoryAdapter {

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

nit: I think the name should now be FakeHistoryAdapter?

[benjhayden, 2012/11/12 18:44:16]

Done.

+ public:
+  FakeHistoryService()
+    : DownloadHistory::HistoryAdapter(NULL),
+      slow_create_download_(false),
+      fail_create_download_(false),
+      handle_counter_(0) {
+  }
+
+  virtual ~FakeHistoryService() {}
+
+  virtual void QueryDownloads(
+      const HistoryService::DownloadQueryCallback& callback) OVERRIDE {

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Suggestion: It would be slightly easier to understand how these functions were
going to be used with DCHECK(CurrentlyOn...)) or comments about threads methods
run on.

[benjhayden, 2012/11/12 18:44:16]

Done.

+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&FakeHistoryService::QueryDownloadsDone,
+            base::Unretained(this), callback));
+  }
+
+  void QueryDownloadsDone(
+      const HistoryService::DownloadQueryCallback& callback) {
+    CHECK(expect_query_downloads_.get());
+    callback.Run(expect_query_downloads_.get());
+    expect_query_downloads_.reset();
+  }
+
+  void set_slow_create_download(bool slow) { slow_create_download_ = slow; }
+
+  virtual void CreateDownload(
+      const DownloadPersistentStoreInfo& info,
+      const HistoryService::DownloadCreateCallback& callback) OVERRIDE {
+    create_download_info_ = info;
+    create_download_callback_ = base::Bind(

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Suggestion: The implication of just having a single place to store the callback
is that you'll always be waiting until FinishCreateDownload() is called before
the next invocation of CreateDownload().  Is it worthwhile putting in a DCHECK()
so that someone who violates that invariant in a new test realizes quickly that
they have to update this code?

[benjhayden, 2012/11/12 18:44:16]

Done.

+        callback,
+        (fail_create_download_ ?
+          history::DownloadDatabase::kUninitializedHandle :
+          handle_counter_++));
+    fail_create_download_ = false;
+    if (!slow_create_download_)
+      FinishCreateDownload();
+  }
+
+  void FinishCreateDownload() {
+    create_download_callback_.Run();
+    create_download_callback_.Reset();
+  }
+
+  virtual void UpdateDownload(
+      const DownloadPersistentStoreInfo& info) OVERRIDE {
+    update_download_ = info;
+  }
+
+  virtual void RemoveDownloads(const HandleSet& handles) OVERRIDE {
+    for (HandleSet::const_iterator it = handles.begin();
+         it != handles.end(); ++it) {
+      remove_downloads_.insert(*it);
+    }
+  }
+
+  void ExpectWillQueryDownloads(scoped_ptr<InfoVector> infos) {
+    expect_query_downloads_ = infos.Pass();
+  }
+
+  void ExpectQueryDownloadsDone() {
+    CHECK(NULL == expect_query_downloads_.get());
+  }
+
+  void FailCreateDownload() {
+    fail_create_download_ = true;
+  }
+
+  void ExpectDownloadCreated(
+      const DownloadPersistentStoreInfo& info) {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CheckInfoEqual(info, create_download_info_);
+    create_download_info_ = DownloadPersistentStoreInfo();
+  }
+
+  void ExpectNoDownloadCreated() {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CheckInfoEqual(DownloadPersistentStoreInfo(), create_download_info_);
+  }
+
+  void ExpectDownloadUpdated(const DownloadPersistentStoreInfo& info) {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CheckInfoEqual(update_download_, info);
+    update_download_ = DownloadPersistentStoreInfo();
+  }
+
+  void ExpectNoDownloadUpdated() {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CheckInfoEqual(DownloadPersistentStoreInfo(), update_download_);
+  }
+
+  void ExpectNoDownloadsRemoved() {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CHECK_EQ(0, static_cast<int>(remove_downloads_.size()));
+  }
+
+  void ExpectDownloadsRemoved(const HandleSet& handles) {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    HandleSet difference;
+    std::insert_iterator<HandleSet> insert_it(
+        difference, difference.begin());
+    std::set_difference(handles.begin(), handles.end(),
+                        remove_downloads_.begin(),
+                        remove_downloads_.end(),
+                        insert_it);
+    for (HandleSet::const_iterator oops = difference.begin();
+         oops != difference.end(); ++oops) {
+      LOG(ERROR) << __FUNCTION__ << " " << *oops;

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

I think of "EXPECT_TRUE(false) << ..." as a better idiom for tests--it signals
that you're outputting test data for failing expectations.  

[benjhayden, 2012/11/12 18:44:16]

Done.

+    }
+    CHECK(difference.empty());

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Why a CHECK?  Isn't this a test expectation, and hence something you want to
signal failure for rather than crash?  (And if you follow my suggestion above,
you don't need the extra signaling.)

[benjhayden, 2012/11/12 18:44:16]

Done.

+    remove_downloads_.clear();
+  }
+
+ private:
+  bool slow_create_download_;
+  bool fail_create_download_;
+  base::Closure create_download_callback_;
+  int handle_counter_;
+  DownloadPersistentStoreInfo update_download_;
+  scoped_ptr<InfoVector> expect_query_downloads_;
+  HandleSet remove_downloads_;
+  DownloadPersistentStoreInfo create_download_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeHistoryService);
+};
+
+class DownloadHistoryTest : public InProcessBrowserTest {
+ public:
+  DownloadHistoryTest()
+    : manager_(new content::MockDownloadManager()),
+      history_(NULL),
+      download_history_(NULL),
+      manager_observer_(NULL),
+      item_observer_(NULL),
+      download_created_index_(0) {
+  }
+  virtual ~DownloadHistoryTest() {
+    STLDeleteElements(&items_);
+  }
+
+ protected:
+  virtual void CleanUpOnMainThread() OVERRIDE {
+    download_history_.reset();
+  }
+
+  content::MockDownloadManager& manager() { return *manager_.get(); }
+  content::MockDownloadItem& item(size_t index) { return *items_[index]; }
+
+  void SetManagerObserver(
+      content::DownloadManager::Observer* manager_observer) {
+    manager_observer_ = manager_observer;
+  }
+  content::DownloadManager::Observer* manager_observer() {
+    return manager_observer_;
+  }
+  void SetItemObserver(
+      content::DownloadItem::Observer* item_observer) {
+    item_observer_ = item_observer;
+  }
+  content::DownloadItem::Observer* item_observer() {
+    return item_observer_;
+  }
+
+  void ExpectWillQueryDownloads(scoped_ptr<InfoVector> infos) {
+    CHECK(infos.get());
+    EXPECT_CALL(manager(), AddObserver(_)).WillOnce(WithArg<0>(Invoke(
+        this, &DownloadHistoryTest::SetManagerObserver)));
+    EXPECT_CALL(manager(), RemoveObserver(_));
+    download_created_index_ = 0;
+    for (size_t index = 0; index < infos->size(); ++index) {
+      EXPECT_CALL(manager(), CreateDownloadItem(
+          infos->at(index).path,
+          infos->at(index).url,
+          infos->at(index).referrer_url,
+          infos->at(index).start_time,
+          infos->at(index).end_time,
+          infos->at(index).received_bytes,
+          infos->at(index).total_bytes,
+          infos->at(index).state,
+          infos->at(index).opened))
+        .WillOnce(DoAll(
+            InvokeWithoutArgs(
+                this, &DownloadHistoryTest::CallOnDownloadCreatedInOrder),
+            Return(&item(index))));
+    }
+    EXPECT_CALL(manager(), CheckForHistoryFilesRemoval());
+    history_ = new FakeHistoryService();
+    history_->ExpectWillQueryDownloads(infos.Pass());
+    EXPECT_CALL(*manager_.get(), GetAllDownloads(_)).WillRepeatedly(Return());
+    download_history_.reset(new DownloadHistory(
+        &manager(), scoped_ptr<DownloadHistory::HistoryAdapter>(history_)));
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    history_->ExpectQueryDownloadsDone();
+  }
+
+  void CallOnDownloadCreated(size_t index) {
+    manager_observer()->OnDownloadCreated(&manager(), &item(index));
+  }
+
+  void CallOnDownloadCreatedInOrder() {
+    // Gmock doesn't appear to support something like InvokeWithTheseArgs. Maybe
+    // gmock needs to learn about base::Callback.
+    CallOnDownloadCreated(download_created_index_++);
+  }
+
+  void set_slow_create_download(bool slow) {
+    history_->set_slow_create_download(slow);
+  }
+
+  void FinishCreateDownload() {
+    history_->FinishCreateDownload();
+  }
+
+  void FailCreateDownload() {
+    history_->FailCreateDownload();
+  }
+
+  void ExpectDownloadCreated(
+      const DownloadPersistentStoreInfo& info) {
+    history_->ExpectDownloadCreated(info);
+  }
+
+  void ExpectNoDownloadCreated() {
+    history_->ExpectNoDownloadCreated();
+  }
+
+  void ExpectDownloadUpdated(const DownloadPersistentStoreInfo& info) {
+    history_->ExpectDownloadUpdated(info);
+  }
+
+  void ExpectNoDownloadUpdated() {
+    history_->ExpectNoDownloadUpdated();
+  }
+
+  void ExpectNoDownloadsRemoved() {
+    history_->ExpectNoDownloadsRemoved();
+  }
+
+  void ExpectDownloadsRemoved(const HandleSet& handles) {
+    history_->ExpectDownloadsRemoved(handles);
+  }
+
+  void InitItem(
+      int32 id,
+      const FilePath& path,
+      const GURL& url,
+      const GURL& referrer,
+      const base::Time& start_time,
+      const base::Time& end_time,
+      int64 received_bytes,
+      int64 total_bytes,
+      content::DownloadItem::DownloadState state,
+      int64 db_handle,
+      bool opened,
+      DownloadPersistentStoreInfo* info) {
+    int32 index = items_.size();
+    NiceMockDownloadItem* mock_item = new NiceMockDownloadItem();
+    items_.push_back(mock_item);
+
+    info->path = path;
+    info->url = url;
+    info->referrer_url = referrer;
+    info->start_time = start_time;
+    info->end_time = end_time;
+    info->received_bytes = received_bytes;
+    info->total_bytes = total_bytes;
+    info->state = state;
+    info->db_handle = db_handle;
+    info->opened = opened;
+
+    EXPECT_CALL(item(index), GetId()).WillRepeatedly(Return(id));
+    EXPECT_CALL(item(index), GetFullPath()).WillRepeatedly(ReturnRef(path));
+    EXPECT_CALL(item(index), GetURL()).WillRepeatedly(ReturnRef(url));
+    EXPECT_CALL(item(index), GetMimeType()).WillRepeatedly(Return(
+        "application/octet-stream"));
+    EXPECT_CALL(item(index), GetReferrerUrl()).WillRepeatedly(ReturnRef(
+        referrer));
+    EXPECT_CALL(item(index), GetStartTime()).WillRepeatedly(Return(start_time));
+    EXPECT_CALL(item(index), GetEndTime()).WillRepeatedly(Return(end_time));
+    EXPECT_CALL(item(index), GetReceivedBytes())
+        .WillRepeatedly(Return(received_bytes));
+    EXPECT_CALL(item(index), GetTotalBytes()).WillRepeatedly(Return(
+        total_bytes));
+    EXPECT_CALL(item(index), GetState()).WillRepeatedly(Return(state));
+    EXPECT_CALL(item(index), GetOpened()).WillRepeatedly(Return(opened));
+    EXPECT_CALL(item(index), GetTargetDisposition()).WillRepeatedly(Return(
+          content::DownloadItem::TARGET_DISPOSITION_OVERWRITE));
+    EXPECT_CALL(manager(), GetDownload(id))
+        .WillRepeatedly(Return(&item(index)));
+    EXPECT_CALL(item(index), AddObserver(_)).WillOnce(WithArg<0>(Invoke(
+        this, &DownloadHistoryTest::SetItemObserver)));
+    EXPECT_CALL(item(index), RemoveObserver(_));
+
+    std::vector<content::DownloadItem*> items;
+    for (size_t i = 0; i < items_.size(); ++i) {
+      items.push_back(&item(i));
+    }
+    EXPECT_CALL(*manager_.get(), GetAllDownloads(_))
+      .WillRepeatedly(SetArgPointee<0>(items));
+  }
+
+ private:
+  std::vector<NiceMockDownloadItem*> items_;
+  scoped_refptr<content::MockDownloadManager> manager_;
+  FakeHistoryService* history_;
+  scoped_ptr<DownloadHistory> download_history_;
+  content::DownloadManager::Observer* manager_observer_;
+  content::DownloadItem::Observer* item_observer_;
+  size_t download_created_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadHistoryTest);
+};
+
+}  // anonymous namespace
+
+// Test loading an item from the database, changing it, saving it back, removing
+// it.
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
+  // Load a download from history, create the item, OnDownloadCreated,
+  // OnDownloadUpdated, OnDownloadRemoved.
+  DownloadPersistentStoreInfo info;
+  FilePath path(FILE_PATH_LITERAL("/foo/bar.pdf"));
+  GURL url("http://example.com/bar.pdf");
+  GURL referrer("http://example.com/referrer.html");
+  InitItem(base::RandInt(0, 1 << 20),
+           path,
+           url,
+           referrer,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(10)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(1)),
+           100,
+           100,
+           content::DownloadItem::COMPLETE,
+           base::RandInt(0, 1 << 20),
+           false,
+           &info);
+  {
+    scoped_ptr<InfoVector> infos(new InfoVector());
+    infos->push_back(info);
+    ExpectWillQueryDownloads(infos.Pass());
+    ExpectNoDownloadCreated();
+  }
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Pretend that something changed on the item.
+  EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
+  item_observer()->OnDownloadUpdated(&item(0));
+  info.opened = true;
+  ExpectDownloadUpdated(info);
+
+  // Pretend that the user removed the item.
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  item_observer()->OnDownloadRemoved(&item(0));
+  ExpectDownloadsRemoved(handles);
+
+  // Pretend that the browser is closing.
+  manager_observer()->ManagerGoingDown(&manager());
+  item_observer()->OnDownloadDestroyed(&item(0));

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Just a crash test, or is there some behavior on the history you can test here?

[benjhayden, 2012/11/12 18:44:16]

I forget why I wrote those. They're swallowed by ADIN.

+}
+
+// Test creating an item, saving it to the database, changing it, saving it
+// back, removing it.
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_Create) {
+  // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
+  // OnDownloadRemoved.
+  ExpectWillQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  FilePath path(FILE_PATH_LITERAL("/foo/bar.pdf"));
+  GURL url("http://example.com/bar.pdf");
+  GURL referrer("http://example.com/referrer.html");
+  InitItem(base::RandInt(0, 1 << 20),
+           path,
+           url,
+           referrer,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(10)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(1)),
+           100,
+           100,
+           content::DownloadItem::COMPLETE,
+           -1,
+           false,
+           &info);
+
+  // Pretend the manager just created |item|.
+  CallOnDownloadCreated(0);
+  // CreateDownload() always gets db_handle=-1.
+  ExpectDownloadCreated(info);
+  info.db_handle = 0;
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Pretend that something changed on the item.
+  EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
+  item_observer()->OnDownloadUpdated(&item(0));
+  info.opened = true;
+  ExpectDownloadUpdated(info);
+
+  // Pretend that the user removed the item.
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  item_observer()->OnDownloadRemoved(&item(0));
+  ExpectDownloadsRemoved(handles);
+
+  // Pretend that the browser is closing.
+  manager_observer()->ManagerGoingDown(&manager());
+  item_observer()->OnDownloadDestroyed(&item(0));
+}
+
+// Test creating a new item, saving it, removing it by setting it Temporary,
+// changing it without saving it back because it's Temporary, clearing
+// IsTemporary, saving it back, changing it, saving it back because it isn't
+// Temporary anymore.
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_Temporary) {
+  // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
+  // OnDownloadRemoved.
+  ExpectWillQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  FilePath path(FILE_PATH_LITERAL("/foo/bar.pdf"));
+  GURL url("http://example.com/bar.pdf");
+  GURL referrer("http://example.com/referrer.html");
+  InitItem(base::RandInt(0, 1 << 20),
+           path,
+           url,
+           referrer,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(10)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(1)),
+           100,
+           100,
+           content::DownloadItem::COMPLETE,
+           -1,
+           false,
+           &info);
+
+  // Pretend the manager just created |item|.
+  CallOnDownloadCreated(0);
+  // CreateDownload() always gets db_handle=-1.
+  ExpectDownloadCreated(info);
+  info.db_handle = 0;
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Pretend the item was marked temporary. DownloadHistory should remove it
+  // from history and start ignoring it.
+  EXPECT_CALL(item(0), IsTemporary()).WillRepeatedly(Return(true));
+  item_observer()->OnDownloadUpdated(&item(0));
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  ExpectDownloadsRemoved(handles);
+
+  // Change something that would make DownloadHistory call UpdateDownload if the
+  // item weren't temporary.
+  EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(4200));
+  item_observer()->OnDownloadUpdated(&item(0));
+  ExpectNoDownloadUpdated();
+
+  // Changing a temporary item back to a non-temporary item should make
+  // DownloadHistory call CreateDownload.
+  EXPECT_CALL(item(0), IsTemporary()).WillRepeatedly(Return(false));
+  item_observer()->OnDownloadUpdated(&item(0));
+  info.received_bytes = 4200;
+  info.db_handle = -1;
+  // CreateDownload() always gets db_handle=-1.
+  ExpectDownloadCreated(info);
+  info.db_handle = 1;
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+
+  EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(100));
+  item_observer()->OnDownloadUpdated(&item(0));
+  info.received_bytes = 100;
+  ExpectDownloadUpdated(info);
+
+  // Pretend that the browser is closing.
+  manager_observer()->ManagerGoingDown(&manager());
+  item_observer()->OnDownloadDestroyed(&item(0));
+}
+
+// Test removing downloads while they're still being added.
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,
+    DownloadHistoryTest_RemoveWhileAdding) {
+  ExpectWillQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  FilePath path(FILE_PATH_LITERAL("/foo/bar.pdf"));
+  GURL url("http://example.com/bar.pdf");
+  GURL referrer("http://example.com/referrer.html");
+  InitItem(base::RandInt(0, 1 << 20),
+           path,
+           url,
+           referrer,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(10)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(1)),
+           100,
+           100,
+           content::DownloadItem::COMPLETE,
+           -1,
+           false,
+           &info);
+
+  // Instruct CreateDownload() to not callback to DownloadHistory immediately,
+  // but to wait for FinishCreateDownload().
+  set_slow_create_download(true);
+
+  // Pretend the manager just created |item|.
+  CallOnDownloadCreated(0);
+  // CreateDownload() always gets db_handle=-1.
+  ExpectDownloadCreated(info);
+  info.db_handle = 0;
+  EXPECT_FALSE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Call OnDownloadRemoved before calling back to DownloadHistory::ItemAdded().
+  // Instead of calling RemoveDownloads() immediately, it should
+  item_observer()->OnDownloadRemoved(&item(0));
+  EXPECT_CALL(manager(), GetDownload(item(0).GetId()))
+    .WillRepeatedly(Return(static_cast<content::DownloadItem*>(NULL)));
+  ExpectNoDownloadsRemoved();
+  EXPECT_FALSE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Now callback to DownloadHistory::ItemAdded(), and expect a call to
+  // RemoveDownloads() for the item that was removed while it was being added.
+  FinishCreateDownload();
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  ExpectDownloadsRemoved(handles);
+  EXPECT_FALSE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Pretend that the browser is closing.
+  manager_observer()->ManagerGoingDown(&manager());
+  item_observer()->OnDownloadDestroyed(&item(0));
+}
+
+// Test loading multiple items from the database and removing them all.
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_Multiple) {
+  // Load a download from history, create the item, OnDownloadCreated,
+  // OnDownloadUpdated, OnDownloadRemoved.
+  DownloadPersistentStoreInfo info0, info1;
+  FilePath path0(FILE_PATH_LITERAL("/foo/bar.pdf"));
+  GURL url0("http://example.com/bar.pdf");
+  GURL referrer0("http://example.com/referrer.html");
+  InitItem(base::RandInt(0, 1 << 10),
+           path0,
+           url0,
+           referrer0,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(11)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(2)),
+           100,
+           100,
+           content::DownloadItem::COMPLETE,
+           base::RandInt(0, 1 << 10),
+           false,
+           &info0);
+  FilePath path1(FILE_PATH_LITERAL("/foo/qux.pdf"));
+  GURL url1("http://example.com/qux.pdf");
+  GURL referrer1("http://example.com/referrer.html");
+  InitItem(item(0).GetId() + base::RandInt(1, 1 << 10),
+           path1,
+           url1,
+           referrer1,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(10)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(1)),
+           200,
+           200,
+           content::DownloadItem::COMPLETE,
+           info0.db_handle + base::RandInt(1, 1 << 10),
+           false,
+           &info1);
+  {
+    scoped_ptr<InfoVector> infos(new InfoVector());
+    infos->push_back(info0);
+    infos->push_back(info1);
+    ExpectWillQueryDownloads(infos.Pass());
+    ExpectNoDownloadCreated();
+  }
+
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(1)));
+
+  // Pretend that the user removed both items.
+  HandleSet handles;
+  handles.insert(info0.db_handle);
+  handles.insert(info1.db_handle);
+  item_observer()->OnDownloadRemoved(&item(0));
+  item_observer()->OnDownloadRemoved(&item(1));
+  ExpectDownloadsRemoved(handles);
+
+  // Pretend that the browser is closing.
+  manager_observer()->ManagerGoingDown(&manager());
+  item_observer()->OnDownloadDestroyed(&item(0));
+  item_observer()->OnDownloadDestroyed(&item(1));
+}
+
+// Test what happens when HistoryService/CreateDownload::CreateDownload() fails.
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_CreateFailed) {
+  // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
+  // OnDownloadRemoved.
+  ExpectWillQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  FilePath path(FILE_PATH_LITERAL("/foo/bar.pdf"));
+  GURL url("http://example.com/bar.pdf");
+  GURL referrer("http://example.com/referrer.html");
+  InitItem(base::RandInt(0, 1 << 20),
+           path,
+           url,
+           referrer,
+           (base::Time::Now() - base::TimeDelta::FromMinutes(10)),
+           (base::Time::Now() - base::TimeDelta::FromMinutes(1)),
+           100,
+           100,
+           content::DownloadItem::COMPLETE,
+           -1,
+           false,
+           &info);
+
+  FailCreateDownload();
+  // Pretend the manager just created |item|.
+  CallOnDownloadCreated(0);
+  // CreateDownload() always gets db_handle=-1.
+  ExpectDownloadCreated(info);
+  EXPECT_FALSE(DownloadHistory::IsPersisted(&item(0)));
+
+  EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(100));
+  item_observer()->OnDownloadUpdated(&item(0));
+  info.received_bytes = 100;
+  ExpectDownloadCreated(info);
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+
+  // Pretend that the browser is closing.
+  manager_observer()->ManagerGoingDown(&manager());
+  item_observer()->OnDownloadDestroyed(&item(0));
+}