[Downloads] Add real observers to MockDownloadItem.

Managing observer lists is needed by all the tests that end up calling
DownloadItem::AddObserver. This CL adds real observers to
MockDownloadItem so that tests don't need to mock this behavior.

BUG=367311

Review URL: https://codereview.chromium.org/248713004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269585 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/download/all_download_item_notifier_unittest.cc

@@ -85,7 +85,6 @@ TEST_F(AllDownloadItemNotifierTest,
   items.push_back(&item());
   EXPECT_CALL(manager(), GetAllDownloads(_))
     .WillOnce(SetArgPointee<0>(items));
-  EXPECT_CALL(item(), AddObserver(_));
   SetNotifier();
 
   EXPECT_CALL(observer(), OnDownloadUpdated(&manager(), &item()));
@@ -97,7 +96,6 @@ TEST_F(AllDownloadItemNotifierTest,
   EXPECT_CALL(observer(), OnDownloadRemoved(&manager(), &item()));
   NotifierAsItemObserver()->OnDownloadRemoved(&item());
 
-  EXPECT_CALL(item(), RemoveObserver(NotifierAsItemObserver()));
   EXPECT_CALL(manager(), RemoveObserver(NotifierAsManagerObserver()));
   ClearNotifier();
 }
@@ -107,7 +105,6 @@ TEST_F(AllDownloadItemNotifierTest,
   EXPECT_CALL(manager(), GetAllDownloads(_));
   SetNotifier();
 
-  EXPECT_CALL(item(), AddObserver(NotifierAsItemObserver()));
   EXPECT_CALL(observer(), OnDownloadCreated(&manager(), &item()));
   NotifierAsManagerObserver()->OnDownloadCreated(
       &manager(), &item());
@@ -115,8 +112,5 @@ TEST_F(AllDownloadItemNotifierTest,
   EXPECT_CALL(manager(), RemoveObserver(NotifierAsManagerObserver()));
   NotifierAsManagerObserver()->ManagerGoingDown(&manager());
 
-  EXPECT_CALL(item(), RemoveObserver(NotifierAsItemObserver()));
-  NotifierAsItemObserver()->OnDownloadDestroyed(&item());
-
   ClearNotifier();
 }

chrome/browser/download/download_danger_prompt_browsertest.cc

@@ -24,8 +24,7 @@ using ::testing::SaveArg;
 class DownloadDangerPromptTest : public InProcessBrowserTest {
  public:
   DownloadDangerPromptTest()
-    : download_observer_(NULL),
-      prompt_(NULL),
+    : prompt_(NULL),
       expected_action_(DownloadDangerPrompt::CANCEL),
       did_receive_callback_(false) {
   }
@@ -66,19 +65,12 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
 
   content::MockDownloadItem& download() { return download_; }
 
-  content::DownloadItem::Observer* download_observer() {
-    return download_observer_;
-  }
-
   DownloadDangerPrompt* prompt() { return prompt_; }
 
  private:
   void SetUpDownloadItemExpectations() {
     EXPECT_CALL(download_, GetFileNameToReportUser()).WillRepeatedly(Return(
         base::FilePath(FILE_PATH_LITERAL("evil.exe"))));
-    EXPECT_CALL(download_, AddObserver(_))
-        .WillOnce(SaveArg<0>(&download_observer_));
-    EXPECT_CALL(download_, RemoveObserver(Eq(ByRef(download_observer_))));
     EXPECT_CALL(download_, GetDangerType())
         .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL));
   }
@@ -100,7 +92,6 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
   }
 
   content::MockDownloadItem download_;
-  content::DownloadItem::Observer* download_observer_;
   DownloadDangerPrompt* prompt_;
   DownloadDangerPrompt::Action expected_action_;
   bool did_receive_callback_;
@@ -125,7 +116,7 @@ IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, TestAll) {
   // dialog should DISMISS itself.
   SetUpExpectations(DownloadDangerPrompt::DISMISS);
   EXPECT_CALL(download(), IsDangerous()).WillOnce(Return(false));
-  download_observer()->OnDownloadUpdated(&download());
+  download().NotifyObserversDownloadUpdated();
   VerifyExpectations();
 
   // If the download is in a terminal state then the dialog should DISMISS
@@ -133,7 +124,7 @@ IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, TestAll) {
   SetUpExpectations(DownloadDangerPrompt::DISMISS);
   EXPECT_CALL(download(), IsDangerous()).WillOnce(Return(true));
   EXPECT_CALL(download(), IsDone()).WillOnce(Return(true));
-  download_observer()->OnDownloadUpdated(&download());
+  download().NotifyObserversDownloadUpdated();
   VerifyExpectations();
 
   // If the download is dangerous and is not in a terminal state, don't dismiss
@@ -141,7 +132,7 @@ IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, TestAll) {
   SetUpExpectations(DownloadDangerPrompt::ACCEPT);
   EXPECT_CALL(download(), IsDangerous()).WillOnce(Return(true));
   EXPECT_CALL(download(), IsDone()).WillOnce(Return(false));
-  download_observer()->OnDownloadUpdated(&download());
+  download().NotifyObserversDownloadUpdated();
   SimulatePromptAction(DownloadDangerPrompt::ACCEPT);
   VerifyExpectations();
 

chrome/browser/download/download_history_unittest.cc

@@ -198,7 +198,6 @@ class DownloadHistoryTest : public testing::Test {
         manager_(new content::MockDownloadManager()),
         history_(NULL),
         manager_observer_(NULL),
-        item_observer_(NULL),
         download_created_index_(0) {}
   virtual ~DownloadHistoryTest() {
     STLDeleteElements(&items_);
@@ -220,15 +219,6 @@ class DownloadHistoryTest : public testing::Test {
     return manager_observer_;
   }
 
-  // Relies on the same object observing all download items.
-  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) {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     CHECK(infos.get());
@@ -432,10 +422,6 @@ class DownloadHistoryTest : public testing::Test {
             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(_));
     EXPECT_CALL(item(index), IsTemporary()).WillRepeatedly(Return(false));
 #if !defined(OS_ANDROID)
     new extensions::DownloadedByExtension(
@@ -458,7 +444,6 @@ class DownloadHistoryTest : public testing::Test {
   FakeHistoryAdapter* 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);
@@ -486,14 +471,14 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
 
   // Pretend that something changed on the item.
   EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   info.opened = true;
   ExpectDownloadUpdated(info);
 
   // Pretend that the user removed the item.
   IdSet ids;
   ids.insert(info.id);
-  item_observer()->OnDownloadRemoved(&item(0));
+  item(0).NotifyObserversDownloadRemoved();
   ExpectDownloadsRemoved(ids);
 }
 
@@ -517,14 +502,14 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Create) {
 
   // Pretend that something changed on the item.
   EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   info.opened = true;
   ExpectDownloadUpdated(info);
 
   // Pretend that the user removed the item.
   IdSet ids;
   ids.insert(info.id);
-  item_observer()->OnDownloadRemoved(&item(0));
+  item(0).NotifyObserversDownloadRemoved();
   ExpectDownloadsRemoved(ids);
 }
 
@@ -550,72 +535,72 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Update) {
   // current_path
   EXPECT_CALL(item(0), GetFullPath()).WillRepeatedly(ReturnRefOfCopy(new_path));
   info.current_path = new_path;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // target_path
   EXPECT_CALL(item(0), GetTargetFilePath())
       .WillRepeatedly(ReturnRefOfCopy(new_path));
   info.target_path = new_path;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // end_time
   EXPECT_CALL(item(0), GetEndTime()).WillRepeatedly(Return(new_time));
   info.end_time = new_time;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // received_bytes
   EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(101));
   info.received_bytes = 101;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // total_bytes
   EXPECT_CALL(item(0), GetTotalBytes()).WillRepeatedly(Return(102));
   info.total_bytes = 102;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // etag
   EXPECT_CALL(item(0), GetETag()).WillRepeatedly(ReturnRefOfCopy(new_etag));
   info.etag = new_etag;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // last_modified
   EXPECT_CALL(item(0), GetLastModifiedTime())
       .WillRepeatedly(ReturnRefOfCopy(new_last_modifed));
   info.last_modified = new_last_modifed;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // state
   EXPECT_CALL(item(0), GetState())
       .WillRepeatedly(Return(content::DownloadItem::INTERRUPTED));
   info.state = content::DownloadItem::INTERRUPTED;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // danger_type
   EXPECT_CALL(item(0), GetDangerType())
       .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT));
   info.danger_type = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // interrupt_reason
   EXPECT_CALL(item(0), GetLastReason())
       .WillRepeatedly(Return(content::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED));
   info.interrupt_reason = content::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 
   // opened
   EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
   info.opened = true;
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   ExpectDownloadUpdated(info);
 }
 
@@ -642,7 +627,7 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Temporary) {
   // 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));
+  item(0).NotifyObserversDownloadUpdated();
   IdSet ids;
   ids.insert(info.id);
   ExpectDownloadsRemoved(ids);
@@ -650,19 +635,19 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Temporary) {
   // 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));
+  item(0).NotifyObserversDownloadUpdated();
   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));
+  item(0).NotifyObserversDownloadUpdated();
   info.received_bytes = 4200;
   ExpectDownloadCreated(info);
   EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
 
   EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(100));
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   info.received_bytes = 100;
   ExpectDownloadUpdated(info);
 }
@@ -690,7 +675,7 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_RemoveWhileAdding) {
   // Instead of calling RemoveDownloads() immediately, DownloadHistory should
   // add the item's id to removed_while_adding_. Then, ItemAdded should
   // immediately remove the item's record from history.
-  item_observer()->OnDownloadRemoved(&item(0));
+  item(0).NotifyObserversDownloadRemoved();
   EXPECT_CALL(manager(), GetDownload(item(0).GetId()))
     .WillRepeatedly(Return(static_cast<content::DownloadItem*>(NULL)));
   ExpectNoDownloadsRemoved();
@@ -733,8 +718,8 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Multiple) {
   IdSet ids;
   ids.insert(info0.id);
   ids.insert(info1.id);
-  item_observer()->OnDownloadRemoved(&item(0));
-  item_observer()->OnDownloadRemoved(&item(1));
+  item(0).NotifyObserversDownloadRemoved();
+  item(1).NotifyObserversDownloadRemoved();
   ExpectDownloadsRemoved(ids);
 }
 
@@ -757,7 +742,7 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_CreateFailed) {
   EXPECT_FALSE(DownloadHistory::IsPersisted(&item(0)));
 
   EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(100));
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
   info.received_bytes = 100;
   ExpectDownloadCreated(info);
   EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
@@ -785,7 +770,7 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_UpdateWhileAdding) {
 
   // Pretend that something changed on the item.
   EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
-  item_observer()->OnDownloadUpdated(&item(0));
+  item(0).NotifyObserversDownloadUpdated();
 
   FinishCreateDownload();
   EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));

chrome/browser/download/download_status_updater_unittest.cc

@@ -54,10 +54,6 @@ class DownloadStatusUpdaterTest : public testing::Test {
   virtual ~DownloadStatusUpdaterTest() {
     for (size_t mgr_idx = 0; mgr_idx < managers_.size(); ++mgr_idx) {
       EXPECT_CALL(*Manager(mgr_idx), RemoveObserver(_));
-      for (size_t item_idx = 0; item_idx < manager_items_[mgr_idx].size();
-           ++item_idx) {
-        EXPECT_CALL(*Item(mgr_idx, item_idx), RemoveObserver(_));
-      }
     }
 
     delete updater_;
@@ -116,8 +112,6 @@ class DownloadStatusUpdaterTest : public testing::Test {
           i < in_progress_count ? content::DownloadItem::IN_PROGRESS
               : content::DownloadItem::CANCELLED;
       EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(state));
-      EXPECT_CALL(*item, AddObserver(_))
-          .WillOnce(Return());
       manager_items_[manager_index].push_back(item);
     }
     EXPECT_CALL(*manager, GetAllDownloads(_))

chrome/browser/download/download_ui_controller_unittest.cc

@@ -53,10 +53,6 @@ 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_|.
-  scoped_ptr<MockDownloadItem> GetMockDownload();
-
   // Returns a TestDelegate. Invoking NotifyDownloadStarting on the returned
   // delegate results in the DownloadItem* being stored in |received_item_|.
   scoped_ptr<DownloadUIController::Delegate> GetTestDelegate();
@@ -65,13 +61,11 @@ class DownloadUIControllerTest : public testing::Test {
   content::DownloadManager::Observer* manager_observer() {
     return manager_observer_;
   }
-  content::DownloadItem::Observer* item_observer() { return item_observer_; }
   content::DownloadItem* received_item() { return received_item_; }
 
  private:
   scoped_ptr<MockDownloadManager> manager_;
   content::DownloadManager::Observer* manager_observer_;
-  content::DownloadItem::Observer* item_observer_;
   content::DownloadItem* received_item_;
 
   base::WeakPtrFactory<content::DownloadItem*> receiver_factory_;
@@ -79,7 +73,6 @@ class DownloadUIControllerTest : public testing::Test {
 
 DownloadUIControllerTest::DownloadUIControllerTest()
     : manager_observer_(NULL),
-      item_observer_(NULL),
       received_item_(NULL),
       receiver_factory_(&received_item_) {
 }
@@ -94,17 +87,6 @@ void DownloadUIControllerTest::SetUp() {
   EXPECT_CALL(*manager_, GetAllDownloads(_));
 }
 
-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)));
-  return item.Pass();
-}
-
 scoped_ptr<DownloadUIController::Delegate>
 DownloadUIControllerTest::GetTestDelegate() {
   scoped_ptr<DownloadUIController::Delegate> delegate(
@@ -116,7 +98,7 @@ DownloadUIControllerTest::GetTestDelegate() {
 // 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();
+  scoped_ptr<MockDownloadItem> item(new MockDownloadItem);
   DownloadUIController controller(manager(), GetTestDelegate());
   EXPECT_CALL(*item, GetTargetFilePath())
       .WillOnce(ReturnRefOfCopy(base::FilePath()));
@@ -129,12 +111,11 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic) {
   // The destination for the download hasn't been determined yet. It should not
   // be displayed.
   EXPECT_FALSE(received_item());
-  ASSERT_TRUE(item_observer());
 
   // 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->NotifyObserversDownloadUpdated();
 
   EXPECT_EQ(static_cast<content::DownloadItem*>(item.get()), received_item());
 }
@@ -143,7 +124,7 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic) {
 // state should be displayed in the UI immediately without requiring an
 // additional OnDownloadUpdated() notification.
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyReadyOnCreate) {
-  scoped_ptr<MockDownloadItem> item = GetMockDownload();
+  scoped_ptr<MockDownloadItem> item(new MockDownloadItem);
   DownloadUIController controller(manager(), GetTestDelegate());
   EXPECT_CALL(*item, GetTargetFilePath())
       .WillOnce(ReturnRefOfCopy(base::FilePath(FILE_PATH_LITERAL("foo"))));
@@ -158,7 +139,7 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyReadyOnCreate) {
 // 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();
+  scoped_ptr<MockDownloadItem> item(new MockDownloadItem);
   DownloadUIController controller(manager(), GetTestDelegate());
   EXPECT_CALL(*item, GetState())
       .WillRepeatedly(Return(content::DownloadItem::COMPLETE));
@@ -167,7 +148,7 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NoNotifyHistory) {
   manager_observer()->OnDownloadCreated(manager(), item.get());
   EXPECT_FALSE(received_item());
 
-  item_observer()->OnDownloadUpdated(item.get());
+  item->NotifyObserversDownloadUpdated();
   EXPECT_FALSE(received_item());
 }
 

chrome/browser/plugins/plugin_installer_unittest.cc

@@ -25,19 +25,15 @@ class PluginInstallerTest : public ChromeRenderViewHostTestHarness {
   virtual void TearDown() OVERRIDE;
 
   PluginInstaller* installer() { return installer_.get(); }
-  content::DownloadItem::Observer* last_download_item_observer() {
-    return last_download_item_observer_;
-  }
 
   scoped_ptr<content::MockDownloadItem> CreateMockDownloadItem();
 
  private:
   scoped_ptr<PluginInstaller> installer_;
-  content::DownloadItem::Observer* last_download_item_observer_;
 };
 
-PluginInstallerTest::PluginInstallerTest()
-    : last_download_item_observer_(NULL) {}
+PluginInstallerTest::PluginInstallerTest() {
+}
 
 void PluginInstallerTest::SetUp() {
   content::RenderViewHostTestHarness::SetUp();
@@ -53,11 +49,6 @@ scoped_ptr<content::MockDownloadItem>
 PluginInstallerTest::CreateMockDownloadItem() {
   scoped_ptr<content::MockDownloadItem> mock_download_item(
       new testing::StrictMock<content::MockDownloadItem>());
-  ON_CALL(*mock_download_item, AddObserver(_))
-      .WillByDefault(testing::SaveArg<0>(&last_download_item_observer_));
-  ON_CALL(*mock_download_item, RemoveObserver(_)).WillByDefault(
-      testing::Assign(&last_download_item_observer_,
-                      static_cast<content::DownloadItem::Observer*>(NULL)));
   ON_CALL(*mock_download_item, GetState())
       .WillByDefault(testing::Return(content::DownloadItem::IN_PROGRESS));
   return mock_download_item.Pass();
@@ -119,7 +110,6 @@ TEST_F(PluginInstallerTest, StartInstalling_SuccessfulDownload) {
           InvokeOnStartedCallback(download_item.get(),
                                   content::DOWNLOAD_INTERRUPT_REASON_NONE),
           InvokeClosure(run_loop.QuitClosure())));
-  EXPECT_CALL(*download_item, AddObserver(_));
   EXPECT_CALL(*download_item, SetOpenWhenComplete(_));
 
   TestPluginInstallerObserver installer_observer(installer());
@@ -127,14 +117,12 @@ TEST_F(PluginInstallerTest, StartInstalling_SuccessfulDownload) {
       GURL(kTestUrl), web_contents(), &mock_download_manager);
   run_loop.Run();
 
-  ASSERT_TRUE(last_download_item_observer());
   EXPECT_TRUE(installer_observer.download_started());
   EXPECT_FALSE(installer_observer.download_finished());
 
   EXPECT_CALL(*download_item, GetState())
       .WillOnce(testing::Return(content::DownloadItem::COMPLETE));
-  EXPECT_CALL(*download_item, RemoveObserver(_));
-  last_download_item_observer()->OnDownloadUpdated(download_item.get());
+  download_item->NotifyObserversDownloadUpdated();
   EXPECT_TRUE(installer_observer.download_finished());
 }
 
@@ -159,7 +147,6 @@ TEST_F(PluginInstallerTest, StartInstalling_FailedStart) {
       GURL(kTestUrl), web_contents(), &mock_download_manager);
   run_loop.Run();
 
-  EXPECT_FALSE(last_download_item_observer());
   EXPECT_TRUE(installer_observer.download_started());
   EXPECT_FALSE(installer_observer.download_finished());
   EXPECT_EQ("Error 20: NETWORK_FAILED", installer_observer.download_error());
@@ -180,7 +167,6 @@ TEST_F(PluginInstallerTest, StartInstalling_Interrupted) {
           InvokeOnStartedCallback(download_item.get(),
                                   content::DOWNLOAD_INTERRUPT_REASON_NONE),
           InvokeClosure(run_loop.QuitClosure())));
-  EXPECT_CALL(*download_item, AddObserver(_));
   EXPECT_CALL(*download_item, SetOpenWhenComplete(_));
 
   TestPluginInstallerObserver installer_observer(installer());
@@ -188,18 +174,15 @@ TEST_F(PluginInstallerTest, StartInstalling_Interrupted) {
       GURL(kTestUrl), web_contents(), &mock_download_manager);
   run_loop.Run();
 
-  ASSERT_TRUE(last_download_item_observer());
   EXPECT_TRUE(installer_observer.download_started());
   EXPECT_FALSE(installer_observer.download_finished());
 
   EXPECT_CALL(*download_item, GetState())
       .WillOnce(testing::Return(content::DownloadItem::INTERRUPTED));
-  EXPECT_CALL(*download_item, RemoveObserver(_));
   EXPECT_CALL(*download_item, GetLastReason()).WillOnce(
       testing::Return(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED));
-  last_download_item_observer()->OnDownloadUpdated(download_item.get());
+  download_item->NotifyObserversDownloadUpdated();
 
-  EXPECT_FALSE(last_download_item_observer());
   EXPECT_TRUE(installer_observer.download_started());
   EXPECT_FALSE(installer_observer.download_finished());
   EXPECT_EQ("NETWORK_FAILED", installer_observer.download_error());

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -365,8 +365,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
   GURL referrer("http://www.google.com/");
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_));
-  EXPECT_CALL(item, RemoveObserver(_));
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -383,8 +381,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
   Mock::VerifyAndClearExpectations(&item);
 
   url_chain.push_back(GURL("file://www.google.com/"));
-  EXPECT_CALL(item, AddObserver(_));
-  EXPECT_CALL(item, RemoveObserver(_));
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -417,8 +413,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
   GURL referrer;
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(4);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(4);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -509,8 +503,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadFetchFailed) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_));
-  EXPECT_CALL(item, RemoveObserver(_));
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -555,8 +547,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(6);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(6);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -705,8 +695,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadHTTPS) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(1);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(1);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -760,8 +748,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(3);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(3);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_zip));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -844,8 +830,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadCorruptZip) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(1);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(1);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_zip));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -893,8 +877,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientCrxDownloadSuccess) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(1);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(1);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_crx));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -937,8 +919,6 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
   std::string remote_address = "10.11.12.13";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(1);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(1);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -1026,8 +1006,6 @@ TEST_F(DownloadProtectionServiceTest,
   std::string remote_address = "10.11.12.13";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(1);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(1);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -1110,8 +1088,6 @@ TEST_F(DownloadProtectionServiceTest,
   std::string remote_address = "10.11.12.13";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(2);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(2);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -1348,8 +1324,6 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadRequestTimeout) {
   std::string hash = "hash";
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(1);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(1);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
@@ -1391,39 +1365,36 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadItemDestroyed) {
   base::FilePath final_path(FILE_PATH_LITERAL("a.exe"));
   std::string hash = "hash";
 
-  content::MockDownloadItem item;
-  content::DownloadItem::Observer* observer = NULL;
-  EXPECT_CALL(item, AddObserver(_)).WillOnce(SaveArg<0>(&observer));
-  EXPECT_CALL(item, RemoveObserver(_)).WillOnce(Assign(
-      &observer, static_cast<content::DownloadItem::Observer*>(NULL)));
-  EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
-  EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
-  EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
-  EXPECT_CALL(item, GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
-  EXPECT_CALL(item, GetTabUrl()).WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
-  EXPECT_CALL(item, GetTabReferrerUrl())
-      .WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
-  EXPECT_CALL(item, GetHash()).WillRepeatedly(ReturnRef(hash));
-  EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(100));
-  EXPECT_CALL(item, HasUserGesture()).WillRepeatedly(Return(true));
-  EXPECT_CALL(item, GetRemoteAddress()).WillRepeatedly(Return(""));
-
-  EXPECT_CALL(*sb_service_->mock_database_manager(),
-              MatchDownloadWhitelistUrl(_))
-      .WillRepeatedly(Return(false));
-  EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path, _));
-  EXPECT_CALL(*binary_feature_extractor_.get(),
-              ExtractImageHeaders(tmp_path, _));
-
-  download_service_->CheckClientDownload(
-      &item,
-      base::Bind(&DownloadProtectionServiceTest::SyncCheckDoneCallback,
-                 base::Unretained(this)));
+  {
+    content::MockDownloadItem item;
+    EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
+    EXPECT_CALL(item, GetTargetFilePath())
+        .WillRepeatedly(ReturnRef(final_path));
+    EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
+    EXPECT_CALL(item, GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
+    EXPECT_CALL(item, GetTabUrl()).WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+    EXPECT_CALL(item, GetTabReferrerUrl())
+        .WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+    EXPECT_CALL(item, GetHash()).WillRepeatedly(ReturnRef(hash));
+    EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(100));
+    EXPECT_CALL(item, HasUserGesture()).WillRepeatedly(Return(true));
+    EXPECT_CALL(item, GetRemoteAddress()).WillRepeatedly(Return(""));
+
+    EXPECT_CALL(*sb_service_->mock_database_manager(),
+                MatchDownloadWhitelistUrl(_))
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path, _));
+    EXPECT_CALL(*binary_feature_extractor_.get(),
+                ExtractImageHeaders(tmp_path, _));
 
-  ASSERT_TRUE(observer != NULL);
-  observer->OnDownloadDestroyed(&item);
+    download_service_->CheckClientDownload(
+        &item,
+        base::Bind(&DownloadProtectionServiceTest::SyncCheckDoneCallback,
+                   base::Unretained(this)));
+    // MockDownloadItem going out of scope triggers the OnDownloadDestroyed
+    // notification.
+  }
 
-  EXPECT_TRUE(observer == NULL);
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
 }
 

content/public/test/mock_download_item.cc

@@ -7,6 +7,29 @@
 namespace content {
 
 MockDownloadItem::MockDownloadItem() {}
-MockDownloadItem::~MockDownloadItem() {}
+
+MockDownloadItem::~MockDownloadItem() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
+}
+
+void MockDownloadItem::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void MockDownloadItem::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void MockDownloadItem::NotifyObserversDownloadOpened() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
+}
+
+void MockDownloadItem::NotifyObserversDownloadRemoved() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadRemoved(this));
+}
+
+void MockDownloadItem::NotifyObserversDownloadUpdated() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
+}
 
 }

content/public/test/mock_download_item.h

@@ -6,6 +6,7 @@
 #define CONTENT_PUBLIC_TEST_MOCK_DOWNLOAD_ITEM_H_
 
 #include "base/callback.h"
+#include "base/observer_list.h"
 #include "base/time/time.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
@@ -19,8 +20,20 @@ class MockDownloadItem : public DownloadItem {
  public:
   MockDownloadItem();
   virtual ~MockDownloadItem();
-  MOCK_METHOD1(AddObserver, void(DownloadItem::Observer*));
-  MOCK_METHOD1(RemoveObserver, void(DownloadItem::Observer*));
+
+  // Management of observer lists is common in tests. So Add/RemoveObserver
+  // methods are not mocks. In addition, any registered observers will receive a
+  // OnDownloadDestroyed() notification when the mock is destroyed.
+  virtual void AddObserver(Observer* observer) OVERRIDE;
+  virtual void RemoveObserver(Observer* observer) OVERRIDE;
+
+  // Dispatches an OnDownloadOpened() notification to observers.
+  void NotifyObserversDownloadOpened();
+  // Dispatches an OnDownloadRemoved() notification to observers.
+  void NotifyObserversDownloadRemoved();
+  // Dispatches an OnDownloadUpdated() notification to observers.
+  void NotifyObserversDownloadUpdated();
+
   MOCK_METHOD0(UpdateObservers, void());
   MOCK_METHOD0(ValidateDangerousDownload, void());
   MOCK_METHOD1(StealDangerousDownload, void(const AcquireFileCallback&));
@@ -87,6 +100,9 @@ class MockDownloadItem : public DownloadItem {
   MOCK_METHOD1(SetOpened, void(bool));
   MOCK_METHOD1(SetDisplayName, void(const base::FilePath&));
   MOCK_CONST_METHOD1(DebugString, std::string(bool));
+
+ private:
+  ObserverList<Observer> observers_;
 };
 
 }  // namespace content