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

chrome/browser/download/download_ui_controller_unittest.cc

Index: chrome/browser/download/download_ui_controller_unittest.cc
diff --git a/chrome/browser/download/download_ui_controller_unittest.cc b/chrome/browser/download/download_ui_controller_unittest.cc
index d3fc9cbf71c143e6c5423579a740f8b9e9cad312..cca70312e0b17cedd4a908ec1e6c858de202fd23 100644
--- a/chrome/browser/download/download_ui_controller_unittest.cc
+++ b/chrome/browser/download/download_ui_controller_unittest.cc
@@ -2,11 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "chrome/browser/download/download_history.h"
+#include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_ui_controller.h"
+#include "chrome/browser/history/download_row.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/mock_download_manager.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -31,7 +40,7 @@ class TestDelegate : public DownloadUIController::Delegate {
   virtual ~TestDelegate() {}
 
  private:
-  virtual void NotifyDownloadStarting(content::DownloadItem* item) OVERRIDE;
+  virtual void OnNewDownloadReady(content::DownloadItem* item) OVERRIDE;
 
   base::WeakPtr<content::DownloadItem*> receiver_;
 };
@@ -40,12 +49,62 @@ TestDelegate::TestDelegate(base::WeakPtr<content::DownloadItem*> receiver)
     : receiver_(receiver) {
 }
 
-void TestDelegate::NotifyDownloadStarting(content::DownloadItem* item) {
+void TestDelegate::OnNewDownloadReady(content::DownloadItem* item) {
   if (receiver_.get())
     *receiver_ = item;
 }
 
-class DownloadUIControllerTest : public testing::Test {
+// A DownloadService that returns a custom DownloadHistory.
+class TestDownloadService : public DownloadService {
+ public:
+  explicit TestDownloadService(Profile* profile);
+  virtual ~TestDownloadService();
+
+  void set_download_history(scoped_ptr<DownloadHistory> download_history) {
+    download_history_.swap(download_history);
+  }
+  virtual DownloadHistory* GetDownloadHistory() OVERRIDE;
+
+ private:
+  scoped_ptr<DownloadHistory> download_history_;
+};
+
+TestDownloadService::TestDownloadService(Profile* profile)
+    : DownloadService(profile) {
+}
+
+TestDownloadService::~TestDownloadService() {
+}
+
+DownloadHistory* TestDownloadService::GetDownloadHistory() {
+  return download_history_.get();
+}
+
+// A MockDownloadItem with a real observer list. Used because observers need to
+// be reliably notified with a OnDownloadDestroyed() notification when the
+// DownloadItem is being destroyed.

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

This is the second time this functionality has been needed (FakeDownloadItem in
download_path_reservation_tracker_unittest.cc).  Worthwhile pulling it into
MockDownloadItem?

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

Good catch. I added it to MockDownloadItem in a separate CL (different owners
etc..) This CL now depends on that CL.

+class MockDownloadItemWithObservers : public MockDownloadItem {
+ public:
+  virtual ~MockDownloadItemWithObservers() {
+    FOR_EACH_OBSERVER(Observer, observer_list_, OnDownloadDestroyed(this));
+  }
+  virtual void AddObserver(content::DownloadItem::Observer* observer) OVERRIDE {
+    observer_list_.AddObserver(observer);
+  }
+  virtual void RemoveObserver(content::DownloadItem::Observer* observer)
+      OVERRIDE {
+    observer_list_.RemoveObserver(observer);
+  }
+  virtual void UpdateObservers() OVERRIDE {
+    FOR_EACH_OBSERVER(Observer, observer_list_, OnDownloadUpdated(this));
+  }
+
+ private:
+  ObserverList<content::DownloadItem::Observer> observer_list_;
+};
+
+// The test fixture:
+class DownloadUIControllerTest : public ChromeRenderViewHostTestHarness {
  public:
   DownloadUIControllerTest();
 
@@ -53,90 +112,188 @@ class DownloadUIControllerTest : public testing::Test {
   // testing::Test
   virtual void SetUp() OVERRIDE;
 
-  // Returns a MockDownloadItem that has AddObserver and RemoveObserver
-  // expectations set up to store the observer in |item_observer_|.
+  // Returns a MockDownloadItem representing an IN_PROGRESS download that has
+  // non-empty current and target paths.
   scoped_ptr<MockDownloadItem> GetMockDownload();
 
-  // Returns a TestDelegate. Invoking NotifyDownloadStarting on the returned
-  // delegate results in the DownloadItem* being stored in |received_item_|.
+  // Returns a TestDelegate. Invoking OnNewDownloadReady on the returned
+  // delegate results in the DownloadItem* being stored in |notified_item_|.
   scoped_ptr<DownloadUIController::Delegate> GetTestDelegate();
 
   MockDownloadManager* manager() { return manager_.get(); }
+
+  // Returns the DownloadManager::Observer registered by a test case. This is
+  // the DownloadUIController's observer for all current test cases.
   content::DownloadManager::Observer* manager_observer() {
     return manager_observer_;
   }
-  content::DownloadItem::Observer* item_observer() { return item_observer_; }
-  content::DownloadItem* received_item() { return received_item_; }
+
+  // The most recent DownloadItem that was passed into OnNewDownloadReady().
+  content::DownloadItem* notified_item() { return notified_item_; }
+
+  // DownloadHistory performs a query of existing downloads when it is first
+  // instantiated. This method returns the completion callback for that query.
+  // It can be used to inject history downloads.
+  const HistoryService::DownloadQueryCallback& history_query_callback() const {
+    return history_adapter_->download_query_callback_;
+  }
+
+  // DownloadManager::Observer registered by DownloadHistory.
+  content::DownloadManager::Observer* download_history_manager_observer() {
+    return download_history_manager_observer_;
+  }
 
  private:
+  // A private history adapter that stores the DownloadQueryCallback when
+  // QueryDownloads is called.
+  class HistoryAdapter : public DownloadHistory::HistoryAdapter {
+   public:
+    HistoryAdapter() : DownloadHistory::HistoryAdapter(NULL) {}
+    HistoryService::DownloadQueryCallback download_query_callback_;
+
+   private:
+    virtual void QueryDownloads(
+        const HistoryService::DownloadQueryCallback& callback) OVERRIDE {
+      download_query_callback_ = callback;
+    }
+  };
+
+  // Constructs and returns a TestDownloadService.
+  static KeyedService* TestingDownloadServiceFactory(
+      content::BrowserContext* brwoser_context);
+
   scoped_ptr<MockDownloadManager> manager_;
+  content::DownloadManager::Observer* download_history_manager_observer_;
   content::DownloadManager::Observer* manager_observer_;
-  content::DownloadItem::Observer* item_observer_;
-  content::DownloadItem* received_item_;
+  content::DownloadItem* notified_item_;
+  base::WeakPtrFactory<content::DownloadItem*> notified_item_receiver_factory_;
 
-  base::WeakPtrFactory<content::DownloadItem*> receiver_factory_;
+  HistoryAdapter* history_adapter_;
 };
 
+// static
+KeyedService* DownloadUIControllerTest::TestingDownloadServiceFactory(
+    content::BrowserContext* browser_context) {
+  return new TestDownloadService(Profile::FromBrowserContext(browser_context));
+}
+
 DownloadUIControllerTest::DownloadUIControllerTest()
-    : manager_observer_(NULL),
-      item_observer_(NULL),
-      received_item_(NULL),
-      receiver_factory_(&received_item_) {
+    : download_history_manager_observer_(NULL),
+      manager_observer_(NULL),
+      notified_item_(NULL),
+      notified_item_receiver_factory_(&notified_item_) {
+}
+
+// Mock action for conditionally resettting a pointer. Used as:
+// EXPECT_CALL(Foo(_))
+//   .WillOnce(ResetToNullIfEqual(&stored_pointer));
+ACTION_P(ResetToNullIfEqual, pointer) {
+  if (*pointer == arg0)
+    *pointer = NULL;
 }
 
 void DownloadUIControllerTest::SetUp() {
+  ChromeRenderViewHostTestHarness::SetUp();
+
   manager_.reset(new testing::StrictMock<MockDownloadManager>());
   EXPECT_CALL(*manager_, AddObserver(_))
+      .WillOnce(SaveArg<0>(&download_history_manager_observer_));
+  EXPECT_CALL(*manager_, GetAllDownloads(_)).Times(AnyNumber());
+
+  scoped_ptr<HistoryAdapter> history_adapter(new HistoryAdapter);
+  history_adapter_ = history_adapter.get();
+  scoped_ptr<DownloadHistory> download_history(new DownloadHistory(
+      manager_.get(),
+      history_adapter.PassAs<DownloadHistory::HistoryAdapter>()));
+  ASSERT_TRUE(download_history_manager_observer_);
+
+  EXPECT_CALL(*manager_, AddObserver(_))

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

At this point, I trust your expertise with gmock more than mine, but shouldn't
there be a verification that the last expect actually happened before it's
replaced?

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

I changed the expectation to explicitly require matching the current value of
manager_observer_. That should be the equivalent of what you are asking since it
would only match if the AddObserver was already seen.

[Randy Smith (Not in Mondays), 2014/04/23 19:18:57]

Sorry, still confused.  There are two EXPECT_CALL(*manager_,
AddObserver(_)).WillOnce() calls in this function.  What prevents the first from
not being tripped (i.e. tripped zero times) before being overridden by the
second?

[asanka, 2014/05/07 23:19:53]

There's an ASSERT_TRUE(download_history_manager_observer_) that should only pass
if the first AddObserver() has already performed its action. I can add
checkpoints along the way, but I think the current setup sufficiently verifies
that the first expectation is met before we get  here.

[Randy Smith (Not in Mondays), 2014/05/08 17:21:04]

Ah, sorry, missed that.  LGTM.

       .WillOnce(SaveArg<0>(&manager_observer_));
   EXPECT_CALL(*manager_, RemoveObserver(_))
-      .WillOnce(Assign(&manager_observer_,
-                       static_cast<content::DownloadManager::Observer*>(NULL)));
-  EXPECT_CALL(*manager_, GetAllDownloads(_));
+      .WillRepeatedly(ResetToNullIfEqual(&manager_observer_));
+  TestDownloadService* download_service = static_cast<TestDownloadService*>(
+      DownloadServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+          browser_context(), &TestingDownloadServiceFactory));
+  ASSERT_TRUE(download_service);
+  download_service->set_download_history(download_history.Pass());
 }
 
 scoped_ptr<MockDownloadItem> DownloadUIControllerTest::GetMockDownload() {
   scoped_ptr<MockDownloadItem> item(
-      new testing::StrictMock<MockDownloadItem>());
-  EXPECT_CALL(*item, AddObserver(_))
-      .WillOnce(SaveArg<0>(&item_observer_));
-  EXPECT_CALL(*item, RemoveObserver(_))
-      .WillOnce(Assign(&item_observer_,
-                       static_cast<content::DownloadItem::Observer*>(NULL)));
+      new testing::StrictMock<MockDownloadItemWithObservers>());
+  EXPECT_CALL(*item, GetBrowserContext())
+      .WillRepeatedly(Return(browser_context()));
+  EXPECT_CALL(*item, GetId()).WillRepeatedly(Return(1));
+  EXPECT_CALL(*item, GetTargetFilePath()).WillRepeatedly(
+      ReturnRefOfCopy(base::FilePath(FILE_PATH_LITERAL("foo"))));
+  EXPECT_CALL(*item, GetFullPath()).WillRepeatedly(
+      ReturnRefOfCopy(base::FilePath(FILE_PATH_LITERAL("foo"))));
+  EXPECT_CALL(*item, GetState())
+      .WillRepeatedly(Return(content::DownloadItem::IN_PROGRESS));
+  EXPECT_CALL(*item, GetUrlChain())
+      .WillRepeatedly(testing::ReturnRefOfCopy(std::vector<GURL>()));
+  EXPECT_CALL(*item, GetReferrerUrl())
+      .WillRepeatedly(testing::ReturnRefOfCopy(GURL()));
+  EXPECT_CALL(*item, GetStartTime()).WillRepeatedly(Return(base::Time()));
+  EXPECT_CALL(*item, GetEndTime()).WillRepeatedly(Return(base::Time()));
+  EXPECT_CALL(*item, GetETag()).WillRepeatedly(ReturnRefOfCopy(std::string()));
+  EXPECT_CALL(*item, GetLastModifiedTime())
+      .WillRepeatedly(ReturnRefOfCopy(std::string()));
+  EXPECT_CALL(*item, GetDangerType())
+      .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS));
+  EXPECT_CALL(*item, GetLastReason())
+      .WillRepeatedly(Return(content::DOWNLOAD_INTERRUPT_REASON_NONE));
+  EXPECT_CALL(*item, GetReceivedBytes()).WillRepeatedly(Return(0));
+  EXPECT_CALL(*item, GetTotalBytes()).WillRepeatedly(Return(0));
+  EXPECT_CALL(*item, GetTargetDisposition()).WillRepeatedly(
+      Return(content::DownloadItem::TARGET_DISPOSITION_OVERWRITE));
+  EXPECT_CALL(*item, GetOpened()).WillRepeatedly(Return(false));
+  EXPECT_CALL(*item, GetMimeType()).WillRepeatedly(Return(std::string()));
+  EXPECT_CALL(*item, GetURL()).WillRepeatedly(testing::ReturnRefOfCopy(GURL()));
+  EXPECT_CALL(*item, IsTemporary()).WillRepeatedly(Return(false));
   return item.Pass();
 }
 
 scoped_ptr<DownloadUIController::Delegate>
 DownloadUIControllerTest::GetTestDelegate() {
   scoped_ptr<DownloadUIController::Delegate> delegate(
-      new TestDelegate(receiver_factory_.GetWeakPtr()));
+      new TestDelegate(notified_item_receiver_factory_.GetWeakPtr()));
   return delegate.Pass();
 }
 
-// Normal downloads that are constructed in the IN_PROGRESS state should be
-// presented to the UI when GetTargetFilePath() returns a non-empty path.
-// I.e. once the download target has been determined.
+// New downloads should be presented to the UI when GetTargetFilePath() returns
+// a non-empty path.  I.e. once the download target has been determined.
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic) {
   scoped_ptr<MockDownloadItem> item = GetMockDownload();
   DownloadUIController controller(manager(), GetTestDelegate());
   EXPECT_CALL(*item, GetTargetFilePath())
       .WillOnce(ReturnRefOfCopy(base::FilePath()));
-  EXPECT_CALL(*item, GetState())
-      .WillRepeatedly(Return(content::DownloadItem::IN_PROGRESS));
 
   ASSERT_TRUE(manager_observer());
   manager_observer()->OnDownloadCreated(manager(), item.get());
 
   // The destination for the download hasn't been determined yet. It should not
   // be displayed.
-  EXPECT_FALSE(received_item());
-  ASSERT_TRUE(item_observer());
+  EXPECT_FALSE(notified_item());
 
   // Once the destination has been determined, then it should be displayed.
   EXPECT_CALL(*item, GetTargetFilePath())
       .WillOnce(ReturnRefOfCopy(base::FilePath(FILE_PATH_LITERAL("foo"))));
-  item_observer()->OnDownloadUpdated(item.get());
+  item->UpdateObservers();
+
+  EXPECT_EQ(static_cast<content::DownloadItem*>(item.get()), notified_item());
+}
+
+// A download that's created in an interrupted state should also be displayed.
+TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic_Interrupted) {
+  scoped_ptr<MockDownloadItem> item = GetMockDownload();
+  DownloadUIController controller(manager(), GetTestDelegate());
+  EXPECT_CALL(*item, GetState())
+      .WillRepeatedly(Return(content::DownloadItem::INTERRUPTED));
 
-  EXPECT_EQ(static_cast<content::DownloadItem*>(item.get()), received_item());
+  ASSERT_TRUE(manager_observer());
+  manager_observer()->OnDownloadCreated(manager(), item.get());
+  EXPECT_EQ(static_cast<content::DownloadItem*>(item.get()), notified_item());
 }
 
 // Downloads that have a target path on creation and are in the IN_PROGRESS
@@ -145,30 +302,68 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic) {
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyReadyOnCreate) {
   scoped_ptr<MockDownloadItem> item = GetMockDownload();
   DownloadUIController controller(manager(), GetTestDelegate());
-  EXPECT_CALL(*item, GetTargetFilePath())
-      .WillOnce(ReturnRefOfCopy(base::FilePath(FILE_PATH_LITERAL("foo"))));
-  EXPECT_CALL(*item, GetState())
-      .WillRepeatedly(Return(content::DownloadItem::IN_PROGRESS));
 
   ASSERT_TRUE(manager_observer());
   manager_observer()->OnDownloadCreated(manager(), item.get());
-  EXPECT_EQ(static_cast<content::DownloadItem*>(item.get()), received_item());
+  EXPECT_EQ(static_cast<content::DownloadItem*>(item.get()), notified_item());
 }
 
-// History downloads (downloads that are not in IN_PROGRESS on create) should
-// not be displayed on the shelf.
-TEST_F(DownloadUIControllerTest, DownloadUIController_NoNotifyHistory) {
-  scoped_ptr<MockDownloadItem> item = GetMockDownload();
+// The UI shouldn't be notified of downloads that were restored from history.
+TEST_F(DownloadUIControllerTest, DownloadUIController_HistoryDownload) {
   DownloadUIController controller(manager(), GetTestDelegate());
-  EXPECT_CALL(*item, GetState())
-      .WillRepeatedly(Return(content::DownloadItem::COMPLETE));
+  // DownloadHistory should already have been created. It performs a query of
+  // existing downloads upon creation. We'll use the callback to inject a
+  // history download.
+  ASSERT_FALSE(history_query_callback().is_null());
+
+  // download_history_manager_observer is the DownloadManager::Observer
+  // registered by the DownloadHistory. DownloadHistory relies on the
+  // OnDownloadCreated notification to mark a download as having been restored
+  // from history.
+  ASSERT_TRUE(download_history_manager_observer());
+
+  scoped_ptr<std::vector<history::DownloadRow> > history_downloads;
+  history_downloads.reset(new std::vector<history::DownloadRow>());
+  history_downloads->push_back(history::DownloadRow());
+  history_downloads->front().id = 1;
+
+  std::vector<GURL> url_chain;
+  GURL url;
+  scoped_ptr<MockDownloadItem> item = GetMockDownload();
+
+  EXPECT_CALL(*manager(), CheckForHistoryFilesRemoval());
+
+  {
+    testing::InSequence s;
+    testing::MockFunction<void()> mock_function;
+    // DownloadHistory will immediately try to create a download using the info
+    // we push through the query callback. When DownloadManager::CreateDownload
+    // is called, we need to first invoke the OnDownloadCreated callback for
+    // DownloadHistory before returning the DownloadItem since that's the
+    // sequence of events expected by DownloadHistory.
+    base::Closure history_on_created_callback =
+        base::Bind(&content::DownloadManager::Observer::OnDownloadCreated,
+                   base::Unretained(download_history_manager_observer()),
+                   manager(),
+                   item.get());
+    EXPECT_CALL(*manager(), MockCreateDownloadItem(_)).WillOnce(
+        testing::DoAll(testing::InvokeWithoutArgs(&history_on_created_callback,
+                                                  &base::Closure::Run),
+                       Return(item.get())));
+    EXPECT_CALL(mock_function, Call());
+
+    history_query_callback().Run(history_downloads.Pass());
+    mock_function.Call();
+  }
 
+  // Now pass along the notification to the OnDownloadCreated observer from
+  // DownloadUIController. It should ignore the download since it's marked as
+  // being restored from history.
   ASSERT_TRUE(manager_observer());
   manager_observer()->OnDownloadCreated(manager(), item.get());
-  EXPECT_FALSE(received_item());
 
-  item_observer()->OnDownloadUpdated(item.get());
-  EXPECT_FALSE(received_item());
+  // Finally, the expectation we've been waiting for:
+  EXPECT_FALSE(notified_item());
 }
 
 } // namespace