Replace the DownloadFileManager with direct ownership of DownloadFileImpl

content/browser/download/download_item_impl_unittest.cc

Index: content/browser/download/download_item_impl_unittest.cc
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index 7a186af7659e9d242a7d1985e78fc8fd8b211a68..8b6a69c505c170a5a68203a60a9a978a8ee001ac 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -7,11 +7,13 @@
 #include "base/threading/thread.h"
 #include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
-#include "content/browser/download/download_file_manager.h"
+#include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
+#include "content/browser/download/mock_download_file.h"
 #include "content/public/browser/download_id.h"
+#include "content/public/browser/download_destination_observer.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/test_browser_thread.h"
@@ -28,17 +30,18 @@ using ::testing::_;
 using ::testing::AllOf;
 using ::testing::Property;
 using ::testing::Return;
+using ::testing::SaveArg;
+using ::testing::StrictMock;
 
 DownloadId::Domain kValidDownloadItemIdDomain = "valid DownloadId::Domain";
 
 namespace {
 class MockDelegate : public DownloadItemImplDelegate {
  public:
-  MockDelegate(DownloadFileManager* file_manager)
-      : file_manager_(file_manager) {
-  }
-  MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath& path));
+  MOCK_METHOD2(DetermineDownloadTarget, void(
+      DownloadItemImpl* download, DownloadTargetCallback callback));
   MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItemImpl* download));
+  MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath& path));
   MOCK_METHOD1(CheckForFileRemoval, void(DownloadItemImpl* download));
   MOCK_METHOD1(MaybeCompleteDownload, void(DownloadItemImpl* download));
   MOCK_CONST_METHOD0(GetBrowserContext, content::BrowserContext*());
@@ -67,52 +70,24 @@ class MockRequestHandle : public DownloadRequestHandleInterface {
   MOCK_CONST_METHOD0(DebugString, std::string());
 };
 
-class MockDownloadFileFactory : public content::DownloadFileFactory {
- public:
-  content::DownloadFile* CreateFile(
-      DownloadCreateInfo* info,
-      scoped_ptr<content::ByteStreamReader> stream_reader,
-      DownloadManager* mgr,
-      bool calculate_hash,
-      const net::BoundNetLog& bound_net_log) {
-    return MockCreateFile(
-        info, stream_reader.get(), info->request_handle, mgr, calculate_hash,
-        bound_net_log);
-  }
-
-  MOCK_METHOD6(MockCreateFile,
-               content::DownloadFile*(DownloadCreateInfo*,
-                                      content::ByteStreamReader*,
-                                      const DownloadRequestHandle&,
-                                      DownloadManager*,
-                                      bool,
-                                      const net::BoundNetLog&));
-};
-
-class MockDownloadFileManager : public DownloadFileManager {
- public:
-  MockDownloadFileManager();
-  MOCK_METHOD0(Shutdown, void());
-  MOCK_METHOD1(CancelDownload, void(DownloadId));
-  MOCK_METHOD2(CompleteDownload, void(DownloadId, const base::Closure&));
-  MOCK_METHOD1(OnDownloadManagerShutdown, void(DownloadManager*));
-  MOCK_METHOD4(RenameDownloadFile, void(DownloadId, const FilePath&, bool,
-                                        const RenameCompletionCallback&));
-  MOCK_CONST_METHOD0(NumberOfActiveDownloads, int());
- private:
-  ~MockDownloadFileManager() {}
-};
-
 // Schedules a task to invoke the RenameCompletionCallback with |new_path| on
 // the UI thread. Should only be used as the action for
-// MockDownloadFileManager::Rename*DownloadFile as follows:
-//   EXPECT_CALL(mock_download_file_manager,
-//               RenameDownloadFile(_,_,_,_))
+// MockDownloadFile::Rename as follows:
+//   EXPECT_CALL(download_file, Rename(_,_,_))
 //       .WillOnce(ScheduleRenameCallback(new_path));
 ACTION_P(ScheduleRenameCallback, new_path) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(arg3, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path));
+      base::Bind(arg2, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path));
+}
+
+// Schedules a task to invoke the input closure on
+// the UI thread. Should only be used as the action for
+// MockDownloadFile::Detach/Cancel as follows:
+//   EXPECT_CALL(download_file, Detach(_))
+//       .WillOnce(ScheduleClosure()));
+ACTION(ScheduleClosure) {
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, arg0);
 }
 
 // Similarly for scheduling a completion callback.
@@ -120,10 +95,6 @@ ACTION(ScheduleCompleteCallback) {
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(arg1));
 }
 
-MockDownloadFileManager::MockDownloadFileManager()
-    : DownloadFileManager(new MockDownloadFileFactory) {
-}
-
 }  // namespace
 
 class DownloadItemTest : public testing::Test {
@@ -183,8 +154,7 @@ class DownloadItemTest : public testing::Test {
   DownloadItemTest()
       : ui_thread_(BrowserThread::UI, &loop_),
         file_thread_(BrowserThread::FILE, &loop_),
-        file_manager_(new MockDownloadFileManager),
-        delegate_(file_manager_.get()) {
+        delegate_() {
   }
 
   ~DownloadItemTest() {
@@ -225,6 +195,43 @@ class DownloadItemTest : public testing::Test {
     return download;
   }
 
+  // Add DownloadFile to DownloadItem
+  MockDownloadFile* AddDownloadFileToDownloadItem(
+      DownloadItemImpl* item,
+      DownloadItemImplDelegate::DownloadTargetCallback *callback) {
+    MockDownloadFile* mock_download_file(new StrictMock<MockDownloadFile>);
+    scoped_ptr<content::DownloadFile> download_file(mock_download_file);
+    EXPECT_CALL(*mock_download_file, Initialize(_));
+    if (callback) {
+      // Save the callback.
+      EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _))
+          .WillOnce(SaveArg<1>(callback));
+    } else {
+      // Drop it on the floor.
+      EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _));
+    }
+
+    item->Start(download_file.Pass());
+    loop_.RunAllPending();
+
+    // So that we don't have a function writing to a stack variable
+    // lying around if the above failed.
+    ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
+
+    return mock_download_file;
+  }
+
+  // Cleanup a download item (specifically get rid of the DownloadFile on it).
+  // The item must be in the IN_PROGRESS state.
+  void CleanupItem(DownloadItemImpl* item, MockDownloadFile* download_file) {
+    EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
+
+    EXPECT_CALL(*download_file, Cancel());
+    EXPECT_CALL(delegate_, DownloadStopped(item));
+    item->Cancel(true);
+    loop_.RunAllPending();
+  }
+
   // Destroy a previously created download item.
   void DestroyDownloadItem(DownloadItem* item) {
     allocated_downloads_.erase(item);
@@ -239,15 +246,10 @@ class DownloadItemTest : public testing::Test {
     return &delegate_;
   }
 
-  MockDownloadFileManager* mock_file_manager() {
-    return file_manager_.get();
-  }
-
  private:
   MessageLoopForUI loop_;
   content::TestBrowserThread ui_thread_;    // UI thread
   content::TestBrowserThread file_thread_;  // FILE thread
-  scoped_refptr<MockDownloadFileManager> file_manager_;
   testing::NiceMock<MockDelegate> delegate_;
   std::set<DownloadItem*> allocated_downloads_;
 };
@@ -281,24 +283,31 @@ TEST_F(DownloadItemTest, NotificationAfterUpdate) {
 
 TEST_F(DownloadItemTest, NotificationAfterCancel) {
   DownloadItemImpl* user_cancel = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file =
+      AddDownloadFileToDownloadItem(user_cancel, NULL);
+  EXPECT_CALL(*download_file, Cancel());
   MockObserver observer1(user_cancel);
 
   user_cancel->Cancel(true);
   ASSERT_TRUE(observer1.CheckUpdated());
+  RunAllPendingInMessageLoops();
 
   DownloadItemImpl* system_cancel =
       CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  download_file = AddDownloadFileToDownloadItem(system_cancel, NULL);
+  EXPECT_CALL(*download_file, Cancel());
   MockObserver observer2(system_cancel);
 
   system_cancel->Cancel(false);
   ASSERT_TRUE(observer2.CheckUpdated());
+  RunAllPendingInMessageLoops();
 }
 
 TEST_F(DownloadItemTest, NotificationAfterComplete) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
   MockObserver observer(item);
 
-  item->OnAllDataSaved(kDownloadChunkSize, DownloadItem::kEmptyFileHash);
+  item->OnAllDataSaved(DownloadItem::kEmptyFileHash);
   ASSERT_TRUE(observer.CheckUpdated());
 
   item->MarkAsComplete();
@@ -315,18 +324,24 @@ TEST_F(DownloadItemTest, NotificationAfterDownloadedFileRemoved) {
 
 TEST_F(DownloadItemTest, NotificationAfterInterrupted) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  EXPECT_CALL(*download_file, Cancel());
   MockObserver observer(item);
 
   item->Interrupt(content::DOWNLOAD_INTERRUPT_REASON_NONE);
   ASSERT_TRUE(observer.CheckUpdated());
+  RunAllPendingInMessageLoops();

[benjhayden, 2012/09/28 20:49:42]

Why are all these RunAllPending calls necessary?
I've started to think of these as flags that the test code might be flaky.

[Randy Smith (Not in Mondays), 2012/10/09 20:20:19]

I don't have the faintest idea; the tests run fine without them. I'm going to
blame it on historical reasons (this CL has been around long enough to deserve a
history text) and just remove them.  Done.

(I basically agree with your flaky instinct, by the way.  Best case, it's ugly
white box testing (I know that I'll need to bounce off this thread, this thread,
and this thread, so I'll put in three message loop polls ...) and worse case
it's shotgun "debugging".  Which doesn't mean I may not do it in the
future--I'll just make sure to have my justification ready :-}).

 }
 
 TEST_F(DownloadItemTest, NotificationAfterDelete) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  EXPECT_CALL(*download_file, Cancel());
   MockObserver observer(item);
 
   item->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
   ASSERT_TRUE(observer.CheckUpdated());
+  RunAllPendingInMessageLoops();
 }
 
 TEST_F(DownloadItemTest, NotificationAfterDestroyed) {
@@ -339,6 +354,8 @@ TEST_F(DownloadItemTest, NotificationAfterDestroyed) {
 
 TEST_F(DownloadItemTest, NotificationAfterRemove) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  EXPECT_CALL(*download_file, Cancel());
   MockObserver observer(item);
 
   item->Remove();
@@ -351,7 +368,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
   DownloadItemImpl* safe_item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
   MockObserver safe_observer(safe_item);
 
-  safe_item->OnAllDataSaved(1, "");
+  safe_item->OnAllDataSaved("");
   EXPECT_TRUE(safe_observer.CheckUpdated());
   safe_item->OnContentCheckCompleted(
       content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
@@ -362,7 +379,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
       CreateDownloadItem(DownloadItem::IN_PROGRESS);
   MockObserver unsafeurl_observer(unsafeurl_item);
 
-  unsafeurl_item->OnAllDataSaved(1, "");
+  unsafeurl_item->OnAllDataSaved("");
   EXPECT_TRUE(unsafeurl_observer.CheckUpdated());
   unsafeurl_item->OnContentCheckCompleted(
       content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL);
@@ -375,7 +392,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
       CreateDownloadItem(DownloadItem::IN_PROGRESS);
   MockObserver unsafefile_observer(unsafefile_item);
 
-  unsafefile_item->OnAllDataSaved(1, "");
+  unsafefile_item->OnAllDataSaved("");
   EXPECT_TRUE(unsafefile_observer.CheckUpdated());
   unsafefile_item->OnContentCheckCompleted(
       content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE);
@@ -386,30 +403,32 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
 }
 
 // DownloadItemImpl::OnDownloadTargetDetermined will schedule a task to run
-// DownloadFileManager::RenameDownloadFile(). Once the rename
+// DownloadFile::Rename(). Once the rename
 // completes, DownloadItemImpl receives a notification with the new file
 // name. Check that observers are updated when the new filename is available and
 // not before.
 TEST_F(DownloadItemTest, NotificationAfterOnDownloadTargetDetermined) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  DownloadItemImplDelegate::DownloadTargetCallback callback;
+  MockDownloadFile* download_file =
+      AddDownloadFileToDownloadItem(item, &callback);
   MockObserver observer(item);
   FilePath target_path(kDummyPath);
   FilePath intermediate_path(target_path.InsertBeforeExtensionASCII("x"));
   FilePath new_intermediate_path(target_path.InsertBeforeExtensionASCII("y"));
-  EXPECT_CALL(*mock_file_manager(),
-              RenameDownloadFile(_,intermediate_path,false,_))
+  EXPECT_CALL(*download_file, Rename(intermediate_path, false, _))
       .WillOnce(ScheduleRenameCallback(new_intermediate_path));
 
   // Currently, a notification would be generated if the danger type is anything
   // other than NOT_DANGEROUS.
-  item->OnDownloadTargetDetermined(target_path,
-                                   DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                                   content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                                   intermediate_path);
+  callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+               content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   EXPECT_FALSE(observer.CheckUpdated());
   RunAllPendingInMessageLoops();
   EXPECT_TRUE(observer.CheckUpdated());
   EXPECT_EQ(new_intermediate_path, item->GetFullPath());
+
+  CleanupItem(item, download_file);
 }
 
 TEST_F(DownloadItemTest, NotificationAfterTogglePause) {
@@ -425,23 +444,35 @@ TEST_F(DownloadItemTest, NotificationAfterTogglePause) {
 
 TEST_F(DownloadItemTest, DisplayName) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  DownloadItemImplDelegate::DownloadTargetCallback callback;
+  MockDownloadFile* download_file =
+      AddDownloadFileToDownloadItem(item, &callback);
   FilePath target_path(FilePath(kDummyPath).AppendASCII("foo.bar"));
   FilePath intermediate_path(target_path.InsertBeforeExtensionASCII("x"));
   EXPECT_EQ(FILE_PATH_LITERAL(""),
             item->GetFileNameToReportUser().value());
-  EXPECT_CALL(*mock_file_manager(),
-              RenameDownloadFile(_,_,false,_))
+  EXPECT_CALL(*download_file, Rename(_, false, _))
       .WillOnce(ScheduleRenameCallback(intermediate_path));
-  item->OnDownloadTargetDetermined(target_path,
-                                   DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                                   content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                                   intermediate_path);
+  callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+               content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
   EXPECT_EQ(FILE_PATH_LITERAL("foo.bar"),
             item->GetFileNameToReportUser().value());
   item->SetDisplayName(FilePath(FILE_PATH_LITERAL("new.name")));
   EXPECT_EQ(FILE_PATH_LITERAL("new.name"),
             item->GetFileNameToReportUser().value());
+  CleanupItem(item, download_file);
+}
+
+// Test to make sure that Start method calls DF initialize properly.
+TEST_F(DownloadItemTest, Start) {
+  MockDownloadFile* mock_download_file(new MockDownloadFile);
+  scoped_ptr<content::DownloadFile> download_file(mock_download_file);
+  DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  EXPECT_CALL(*mock_download_file, Initialize(_));
+  item->Start(download_file.Pass());
+
+  CleanupItem(item, mock_download_file);
 }
 
 // Test that the delegate is invoked after the download file is renamed.
@@ -451,13 +482,15 @@ TEST_F(DownloadItemTest, DisplayName) {
 // rename.
 TEST_F(DownloadItemTest, CallbackAfterRename) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  DownloadItemImplDelegate::DownloadTargetCallback callback;
+  MockDownloadFile* download_file =
+      AddDownloadFileToDownloadItem(item, &callback);
   FilePath final_path(FilePath(kDummyPath).AppendASCII("foo.bar"));
   FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
   FilePath new_intermediate_path(final_path.InsertBeforeExtensionASCII("y"));
-  EXPECT_CALL(*mock_file_manager(),
-              RenameDownloadFile(item->GetGlobalId(),
-                                 intermediate_path, false, _))
+  EXPECT_CALL(*download_file, Rename(intermediate_path, false, _))
       .WillOnce(ScheduleRenameCallback(new_intermediate_path));
+
   // DownloadItemImpl should invoke this callback on the delegate once the
   // download is renamed to the intermediate name. Also check that GetFullPath()
   // returns the intermediate path at the time of the call.
@@ -466,23 +499,16 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
                   AllOf(item,
                         Property(&DownloadItem::GetFullPath,
                                  new_intermediate_path))));
-  item->OnDownloadTargetDetermined(final_path,
-                                   DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                                   content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                                   intermediate_path);
+  callback.Run(final_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+               content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
   // All the callbacks should have happened by now.
-  ::testing::Mock::VerifyAndClearExpectations(mock_file_manager());
+  ::testing::Mock::VerifyAndClearExpectations(download_file);
   ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
 
-  item->OnAllDataSaved(10, "");
-  EXPECT_CALL(*mock_file_manager(),
-              RenameDownloadFile(item->GetGlobalId(),
-                                 final_path, true, _))
+  item->OnAllDataSaved("");
+  EXPECT_CALL(*download_file, Rename(final_path, true, _))
       .WillOnce(ScheduleRenameCallback(final_path));
-  EXPECT_CALL(*mock_file_manager(),
-              CompleteDownload(item->GetGlobalId(), _))
-      .WillOnce(ScheduleCompleteCallback());
   // DownloadItemImpl should invoke this callback on the delegate after the
   // final rename has completed. Also check that GetFullPath() and
   // GetTargetFilePath() return the final path at the time of the call.
@@ -495,20 +521,25 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
   EXPECT_CALL(*mock_delegate(), DownloadCompleted(item));
   EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item))
       .WillOnce(Return(true));
+  EXPECT_CALL(*download_file, Detach(_))
+      .WillOnce(ScheduleClosure());
   item->OnDownloadCompleting();
   RunAllPendingInMessageLoops();
-  ::testing::Mock::VerifyAndClearExpectations(mock_file_manager());
+  ::testing::Mock::VerifyAndClearExpectations(download_file);
   ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
 }
 
 TEST_F(DownloadItemTest, Interrupted) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
 
   const content::DownloadInterruptReason reason(
       content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
 
   // Confirm interrupt sets state properly.
+  EXPECT_CALL(*download_file, Cancel());
   item->Interrupt(reason);
+  RunAllPendingInMessageLoops();
   EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_EQ(reason, item->GetLastReason());
 
@@ -521,11 +552,14 @@ TEST_F(DownloadItemTest, Interrupted) {
 
 TEST_F(DownloadItemTest, Canceled) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
 
   // Confirm cancel sets state properly.
   EXPECT_CALL(*mock_delegate(), DownloadStopped(item));
+  EXPECT_CALL(*download_file, Cancel());
   item->Cancel(true);
   EXPECT_EQ(DownloadItem::CANCELLED, item->GetState());
+  RunAllPendingInMessageLoops();
 }
 
 TEST_F(DownloadItemTest, FileRemoved) {
@@ -536,6 +570,88 @@ TEST_F(DownloadItemTest, FileRemoved) {
   EXPECT_TRUE(item->GetFileExternallyRemoved());
 }
 
+TEST_F(DownloadItemTest, DestinationUpdate) {
+  DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  base::WeakPtr<content::DownloadDestinationObserver> as_observer(
+      item->DestinationObserverAsWeakPtr());
+  MockObserver observer(item);
+
+  EXPECT_EQ(0l, item->CurrentSpeed());
+  EXPECT_EQ("", item->GetHashState());
+  EXPECT_EQ(0l, item->GetReceivedBytes());
+  EXPECT_EQ(0l, item->GetTotalBytes());
+  EXPECT_FALSE(observer.CheckUpdated());
+  item->SetTotalBytes(100l);
+  EXPECT_EQ(100l, item->GetTotalBytes());
+
+  as_observer->DestinationUpdate(10, 20, "deadbeef");
+  EXPECT_EQ(20l, item->CurrentSpeed());
+  EXPECT_EQ("deadbeef", item->GetHashState());
+  EXPECT_EQ(10l, item->GetReceivedBytes());
+  EXPECT_EQ(100l, item->GetTotalBytes());
+  EXPECT_TRUE(observer.CheckUpdated());
+
+  as_observer->DestinationUpdate(200, 20, "livebeef");
+  EXPECT_EQ(20l, item->CurrentSpeed());
+  EXPECT_EQ("livebeef", item->GetHashState());
+  EXPECT_EQ(200l, item->GetReceivedBytes());
+  EXPECT_EQ(0l, item->GetTotalBytes());
+  EXPECT_TRUE(observer.CheckUpdated());
+}
+
+TEST_F(DownloadItemTest, DestinationError) {
+  DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  base::WeakPtr<content::DownloadDestinationObserver> as_observer(
+      item->DestinationObserverAsWeakPtr());
+  MockObserver observer(item);
+
+  EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, item->GetLastReason());
+  EXPECT_FALSE(observer.CheckUpdated());
+
+  EXPECT_CALL(*mock_delegate(), DownloadStopped(item));
+  EXPECT_CALL(*download_file, Cancel());
+  as_observer->DestinationError(
+      content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
+  ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
+  EXPECT_TRUE(observer.CheckUpdated());
+  EXPECT_EQ(content::DownloadItem::INTERRUPTED, item->GetState());
+  EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
+            item->GetLastReason());
+  RunAllPendingInMessageLoops();
+}
+
+TEST_F(DownloadItemTest, DestinationCompleted) {
+  DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  base::WeakPtr<content::DownloadDestinationObserver> as_observer(
+      item->DestinationObserverAsWeakPtr());
+  MockObserver observer(item);
+
+  EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_EQ("", item->GetHash());
+  EXPECT_EQ("", item->GetHashState());
+  EXPECT_FALSE(item->AllDataSaved());
+  EXPECT_FALSE(observer.CheckUpdated());
+
+  as_observer->DestinationUpdate(10, 20, "deadbeef");
+  EXPECT_TRUE(observer.CheckUpdated());
+  EXPECT_FALSE(observer.CheckUpdated()); // Confirm reset.
+  EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_EQ("", item->GetHash());
+  EXPECT_EQ("deadbeef", item->GetHashState());
+  EXPECT_FALSE(item->AllDataSaved());
+
+  EXPECT_CALL(*mock_delegate(), MaybeCompleteDownload(item));
+  as_observer->DestinationCompleted("livebeef");
+  ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
+  EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_TRUE(observer.CheckUpdated());
+  EXPECT_EQ("livebeef", item->GetHash());
+  EXPECT_EQ("", item->GetHashState());
+  EXPECT_TRUE(item->AllDataSaved());
+}
+
 TEST(MockDownloadItem, Compiles) {
   MockDownloadItem mock_item;
 }