Make DownloadHistory observe manager, items

chrome/browser/download/download_history_unittest.cc

Index: chrome/browser/download/download_history_unittest.cc
diff --git a/chrome/browser/download/download_history_unittest.cc b/chrome/browser/download/download_history_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ca697bfbac3f63ad5a700efc7a6d8b6c9f313ade
--- /dev/null
+++ b/chrome/browser/download/download_history_unittest.cc
@@ -0,0 +1,512 @@
+// 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 "base/rand_util.h"
+#include "chrome/browser/download/download_history.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 content::BrowserThread;
+using testing::DoAll;
+using testing::Return;
+using testing::ReturnRef;
+using testing::_;
+
+namespace {
+
+bool InfoEqual(const DownloadPersistentStoreInfo& left,
+               const DownloadPersistentStoreInfo& right) {
+  if (left.path != right.path) {
+    LOG(ERROR) << left.path.value() << " != " << right.path.value();

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

Wow, this is going to produce a lot of output, and at a fairly high log level
too.  This might be ok if it only happens when tests fail, but if you call
InfoEqual sometimes expecting the infos not to be equal (which seems like a
reasonable thing for some future test to do even if no current one does) this is
probably overkill.

[benjhayden, 2012/09/21 20:45:46]

Done.

+    return false;
+  } else if (left.url != right.url) {
+    LOG(ERROR) << left.url.spec() << " != " << right.url.spec();
+    return false;
+  } else if (left.referrer_url != right.referrer_url) {
+    LOG(ERROR) << left.referrer_url.spec() << " != "
+               << right.referrer_url.spec();
+    return false;
+  } else if (left.start_time != right.start_time) {
+    LOG(ERROR) << left.start_time.ToTimeT() << " != "
+               << right.start_time.ToTimeT();
+    return false;
+  } else if (left.end_time != right.end_time) {
+    LOG(ERROR) << left.end_time.ToTimeT() << " != " << right.end_time.ToTimeT();
+    return false;
+  } else if (left.received_bytes != right.received_bytes) {
+    LOG(ERROR) << left.received_bytes << " != " << right.received_bytes;
+    return false;
+  } else if (left.total_bytes != right.total_bytes) {
+    LOG(ERROR) << left.total_bytes << " != " << right.total_bytes;
+    return false;
+  } else if (left.state != right.state) {
+    LOG(ERROR) << left.state << " != " << right.state;
+    return false;
+  } else if (left.db_handle != right.db_handle) {
+    LOG(ERROR) << left.db_handle << " != " << right.db_handle;
+    return false;
+  } else if (left.opened != right.opened) {
+    LOG(ERROR) << left.opened << " != " << right.opened;
+    return false;
+  }
+  return true;
+}
+
+typedef std::set<int64> HandleSet;
+typedef std::vector<DownloadPersistentStoreInfo> InfoVector;
+
+class FakeHistoryService
+  : public HistoryService{

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

nit, suggestion: I think it's more customary to have these on the same line.

nit: I'm pretty sure that a space before the { is required.

[benjhayden, 2012/09/21 20:45:46]

Done.

+ public:
+  FakeHistoryService()
+    : slow_create_download_(false),
+      handle_counter_(0) {

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

nit: Curlies on same line is allowed but not required, and I think it our usual
convention.

[benjhayden, 2012/09/21 20:45:46]

When the constructor spans multiple lines, I like how the close curly indicates
the end of the constructor.

+  }
+
+  virtual void QueryDownloads(
+      const HistoryService::DownloadQueryCallback& callback) OVERRIDE {
+    BrowserThread::PostTask(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_.Pass());
+  }
+
+  virtual void GetVisibleVisitCountToHostSimple(
+      const GURL& referrer_url,
+      const HistoryService::GetVisibleVisitCountToHostSimpleCallback&
+        callback) OVERRIDE {
+    NOTIMPLEMENTED();
+  }
+
+  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(callback, handle_counter_++);
+    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 ExpectQueryDownloads(scoped_ptr<InfoVector> infos) {
+    expect_query_downloads_ = infos.Pass();
+  }
+
+  void ExpectQueryDownloadsDone() {
+    CHECK(NULL == expect_query_downloads_.get());
+  }
+
+  void ExpectCreateDownload(
+      const DownloadPersistentStoreInfo& info) {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CHECK(InfoEqual(info, create_download_info_));
+    create_download_info_ = DownloadPersistentStoreInfo();
+  }
+
+  void ExpectNoCreateDownload() {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CHECK(InfoEqual(DownloadPersistentStoreInfo(), create_download_info_));
+  }
+
+  void ExpectUpdateDownload(const DownloadPersistentStoreInfo& info) {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CHECK(InfoEqual(update_download_, info));
+    update_download_ = DownloadPersistentStoreInfo();
+  }
+
+  void ExpectNoUpdateDownload() {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CHECK(InfoEqual(DownloadPersistentStoreInfo(), update_download_));
+  }
+  void ExpectNoRemoveDownloads() {
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    CHECK_EQ(0, static_cast<int>(remove_downloads_.size()));
+  }
+
+  void ExpectRemoveDownloads(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);
+    CHECK(difference.empty());
+    remove_downloads_.clear();
+  }
+
+ private:
+  virtual ~FakeHistoryService() {}
+
+  bool slow_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()),
+      download_history_(NULL) {
+  }
+  virtual ~DownloadHistoryTest() {}
+
+ protected:
+  virtual void CleanUpOnMainThread() OVERRIDE {
+    download_history_.reset();
+  }
+
+  DownloadHistory* download_history() { return download_history_.get(); }
+
+  content::MockDownloadManager& manager() { return *manager_.get(); }
+  content::MockDownloadItem& item() { return item_; }
+
+  void ExpectQueryDownloads(scoped_ptr<InfoVector> infos) {
+    CHECK(infos.get());
+    EXPECT_CALL(manager(), AddObserver(_));
+    EXPECT_CALL(manager(), RemoveObserver(_));
+    if (infos->size() != 0) {
+      EXPECT_EQ(1, static_cast<int>(infos->size()));
+      EXPECT_CALL(manager(), CreateDownloadItem(
+          infos->at(0).path,
+          infos->at(0).url,
+          infos->at(0).referrer_url,
+          infos->at(0).start_time,
+          infos->at(0).end_time,
+          infos->at(0).received_bytes,
+          infos->at(0).total_bytes,
+          infos->at(0).state,
+          infos->at(0).opened))
+        .WillOnce(DoAll(
+            InvokeWithoutArgs(
+                this, &DownloadHistoryTest::CallOnDownloadCreated),
+            Return(&item())));
+    }
+    EXPECT_CALL(manager(), CheckForHistoryFilesRemoval());
+    history_ = new FakeHistoryService();
+    history_->ExpectQueryDownloads(infos.Pass());
+    download_history_.reset(new DownloadHistory(&manager(), history_.get()));
+    content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
+    history_->ExpectQueryDownloadsDone();
+  }
+
+  void CallOnDownloadCreated() {
+    download_history_->OnDownloadCreated(&manager(), &item());
+  }
+
+  void set_slow_create_download(bool slow) {
+    history_->set_slow_create_download(slow);
+  }
+
+  void FinishCreateDownload() {
+    history_->FinishCreateDownload();
+  }
+
+  void ExpectCreateDownload(
+      const DownloadPersistentStoreInfo& info) {
+    history_->ExpectCreateDownload(info);
+  }
+
+  void ExpectNoCreateDownload() {
+    history_->ExpectNoCreateDownload();
+  }
+
+  void ExpectUpdateDownload(const DownloadPersistentStoreInfo& info) {
+    history_->ExpectUpdateDownload(info);
+  }
+
+  void ExpectNoUpdateDownload() {
+    history_->ExpectNoUpdateDownload();
+  }
+
+  void ExpectNoRemoveDownloads() {
+    history_->ExpectNoRemoveDownloads();
+  }
+
+  void ExpectRemoveDownloads(const HandleSet& handles) {
+    history_->ExpectRemoveDownloads(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) {
+    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(), GetId()).WillRepeatedly(Return(id));
+    EXPECT_CALL(item(), GetFullPath()).WillRepeatedly(ReturnRef(path));
+    EXPECT_CALL(item(), GetURL()).WillRepeatedly(ReturnRef(url));
+    EXPECT_CALL(item(), GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
+    EXPECT_CALL(item(), GetStartTime()).WillRepeatedly(Return(start_time));
+    EXPECT_CALL(item(), GetEndTime()).WillRepeatedly(Return(end_time));
+    EXPECT_CALL(item(), GetReceivedBytes())
+      .WillRepeatedly(Return(received_bytes));
+    EXPECT_CALL(item(), GetTotalBytes()).WillRepeatedly(Return(total_bytes));
+    EXPECT_CALL(item(), GetState()).WillRepeatedly(Return(state));
+    EXPECT_CALL(item(), GetOpened()).WillRepeatedly(Return(opened));
+    EXPECT_CALL(item(), GetTargetDisposition()).WillRepeatedly(Return(
+          content::DownloadItem::TARGET_DISPOSITION_OVERWRITE));
+    EXPECT_CALL(manager(), GetDownload(id))
+      .WillRepeatedly(Return(&item()));
+    EXPECT_CALL(item(), AddObserver(_));
+    EXPECT_CALL(item(), RemoveObserver(_));
+  }
+
+ private:
+  testing::NiceMock<content::MockDownloadItem> item_;
+  scoped_refptr<FakeHistoryService> history_;
+  scoped_refptr<content::MockDownloadManager> manager_;
+  scoped_ptr<DownloadHistory> download_history_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadHistoryTest);
+};
+
+}  // anonymous namespace
+
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,
+    DownloadHistoryTest_Load) {

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

nit, suggestion: It looks really weird to me to have these on different lines.

[benjhayden, 2012/09/21 20:45:46]

Done.

+  // Load a download from history, create the item, OnDownloadCreated,
+  // OnDownloadUpdated, OnDownloadRemoved, OnDownloadDestroyed.
+  DownloadPersistentStoreInfo info;
+  InitItem(base::RandInt(0, 1 << 20),
+           FilePath(FILE_PATH_LITERAL("/foo/bar.pdf")),
+           GURL("http://example.com/bar.pdf"),
+           GURL("http://example.com/referrer.html"),
+           (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);
+  {

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

Why the extra curlies?

[benjhayden, 2012/09/21 20:45:46]

To delineate the lifetime of infos.

+    scoped_ptr<InfoVector> infos(new InfoVector());
+    infos->push_back(info);
+    ExpectQueryDownloads(infos.Pass());
+    ExpectNoCreateDownload();
+  }
+
+  // Pretend that something changed on the item.
+  EXPECT_CALL(item(), GetOpened()).WillRepeatedly(Return(true));
+  download_history()->OnDownloadUpdated(&item());
+  info.opened = true;
+  ExpectUpdateDownload(info);

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

For what it's worth, I find it confusing to read through a test when in some
cases Expect* sometimes means "in the future I expect to see this" and sometimes
means "I expect I will already have seen this".  I know that we already have
that distinction in the support libraries (EXPECT_EQ versus EXPECT_CALL), but
would you be willing to make the distinction in your routine names for routines
you write?

[benjhayden, 2012/09/21 20:45:46]

Done.

+
+  // Pretend that the user removed the item.
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  download_history()->OnDownloadRemoved(&item());
+  ExpectRemoveDownloads(handles);
+
+  // Pretend that the browser is closing.
+  download_history()->ManagerGoingDown(&manager());
+  download_history()->OnDownloadDestroyed(&item());
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

Intro line or two before tests describing what you're testing?  (Applies to all
tests.)

[benjhayden, 2012/09/21 20:45:46]

Done.

+    DownloadHistoryTest_Create) {
+  // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
+  // OnDownloadRemoved, OnDownloadDestroyed.
+  ExpectQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

Huh; I wouldn't have thought this would compile (I'd think you'd need the
.Pass() at this level too).

[benjhayden, 2012/09/21 20:45:46]

I think this is what compilers call a "temporary".

+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  InitItem(base::RandInt(0, 1 << 20),
+           FilePath(FILE_PATH_LITERAL("/foo/bar.pdf")),
+           GURL("http://example.com/bar.pdf"),
+           GURL("http://example.com/referrer.html"),
+           (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|.
+  download_history()->OnDownloadCreated(&manager(), &item());
+  // CreateDownload() always gets db_handle=-1.
+  ExpectCreateDownload(info);
+  info.db_handle = 0;
+
+  // Pretend that something changed on the item.
+  EXPECT_CALL(item(), GetOpened()).WillRepeatedly(Return(true));
+  download_history()->OnDownloadUpdated(&item());
+  info.opened = true;
+  ExpectUpdateDownload(info);
+
+  // Pretend that the user removed the item.
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  download_history()->OnDownloadRemoved(&item());
+  ExpectRemoveDownloads(handles);
+
+  // Pretend that the browser is closing.
+  download_history()->ManagerGoingDown(&manager());
+  download_history()->OnDownloadDestroyed(&item());
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,
+    DownloadHistoryTest_Temporary) {
+  // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
+  // OnDownloadRemoved, OnDownloadDestroyed.
+  ExpectQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  InitItem(base::RandInt(0, 1 << 20),
+           FilePath(FILE_PATH_LITERAL("/foo/bar.pdf")),
+           GURL("http://example.com/bar.pdf"),
+           GURL("http://example.com/referrer.html"),
+           (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|.
+  download_history()->OnDownloadCreated(&manager(), &item());
+  // CreateDownload() always gets db_handle=-1.
+  ExpectCreateDownload(info);
+  info.db_handle = 0;
+
+  // Pretend the item was marked temporary. DownloadHistory should remove it
+  // from history and start ignoring it.
+  EXPECT_CALL(item(), IsTemporary()).WillRepeatedly(Return(true));
+  download_history()->OnDownloadUpdated(&item());
+  HandleSet handles;
+  handles.insert(info.db_handle);
+  ExpectRemoveDownloads(handles);
+
+  // Change something that would make DownloadHistory call UpdateDownload if the
+  // item weren't temporary.
+  EXPECT_CALL(item(), GetReceivedBytes()).WillRepeatedly(Return(4200));
+  download_history()->OnDownloadUpdated(&item());
+  ExpectNoUpdateDownload();
+
+  // Changing a temporary item back to a non-temporary item should make
+  // DownloadHistory call CreateDownload.
+  EXPECT_CALL(item(), IsTemporary()).WillRepeatedly(Return(false));
+  download_history()->OnDownloadUpdated(&item());
+  info.received_bytes = 4200;
+  info.db_handle = -1;
+  // CreateDownload() always gets db_handle=-1.
+  ExpectCreateDownload(info);
+  info.db_handle = 1;
+
+  EXPECT_CALL(item(), GetReceivedBytes()).WillRepeatedly(Return(100));
+  download_history()->OnDownloadUpdated(&item());
+  info.received_bytes = 100;
+  ExpectUpdateDownload(info);
+
+  // Pretend that the browser is closing.
+  download_history()->ManagerGoingDown(&manager());
+  download_history()->OnDownloadDestroyed(&item());
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,
+    DownloadHistoryTest_RemoveWhileAdding) {

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

Do you test the straight remove path anywhere in here?

[benjhayden, 2012/09/21 20:45:46]

DownloadHistoryTest_Create tests OnDownloadRemoved().

+  ExpectQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  // Note that db_handle must be -1 at first because it isn't in the db yet.
+  DownloadPersistentStoreInfo info;
+  InitItem(base::RandInt(0, 1 << 20),
+           FilePath(FILE_PATH_LITERAL("/foo/bar.pdf")),
+           GURL("http://example.com/bar.pdf"),
+           GURL("http://example.com/referrer.html"),
+           (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|.
+  download_history()->OnDownloadCreated(&manager(), &item());
+  // CreateDownload() always gets db_handle=-1.
+  ExpectCreateDownload(info);
+  info.db_handle = 0;
+
+  // Call OnDownloadRemoved before calling back to DownloadHistory::ItemAdded().
+  // Instead of calling RemoveDownloads() immediately, it should
+  download_history()->OnDownloadRemoved(&item());
+  EXPECT_CALL(manager(), GetDownload(item().GetId()))
+    .WillRepeatedly(Return(static_cast<content::DownloadItem*>(NULL)));
+  ExpectNoRemoveDownloads();
+
+  // 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);
+  ExpectRemoveDownloads(handles);
+
+  // Pretend that the browser is closing.
+  download_history()->ManagerGoingDown(&manager());
+  download_history()->OnDownloadDestroyed(&item());
+}