Clean up last half of download flow, simplifying delegate interface, and moving more code for driving the download into DownloadItemImpl.

Review URL: https://chromiumcodereview.appspot.com/10984056

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

11 files changed, 167 insertions, 142 deletions

diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 8ab5872c..aeb7fc3 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -2,13 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// File method ordering: Methods in this file are in the same order
-// as in download_item_impl.h, with the following exception: The public
-// interfaces DelayedDownloadOpened, OnDownloadTargetDetermined, and
-// OnDownloadCompleting are placed in chronological order with the other
-// (private) routines that together define a DownloadItem's state transitions
-// as the download progresses.  See "Download progression cascade" later in
-// this file.
+// File method ordering: Methods in this file are in the same order as
+// in download_item_impl.h, with the following exception: The public
+// interfaces DelayedDownloadOpened, OnDownloadTargetDetermined,
+// MaybeCompleteDownload, and OnDownloadCompleting are placed in
+// chronological order with the other (private) routines that together
+// define a DownloadItem's state transitions as the download
+// progresses.  See "Download progression cascade" later in this file.
 
 // A regular DownloadItem (created for a download in this session of the
 // browser) normally goes through the following states:
@@ -321,7 +321,7 @@ void DownloadItemImpl::DangerousDownloadValidated() {
 
   UpdateObservers();
 
-  delegate_->MaybeCompleteDownload(this);
+  MaybeCompleteDownload();
 }
 
 void DownloadItemImpl::TogglePause() {
@@ -1012,19 +1012,55 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
   delegate_->DownloadRenamedToIntermediateName(this);
 }
 
+// When SavePackage downloads MHTML to GData (see
+// SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
+// does for non-SavePackage downloads, but SavePackage downloads never satisfy
+// IsDownloadReadyForCompletion(). GDataDownloadObserver manually calls
+// DownloadItem::UpdateObservers() when the upload completes so that SavePackage
+// notices that the upload has completed and runs its normal Finish() pathway.
+// MaybeCompleteDownload() is never the mechanism by which SavePackage completes
+// downloads. SavePackage always uses its own Finish() to mark downloads
+// complete.
 void DownloadItemImpl::MaybeCompleteDownload() {
-  // TODO(rdsmith): Move logic for this function here.
-  delegate_->MaybeCompleteDownload(this);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (!IsDownloadReadyForCompletion())
+    return;
+
+  // TODO(rdsmith): DCHECK that we only pass through this point
+  // once per download.  The natural way to do this is by a state
+  // transition on the DownloadItem.
+
+  // Confirm we're in the proper set of states to be here;
+  // have all data, have a history handle, (validated or safe).
+  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
+  DCHECK_NE(DownloadItem::DANGEROUS, GetSafetyState());
+  DCHECK(all_data_saved_);
+  DCHECK(is_persisted_);
+
+  delegate_->UpdatePersistence(this);
+
+  OnDownloadCompleting();
 }
 
-// Called by DownloadManagerImpl::MaybeCompleteDownload() when it has
-// determined that the download is ready for completion.
+// Called by MaybeCompleteDownload() when it has determined that the download
+// is ready for completion.
 void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (state_ != IN_PROGRESS_INTERNAL)
     return;
 
+  // Give the delegate a chance to override.
+  delegate_->ReadyForDownloadCompletion(
+      this, base::Bind(&DownloadItemImpl::ReadyForDownloadCompletionDone,
+                       weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadItemImpl::ReadyForDownloadCompletionDone() {
+  if (state_ != IN_PROGRESS_INTERNAL)
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
@@ -1131,6 +1167,31 @@ void DownloadItemImpl::Completed() {
 
 // **** End of Download progression cascade
 
+bool DownloadItemImpl::IsDownloadReadyForCompletion() {
+  // If we don't have all the data, the download is not ready for
+  // completion.
+  if (!AllDataSaved())
+    return false;
+
+  // If the download is dangerous, but not yet validated, it's not ready for
+  // completion.
+  if (GetSafetyState() == DownloadItem::DANGEROUS)
+    return false;
+
+  // If the download isn't active (e.g. has been cancelled) it's not
+  // ready for completion.
+  if (state_ != IN_PROGRESS_INTERNAL)
+    return false;
+
+  // If the download hasn't been inserted into the history system
+  // (which occurs strictly after file name determination, intermediate
+  // file rename, and UI display) then it's not ready for completion.
+  if (!IsPersisted())
+    return false;
+
+  return true;
+}
+
 bool DownloadItemImpl::NeedsRename() const {
   DCHECK(target_path_.DirName() == current_path_.DirName());
   return target_path_ != current_path_;
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 72d8c26..2acd30a 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -155,10 +155,10 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
       content::DownloadDangerType danger_type,
       const FilePath& intermediate_path);
 
-  // Called when the download is ready to complete.
-  // This may perform final rename if necessary and will eventually call
-  // DownloadItem::Completed().
-  virtual void OnDownloadCompleting();
+  // If all pre-requisites have been met, complete download processing, i.e. do
+  // internal cleanup, file rename, and potentially auto-open.  (Dangerous
+  // downloads still may block on user acceptance after this point.)
+  virtual void MaybeCompleteDownload();
 
   // Needed because of interwining with DownloadManagerImpl --------------------
 
@@ -252,10 +252,14 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   void OnDownloadRenamedToIntermediateName(
       content::DownloadInterruptReason reason, const FilePath& full_path);
 
-  // If all pre-requisites have been met, complete download processing, i.e. do
-  // internal cleanup, file rename, and potentially auto-open.  (Dangerous
-  // downloads still may block on user acceptance after this point.)
-  void MaybeCompleteDownload();
+  // Called when the download is ready to complete.
+  // This may perform final rename if necessary and will eventually call
+  // DownloadItem::Completed().
+  virtual void OnDownloadCompleting();
+
+  // Called after the delegate has given the go-ahead to actually complete
+  // the download.
+  void ReadyForDownloadCompletionDone();
 
   void OnDownloadRenamedToFinalName(content::DownloadInterruptReason reason,
                                     const FilePath& full_path);
@@ -268,6 +272,9 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
 
   // Helper routines -----------------------------------------------------------
 
+  // Check if a download is ready for completion.
+  bool IsDownloadReadyForCompletion();
+
   // Returns true if the download still needs to be renamed to
   // GetTargetFilePath().
   bool NeedsRename() const;
diff --git a/content/browser/download/download_item_impl_delegate.cc b/content/browser/download/download_item_impl_delegate.cc
index 4a889f7..f7d2d12 100644
--- a/content/browser/download/download_item_impl_delegate.cc
+++ b/content/browser/download/download_item_impl_delegate.cc
@@ -25,6 +25,12 @@ void DownloadItemImplDelegate::Detach() {
   --count_;
 }
 
+void DownloadItemImplDelegate::ReadyForDownloadCompletion(
+    DownloadItemImpl* download,
+    const base::Closure& complete_callback) {
+  complete_callback.Run();
+}
+
 bool DownloadItemImplDelegate::ShouldOpenFileBasedOnExtension(
     const FilePath& path) {
   return false;
@@ -37,9 +43,6 @@ bool DownloadItemImplDelegate::ShouldOpenDownload(DownloadItemImpl* download) {
 void DownloadItemImplDelegate::CheckForFileRemoval(
     DownloadItemImpl* download_item) {}
 
-void DownloadItemImplDelegate::MaybeCompleteDownload(
-    DownloadItemImpl* download) {}
-
 content::BrowserContext* DownloadItemImplDelegate::GetBrowserContext() const {
   return NULL;
 }
@@ -48,6 +51,8 @@ DownloadFileManager* DownloadItemImplDelegate::GetDownloadFileManager() {
   return NULL;
 }
 
+void DownloadItemImplDelegate::UpdatePersistence(DownloadItemImpl* download) {}
+
 void DownloadItemImplDelegate::DownloadStopped(DownloadItemImpl* download) {}
 
 void DownloadItemImplDelegate::DownloadCompleted(DownloadItemImpl* download) {}
diff --git a/content/browser/download/download_item_impl_delegate.h b/content/browser/download/download_item_impl_delegate.h
index 0092454..9e5e2aa 100644
--- a/content/browser/download/download_item_impl_delegate.h
+++ b/content/browser/download/download_item_impl_delegate.h
@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_DELEGATE_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_DELEGATE_H_
 
+#include "base/callback.h"
 #include "base/file_path.h"
 #include "content/common/content_export.h"
 
@@ -28,6 +29,15 @@ class CONTENT_EXPORT DownloadItemImplDelegate {
   void Attach();
   void Detach();
 
+  // Allows the delegate to delay completion of the download.  This function
+  // will call the callback passed when the download is ready for completion.
+  // This may be done immediately, from within the routine itself, or it
+  // may be delayed.
+  // This routine should only be called once per download.
+  virtual void ReadyForDownloadCompletion(
+      DownloadItemImpl* download,
+      const base::Closure& complete_callback);
+
   // Tests if a file type should be opened automatically.
   virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path);
 
@@ -41,16 +51,15 @@ class CONTENT_EXPORT DownloadItemImplDelegate {
   // to OnDownloadedFileRemoved().
   virtual void CheckForFileRemoval(DownloadItemImpl* download_item);
 
-  // If all pre-requisites have been met, complete download processing.
-  // TODO(rdsmith): Move into DownloadItem.
-  virtual void MaybeCompleteDownload(DownloadItemImpl* download);
-
   // For contextual issues like language and prefs.
   virtual content::BrowserContext* GetBrowserContext() const;
 
   // Get the DownloadFileManager to use for this download.
   virtual DownloadFileManager* GetDownloadFileManager();
 
+  // Update the persistent store with our information.
+  virtual void UpdatePersistence(DownloadItemImpl* download);
+
   // Handle any delegate portions of a state change operation on the
   // DownloadItem.
   virtual void DownloadStopped(DownloadItemImpl* download);
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index c2e0b91..46ec7a5 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -37,11 +37,13 @@ class MockDelegate : public DownloadItemImplDelegate {
   MockDelegate(DownloadFileManager* file_manager)
       : file_manager_(file_manager) {
   }
+  MOCK_METHOD2(ReadyForDownloadCompletion,
+               void(DownloadItemImpl*, const base::Closure&));
   MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath& path));
   MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItemImpl* download));
   MOCK_METHOD1(CheckForFileRemoval, void(DownloadItemImpl* download));
-  MOCK_METHOD1(MaybeCompleteDownload, void(DownloadItemImpl* download));
   MOCK_CONST_METHOD0(GetBrowserContext, content::BrowserContext*());
+  MOCK_METHOD1(UpdatePersistence, void(DownloadItemImpl* download));
   MOCK_METHOD1(DownloadStopped, void(DownloadItemImpl* download));
   MOCK_METHOD1(DownloadCompleted, void(DownloadItemImpl* download));
   MOCK_METHOD1(DownloadOpened, void(DownloadItemImpl* download));
@@ -120,6 +122,11 @@ ACTION(ScheduleCompleteCallback) {
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(arg1));
 }
 
+// Just run the callback in arg1
+ACTION(RunArg1Callback) {
+  arg1.Run();
+}
+
 MockDownloadFileManager::MockDownloadFileManager()
     : DownloadFileManager(new MockDownloadFileFactory) {
 }
@@ -267,7 +274,6 @@ const FilePath::CharType kDummyPath[] = FILE_PATH_LITERAL("/testpath");
 //  void OpenDownload();
 //  void ShowDownloadInShell();
 //  void CompleteDelayedDownload();
-//  void OnDownloadCompleting();
 //  set_* mutators
 
 TEST_F(DownloadItemTest, NotificationAfterUpdate) {
@@ -476,6 +482,9 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
   ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
 
   item->OnAllDataSaved(10, "");
+  EXPECT_CALL(*mock_delegate(), ReadyForDownloadCompletion(item, _))
+      .WillOnce(RunArg1Callback());
+  EXPECT_CALL(*mock_delegate(), UpdatePersistence(item));
   EXPECT_CALL(*mock_file_manager(),
               RenameDownloadFile(item->GetGlobalId(),
                                  final_path, true, _))
@@ -495,7 +504,8 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
   EXPECT_CALL(*mock_delegate(), DownloadCompleted(item));
   EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item))
       .WillOnce(Return(true));
-  item->OnDownloadCompleting();
+  item->SetIsPersisted();
+  item->MaybeCompleteDownload();
   RunAllPendingInMessageLoops();
   ::testing::Mock::VerifyAndClearExpectations(mock_file_manager());
   ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 3d18682..0287d02 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -233,11 +233,14 @@ DownloadFileManager* DownloadManagerImpl::GetDownloadFileManager() {
   return file_manager_;
 }
 
-bool DownloadManagerImpl::ShouldOpenDownload(DownloadItemImpl* item) {
-  if (!delegate_)
-    return true;
-
-  return delegate_->ShouldOpenDownload(item);
+void DownloadManagerImpl::ReadyForDownloadCompletion(
+    DownloadItemImpl* item, const base::Closure& complete_callback) {
+  if (!delegate_ ||
+      delegate_->ShouldCompleteDownload(item, complete_callback)) {
+    complete_callback.Run();
+  }
+  // Otherwise, the delegate has accepted responsibility to run the
+  // callback when the download is ready for completion.
 }
 
 bool DownloadManagerImpl::ShouldOpenFileBasedOnExtension(const FilePath& path) {
@@ -247,6 +250,13 @@ bool DownloadManagerImpl::ShouldOpenFileBasedOnExtension(const FilePath& path) {
   return delegate_->ShouldOpenFileBasedOnExtension(path);
 }
 
+bool DownloadManagerImpl::ShouldOpenDownload(DownloadItemImpl* item) {
+  if (!delegate_)
+    return true;
+
+  return delegate_->ShouldOpenDownload(item);
+}
+
 void DownloadManagerImpl::SetDelegate(
     content::DownloadManagerDelegate* delegate) {
   delegate_ = delegate;
@@ -546,8 +556,12 @@ void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
     return;
 
   DownloadItemImpl* download = active_downloads_[download_id];
+  // TODO(rdsmith): Make OnAllDataSaved call MaybeCompleteDownload() directly.
+  // This would allow MaybeCompleteDownload() to be private to the
+  // DownloadItemImpl.  It can't currently be done because SavePackage
+  // calls OnAllDataSaved and shouldn't initiate the download cascade.
   download->OnAllDataSaved(size, hash);
-  MaybeCompleteDownload(download);
+  download->MaybeCompleteDownload();
 }
 
 void DownloadManagerImpl::AssertStateConsistent(
@@ -566,85 +580,6 @@ void DownloadManagerImpl::AssertStateConsistent(
     CHECK(ContainsKey(active_downloads_, download->GetId()));
 }
 
-bool DownloadManagerImpl::IsDownloadReadyForCompletion(
-    DownloadItemImpl* download) {
-  // If we don't have all the data, the download is not ready for
-  // completion.
-  if (!download->AllDataSaved())
-    return false;
-
-  // If the download is dangerous, but not yet validated, it's not ready for
-  // completion.
-  if (download->GetSafetyState() == DownloadItem::DANGEROUS)
-    return false;
-
-  // If the download isn't active (e.g. has been cancelled) it's not
-  // ready for completion.
-  if (active_downloads_.count(download->GetId()) == 0)
-    return false;
-
-  // If the download hasn't been inserted into the history system
-  // (which occurs strictly after file name determination, intermediate
-  // file rename, and UI display) then it's not ready for completion.
-  if (!download->IsPersisted())
-    return false;
-
-  return true;
-}
-
-// When SavePackage downloads MHTML to GData (see
-// SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
-// does for non-SavePackage downloads, but SavePackage downloads never satisfy
-// IsDownloadReadyForCompletion(). GDataDownloadObserver manually calls
-// DownloadItem::UpdateObservers() when the upload completes so that SavePackage
-// notices that the upload has completed and runs its normal Finish() pathway.
-// MaybeCompleteDownload() is never the mechanism by which SavePackage completes
-// downloads. SavePackage always uses its own Finish() to mark downloads
-// complete.
-
-void DownloadManagerImpl::MaybeCompleteDownload(
-    DownloadItemImpl* download) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << "()" << " download = "
-           << download->DebugString(false);
-
-  if (!IsDownloadReadyForCompletion(download))
-    return;
-
-  // TODO(rdsmith): DCHECK that we only pass through this point
-  // once per download.  The natural way to do this is by a state
-  // transition on the DownloadItem.
-
-  // Confirm we're in the proper set of states to be here;
-  // have all data, have a history handle, (validated or safe).
-  DCHECK(download->IsInProgress());
-  DCHECK_NE(DownloadItem::DANGEROUS, download->GetSafetyState());
-  DCHECK(download->AllDataSaved());
-  DCHECK(download->IsPersisted());
-
-  // Give the delegate a chance to override.  It's ok to keep re-setting the
-  // delegate's |complete_callback| cb as long as there isn't another call-point
-  // trying to set it to a different cb.  TODO(benjhayden): Change the callback
-  // to point directly to the item instead of |this| when DownloadItem supports
-  // weak-ptrs.
-  if (delegate_ && !delegate_->ShouldCompleteDownload(download, base::Bind(
-          &DownloadManagerImpl::MaybeCompleteDownloadById,
-          this, download->GetId())))
-    return;
-
-  VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
-           << download->DebugString(false);
-
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
-  download->OnDownloadCompleting();
-}
-
-void DownloadManagerImpl::MaybeCompleteDownloadById(int download_id) {
-  if (ContainsKey(active_downloads_, download_id))
-    MaybeCompleteDownload(active_downloads_[download_id]);
-}
-
 void DownloadManagerImpl::DownloadCompleted(DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(download);
@@ -661,6 +596,11 @@ void DownloadManagerImpl::CancelDownload(int32 download_id) {
     active_downloads_[download_id]->Cancel(true);
 }
 
+void DownloadManagerImpl::UpdatePersistence(DownloadItemImpl* download) {
+  if (delegate_)
+    delegate_->UpdateItemInPersistentStore(download);
+}
+
 void DownloadManagerImpl::DownloadStopped(DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -875,7 +815,7 @@ void DownloadManagerImpl::OnDownloadItemAddedToPersistentStore(
   // completion status, and also inform any observers so that they get
   // more than just the start notification.
   if (item->IsInProgress()) {
-    MaybeCompleteDownload(item);
+    item->MaybeCompleteDownload();
   } else {
     DCHECK(item->IsCancelled());
     active_downloads_.erase(item->GetId());
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index fd148fe..bd52481 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -102,14 +102,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Returns the |BoundNetLog| used by the |DownloadItem|.
   virtual net::BoundNetLog CreateDownloadItem(DownloadCreateInfo* info);
 
-  // Does nothing if |download_id| is not an active download.
-  void MaybeCompleteDownloadById(int download_id);
-
-  // Determine if the download is ready for completion, i.e. has had
-  // all data saved, and completed the filename determination and
-  // history insertion.
-  bool IsDownloadReadyForCompletion(DownloadItemImpl* download);
-
   // Show the download in the browser.
   void ShowDownloadInBrowser(DownloadItemImpl* download);
 
@@ -167,10 +159,12 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Overridden from DownloadItemImplDelegate
   // (Note that |GetBrowserContext| are present in both interfaces.)
   virtual DownloadFileManager* GetDownloadFileManager() OVERRIDE;
-  virtual bool ShouldOpenDownload(DownloadItemImpl* item) OVERRIDE;
+  virtual void ReadyForDownloadCompletion(
+      DownloadItemImpl* item, const base::Closure& complete_callback) OVERRIDE;
   virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) OVERRIDE;
+  virtual bool ShouldOpenDownload(DownloadItemImpl* item) OVERRIDE;
   virtual void CheckForFileRemoval(DownloadItemImpl* download_item) OVERRIDE;
-  virtual void MaybeCompleteDownload(DownloadItemImpl* download) OVERRIDE;
+  virtual void UpdatePersistence(DownloadItemImpl* download) OVERRIDE;
   virtual void DownloadStopped(DownloadItemImpl* download) OVERRIDE;
   virtual void DownloadCompleted(DownloadItemImpl* download) OVERRIDE;
   virtual void DownloadOpened(DownloadItemImpl* download) OVERRIDE;
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index e0368bb..ff28c63 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -100,7 +100,6 @@ class MockDownloadItemImpl : public DownloadItemImpl {
   MOCK_CONST_METHOD0(PercentComplete, int());
   MOCK_CONST_METHOD0(AllDataSaved, bool());
   MOCK_METHOD0(TogglePause, void());
-  MOCK_METHOD0(OnDownloadCompleting, void());
   MOCK_CONST_METHOD1(MatchesQuery, bool(const string16& query));
   MOCK_CONST_METHOD0(IsPartialDownload, bool());
   MOCK_CONST_METHOD0(IsInProgress, bool());
@@ -178,8 +177,8 @@ class MockDownloadManagerDelegate : public content::DownloadManagerDelegate {
                     const content::DownloadTargetCallback&));
   MOCK_METHOD0(GetAlternativeWebContentsToNotifyForDownload, WebContents*());
   MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath&));
-  MOCK_METHOD2(ShouldCompleteDownload, bool(
-      DownloadItem*, const base::Closure&));
+  MOCK_METHOD2(ShouldCompleteDownload,
+               bool(DownloadItem*, const base::Closure&));
   MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItem*));
   MOCK_METHOD0(GenerateFileHash, bool());
   MOCK_METHOD1(AddItemToPersistentStore, void(DownloadItem*));
@@ -552,9 +551,9 @@ class DownloadManagerTest : public testing::Test {
     EXPECT_CALL(item, IsInProgress())
         .WillOnce(Return(true));
 
-    // Null out MaybeCompleteDownload
-    EXPECT_CALL(item, AllDataSaved())
-        .WillOnce(Return(false));
+    // History addition should result in a call into MaybeCompleteDownload().
+    EXPECT_CALL(item, MaybeCompleteDownload())
+        .WillOnce(Return());
 
     download_manager_->OnItemAddedToPersistentStore(item.GetId(), db_handle);
   }
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index 3b8fffa..1ec2a61 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -346,8 +346,12 @@ void SavePackage::OnMHTMLGenerated(const FilePath& path, int64 size) {
     download_->OnAllDataSaved(size, DownloadItem::kEmptyFileHash);
   }
 
-  if (!download_manager_->GetDelegate() ||
-      download_manager_->GetDelegate()->ShouldCompleteDownload(
+  if (!download_manager_->GetDelegate()) {
+    Finish();
+    return;
+  }
+
+  if (download_manager_->GetDelegate()->ShouldCompleteDownload(
           download_, base::Bind(&SavePackage::Finish, this))) {
     Finish();
   }
diff --git a/content/public/browser/download_manager_delegate.cc b/content/public/browser/download_manager_delegate.cc
index 1896874..c836029 100644
--- a/content/public/browser/download_manager_delegate.cc
+++ b/content/public/browser/download_manager_delegate.cc
@@ -31,7 +31,7 @@ bool DownloadManagerDelegate::ShouldOpenFileBasedOnExtension(
 
 bool DownloadManagerDelegate::ShouldCompleteDownload(
     DownloadItem* item,
-    const base::Closure& complete_callback) {
+    const base::Closure& callback) {
   return true;
 }
 
diff --git a/content/public/browser/download_manager_delegate.h b/content/public/browser/download_manager_delegate.h
index 831d4d4..084cbb7 100644
--- a/content/public/browser/download_manager_delegate.h
+++ b/content/public/browser/download_manager_delegate.h
@@ -80,13 +80,9 @@ class CONTENT_EXPORT DownloadManagerDelegate {
   virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path);
 
   // Allows the delegate to delay completion of the download.  This function
-  // will either return true (in which case the download is ready to complete)
-  // or arrange for complete_callback to be called at some point in the future
-  // when the download is ready to complete.
-  //
-  // ShouldCompleteDownload() may be called multiple times; if it is, only the
-  // last callback specified (while the delegate is delaying completion) will be
-  // run.  Calls made after the callback is run are guaranteed to return true.
+  // will either return true (in which case the download may complete)
+  // or will call the callback passed when the download is ready for
+  // completion.  This routine should only be called once per download.
   virtual bool ShouldCompleteDownload(
       DownloadItem* item,
       const base::Closure& complete_callback);