[Downloads] Ask DownloadHistory if a download was from history.

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
index 0316883af53315d071c2e778266ce085da25f2e0..1b1918e26ebc34edd1302b5488a368d056cb5de3 100644
--- a/chrome/browser/download/download_history_unittest.cc
+++ b/chrome/browser/download/download_history_unittest.cc
@@ -5,6 +5,7 @@
 #include <set>
 #include <vector>
 
+#include "base/memory/weak_ptr.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "chrome/browser/download/download_history.h"
@@ -193,6 +194,9 @@ class FakeHistoryAdapter : public DownloadHistory::HistoryAdapter {
 
 class DownloadHistoryTest : public testing::Test {
  public:
+  // Generic callback that receives a pointer to a StrictMockDownloadItem.
+  typedef base::Callback<void(content::MockDownloadItem*)> DownloadItemCallback;
+
   DownloadHistoryTest()
       : ui_thread_(content::BrowserThread::UI, &loop_),
         manager_(new content::MockDownloadManager()),
@@ -211,6 +215,7 @@ class DownloadHistoryTest : public testing::Test {
 
   content::MockDownloadManager& manager() { return *manager_.get(); }
   content::MockDownloadItem& item(size_t index) { return *items_[index]; }
+  DownloadHistory* download_history() { return download_history_.get(); }
 
   void SetManagerObserver(
       content::DownloadManager::Observer* manager_observer) {
@@ -271,7 +276,11 @@ class DownloadHistoryTest : public testing::Test {
 
   void CallOnDownloadCreated(size_t index) {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    if (!pre_on_create_handler_.is_null())
+      pre_on_create_handler_.Run(&item(index));
     manager_observer()->OnDownloadCreated(&manager(), &item(index));
+    if (!post_on_create_handler_.is_null())
+      post_on_create_handler_.Run(&item(index));
   }
 
   void CallOnDownloadCreatedInOrder() {
@@ -327,6 +336,18 @@ class DownloadHistoryTest : public testing::Test {
     history_->ExpectDownloadsRemoved(ids);
   }
 
+  void ExpectDownloadsRestoredFromHistory(bool expected_value) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    pre_on_create_handler_ =
+        base::Bind(&DownloadHistoryTest::CheckDownloadWasRestoredFromHistory,
+                   base::Unretained(this),
+                   expected_value);
+    post_on_create_handler_ =
+        base::Bind(&DownloadHistoryTest::CheckDownloadWasRestoredFromHistory,
+                   base::Unretained(this),
+                   expected_value);
+  }
+
   void InitBasicItem(const base::FilePath::CharType* path,
                      const char* url_string,
                      const char* referrer_string,
@@ -450,6 +471,12 @@ class DownloadHistoryTest : public testing::Test {
   }
 
  private:
+  void CheckDownloadWasRestoredFromHistory(bool expected_value,
+                                           content::MockDownloadItem* item) {
+    ASSERT_TRUE(download_history_.get());
+    EXPECT_EQ(expected_value, download_history_->WasRestoredFromHistory(item));
+  }
+
   base::MessageLoopForUI loop_;
   content::TestBrowserThread ui_thread_;
   std::vector<StrictMockDownloadItem*> items_;
@@ -459,12 +486,12 @@ class DownloadHistoryTest : public testing::Test {
   content::DownloadManager::Observer* manager_observer_;
   content::DownloadItem::Observer* item_observer_;
   size_t download_created_index_;
+  DownloadItemCallback pre_on_create_handler_;
+  DownloadItemCallback post_on_create_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadHistoryTest);
 };
 
-}  // anonymous namespace
-
 // Test loading an item from the database, changing it, saving it back, removing
 // it.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
@@ -496,6 +523,39 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
   ExpectDownloadsRemoved(ids);
 }
 
+// Test that WasRestoredFromHistory accurately identifies downloads that were
+// created from history, even during an OnDownloadCreated() handler.
+TEST_F(DownloadHistoryTest, DownloadHistoryTest_WasRestoredFromHistory_True) {
+  ExpectDownloadsRestoredFromHistory(true);
+  history::DownloadRow info;
+  InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
+                "http://example.com/bar.pdf",
+                "http://example.com/referrer.html",
+                &info);
+  {
+    scoped_ptr<InfoVector> infos(new InfoVector());
+    infos->push_back(info);
+    ExpectWillQueryDownloads(infos.Pass());

[Randy Smith (Not in Mondays), 2014/04/21 18:21:22]

This may be due to my own cluelessness (I kept mistaking the version of
ExpectWillQueryDownloads on the history adapter for the one on the
DownloadHistoryTest that's actually called by this function) but could you
clarify that this function actually does something rather than setup an
expectation?  By changing the name or putting in a comment or something similar.

[asanka, 2014/04/23 05:51:09]

I changed the name.

+  }
+  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
+}
+
+// Test that WasRestoredFromHistory accurately identifies downloads that were
+// not created from history.
+TEST_F(DownloadHistoryTest, DownloadHistoryTest_WasRestoredFromHistory_False) {
+  ExpectDownloadsRestoredFromHistory(false);
+  ExpectWillQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
+
+  history::DownloadRow info;
+  InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
+                "http://example.com/bar.pdf",
+                "http://example.com/referrer.html",
+                &info);
+
+  CallOnDownloadCreated(0);

[Randy Smith (Not in Mondays), 2014/04/21 18:21:22]

It was harder to understand these tests because of the lack of parallelism
between them.  Maybe add parallel comments so that it's clear what portions of
the two tests are parallel, and the reader gets a clue why they're different?

[asanka, 2014/04/23 05:51:09]

Hmm. Yeah. I added some comments that should explain what's going on. Does it
look better now?

+  ExpectDownloadCreated(info);
+}
+
 // Test creating an item, saving it to the database, changing it, saving it
 // back, removing it.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_Create) {
@@ -794,3 +854,5 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_UpdateWhileAdding) {
   info.opened = true;
   ExpectDownloadUpdated(info);
 }
+
+}  // anonymous namespace