Replace the DownloadFileManager with direct ownership of DownloadFileImpl

content/browser/download/download_manager_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/debug/alias.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/supports_user_data.h"
 #include "base/synchronization/lock.h"
 #include "base/sys_string_conversions.h"
 #include "build/build_config.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_factory.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_manager_delegate.h"
@@ skipping 11 matching lines @@
 
 using content::BrowserThread;
 using content::DownloadId;
 using content::DownloadItem;
 using content::DownloadPersistentStoreInfo;
 using content::ResourceDispatcherHostImpl;
 using content::WebContents;
 
 namespace {
 
-// This is just used to remember which DownloadItems come from SavePage.
-class SavePageData : public base::SupportsUserData::Data {
- public:
-  // A spoonful of syntactic sugar.
-  static bool Get(DownloadItem* item) {
-    return item->GetUserData(kKey) != NULL;
-  }
-
-  explicit SavePageData(DownloadItem* item) {
-    item->SetUserData(kKey, this);
-  }
-
-  virtual ~SavePageData() {}
-
- private:
-  static const char kKey[];
-
-  DISALLOW_COPY_AND_ASSIGN(SavePageData);
-};
-
-const char SavePageData::kKey[] = "DownloadItem SavePageData";
-
 void BeginDownload(content::DownloadUrlParameters* params) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // ResourceDispatcherHost{Base} is-not-a URLRequest::Delegate, and
   // DownloadUrlParameters can-not include resource_dispatcher_host_impl.h, so
   // we must down cast. RDHI is the only subclass of RDH as of 2012 May 4.
   scoped_ptr<net::URLRequest> request(
       params->resource_context()->GetRequestContext()->CreateRequest(
           params->url(), NULL));
   request->set_referrer(params->referrer().url.spec());
   webkit_glue::ConfigureURLRequestForReferrerPolicy(
@@ skipping 49 matching lines @@
 
   bool operator!=(const MapValueIteratorAdapter& that) const {
     return iter_ != that.iter_;
   }
 
  private:
   base::hash_map<int64, DownloadItem*>::const_iterator iter_;
   // Allow copy and assign.
 };
 
-void EnsureNoPendingDownloadsOnFile(scoped_refptr<DownloadFileManager> dfm,
-                                    bool* result) {
-  if (dfm->NumberOfActiveDownloads())
-    *result = false;
+void EnsureNoPendingDownloadJobsOnFile(bool* result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  *result = (content::DownloadFile::GetNumberOfDownloadFiles() == 0);
   BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE, MessageLoop::QuitClosure());
 }
 
-void EnsureNoPendingDownloadJobsOnIO(bool* result) {
-  scoped_refptr<DownloadFileManager> download_file_manager =
-      ResourceDispatcherHostImpl::Get()->download_file_manager();
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&EnsureNoPendingDownloadsOnFile,
-                 download_file_manager, result));
-}
-
 class DownloadItemFactoryImpl : public content::DownloadItemFactory {
  public:
     DownloadItemFactoryImpl() {}
     virtual ~DownloadItemFactoryImpl() {}
 
     virtual DownloadItemImpl* CreatePersistedItem(
         DownloadItemImplDelegate* delegate,
         content::DownloadId download_id,
         const content::DownloadPersistentStoreInfo& info,
         const net::BoundNetLog& bound_net_log) OVERRIDE {
@@ skipping 22 matching lines @@
 };
 
 }  // namespace
 
 namespace content {
 
 bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   bool result = true;
   BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&EnsureNoPendingDownloadJobsOnIO, &result));
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&EnsureNoPendingDownloadJobsOnFile, &result));
   MessageLoop::current()->Run();
   return result;
 }
 
 }  // namespace content
 
 DownloadManagerImpl::DownloadManagerImpl(
-    DownloadFileManager* file_manager,
-    scoped_ptr<content::DownloadItemFactory> factory,
     net::NetLog* net_log)
-    : factory_(factory.Pass()),
+    : item_factory_(new DownloadItemFactoryImpl()),
+      file_factory_(new content::DownloadFileFactory()),
       history_size_(0),
       shutdown_needed_(false),
       browser_context_(NULL),
-      file_manager_(file_manager),
       delegate_(NULL),
       net_log_(net_log) {
-  DCHECK(file_manager);
-  if (!factory_.get())
-    factory_.reset(new DownloadItemFactoryImpl());
 }
 
 DownloadManagerImpl::~DownloadManagerImpl() {
   DCHECK(!shutdown_needed_);
 }
 
 DownloadId DownloadManagerImpl::GetNextId() {
   DownloadId id;
   if (delegate_)
    id = delegate_->GetNextId();
   if (!id.IsValid()) {
     static int next_id;
     id = DownloadId(browser_context_, ++next_id);
   }
 
   return id;
 }
 
-DownloadFileManager* DownloadManagerImpl::GetDownloadFileManager() {
-  return file_manager_;
+void DownloadManagerImpl::DetermineDownloadTarget(
+    DownloadItemImpl* item, DownloadTargetCallback callback) {
+  // Note that this next call relies on
+  // DownloadItemImplDelegate::DownloadTargetCallback and
+  // DownloadManagerDelegate::DownloadTargetCallback having the same
+  // type.  If the types ever diverge, gasket code will need to
+  // be written here.
+  if (!delegate_ || !delegate_->DetermineDownloadTarget(item, callback)) {
+    FilePath target_path = item->GetForcedFilePath();
+    // TODO(asanka): Determine a useful path if |target_path| is empty.
+    callback.Run(target_path,
+                 DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                 target_path);
+  }
 }
 
 bool DownloadManagerImpl::ShouldOpenDownload(DownloadItemImpl* item) {
   if (!delegate_)
     return true;
 
   return delegate_->ShouldOpenDownload(item);
 }
 
 bool DownloadManagerImpl::ShouldOpenFileBasedOnExtension(const FilePath& path) {
@@ skipping 15 matching lines @@
 void DownloadManagerImpl::Shutdown() {
   VLOG(20) << __FUNCTION__ << "()"
            << " shutdown_needed_ = " << shutdown_needed_;
   if (!shutdown_needed_)
     return;
   shutdown_needed_ = false;
 
   FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this));
   // TODO(benjhayden): Consider clearing observers_.
 
-  DCHECK(file_manager_);
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::OnDownloadManagerShutdown,
-                 file_manager_, make_scoped_refptr(this)));
+  // The DownloadFiles will be canceled and deleted by their DownloadItems.
 
   AssertContainersConsistent();
 
   // Go through all downloads in downloads_.  Dangerous ones we need to
   // remove on disk, and in progress ones we need to cancel.
   for (DownloadMap::iterator it = downloads_.begin(); it != downloads_.end();) {
     DownloadItemImpl* download = it->second;
 
     // Save iterator from potential erases in this set done by called code.
     // Iterators after an erasure point are still valid for lists and
@@ skipping 22 matching lines @@
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
 
   active_downloads_.clear();
   STLDeleteValues(&downloads_);
   downloads_.clear();
 
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
 
-  file_manager_ = NULL;
   if (delegate_)
     delegate_->Shutdown();
   delegate_ = NULL;
 }
 
 bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
   DCHECK(browser_context);
   DCHECK(!shutdown_needed_)  << "DownloadManager already initialized.";
   shutdown_needed_ = true;
 
   browser_context_ = browser_context;
 
   return true;
 }
 
-// We have received a message from DownloadFileManager about a new download.
 content::DownloadId DownloadManagerImpl::StartDownload(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<content::ByteStreamReader> stream) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // |bound_net_log| will be used for logging both the download item's and
-  // the download file's events.
-  net::BoundNetLog bound_net_log = CreateDownloadItem(info.get());
+  net::BoundNetLog bound_net_log =
+      net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
 
-  // If info->download_id was unknown on entry to this function, it was
-  // assigned in CreateDownloadItem.
-  DownloadId download_id = info->download_id;
-
+  FilePath default_download_directory;
   if (delegate_) {
     FilePath website_save_directory;      // Unused
     bool skip_dir_check = false;          // Unused
     delegate_->GetSaveDir(GetBrowserContext(), &website_save_directory,
-                          &info->default_download_directory, &skip_dir_check);
+                          &default_download_directory, &skip_dir_check);
   }
 
-  DownloadFileManager::CreateDownloadFileCallback callback(
-      base::Bind(&DownloadManagerImpl::OnDownloadFileCreated,
-                 this, download_id.local()));
+  // We create the DownloadItem before the DownloadFile because the
+  // DownloadItem already needs to handle a state in which there is
+  // no associated DownloadFile (history downloads, !IN_PROGRESS downloads)
+  DownloadItemImpl* download =
+      CreateDownloadItem(info.get(), bound_net_log);
+  scoped_ptr<content::DownloadFile> download_file(
+      file_factory_->CreateFile(
+          info->save_info, default_download_directory,
+          info->url(), info->referrer_url,
+          info->received_bytes, delegate_->GenerateFileHash(),
+          stream.Pass(), bound_net_log,
+          download->DestinationObserverAsWeakPtr()));
+  download->Start(download_file.Pass());
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CreateDownloadFile,
-                 file_manager_, base::Passed(info.Pass()),
-                 base::Passed(stream.Pass()), make_scoped_refptr(this),
-                 (delegate_ && delegate_->GenerateFileHash()), bound_net_log,
-                 callback));
+  // Delay notification until after Start() so that download_file is bound
+  // to download and all the usual setters (e.g. Cancel) work.
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
 
-  return download_id;
-}
-
-void DownloadManagerImpl::OnDownloadFileCreated(
-    int32 download_id, content::DownloadInterruptReason reason) {
-  if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
-    OnDownloadInterrupted(download_id, reason);
-    // TODO(rdsmith): It makes no sense to continue along the
-    // regular download path after we've gotten an error.  But it's
-    // the way the code has historically worked, and this allows us
-    // to get the download persisted and observers of the download manager
-    // notified, so tests work.  When we execute all side effects of cancel
-    // (including queue removal) immedately rather than waiting for
-    // persistence we should replace this comment with a "return;".
-  }
-
-  DownloadMap::iterator download_iter = active_downloads_.find(download_id);
-  if (download_iter == active_downloads_.end())
-    return;
-
-  DownloadItemImpl* download = download_iter->second;
-  content::DownloadTargetCallback callback =
-      base::Bind(&DownloadManagerImpl::OnDownloadTargetDetermined,
-                 this, download_id);
-  if (!delegate_ || !delegate_->DetermineDownloadTarget(download, callback)) {
-    FilePath target_path = download->GetForcedFilePath();
-    // TODO(asanka): Determine a useful path if |target_path| is empty.
-    callback.Run(target_path,
-                 DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                 target_path);
-  }
-}
-
-void DownloadManagerImpl::OnDownloadTargetDetermined(
-    int32 download_id,
-    const FilePath& target_path,
-    DownloadItem::TargetDisposition disposition,
-    content::DownloadDangerType danger_type,
-    const FilePath& intermediate_path) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator download_iter = active_downloads_.find(download_id);
-  if (download_iter != active_downloads_.end()) {
-    // Once DownloadItem::OnDownloadTargetDetermined() is called, we expect a
-    // DownloadRenamedToIntermediateName() callback. This is necessary for the
-    // download to proceed.
-    download_iter->second->OnDownloadTargetDetermined(
-        target_path, disposition, danger_type, intermediate_path);
-  }
+  return download->GetGlobalId();
 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
     if (item->IsPersisted())
       CheckForFileRemoval(item);
   }
@@ skipping 26 matching lines @@
 void DownloadManagerImpl::OnFileRemovalDetected(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (ContainsKey(downloads_, download_id))
     downloads_[download_id]->OnDownloadedFileRemoved();
 }
 
 content::BrowserContext* DownloadManagerImpl::GetBrowserContext() const {
   return browser_context_;
 }
 
-net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
-    DownloadCreateInfo* info) {
+DownloadItemImpl* DownloadManagerImpl::CreateDownloadItem(
+    DownloadCreateInfo* info, const net::BoundNetLog& bound_net_log) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  net::BoundNetLog bound_net_log =
-      net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
   if (!info->download_id.IsValid())
     info->download_id = GetNextId();
-  DownloadItemImpl* download = factory_->CreateActiveItem(
+  DownloadItemImpl* download = item_factory_->CreateActiveItem(
       this, *info,
       scoped_ptr<DownloadRequestHandleInterface>(
           new DownloadRequestHandle(info->request_handle)).Pass(),
       bound_net_log);
 
   DCHECK(!ContainsKey(downloads_, download->GetId()));
   downloads_[download->GetId()] = download;
   DCHECK(!ContainsKey(active_downloads_, download->GetId()));
   active_downloads_[download->GetId()] = download;
-  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
 
-  return bound_net_log;
+  return download;
 }
 
 DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
     const FilePath& main_file_path,
     const GURL& page_url,
     const std::string& mime_type,
     DownloadItem::Observer* observer) {
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-  DownloadItemImpl* download = factory_->CreateSavePageItem(
+  DownloadItemImpl* download = item_factory_->CreateSavePageItem(
       this,
       main_file_path,
       page_url,
       GetNextId(),
       mime_type,
       bound_net_log);
 
   download->AddObserver(observer);
 
   DCHECK(!ContainsKey(downloads_, download->GetId()));
   downloads_[download->GetId()] = download;
-  DCHECK(!SavePageData::Get(download));
-  new SavePageData(download);
-  DCHECK(SavePageData::Get(download));
 
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
 
   // Will notify the observer in the callback.
   if (delegate_)
     delegate_->AddItemToPersistentStore(download);
 
   return download;
 }
 
-void DownloadManagerImpl::UpdateDownload(int32 download_id,
-                                         int64 bytes_so_far,
-                                         int64 bytes_per_sec,
-                                         const std::string& hash_state) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = active_downloads_.find(download_id);
-  if (it != active_downloads_.end()) {
-    DownloadItemImpl* download = it->second;
-    if (download->IsInProgress()) {
-      download->UpdateProgress(bytes_so_far, bytes_per_sec, hash_state);
-      if (delegate_)
-        delegate_->UpdateItemInPersistentStore(download);
-    }
-  }
-}
-
-void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
-                                              int64 size,
-                                              const std::string& hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
-           << " size = " << size;
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // If it's not in active_downloads_, that means it was cancelled; just
-  // ignore the notification.
-  if (active_downloads_.count(download_id) == 0)
-    return;
-
-  DownloadItemImpl* download = active_downloads_[download_id];
-  download->OnAllDataSaved(size, hash);
-  MaybeCompleteDownload(download);
-}
-
 void DownloadManagerImpl::AssertStateConsistent(
     DownloadItemImpl* download) const {
   CHECK(ContainsKey(downloads_, download->GetId()));
 
   int64 state = download->GetState();
   base::debug::Alias(&state);
   if (ContainsKey(active_downloads_, download->GetId())) {
     if (download->IsPersisted())
       CHECK_EQ(DownloadItem::IN_PROGRESS, download->GetState());
     if (DownloadItem::IN_PROGRESS != download->GetState())
@@ skipping 101 matching lines @@
 void DownloadManagerImpl::DownloadStopped(DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
   RemoveFromActiveList(download);
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
   AssertStateConsistent(download);
-
-  DCHECK(file_manager_);
-  download->OffThreadCancel();
-}
-
-void DownloadManagerImpl::OnDownloadInterrupted(
-    int32 download_id,
-    content::DownloadInterruptReason reason) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (!ContainsKey(active_downloads_, download_id))
-    return;
-  active_downloads_[download_id]->Interrupt(reason);
 }
 
 void DownloadManagerImpl::RemoveFromActiveList(DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(download);
 
   // Clean up will happen when the history system create callback runs if we
   // don't have a valid db_handle yet.
   if (download->IsPersisted()) {
     active_downloads_.erase(download->GetId());
     if (delegate_)
       delegate_->UpdateItemInPersistentStore(download);
   }
 }
 
+void DownloadManagerImpl::SetDownloadItemFactoryForTesting(
+    scoped_ptr<content::DownloadItemFactory> item_factory) {
+  item_factory_ = item_factory.Pass();
+}
+
+void DownloadManagerImpl::SetDownloadFileFactoryForTesting(
+    scoped_ptr<content::DownloadFileFactory> file_factory) {
+  file_factory_ = file_factory.Pass();
+}
+
+content::DownloadFileFactory*
+DownloadManagerImpl::GetDownloadFileFactoryForTesting() {

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

Why do we need a getter for the FileFactory but not the ItemFactory?

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

Caveat: I'm not claiming this is a good reason, it's simply the answer to your
question.

In test_file_error_injector.cc we set the file factory, and then somewhat later
we inject errors into the file factory.  The file factory is owned by the
DownloadFileManager, and might have been replaced on us by "something else".  So
I considered it a reasonably high priority to confirm that that hasn't happened
at the injection point, so we don't do a use after free without a pretty clear
assertion.  There's no similar problem for the download item factory (it's only
used in unit tests, which have tighter control--TFEI is used in browser_tests,
where there's less control), and no need for a getter ==> didn't put one in.

Having written this out, I'm thinking that a weak pointer based implementation
would probably be a better plan, but Andy & I weren't as familiar with weak
pointers when TFEI was implemented.  I'll put that on my list :-}.

+  return file_factory_.get();
+}
+
 int DownloadManagerImpl::RemoveDownloadItems(
     const DownloadItemImplVector& pending_deletes) {
   if (pending_deletes.empty())
     return 0;
 
   // Delete from internal maps.
   for (DownloadItemImplVector::const_iterator it = pending_deletes.begin();
        it != pending_deletes.end();
        ++it) {
     DownloadItemImpl* download = *it;
@@ skipping 87 matching lines @@
 // 'DownloadPersistentStoreInfo's in sorted order (by ascending start_time).
 void DownloadManagerImpl::OnPersistentStoreQueryComplete(
     std::vector<DownloadPersistentStoreInfo>* entries) {
   history_size_ = entries->size();
   for (size_t i = 0; i < entries->size(); ++i) {
     int64 db_handle = entries->at(i).db_handle;
     base::debug::Alias(&db_handle);
 
     net::BoundNetLog bound_net_log =
         net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-    DownloadItemImpl* download = factory_->CreatePersistedItem(
+    DownloadItemImpl* download = item_factory_->CreatePersistedItem(
         this, GetNextId(), entries->at(i), bound_net_log);
     DCHECK(!ContainsKey(downloads_, download->GetId()));
     downloads_[download->GetId()] = download;
     FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
   }
   NotifyModelChanged();
   CheckForHistoryFilesRemoval();
 }
@@ skipping 11 matching lines @@
   ++history_size_;
 
   // Show in the appropriate browser UI.
   // This includes buttons to save or cancel, for a dangerous download.
   ShowDownloadInBrowser(download);
 
   // Inform interested objects about the new download.
   NotifyModelChanged();
 }
 
-
 void DownloadManagerImpl::OnItemAddedToPersistentStore(int32 download_id,
                                                        int64 db_handle) {
   // It's valid that we don't find a matching item, i.e. on shutdown.
   if (!ContainsKey(downloads_, download_id))
     return;
 
   DownloadItemImpl* item = downloads_[download_id];
   AddDownloadItemToHistory(item, db_handle);
-  if (SavePageData::Get(item)) {
+  if (item->IsSavePackageDownload()) {
     OnSavePageItemAddedToPersistentStore(item);
   } else {
     OnDownloadItemAddedToPersistentStore(item);
   }
 }
 
 // Once the new DownloadItem has been committed to the persistent store,
 // associate it with its db_handle (TODO(benjhayden) merge db_handle with id),
 // show it in the browser (TODO(benjhayden) the ui should observe us instead),
 // and notify observers (TODO(benjhayden) observers should be able to see the
@@ skipping 155 matching lines @@
       ++num_unopened;
   }
   download_stats::RecordOpensOutstanding(num_unopened);
 }
 
 void DownloadManagerImpl::DownloadRenamedToIntermediateName(
     DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // download->GetFullPath() is only expected to be meaningful after this
   // callback is received. Therefore we can now add the download to a persistent
-  // store. If the rename failed, we receive an OnDownloadInterrupted() call
+  // store. If the rename failed, we processed an interrupt
   // before we receive the DownloadRenamedToIntermediateName() call.
   if (delegate_) {
     delegate_->AddItemToPersistentStore(download);
   } else {
     OnItemAddedToPersistentStore(download->GetId(),
                                  DownloadItem::kUninitializedHandle);
   }
 }
 
 void DownloadManagerImpl::DownloadRenamedToFinalName(
     DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // If the rename failed, we receive an OnDownloadInterrupted() call before we
-  // receive the DownloadRenamedToFinalName() call.
+  // If the rename failed, we processed an interrupt before we get here.
   if (delegate_) {
     delegate_->UpdatePathForItemInPersistentStore(
         download, download->GetFullPath());
   }
 }