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

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/memory/weak_ptr.h"
 #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_row.h"
 #include "chrome/browser/history/history_service.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/mock_download_manager.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_utils.h"
@@ skipping 168 matching lines @@
   history::DownloadRow update_download_;
   scoped_ptr<InfoVector> expect_query_downloads_;
   IdSet remove_downloads_;
   history::DownloadRow create_download_info_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeHistoryAdapter);
 };
 
 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()),
         history_(NULL),
         manager_observer_(NULL),
         item_observer_(NULL),
         download_created_index_(0) {}
   virtual ~DownloadHistoryTest() {
     STLDeleteElements(&items_);
   }
 
  protected:
   virtual void TearDown() OVERRIDE {
     download_history_.reset();
   }
 
   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) {
     manager_observer_ = manager_observer;
   }
   content::DownloadManager::Observer* manager_observer() {
     return manager_observer_;
   }
 
   // Relies on the same object observing all download items.
@@ skipping 40 matching lines @@
     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) {
     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() {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     // 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) {
@@ skipping 35 matching lines @@
   void ExpectNoDownloadsRemoved() {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     history_->ExpectNoDownloadsRemoved();
   }
 
   void ExpectDownloadsRemoved(const IdSet& ids) {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     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,
                      history::DownloadRow* info) {
     GURL url(url_string);
     GURL referrer(referrer_string);
     std::vector<GURL> url_chain;
     url_chain.push_back(url);
     InitItem(static_cast<uint32>(items_.size() + 1),
              base::FilePath(path),
@@ skipping 103 matching lines @@
 
     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:
+  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_;
   scoped_ptr<content::MockDownloadManager> manager_;
   FakeHistoryAdapter* history_;
   scoped_ptr<DownloadHistory> download_history_;
   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) {
   // Load a download from history, create the item, OnDownloadCreated,
   // OnDownloadUpdated, OnDownloadRemoved.
   history::DownloadRow info;
   InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
                 "http://example.com/bar.pdf",
                 "http://example.com/referrer.html",
                 &info);
@@ skipping 11 matching lines @@
   info.opened = true;
   ExpectDownloadUpdated(info);
 
   // Pretend that the user removed the item.
   IdSet ids;
   ids.insert(info.id);
   item_observer()->OnDownloadRemoved(&item(0));
   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) {
   // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
   // OnDownloadRemoved.
   ExpectWillQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
 
   history::DownloadRow info;
   InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
                 "http://example.com/bar.pdf",
@@ skipping 278 matching lines @@
   item_observer()->OnDownloadUpdated(&item(0));
 
   FinishCreateDownload();
   EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
 
   // ItemAdded should call OnDownloadUpdated, which should detect that the item
   // changed while it was being added and call UpdateDownload immediately.
   info.opened = true;
   ExpectDownloadUpdated(info);
 }
+
+}  // anonymous namespace