DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

BUG=101214


Review URL: http://codereview.chromium.org/8697006

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

11 files changed, 263 insertions, 206 deletions

diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index 625d875..60c5f34 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -24,8 +24,8 @@
 #include "content/browser/download/download_state_info.h"
 #include "content/browser/download/interrupt_reasons.h"
 
-class DownloadFileManager;
 class DownloadId;
+class DownloadFileManager;
 class DownloadManager;
 class FilePath;
 class GURL;
@@ -37,6 +37,10 @@ class Time;
 class TimeDelta;
 }
 
+namespace content {
+class BrowserContext;
+}
+
 // One DownloadItem per download. This is the model class that stores all the
 // state for a download. Multiple views, such as a tab's download shelf and the
 // Destination tab's download view, may refer to a given DownloadItem.
@@ -153,6 +157,12 @@ class CONTENT_EXPORT DownloadItem {
   // Called when the downloaded file is removed.
   virtual void OnDownloadedFileRemoved() = 0;
 
+  // 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() = 0;
+
   // Download operation had an error.
   // |size| is the amount of data received at interruption.
   // |reason| is the download interrupt reason code that the operation received.
@@ -247,7 +257,6 @@ class CONTENT_EXPORT DownloadItem {
   virtual base::Time GetEndTime() const = 0;
   virtual void SetDbHandle(int64 handle) = 0;
   virtual int64 GetDbHandle() const = 0;
-  virtual DownloadManager* GetDownloadManager() = 0;
   virtual bool IsPaused() const = 0;
   virtual bool GetOpenWhenComplete() const = 0;
   virtual void SetOpenWhenComplete(bool open) = 0;
@@ -272,6 +281,7 @@ class CONTENT_EXPORT DownloadItem {
   virtual InterruptReason GetLastReason() const = 0;
   virtual DownloadPersistentStoreInfo GetPersistentStoreInfo() const = 0;
   virtual DownloadStateInfo GetStateInfo() const = 0;
+  virtual content::BrowserContext* BrowserContext() const = 0;
   virtual TabContents* GetTabContents() const = 0;
 
   // Returns the final target file path for the download.
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 02b5b64..718fe91 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -20,7 +20,6 @@
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_id.h"
-#include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_persistent_store_info.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_stats.h"
@@ -28,7 +27,6 @@
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
-#include "content/public/browser/download_manager_delegate.h"
 #include "net/base/net_util.h"
 
 using content::BrowserThread;
@@ -118,15 +116,34 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
 
 }  // namespace
 
+// Infrastructure in DownloadItemImpl::Delegate to assert invariant that
+// delegate always outlives all attached DownloadItemImpls.
+DownloadItemImpl::Delegate::Delegate()
+    : count_(0) {}
+
+DownloadItemImpl::Delegate::~Delegate() {
+  DCHECK_EQ(0, count_);
+}
+
+void DownloadItemImpl::Delegate::Attach() {
+  ++count_;
+}
+
+void DownloadItemImpl::Delegate::Detach() {
+  DCHECK_LT(0, count_);
+  --count_;
+}
+
 // Our download table ID starts at 1, so we use 0 to represent a download that
 // has started, but has not yet had its data persisted in the table. We use fake
 // database handles in incognito mode starting at -1 and progressively getting
 // more negative.
 
 // Constructor for reading from the history service.
-DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
+DownloadItemImpl::DownloadItemImpl(Delegate* delegate,
+                                   DownloadId download_id,
                                    const DownloadPersistentStoreInfo& info)
-    : download_id_(download_manager->GetNextId()),
+    : download_id_(download_id),
       full_path_(info.path),
       url_chain_(1, info.url),
       referrer_url_(info.referrer_url),
@@ -138,7 +155,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
       start_time_(info.start_time),
       end_time_(info.end_time),
       db_handle_(info.db_handle),
-      download_manager_(download_manager),
+      delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
@@ -150,6 +167,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
       opened_(info.opened),
       open_enabled_(true),
       delegate_delayed_complete_(false) {
+  delegate_->Attach();
   if (IsInProgress())
     state_ = CANCELLED;
   if (IsComplete())
@@ -159,7 +177,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
 
 // Constructing for a regular download:
 DownloadItemImpl::DownloadItemImpl(
-    DownloadManager* download_manager,
+    Delegate* delegate,
     const DownloadCreateInfo& info,
     DownloadRequestHandleInterface* request_handle,
     bool is_otr)
@@ -185,7 +203,7 @@ DownloadItemImpl::DownloadItemImpl(
       state_(IN_PROGRESS),
       start_time_(info.start_time),
       db_handle_(DownloadItem::kUninitializedHandle),
-      download_manager_(download_manager),
+      delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
@@ -197,11 +215,12 @@ DownloadItemImpl::DownloadItemImpl(
       opened_(false),
       open_enabled_(true),
       delegate_delayed_complete_(false) {
+  delegate_->Attach();
   Init(true /* actively downloading */);
 }
 
 // Constructing for the "Save Page As..." feature:
-DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
+DownloadItemImpl::DownloadItemImpl(Delegate* delegate,
                                    const FilePath& path,
                                    const GURL& url,
                                    bool is_otr,
@@ -219,7 +238,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
       state_(IN_PROGRESS),
       start_time_(base::Time::Now()),
       db_handle_(DownloadItem::kUninitializedHandle),
-      download_manager_(download_manager),
+      delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
@@ -231,6 +250,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
       opened_(false),
       open_enabled_(true),
       delegate_delayed_complete_(false) {
+  delegate_->Attach();
   Init(true /* actively downloading */);
 }
 
@@ -239,7 +259,8 @@ DownloadItemImpl::~DownloadItemImpl() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   TransitionTo(REMOVING);
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);
+  delegate_->Detach();
 }
 
 void DownloadItemImpl::AddObserver(Observer* observer) {
@@ -272,8 +293,7 @@ bool DownloadItemImpl::CanOpenDownload() {
 }
 
 bool DownloadItemImpl::ShouldOpenFileBasedOnExtension() {
-  return download_manager_->delegate()->ShouldOpenFileBasedOnExtension(
-      GetUserVerifiedFilePath());
+  return delegate_->ShouldOpenFileBasedOnExtension(GetUserVerifiedFilePath());
 }
 
 void DownloadItemImpl::OpenDownload() {
@@ -292,11 +312,11 @@ void DownloadItemImpl::OpenDownload() {
   // don't generally have the proper interface for that to the external
   // program that opens the file.  So instead we spawn a check to update
   // the UI if the file has been deleted in parallel with the open.
-  download_manager_->CheckForFileRemoval(this);
+  delegate_->CheckForFileRemoval(this);
   download_stats::RecordOpen(GetEndTime(), !GetOpened());
   opened_ = true;
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
-  download_manager_->MarkDownloadOpened(this);
+  delegate_->DownloadOpened(this);
 
   // For testing: If download opening is disabled on this item,
   // make the rest of the routine a no-op.
@@ -324,7 +344,7 @@ void DownloadItemImpl::DangerousDownloadValidated() {
   safety_state_ = DANGEROUS_BUT_VALIDATED;
   UpdateObservers();
 
-  download_manager_->MaybeCompleteDownload(this);
+  delegate_->MaybeCompleteDownload(this);
 }
 
 void DownloadItemImpl::UpdateSize(int64 bytes_so_far) {
@@ -375,7 +395,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 
   TransitionTo(CANCELLED);
   if (user_cancel)
-    download_manager_->DownloadCancelledInternal(this);
+    delegate_->DownloadCancelled(this);
 }
 
 void DownloadItemImpl::MarkAsComplete() {
@@ -408,6 +428,11 @@ void DownloadItemImpl::OnDownloadedFileRemoved() {
   UpdateObservers();
 }
 
+void DownloadItemImpl::MaybeCompleteDownload() {
+  // TODO(rdsmith): Move logic for this function here.
+  delegate_->MaybeCompleteDownload(this);
+}
+
 void DownloadItemImpl::Completed() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -417,7 +442,7 @@ void DownloadItemImpl::Completed() {
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
   TransitionTo(COMPLETE);
-  download_manager_->DownloadCompleted(GetId());
+  delegate_->DownloadCompleted(this);
   download_stats::RecordDownloadCompleted(start_tick_, received_bytes_);
 
   if (auto_opened_) {
@@ -503,12 +528,12 @@ void DownloadItemImpl::Remove() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);
   Cancel(true);
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);
 
   TransitionTo(REMOVING);
-  download_manager_->RemoveDownload(db_handle_);
+  delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }
 
@@ -582,16 +607,17 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
   if (NeedsRename()) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadFileManager::RenameCompletingDownloadFile,
-                   file_manager, GetGlobalId(),
+                   file_manager, download_id_,
                    GetTargetFilePath(), GetSafetyState() == SAFE));
     return;
   }
 
   Completed();
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DownloadFileManager::CompleteDownload,
-                                     file_manager, GetGlobalId()));
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CompleteDownload,
+                 file_manager, download_id_));
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) {
@@ -606,7 +632,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) {
 
   Rename(full_path);
 
-  if (download_manager_->delegate()->ShouldOpenDownload(this)) {
+  if (delegate_->ShouldOpenDownload(this)) {
     Completed();
   } else {
     delegate_delayed_complete_ = true;
@@ -629,11 +655,8 @@ bool DownloadItemImpl::MatchesQuery(const string16& query) const {
   //   L"/\x4f60\x597d\x4f60\x597d",
   //   "/%E4%BD%A0%E5%A5%BD%E4%BD%A0%E5%A5%BD"
   std::string languages;
-  TabContents* tab = GetTabContents();
-  if (tab) {
-    languages = content::GetContentClient()->browser()->GetAcceptLangs(
-        tab->browser_context());
-  }
+  languages = content::GetContentClient()->browser()->GetAcceptLangs(
+      BrowserContext());
   string16 url_formatted(net::FormatUrl(GetURL(), languages));
   if (base::i18n::StringSearchIgnoringCaseAndAccents(query, url_formatted))
     return true;
@@ -705,6 +728,10 @@ TabContents* DownloadItemImpl::GetTabContents() const {
   return NULL;
 }
 
+content::BrowserContext* DownloadItemImpl::BrowserContext() const {
+  return delegate_->BrowserContext();
+}
+
 FilePath DownloadItemImpl::GetTargetFilePath() const {
   return full_path_.DirName().Append(state_info_.target_name);
 }
@@ -727,9 +754,10 @@ void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   request_handle_->CancelRequest();
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DownloadFileManager::CancelDownload,
-                                     file_manager, GetGlobalId()));
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CancelDownload,
+                 file_manager, download_id_));
 }
 
 void DownloadItemImpl::Init(bool active) {
@@ -861,9 +889,6 @@ base::Time DownloadItemImpl::GetStartTime() const { return start_time_; }
 base::Time DownloadItemImpl::GetEndTime() const { return end_time_; }
 void DownloadItemImpl::SetDbHandle(int64 handle) { db_handle_ = handle; }
 int64 DownloadItemImpl::GetDbHandle() const { return db_handle_; }
-DownloadManager* DownloadItemImpl::GetDownloadManager() {
-  return download_manager_;
-}
 bool DownloadItemImpl::IsPaused() const { return is_paused_; }
 bool DownloadItemImpl::GetOpenWhenComplete() const {
   return open_when_complete_;
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 04326fc..79d2c44 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -24,29 +24,87 @@
 #include "net/base/net_errors.h"
 
 class DownloadFileManager;
-class DownloadId;
-class DownloadManager;
 class TabContents;
 
 struct DownloadCreateInfo;
 struct DownloadPersistentStoreInfo;
 
+namespace content {
+class BrowserContext;
+}
+
 // See download_item.h for usage.
 class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
  public:
+  // Delegate is defined in DownloadItemImpl (rather than DownloadItem)
+  // as it's relevant to the class implementation (class methods need to
+  // call into it) and doesn't have anything to do with its interface.
+  // Despite this, the delegate methods take DownloadItems as arguments
+  // (rather than DownloadItemImpls) so that classes that inherit from it
+  // can be used with DownloadItem mocks rather than being tied to
+  // DownloadItemImpls.
+  class Delegate {
+   public:
+    Delegate();
+    virtual ~Delegate();
+
+    // Used for catching use-after-free errors.
+    void Attach();
+    void Detach();
+
+    // Tests if a file type should be opened automatically.
+    virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) = 0;
+
+    // Allows the delegate to override the opening of a download. If it returns
+    // true then it's reponsible for opening the item.
+    virtual bool ShouldOpenDownload(DownloadItem* download) = 0;
+
+    // Checks whether a downloaded file still exists and updates the
+    // file's state if the file is already removed.
+    // The check may or may not result in a later asynchronous call
+    // to OnDownloadedFileRemoved().
+    virtual void CheckForFileRemoval(DownloadItem* download_item) = 0;
+
+    // If all pre-requisites have been met, complete download processing.
+    // TODO(rdsmith): Move into DownloadItem.
+    virtual void MaybeCompleteDownload(DownloadItem* download) = 0;
+
+    // For contextual issues like language and prefs.
+    virtual content::BrowserContext* BrowserContext() const = 0;
+
+    // Handle any delegate portions of a state change operation on the
+    // DownloadItem.
+    virtual void DownloadCancelled(DownloadItem* download) = 0;
+    virtual void DownloadCompleted(DownloadItem* download) = 0;
+    virtual void DownloadOpened(DownloadItem* download) = 0;
+    virtual void DownloadRemoved(DownloadItem* download) = 0;
+
+    // Assert consistent state for delgate object at various transitions.
+    virtual void AssertStateConsistent(DownloadItem* download) const = 0;
+
+   private:
+    // For "Outlives attached DownloadItemImpl" invariant assertion.
+    int count_;
+  };
+
+  // Note that it is the responsibility of the caller to ensure that a
+  // DownloadItemImpl::Delegate passed to a DownloadItemImpl constructor
+  // outlives the DownloadItemImpl.
+
   // Constructing from persistent store:
-  DownloadItemImpl(DownloadManager* download_manager,
+  DownloadItemImpl(Delegate* delegate,
+                   DownloadId download_id,
                    const DownloadPersistentStoreInfo& info);
 
   // Constructing for a regular download.
   // Takes ownership of the object pointed to by |request_handle|.
-  DownloadItemImpl(DownloadManager* download_manager,
+  DownloadItemImpl(Delegate* delegate,
                    const DownloadCreateInfo& info,
                    DownloadRequestHandleInterface* request_handle,
                    bool is_otr);
 
   // Constructing for the "Save Page As..." feature:
-  DownloadItemImpl(DownloadManager* download_manager,
+  DownloadItemImpl(Delegate* delegate,
                    const FilePath& path,
                    const GURL& url,
                    bool is_otr,
@@ -71,6 +129,7 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   virtual void OnAllDataSaved(
       int64 size, const std::string& final_hash) OVERRIDE;
   virtual void OnDownloadedFileRemoved() OVERRIDE;
+  virtual void MaybeCompleteDownload() OVERRIDE;
   virtual void Interrupted(int64 size, InterruptReason reason) OVERRIDE;
   virtual void Delete(DeleteReason reason) OVERRIDE;
   virtual void Remove() OVERRIDE;
@@ -112,7 +171,6 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   virtual base::Time GetEndTime() const OVERRIDE;
   virtual void SetDbHandle(int64 handle) OVERRIDE;
   virtual int64 GetDbHandle() const OVERRIDE;
-  virtual DownloadManager* GetDownloadManager() OVERRIDE;
   virtual bool IsPaused() const OVERRIDE;
   virtual bool GetOpenWhenComplete() const OVERRIDE;
   virtual void SetOpenWhenComplete(bool open) OVERRIDE;
@@ -134,6 +192,7 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   virtual InterruptReason GetLastReason() const OVERRIDE;
   virtual DownloadPersistentStoreInfo GetPersistentStoreInfo() const OVERRIDE;
   virtual DownloadStateInfo GetStateInfo() const OVERRIDE;
+  virtual content::BrowserContext* BrowserContext() const OVERRIDE;
   virtual TabContents* GetTabContents() const OVERRIDE;
   virtual FilePath GetTargetFilePath() const OVERRIDE;
   virtual FilePath GetFileNameToReportUser() const OVERRIDE;
@@ -245,8 +304,8 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   // Our persistent store handle
   int64 db_handle_;
 
-  // Our owning object
-  DownloadManager* download_manager_;
+  // Our delegate.
+  Delegate* delegate_;
 
   // In progress downloads may be paused by the user, we note it here
   bool is_paused_;
diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h
index 83540b5..257f3b8 100644
--- a/content/browser/download/download_manager.h
+++ b/content/browser/download/download_manager.h
@@ -124,25 +124,7 @@ class CONTENT_EXPORT DownloadManager
   // |size| is the number of bytes that are currently downloaded.
   // |reason| is a download interrupt reason code.
   virtual void OnDownloadInterrupted(int32 download_id, int64 size,
-                             InterruptReason reason) = 0;
-
-  // Called from DownloadItem to handle the DownloadManager portion of a
-  // Cancel; should not be called from other locations.
-  virtual void DownloadCancelledInternal(DownloadItem* download) = 0;
-
-  // Called from a view when a user clicks a UI button or link.
-  virtual void RemoveDownload(int64 download_handle) = 0;
-
-  // Determine if the download is ready for completion, i.e. has had
-  // all data saved, and completed the filename determination and
-  // history insertion.
-  virtual bool IsDownloadReadyForCompletion(DownloadItem* download) = 0;
-
-  // 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(DownloadItem* download) = 0;
+                                     InterruptReason reason) = 0;
 
   // Called when the download is renamed to its final name.
   // |uniquifier| is a number used to make unique names for the file.  It is
@@ -166,10 +148,6 @@ class CONTENT_EXPORT DownloadManager
   // deleted is returned back to the caller.
   virtual int RemoveAllDownloads() = 0;
 
-  // Final download manager transition for download: Update the download
-  // history and remove the download from |active_downloads_|.
-  virtual void DownloadCompleted(int32 download_id) = 0;
-
   // Download the object at the URL. Used in cases such as "Save Link As..."
   virtual void DownloadUrl(const GURL& url,
                    const GURL& referrer,
@@ -201,19 +179,26 @@ class CONTENT_EXPORT DownloadManager
   virtual void OnItemAddedToPersistentStore(int32 download_id,
                                             int64 db_handle) = 0;
 
-  // Display a new download in the appropriate browser UI.
-  virtual void ShowDownloadInBrowser(DownloadItem* download) = 0;
-
   // The number of in progress (including paused) downloads.
   virtual int InProgressCount() const = 0;
 
-  virtual content::BrowserContext* BrowserContext() = 0;
+  virtual content::BrowserContext* BrowserContext() const = 0;
 
   virtual FilePath LastDownloadPath() = 0;
 
   // Creates the download item.  Must be called on the UI thread.
-  virtual void CreateDownloadItem(DownloadCreateInfo* info,
-                          const DownloadRequestHandle& request_handle) = 0;
+  virtual void CreateDownloadItem(
+      DownloadCreateInfo* info,
+      const DownloadRequestHandle& request_handle) = 0;
+
+  // Creates a download item for the SavePackage system.
+  // Must be called on the UI thread.  Note that the DownloadManager
+  // retains ownership.
+  virtual DownloadItem* CreateSavePackageDownloadItem(
+      const FilePath& main_file_path,
+      const GURL& page_url,
+      bool is_otr,
+      DownloadItem::Observer* observer) = 0;
 
   // Clears the last download path, used to initialize "save as" dialogs.
   virtual void ClearLastDownloadPath() = 0;
@@ -226,31 +211,15 @@ class CONTENT_EXPORT DownloadManager
   // DownloadManagerDelegate::ShouldStartDownload and now is ready.
   virtual void RestartDownload(int32 download_id) = 0;
 
-  // Mark the download opened in the persistent store.
-  virtual void MarkDownloadOpened(DownloadItem* download) = 0;
-
   // Checks whether downloaded files still exist. Updates state of downloads
   // that refer to removed files. The check runs in the background and may
   // finish asynchronously after this method returns.
   virtual void CheckForHistoryFilesRemoval() = 0;
 
-  // Checks whether a downloaded file still exists and updates the file's state
-  // if the file is already removed. The check runs in the background and may
-  // finish asynchronously after this method returns.
-  virtual void CheckForFileRemoval(DownloadItem* download_item) = 0;
-
-  // Assert the named download item is on the correct queues
-  // in the DownloadManager.  For debugging.
-  virtual void AssertQueueStateConsistent(DownloadItem* download) = 0;
-
   // Get the download item from the history map.  Useful after the item has
   // been removed from the active map, or was retrieved from the history DB.
   virtual DownloadItem* GetDownloadItem(int id) = 0;
 
-  // Called when Save Page download starts. Transfers ownership of |download|
-  // to the DownloadManager.
-  virtual void SavePageDownloadStarted(DownloadItem* download) = 0;
-
   // Called when Save Page download is done.
   virtual void SavePageDownloadFinished(DownloadItem* download) = 0;
 
@@ -265,8 +234,6 @@ class CONTENT_EXPORT DownloadManager
   virtual void SetDownloadManagerDelegate(
       content::DownloadManagerDelegate* delegate) = 0;
 
-  virtual DownloadId GetNextId() = 0;
-
  protected:
   // These functions are here for unit tests.
 
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 77c30c0..1168a29 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -101,6 +101,14 @@ DownloadId DownloadManagerImpl::GetNextId() {
   return id_factory_->GetNextId();
 }
 
+bool DownloadManagerImpl::ShouldOpenDownload(DownloadItem* item) {
+  return delegate_->ShouldOpenDownload(item);
+}
+
+bool DownloadManagerImpl::ShouldOpenFileBasedOnExtension(const FilePath& path) {
+  return delegate_->ShouldOpenFileBasedOnExtension(path);
+}
+
 void DownloadManagerImpl::Shutdown() {
   VLOG(20) << __FUNCTION__ << "()"
            << " shutdown_needed_ = " << shutdown_needed_;
@@ -134,7 +142,7 @@ void DownloadManagerImpl::Shutdown() {
       // The user hasn't accepted it, so we need to remove it
       // from the disk.  This may or may not result in it being
       // removed from the DownloadManager queues and deleted
-      // (specifically, DownloadManager::RemoveDownload only
+      // (specifically, DownloadManager::DownloadRemoved only
       // removes and deletes it if it's known to the history service)
       // so the only thing we know after calling this function is that
       // the download was deleted if-and-only-if it was removed
@@ -317,7 +325,7 @@ void DownloadManagerImpl::RestartDownload(
   }
 }
 
-content::BrowserContext* DownloadManagerImpl::BrowserContext() {
+content::BrowserContext* DownloadManagerImpl::BrowserContext() const {
   return browser_context_;
 }
 
@@ -341,6 +349,26 @@ void DownloadManagerImpl::CreateDownloadItem(
   active_downloads_[download_id] = download;
 }
 
+DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
+    const FilePath& main_file_path,
+    const GURL& page_url,
+    bool is_otr,
+    DownloadItem::Observer* observer) {
+  DownloadItem* download = new DownloadItemImpl(
+      this, main_file_path, page_url, is_otr, GetNextId());
+
+  download->AddObserver(observer);
+
+  DCHECK(!ContainsKey(save_page_downloads_, download->GetId()));
+  downloads_.insert(download);
+  save_page_downloads_[download->GetId()] = download;
+
+  // Will notify the observer in the callback.
+  delegate_->AddItemToPersistentStore(download);
+
+  return download;
+}
+
 void DownloadManagerImpl::ContinueDownloadWithPath(
     DownloadItem* download, const FilePath& chosen_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -371,7 +399,8 @@ void DownloadManagerImpl::ContinueDownloadWithPath(
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFileManager::RenameInProgressDownloadFile,
-                 file_manager_, download->GetGlobalId(), download_path));
+                 file_manager_, download->GetGlobalId(),
+                 download_path));
 
   download->Rename(download_path);
 
@@ -409,10 +438,10 @@ void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
   download->OnAllDataSaved(size, hash);
   delegate_->OnResponseCompleted(download);
 
-  MaybeCompleteDownload(download);
+  download->MaybeCompleteDownload();
 }
 
-void DownloadManagerImpl::AssertQueueStateConsistent(DownloadItem* download) {
+void DownloadManagerImpl::AssertStateConsistent(DownloadItem* download) const {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   if (download->GetState() == DownloadItem::REMOVING) {
     CHECK(!ContainsKey(downloads_, download));
@@ -431,7 +460,7 @@ void DownloadManagerImpl::AssertQueueStateConsistent(DownloadItem* download) {
   } else {
     // TODO(rdsmith): Somewhat painful; make sure to disable in
     // release builds after resolution of http://crbug.com/85408.
-    for (DownloadMap::iterator it = history_downloads_.begin();
+    for (DownloadMap::const_iterator it = history_downloads_.begin();
          it != history_downloads_.end(); ++it) {
       CHECK(it->second != download);
     }
@@ -511,13 +540,12 @@ void DownloadManagerImpl::MaybeCompleteDownload(DownloadItem* download) {
   download->OnDownloadCompleting(file_manager_);
 }
 
-void DownloadManagerImpl::DownloadCompleted(int32 download_id) {
+void DownloadManagerImpl::DownloadCompleted(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadItem* download = GetDownloadItem(download_id);
   DCHECK(download);
   delegate_->UpdateItemInPersistentStore(download);
-  active_downloads_.erase(download_id);
-  AssertQueueStateConsistent(download);
+  active_downloads_.erase(download->GetId());
+  AssertStateConsistent(download);
 }
 
 void DownloadManagerImpl::OnDownloadRenamedToFinalName(
@@ -559,7 +587,7 @@ void DownloadManagerImpl::CancelDownload(int32 download_id) {
   download->Cancel(true);
 }
 
-void DownloadManagerImpl::DownloadCancelledInternal(DownloadItem* download) {
+void DownloadManagerImpl::DownloadCancelled(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
@@ -568,7 +596,7 @@ void DownloadManagerImpl::DownloadCancelledInternal(DownloadItem* download) {
   RemoveFromActiveList(download);
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
-  AssertQueueStateConsistent(download);
+  AssertStateConsistent(download);
 
   if (file_manager_)
     download->OffThreadCancel(file_manager_);
@@ -660,13 +688,12 @@ int DownloadManagerImpl::RemoveDownloadItems(
   return num_deleted;
 }
 
-void DownloadManagerImpl::RemoveDownload(int64 download_handle) {
-  DownloadMap::iterator it = history_downloads_.find(download_handle);
-  if (it == history_downloads_.end())
+void DownloadManagerImpl::DownloadRemoved(DownloadItem* download) {
+  if (history_downloads_.find(download->GetDbHandle()) ==
+      history_downloads_.end())
     return;
 
   // Make history update.
-  DownloadItem* download = it->second;
   delegate_->RemoveItemFromPersistentStore(download);
 
   // Remove from our tables and delete.
@@ -688,7 +715,7 @@ int DownloadManagerImpl::RemoveDownloadsBetween(const base::Time remove_begin,
     if (download->GetStartTime() >= remove_begin &&
         (remove_end.is_null() || download->GetStartTime() < remove_end) &&
         (download->IsComplete() || download->IsCancelled())) {
-      AssertQueueStateConsistent(download);
+      AssertStateConsistent(download);
       pending_deletes.push_back(download);
     }
   }
@@ -835,7 +862,8 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
   largest_db_handle_in_history_ = 0;
 
   for (size_t i = 0; i < entries->size(); ++i) {
-    DownloadItem* download = new DownloadItemImpl(this, entries->at(i));
+    DownloadItem* download = new DownloadItemImpl(
+        this, GetNextId(), entries->at(i));
     // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
     CHECK(!ContainsKey(history_downloads_, download->GetDbHandle()));
     downloads_.insert(download);
@@ -1025,16 +1053,6 @@ void DownloadManagerImpl::AssertContainersConsistent() const {
 #endif
 }
 
-void DownloadManagerImpl::SavePageDownloadStarted(DownloadItem* download) {
-  DCHECK(!ContainsKey(save_page_downloads_, download->GetId()));
-  downloads_.insert(download);
-  save_page_downloads_[download->GetId()] = download;
-
-  // Add this entry to the history service.
-  // Additionally, the UI is notified in the callback.
-  delegate_->AddItemToPersistentStore(download);
-}
-
 // SavePackage will call SavePageDownloadFinished upon completion/cancellation.
 // The history callback will call OnSavePageItemAddedToPersistentStore.
 // If the download finishes before the history callback,
@@ -1091,7 +1109,7 @@ void DownloadManagerImpl::SavePageDownloadFinished(DownloadItem* download) {
   }
 }
 
-void DownloadManagerImpl::MarkDownloadOpened(DownloadItem* download) {
+void DownloadManagerImpl::DownloadOpened(DownloadItem* download) {
   delegate_->UpdateItemInPersistentStore(download);
   int num_unopened = 0;
   for (DownloadMap::iterator it = history_downloads_.begin();
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index d3c5d34..3953499 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -18,6 +18,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/synchronization/lock.h"
+#include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_status_updater_delegate.h"
 #include "content/common/content_export.h"
 
@@ -26,11 +27,12 @@ class DownloadStatusUpdater;
 
 class CONTENT_EXPORT DownloadManagerImpl
     : public DownloadManager,
+      public DownloadItemImpl::Delegate,
       public DownloadStatusUpdaterDelegate {
  public:
   DownloadManagerImpl(content::DownloadManagerDelegate* delegate,
-                  DownloadIdFactory* id_factory,
-                  DownloadStatusUpdater* status_updater);
+                      DownloadIdFactory* id_factory,
+                      DownloadStatusUpdater* status_updater);
 
   // DownloadManager functions.
   virtual void Shutdown() OVERRIDE;
@@ -49,10 +51,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual void CancelDownload(int32 download_id) OVERRIDE;
   virtual void OnDownloadInterrupted(int32 download_id, int64 size,
                                      InterruptReason reason) OVERRIDE;
-  virtual void DownloadCancelledInternal(DownloadItem* download) OVERRIDE;
-  virtual void RemoveDownload(int64 download_handle) OVERRIDE;
-  virtual bool IsDownloadReadyForCompletion(DownloadItem* download) OVERRIDE;
-  virtual void MaybeCompleteDownload(DownloadItem* download) OVERRIDE;
   virtual void OnDownloadRenamedToFinalName(int download_id,
                                             const FilePath& full_path,
                                             int uniquifier) OVERRIDE;
@@ -60,7 +58,6 @@ class CONTENT_EXPORT DownloadManagerImpl
                                      const base::Time remove_end) OVERRIDE;
   virtual int RemoveDownloads(const base::Time remove_begin) OVERRIDE;
   virtual int RemoveAllDownloads() OVERRIDE;
-  virtual void DownloadCompleted(int32 download_id) OVERRIDE;
   virtual void DownloadUrl(const GURL& url,
                            const GURL& referrer,
                            const std::string& referrer_encoding,
@@ -76,29 +73,41 @@ class CONTENT_EXPORT DownloadManagerImpl
       std::vector<DownloadPersistentStoreInfo>* entries) OVERRIDE;
   virtual void OnItemAddedToPersistentStore(int32 download_id,
                                             int64 db_handle) OVERRIDE;
-  virtual void ShowDownloadInBrowser(DownloadItem* download) OVERRIDE;
   virtual int InProgressCount() const OVERRIDE;
-  virtual content::BrowserContext* BrowserContext() OVERRIDE;
+  virtual content::BrowserContext* BrowserContext() const OVERRIDE;
   virtual FilePath LastDownloadPath() OVERRIDE;
   virtual void CreateDownloadItem(
       DownloadCreateInfo* info,
       const DownloadRequestHandle& request_handle) OVERRIDE;
+  virtual DownloadItem* CreateSavePackageDownloadItem(
+      const FilePath& main_file_path,
+      const GURL& page_url,
+      bool is_otr,
+      DownloadItem::Observer* observer) OVERRIDE;
   virtual void ClearLastDownloadPath() OVERRIDE;
   virtual void FileSelected(const FilePath& path, void* params) OVERRIDE;
   virtual void FileSelectionCanceled(void* params) OVERRIDE;
   virtual void RestartDownload(int32 download_id) OVERRIDE;
-  virtual void MarkDownloadOpened(DownloadItem* download) OVERRIDE;
   virtual void CheckForHistoryFilesRemoval() OVERRIDE;
-  virtual void CheckForFileRemoval(DownloadItem* download_item) OVERRIDE;
-  virtual void AssertQueueStateConsistent(DownloadItem* download) OVERRIDE;
   virtual DownloadItem* GetDownloadItem(int id) OVERRIDE;
-  virtual void SavePageDownloadStarted(DownloadItem* download) OVERRIDE;
   virtual void SavePageDownloadFinished(DownloadItem* download) OVERRIDE;
   virtual DownloadItem* GetActiveDownloadItem(int id) OVERRIDE;
   virtual content::DownloadManagerDelegate* delegate() const OVERRIDE;
   virtual void SetDownloadManagerDelegate(
       content::DownloadManagerDelegate* delegate) OVERRIDE;
-  virtual DownloadId GetNextId() OVERRIDE;
+
+  // Overridden from DownloadItemImpl::Delegate
+  // (Note that |BrowserContext| are present in both interfaces.)
+  virtual bool ShouldOpenDownload(DownloadItem* item) OVERRIDE;
+  virtual bool ShouldOpenFileBasedOnExtension(
+      const FilePath& path) OVERRIDE;
+  virtual void CheckForFileRemoval(DownloadItem* download_item) OVERRIDE;
+  virtual void MaybeCompleteDownload(DownloadItem* download) OVERRIDE;
+  virtual void DownloadCancelled(DownloadItem* download) OVERRIDE;
+  virtual void DownloadCompleted(DownloadItem* download) OVERRIDE;
+  virtual void DownloadOpened(DownloadItem* download) OVERRIDE;
+  virtual void DownloadRemoved(DownloadItem* download) OVERRIDE;
+  virtual void AssertStateConsistent(DownloadItem* download) const OVERRIDE;
 
   // Overridden from DownloadStatusUpdaterDelegate:
   virtual bool IsDownloadProgressKnown() const OVERRIDE;
@@ -113,7 +122,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   // For testing.
   friend class DownloadManagerTest;
   friend class DownloadTest;
-  friend class MockDownloadManager;
 
   friend class base::RefCountedThreadSafe<
       DownloadManagerImpl, content::BrowserThread::DeleteOnUIThread>;
@@ -123,6 +131,17 @@ class CONTENT_EXPORT DownloadManagerImpl
 
   virtual ~DownloadManagerImpl();
 
+  // 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(DownloadItem* download);
+
+  // Show the download in the browser.
+  void ShowDownloadInBrowser(DownloadItem* download);
+
+  // Get next download id from factory.
+  DownloadId GetNextId();
+
   // Called on the FILE thread to check the existence of a downloaded file.
   void CheckForFileRemovalOnFileThread(int64 db_handle, const FilePath& path);
 
diff --git a/content/browser/download/mock_download_item.h b/content/browser/download/mock_download_item.h
index 6b222c5..82895bf 100644
--- a/content/browser/download/mock_download_item.h
+++ b/content/browser/download/mock_download_item.h
@@ -33,6 +33,7 @@ class MockDownloadItem : public DownloadItem {
   MOCK_METHOD0(DelayedDownloadOpened, void());
   MOCK_METHOD2(OnAllDataSaved, void(int64, const std::string&));
   MOCK_METHOD0(OnDownloadedFileRemoved, void());
+  MOCK_METHOD0(MaybeCompleteDownload, void());
   MOCK_METHOD2(Interrupted, void(int64, InterruptReason));
   MOCK_METHOD1(Delete, void(DeleteReason));
   MOCK_METHOD0(Remove, void());
@@ -96,6 +97,7 @@ class MockDownloadItem : public DownloadItem {
   MOCK_CONST_METHOD0(GetLastReason, InterruptReason());
   MOCK_CONST_METHOD0(GetPersistentStoreInfo, DownloadPersistentStoreInfo());
   MOCK_CONST_METHOD0(GetStateInfo, DownloadStateInfo());
+  MOCK_CONST_METHOD0(BrowserContext, content::BrowserContext*());
   MOCK_CONST_METHOD0(GetTabContents, TabContents*());
   MOCK_CONST_METHOD0(GetTargetFilePath, FilePath());
   MOCK_CONST_METHOD0(GetFileNameToReportUser, FilePath());
diff --git a/content/browser/download/mock_download_manager.cc b/content/browser/download/mock_download_manager.cc
index 263ce7c..8152617 100644
--- a/content/browser/download/mock_download_manager.cc
+++ b/content/browser/download/mock_download_manager.cc
@@ -65,24 +65,6 @@ void MockDownloadManager::OnDownloadInterrupted(int32 download_id, int64 size,
                                                 InterruptReason reason) {
 }
 
-void MockDownloadManager::DownloadCancelledInternal(DownloadItem* download) {
-  download->Cancel(true);
-  item_map_.erase(download->GetId());
-  inactive_item_map_[download->GetId()] = download;
-}
-
-void MockDownloadManager::RemoveDownload(int64 download_handle) {
-}
-
-bool MockDownloadManager::IsDownloadReadyForCompletion(DownloadItem* download) {
-  return download->AllDataSaved();
-}
-
-void MockDownloadManager::MaybeCompleteDownload(DownloadItem* download) {
-  if (IsDownloadReadyForCompletion(download))
-    download->OnDownloadRenamedToFinalName(download->GetFullPath());
-}
-
 void MockDownloadManager::OnDownloadRenamedToFinalName(
     int download_id,
     const FilePath& full_path,
@@ -102,9 +84,6 @@ int MockDownloadManager::RemoveAllDownloads() {
   return 1;
 }
 
-void MockDownloadManager::DownloadCompleted(int32 download_id) {
-}
-
 void MockDownloadManager::DownloadUrl(const GURL& url,
                                       const GURL& referrer,
                                       const std::string& referrer_encoding,
@@ -133,14 +112,11 @@ void MockDownloadManager::OnItemAddedToPersistentStore(int32 download_id,
                                                        int64 db_handle) {
 }
 
-void MockDownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
-}
-
 int MockDownloadManager::InProgressCount() const {
   return 1;
 }
 
-content::BrowserContext* MockDownloadManager::BrowserContext() {
+content::BrowserContext* MockDownloadManager::BrowserContext() const {
   return NULL;
 }
 
@@ -151,9 +127,17 @@ FilePath MockDownloadManager::LastDownloadPath() {
 void MockDownloadManager::CreateDownloadItem(
     DownloadCreateInfo* info,
     const DownloadRequestHandle& request_handle) {
-  item_map_.insert(std::make_pair(
-      info->download_id.local(), new DownloadItemImpl(
-        this, *info, new DownloadRequestHandle(request_handle), false)));
+  NOTREACHED();                         // Not yet implemented.
+  return;
+}
+
+DownloadItem* MockDownloadManager::CreateSavePackageDownloadItem(
+      const FilePath& main_file_path,
+      const GURL& page_url,
+      bool is_otr,
+      DownloadItem::Observer* observer) {
+  NOTREACHED();                         // Not yet implemented.
+  return NULL;
 }
 
 void MockDownloadManager::ClearLastDownloadPath() {
@@ -168,19 +152,9 @@ void MockDownloadManager::FileSelectionCanceled(void* params) {
 void MockDownloadManager::RestartDownload(int32 download_id) {
 }
 
-void MockDownloadManager::MarkDownloadOpened(DownloadItem* download) {
-  download->SetOpenWhenComplete(true);
-}
-
 void MockDownloadManager::CheckForHistoryFilesRemoval() {
 }
 
-void MockDownloadManager::CheckForFileRemoval(DownloadItem* download_item) {
-}
-
-void MockDownloadManager::AssertQueueStateConsistent(DownloadItem* download) {
-}
-
 DownloadItem* MockDownloadManager::GetDownloadItem(int id) {
   std::map<int32, DownloadItem*>::iterator it = item_map_.find(id);
   if (it == item_map_.end())
@@ -188,9 +162,6 @@ DownloadItem* MockDownloadManager::GetDownloadItem(int id) {
   return it->second;
 }
 
-void MockDownloadManager::SavePageDownloadStarted(DownloadItem* download) {
-}
-
 void MockDownloadManager::SavePageDownloadFinished(DownloadItem* download) {
 }
 
@@ -206,10 +177,6 @@ void MockDownloadManager::SetDownloadManagerDelegate(
     content::DownloadManagerDelegate* delegate) {
 }
 
-DownloadId MockDownloadManager::GetNextId() {
-  return DownloadId(this, 1);
-}
-
 void MockDownloadManager::ContinueDownloadWithPath(
     DownloadItem* download,
     const FilePath& chosen_file) {
diff --git a/content/browser/download/mock_download_manager.h b/content/browser/download/mock_download_manager.h
index 956328d..4608d71 100644
--- a/content/browser/download/mock_download_manager.h
+++ b/content/browser/download/mock_download_manager.h
@@ -6,6 +6,7 @@
 #define CONTENT_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_
 #pragma once
 
+#include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_id.h"
 #include "content/browser/download/download_id_factory.h"
@@ -13,7 +14,8 @@
 class DownloadStatusUpdater;
 class DownloadItem;
 
-class MockDownloadManager : public DownloadManager {
+class MockDownloadManager
+    : public DownloadManager {
  public:
   explicit MockDownloadManager(content::DownloadManagerDelegate* delegate,
                                DownloadIdFactory* id_factory,
@@ -37,10 +39,6 @@ class MockDownloadManager : public DownloadManager {
   virtual void CancelDownload(int32 download_id) OVERRIDE;
   virtual void OnDownloadInterrupted(int32 download_id, int64 size,
                                      InterruptReason reason) OVERRIDE;
-  virtual void DownloadCancelledInternal(DownloadItem* download) OVERRIDE;
-  virtual void RemoveDownload(int64 download_handle) OVERRIDE;
-  virtual bool IsDownloadReadyForCompletion(DownloadItem* download) OVERRIDE;
-  virtual void MaybeCompleteDownload(DownloadItem* download) OVERRIDE;
   virtual void OnDownloadRenamedToFinalName(int download_id,
                                             const FilePath& full_path,
                                             int uniquifier) OVERRIDE;
@@ -48,7 +46,6 @@ class MockDownloadManager : public DownloadManager {
                                      const base::Time remove_end) OVERRIDE;
   virtual int RemoveDownloads(const base::Time remove_begin) OVERRIDE;
   virtual int RemoveAllDownloads() OVERRIDE;
-  virtual void DownloadCompleted(int32 download_id) OVERRIDE;
   virtual void DownloadUrl(const GURL& url,
                            const GURL& referrer,
                            const std::string& referrer_encoding,
@@ -64,29 +61,28 @@ class MockDownloadManager : public DownloadManager {
       std::vector<DownloadPersistentStoreInfo>* entries) OVERRIDE;
   virtual void OnItemAddedToPersistentStore(int32 download_id,
                                             int64 db_handle) OVERRIDE;
-  virtual void ShowDownloadInBrowser(DownloadItem* download) OVERRIDE;
   virtual int InProgressCount() const OVERRIDE;
-  virtual content::BrowserContext* BrowserContext() OVERRIDE;
+  virtual content::BrowserContext* BrowserContext() const OVERRIDE;
   virtual FilePath LastDownloadPath() OVERRIDE;
   virtual void CreateDownloadItem(
       DownloadCreateInfo* info,
       const DownloadRequestHandle& request_handle) OVERRIDE;
+  virtual DownloadItem* CreateSavePackageDownloadItem(
+      const FilePath& main_file_path,
+      const GURL& page_url,
+      bool is_otr,
+      DownloadItem::Observer* observer) OVERRIDE;
   virtual void ClearLastDownloadPath() OVERRIDE;
   virtual void FileSelected(const FilePath& path, void* params) OVERRIDE;
   virtual void FileSelectionCanceled(void* params) OVERRIDE;
   virtual void RestartDownload(int32 download_id) OVERRIDE;
-  virtual void MarkDownloadOpened(DownloadItem* download) OVERRIDE;
   virtual void CheckForHistoryFilesRemoval() OVERRIDE;
-  virtual void CheckForFileRemoval(DownloadItem* download_item) OVERRIDE;
-  virtual void AssertQueueStateConsistent(DownloadItem* download) OVERRIDE;
   virtual DownloadItem* GetDownloadItem(int id) OVERRIDE;
-  virtual void SavePageDownloadStarted(DownloadItem* download) OVERRIDE;
   virtual void SavePageDownloadFinished(DownloadItem* download) OVERRIDE;
   virtual DownloadItem* GetActiveDownloadItem(int id) OVERRIDE;
   virtual content::DownloadManagerDelegate* delegate() const OVERRIDE;
   virtual void SetDownloadManagerDelegate(
       content::DownloadManagerDelegate* delegate) OVERRIDE;
-  virtual DownloadId GetNextId() OVERRIDE;
   virtual void ContinueDownloadWithPath(DownloadItem* download,
                                         const FilePath& chosen_file) OVERRIDE;
   virtual DownloadItem* GetActiveDownload(int32 download_id) OVERRIDE;
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index a87fc60..bc8bbb5 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -268,16 +268,10 @@ bool SavePackage::Init() {
     return false;
   }
 
-  // Create the download item, and add ourself as an observer.
-  download_ = new DownloadItemImpl(download_manager_,
-                                   saved_main_file_path_,
-                                   page_url_,
-                                   browser_context->IsOffTheRecord(),
-                                   download_manager_->GetNextId());
-  download_->AddObserver(this);
-
-  // Transfer ownership to the download manager.
-  download_manager_->SavePageDownloadStarted(download_);
+  // The download manager keeps ownership but adds us as an observer.
+  download_ = download_manager_->CreateSavePackageDownloadItem(
+      saved_main_file_path_, page_url_,
+      browser_context->IsOffTheRecord(), this);
 
   // Check save type and process the save page job.
   if (save_type_ == SAVE_AS_COMPLETE_HTML) {
diff --git a/content/public/browser/download_manager_delegate.h b/content/public/browser/download_manager_delegate.h
index 35c60cc..f902882 100644
--- a/content/public/browser/download_manager_delegate.h
+++ b/content/public/browser/download_manager_delegate.h
@@ -54,7 +54,7 @@ class DownloadManagerDelegate {
   // Allows the delegate to override completion of the download.  If this
   // function returns false, the download completion is delayed and the
   // delegate is responsible for making sure that
-  // DownloadManager::MaybeCompleteDownload is called at some point in the
+  // DownloadItem::MaybeCompleteDownload is called at some point in the
   // future.  Note that at that point this function will be called again,
   // and is responsible for returning true when it really is ok for the
   // download to complete.