[Downloads] Add real observers to MockDownloadItem.

chrome/browser/download/download_path_reservation_tracker_unittest.cc

Index: chrome/browser/download/download_path_reservation_tracker_unittest.cc
diff --git a/chrome/browser/download/download_path_reservation_tracker_unittest.cc b/chrome/browser/download/download_path_reservation_tracker_unittest.cc
index de1279b6acbc7549ddac39d5755ff490682c1b4b..df1df180be3a7841e761570c8770193768afeb61 100644
--- a/chrome/browser/download/download_path_reservation_tracker_unittest.cc
+++ b/chrome/browser/download/download_path_reservation_tracker_unittest.cc
@@ -27,40 +27,6 @@ using testing::ReturnRefOfCopy;
 
 namespace {
 
-// MockDownloadItem with real observers and state.
-class FakeDownloadItem : public MockDownloadItem {
- public:
-  explicit FakeDownloadItem()
-      : state_(IN_PROGRESS) {
-  }
-  virtual ~FakeDownloadItem() {
-    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
-    EXPECT_FALSE(observers_.might_have_observers());
-  }
-  virtual void AddObserver(Observer* observer) OVERRIDE {
-    observers_.AddObserver(observer);
-  }
-  virtual void RemoveObserver(Observer* observer) OVERRIDE {
-    observers_.RemoveObserver(observer);
-  }
-  virtual void UpdateObservers() OVERRIDE {
-    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
-  }
-
-  virtual DownloadState GetState() const OVERRIDE {
-    return state_;
-  }
-
-  void SetState(DownloadState state) {
-    state_ = state;
-    UpdateObservers();
-  }
-
- private:
-  DownloadState state_;
-  ObserverList<Observer> observers_;
-};
-
 class DownloadPathReservationTrackerTest : public testing::Test {
  public:
   DownloadPathReservationTrackerTest();
@@ -69,7 +35,7 @@ class DownloadPathReservationTrackerTest : public testing::Test {
   virtual void SetUp() OVERRIDE;
   virtual void TearDown() OVERRIDE;
 
-  FakeDownloadItem* CreateDownloadItem(int32 id);
+  MockDownloadItem* CreateDownloadItem(int32 id);
   base::FilePath GetPathInDownloadsDirectory(
       const base::FilePath::CharType* suffix);
   bool IsPathInUse(const base::FilePath& path);
@@ -118,9 +84,9 @@ void DownloadPathReservationTrackerTest::TearDown() {
   message_loop_.RunUntilIdle();
 }
 
-FakeDownloadItem* DownloadPathReservationTrackerTest::CreateDownloadItem(
+MockDownloadItem* DownloadPathReservationTrackerTest::CreateDownloadItem(
     int32 id) {
-  FakeDownloadItem* item = new ::testing::StrictMock<FakeDownloadItem>;
+  MockDownloadItem* item = new ::testing::StrictMock<MockDownloadItem>;
   EXPECT_CALL(*item, GetId())
       .WillRepeatedly(Return(id));
   EXPECT_CALL(*item, GetTargetFilePath())
@@ -183,7 +149,7 @@ DownloadPathReservationTrackerTest::GetLongNamePathInDownloadsDirectory(
 
 // A basic reservation is acquired and committed.
 TEST_F(DownloadPathReservationTrackerTest, BasicReservation) {
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   ASSERT_FALSE(IsPathInUse(path));
@@ -205,7 +171,8 @@ TEST_F(DownloadPathReservationTrackerTest, BasicReservation) {
   EXPECT_EQ(path.value(), reserved_path.value());
 
   // Destroying the item should release the reservation.
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
   item.reset();
   message_loop_.RunUntilIdle();
   EXPECT_FALSE(IsPathInUse(path));
@@ -213,7 +180,7 @@ TEST_F(DownloadPathReservationTrackerTest, BasicReservation) {
 
 // A download that is interrupted should lose its reservation.
 TEST_F(DownloadPathReservationTrackerTest, InterruptedDownload) {
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   ASSERT_FALSE(IsPathInUse(path));
@@ -235,14 +202,16 @@ TEST_F(DownloadPathReservationTrackerTest, InterruptedDownload) {
   EXPECT_EQ(path.value(), reserved_path.value());
 
   // Once the download is interrupted, the path should become available again.
-  item->SetState(DownloadItem::INTERRUPTED);
+  EXPECT_CALL(*item, GetState())
+      .WillRepeatedly(Return(DownloadItem::INTERRUPTED));
+  item->UpdateObservers();
   message_loop_.RunUntilIdle();
   EXPECT_FALSE(IsPathInUse(path));
 }
 
 // A completed download should also lose its reservation.
 TEST_F(DownloadPathReservationTrackerTest, CompleteDownload) {
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   ASSERT_FALSE(IsPathInUse(path));
@@ -267,7 +236,8 @@ TEST_F(DownloadPathReservationTrackerTest, CompleteDownload) {
   // real download, at this point only the path reservation will be released.
   // The path wouldn't be available since it is occupied on disk by the
   // completed download.
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
   message_loop_.RunUntilIdle();
   EXPECT_FALSE(IsPathInUse(path));
 }
@@ -275,7 +245,7 @@ TEST_F(DownloadPathReservationTrackerTest, CompleteDownload) {
 // If there are files on the file system, a unique reservation should uniquify
 // around it.
 TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) {
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   base::FilePath path1(
@@ -308,7 +278,8 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) {
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo (1).txt")).value(),
       reserved_path.value());
 
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
   item.reset();
   message_loop_.RunUntilIdle();
   EXPECT_TRUE(IsPathInUse(path));
@@ -317,7 +288,7 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) {
 
 // Multiple reservations for the same path should uniquify around each other.
 TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
-  scoped_ptr<FakeDownloadItem> item1(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item1(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   base::FilePath uniquified_path(
@@ -345,7 +316,7 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
   {
     // Requesting a reservation for the same path with uniquification results in
     // a uniquified path.
-    scoped_ptr<FakeDownloadItem> item2(CreateDownloadItem(2));
+    scoped_ptr<MockDownloadItem> item2(CreateDownloadItem(2));
     base::FilePath reserved_path2;
     CallGetReservedPath(
         item2.get(),
@@ -357,7 +328,9 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
     EXPECT_TRUE(IsPathInUse(path));
     EXPECT_TRUE(IsPathInUse(uniquified_path));
     EXPECT_EQ(uniquified_path.value(), reserved_path2.value());
-    item2->SetState(DownloadItem::COMPLETE);
+    EXPECT_CALL(*item2, GetState())
+        .WillRepeatedly(Return(DownloadItem::COMPLETE));
+    item2->UpdateObservers();

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

You do this often enough that maybe a helper function is appropriate? 
Static/anonymous namespace seems fine to me; no need for any fancy inheritance
or anything.

[asanka, 2014/04/25 21:31:27]

Done. Added a SetDownloadItemState() function.

   }
   message_loop_.RunUntilIdle();
   EXPECT_TRUE(IsPathInUse(path));
@@ -366,7 +339,7 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
   {
     // Since the previous download item was removed, requesting a reservation
     // for the same path should result in the same uniquified path.
-    scoped_ptr<FakeDownloadItem> item2(CreateDownloadItem(2));
+    scoped_ptr<MockDownloadItem> item2(CreateDownloadItem(2));
     base::FilePath reserved_path2;
     CallGetReservedPath(
         item2.get(),
@@ -378,13 +351,15 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
     EXPECT_TRUE(IsPathInUse(path));
     EXPECT_TRUE(IsPathInUse(uniquified_path));
     EXPECT_EQ(uniquified_path.value(), reserved_path2.value());
-    item2->SetState(DownloadItem::COMPLETE);
+    EXPECT_CALL(*item2, GetState())
+        .WillRepeatedly(Return(DownloadItem::COMPLETE));
+    item2->UpdateObservers();
   }
   message_loop_.RunUntilIdle();
 
   // Now acquire an overwriting reservation. We should end up with the same
   // non-uniquified path for both reservations.
-  scoped_ptr<FakeDownloadItem> item3(CreateDownloadItem(2));
+  scoped_ptr<MockDownloadItem> item3(CreateDownloadItem(2));
   base::FilePath reserved_path3;
   conflict_action = DownloadPathReservationTracker::OVERWRITE;
   CallGetReservedPath(
@@ -400,8 +375,12 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
   EXPECT_EQ(path.value(), reserved_path1.value());
   EXPECT_EQ(path.value(), reserved_path3.value());
 
-  item1->SetState(DownloadItem::COMPLETE);
-  item3->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item1, GetState())
+      .WillRepeatedly(Return(DownloadItem::COMPLETE));
+  EXPECT_CALL(*item3, GetState())
+      .WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item1->UpdateObservers();
+  item3->UpdateObservers();
 }
 
 // If a unique path cannot be determined after trying kMaxUniqueFiles
@@ -410,8 +389,8 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {
 TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
-  scoped_ptr<FakeDownloadItem> items[
-      DownloadPathReservationTracker::kMaxUniqueFiles + 1];
+  scoped_ptr<MockDownloadItem>
+      items[DownloadPathReservationTracker::kMaxUniqueFiles + 1];
   DownloadPathReservationTracker::FilenameConflictAction conflict_action =
     DownloadPathReservationTracker::UNIQUIFY;
   bool create_directory = false;
@@ -442,7 +421,7 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {
     EXPECT_TRUE(verified);
   }
   // The next reservation for |path| will fail to be unique.
-  scoped_ptr<FakeDownloadItem> item(
+  scoped_ptr<MockDownloadItem> item(
       CreateDownloadItem(DownloadPathReservationTracker::kMaxUniqueFiles + 1));
   base::FilePath reserved_path;
   bool verified = true;
@@ -456,16 +435,19 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {
   EXPECT_FALSE(verified);
   EXPECT_EQ(path.value(), reserved_path.value());
 
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
   for (int i = 0; i <= DownloadPathReservationTracker::kMaxUniqueFiles; i++) {
-    items[i]->SetState(DownloadItem::COMPLETE);
+    EXPECT_CALL(*items[i], GetState())
+        .WillRepeatedly(Return(DownloadItem::COMPLETE));
+    items[i]->UpdateObservers();
   }
 }
 
 // If the target directory is unwriteable, then callback should be notified that
 // verification failed.
 TEST_F(DownloadPathReservationTrackerTest, UnwriteableDirectory) {
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   base::FilePath dir(path.DirName());
@@ -491,7 +473,8 @@ TEST_F(DownloadPathReservationTrackerTest, UnwriteableDirectory) {
     EXPECT_FALSE(verified);
     EXPECT_EQ(path.BaseName().value(), reserved_path.BaseName().value());
   }
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
 }
 
 // If the default download directory doesn't exist, then it should be
@@ -506,7 +489,7 @@ TEST_F(DownloadPathReservationTrackerTest, CreateDefaultDownloadPath) {
   bool create_directory = false;
 
   {
-    scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+    scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
     base::FilePath reserved_path;
     bool verified = true;
     CallGetReservedPath(
@@ -518,11 +501,13 @@ TEST_F(DownloadPathReservationTrackerTest, CreateDefaultDownloadPath) {
         &verified);
     // Verification fails because the directory doesn't exist.
     EXPECT_FALSE(verified);
-    item->SetState(DownloadItem::COMPLETE);
+    EXPECT_CALL(*item, GetState())
+        .WillRepeatedly(Return(DownloadItem::COMPLETE));
+    item->UpdateObservers();
   }
   ASSERT_FALSE(IsPathInUse(path));
   {
-    scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+    scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
     base::FilePath reserved_path;
     bool verified = true;
     set_default_download_path(dir);
@@ -536,14 +521,16 @@ TEST_F(DownloadPathReservationTrackerTest, CreateDefaultDownloadPath) {
     // Verification succeeds because the directory is created.
     EXPECT_TRUE(verified);
     EXPECT_TRUE(base::DirectoryExists(dir));
-    item->SetState(DownloadItem::COMPLETE);
+    EXPECT_CALL(*item, GetState())
+        .WillRepeatedly(Return(DownloadItem::COMPLETE));
+    item->UpdateObservers();
   }
 }
 
 // If the target path of the download item changes, the reservation should be
 // updated to match.
 TEST_F(DownloadPathReservationTrackerTest, UpdatesToTargetPath) {
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
   ASSERT_FALSE(IsPathInUse(path));
@@ -583,7 +570,8 @@ TEST_F(DownloadPathReservationTrackerTest, UpdatesToTargetPath) {
   EXPECT_TRUE(IsPathInUse(new_target_path));
 
   // Destroying the item should release the reservation.
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
   item.reset();
   message_loop_.RunUntilIdle();
   EXPECT_FALSE(IsPathInUse(new_target_path));
@@ -602,7 +590,7 @@ TEST_F(DownloadPathReservationTrackerTest, BasicTruncation) {
   // ".crdownload". So take it into account. Should be removed in the future.
   const size_t max_length = real_max_length - 11;
 
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(GetLongNamePathInDownloadsDirectory(
       max_length, FILE_PATH_LITERAL(".txt")));
   ASSERT_FALSE(IsPathInUse(path));
@@ -625,7 +613,8 @@ TEST_F(DownloadPathReservationTrackerTest, BasicTruncation) {
   EXPECT_EQ(max_length, reserved_path.BaseName().value().size());
   // But the extension is kept unchanged.
   EXPECT_EQ(path.Extension(), reserved_path.Extension());
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
 }
 
 TEST_F(DownloadPathReservationTrackerTest, TruncationConflict) {
@@ -634,7 +623,7 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationConflict) {
   ASSERT_NE(-1, real_max_length);
   const size_t max_length = real_max_length - 11;
 
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(GetLongNamePathInDownloadsDirectory(
       max_length, FILE_PATH_LITERAL(".txt")));
   base::FilePath path0(GetLongNamePathInDownloadsDirectory(
@@ -665,7 +654,8 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationConflict) {
   EXPECT_TRUE(IsPathInUse(reserved_path));
   EXPECT_TRUE(verified);
   EXPECT_EQ(path2, reserved_path);
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
 }
 
 TEST_F(DownloadPathReservationTrackerTest, TruncationFail) {
@@ -674,7 +664,7 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationFail) {
   ASSERT_NE(-1, real_max_length);
   const size_t max_length = real_max_length - 11;
 
-  scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
+  scoped_ptr<MockDownloadItem> item(CreateDownloadItem(1));
   base::FilePath path(GetPathInDownloadsDirectory(
       (FILE_PATH_LITERAL("a.") +
           base::FilePath::StringType(max_length, 'b')).c_str()));
@@ -694,7 +684,8 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationFail) {
       &verified);
   // We cannot truncate a path with very long extension.
   EXPECT_FALSE(verified);
-  item->SetState(DownloadItem::COMPLETE);
+  EXPECT_CALL(*item, GetState()).WillRepeatedly(Return(DownloadItem::COMPLETE));
+  item->UpdateObservers();
 }
 
 #endif