Make DownloadHistory observe manager, items

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"
@@ skipping 14 matching lines @@
 #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"
-#include "content/public/browser/download_persistent_store_info.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "net/base/load_flags.h"
 #include "net/base/upload_data.h"
 #include "webkit/glue/webkit_glue.h"
 
 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) {
@@ skipping 102 matching lines @@
 }
 
 class DownloadItemFactoryImpl : public content::DownloadItemFactory {
  public:
     DownloadItemFactoryImpl() {}
     virtual ~DownloadItemFactoryImpl() {}
 
     virtual DownloadItemImpl* CreatePersistedItem(
         DownloadItemImplDelegate* delegate,
         content::DownloadId download_id,
-        const content::DownloadPersistentStoreInfo& info,
+        const FilePath& path,
+        const GURL& url,
+        const GURL& referrer_url,
+        const base::Time& start_time,
+        const base::Time& end_time,
+        int64 received_bytes,
+        int64 total_bytes,
+        DownloadItem::DownloadState state,
+        bool opened,
         const net::BoundNetLog& bound_net_log) OVERRIDE {
-      return new DownloadItemImpl(delegate, download_id, info, bound_net_log);
+      return new DownloadItemImpl(
+          delegate,
+          download_id,
+          path,
+          url,
+          referrer_url,
+          start_time,
+          end_time,
+          received_bytes,
+          total_bytes,
+          state,
+          opened,
+          bound_net_log);
     }
 
     virtual DownloadItemImpl* CreateActiveItem(
         DownloadItemImplDelegate* delegate,
         const DownloadCreateInfo& info,
         scoped_ptr<DownloadRequestHandleInterface> request_handle,
         const net::BoundNetLog& bound_net_log) OVERRIDE {
       return new DownloadItemImpl(delegate, info, request_handle.Pass(),
                                   bound_net_log);
     }
@@ skipping 119 matching lines @@
       // from the disk.  This may or may not result in it being
       // removed from the DownloadManager queues and deleted
       // (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
       // from all queues.
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
       download->Cancel(false);
-      if (delegate_)
-        delegate_->UpdateItemInPersistentStore(download);
     }
   }
 
   // At this point, all dangerous downloads have had their files removed
   // 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;
 }
 
 void DownloadManagerImpl::GetTemporaryDownloads(
     const FilePath& dir_path, DownloadVector* result) {
   DCHECK(result);
 
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
-    // TODO(benjhayden): Don't check IsPersisted().
     if (item->IsTemporary() &&
-        item->IsPersisted() &&
         (dir_path.empty() ||
          item->GetTargetFilePath().DirName() == dir_path))
       result->push_back(item);
   }
 }
 
 void DownloadManagerImpl::GetAllDownloads(
     const FilePath& dir_path, DownloadVector* result) {
   DCHECK(result);
 
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
-    // TODO(benjhayden): Don't check IsPersisted().
     if (!item->IsTemporary() &&
-        item->IsPersisted() &&
         (dir_path.empty() ||
          item->GetTargetFilePath().DirName() == dir_path))
       result->push_back(item);
   }
 }
 
 void DownloadManagerImpl::SearchDownloads(const string16& query,
                                           DownloadVector* result) {
   string16 query_lower(base::i18n::ToLower(query));
 
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* download_item = it->second;
-    // TODO(benjhayden): Don't check IsPersisted().
     if (!download_item->IsTemporary() &&
-        download_item->IsPersisted() &&
         download_item->MatchesQuery(query_lower)) {
       result->push_back(download_item);
     }
   }
 }
 
 bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
   DCHECK(browser_context);
   DCHECK(!shutdown_needed_)  << "DownloadManager already initialized.";
   shutdown_needed_ = true;
@@ skipping 64 matching lines @@
   }
 }
 
 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);
+  DownloadMap::iterator download_iter = downloads_.find(download_id);
+  if (download_iter == downloads_.end())
+    return;
+
+  // Once DownloadItem::OnDownloadTargetDetermined() is called, we expect a
+  // DownloadRenamedToIntermediateName() callback. This is necessary for the
+  // download to proceed.
+  DownloadItemImpl* item = download_iter->second;
+  item->OnDownloadTargetDetermined(
+      target_path, disposition, danger_type, intermediate_path);
+  ShowDownloadInBrowser(item);
+  NotifyModelChanged();
+  if (!SavePageData::Get(item)) {
+    MaybeCompleteDownload(item);
   }
 }
 
 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);
+    CheckForFileRemoval(item);
   }
 }
 
 void DownloadManagerImpl::CheckForFileRemoval(DownloadItemImpl* download_item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (download_item->IsComplete() &&
       !download_item->GetFileExternallyRemoved()) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadManagerImpl::CheckForFileRemovalOnFileThread,
@@ skipping 47 matching lines @@
   return bound_net_log;
 }
 
 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));
-
-  // TODO(benjhayden): Fire OnDownloadCreated for SavePackage downloads when
-  // we're comfortable with the user interacting with them.
-  // FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
-
-  // Will notify the observer in the callback.
-  if (delegate_)
-    delegate_->AddItemToPersistentStore(download);
-
-  return download;
+  download_item->AddObserver(observer);
+  DCHECK(!ContainsKey(downloads_, download_item->GetId()));
+  downloads_[download_item->GetId()] = download_item;
+  DCHECK(!SavePageData::Get(download_item));
+  new SavePageData(download_item);
+  DCHECK(SavePageData::Get(download_item));
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(
+      this, download_item));
+  OnDownloadTargetDetermined(
+      download_item->GetId(),
+      main_file_path,
+      DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+      content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+      main_file_path);
+  return download_item;
 }
 
 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())
-      CHECK_EQ(DownloadItem::kUninitializedHandle, download->GetDbHandle());
-  }
   if (DownloadItem::IN_PROGRESS == download->GetState())
     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())
+  if (download->GetTargetFilePath().DirName() !=
+      download->GetFullPath().DirName())
+    return false;
+  if (download->GetTargetName().empty())
     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
@@ skipping 13 matching lines @@
 
   // 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);
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
   active_downloads_.erase(download->GetId());
   AssertStateConsistent(download);
 }
 
 void DownloadManagerImpl::CancelDownload(int32 download_id) {
   // A cancel at the right time could remove the download from the
   // |active_downloads_| map before we get here.
   if (ContainsKey(active_downloads_, download_id))
     active_downloads_[download_id]->Cancel(true);
 }
@@ skipping 19 matching lines @@
   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);
-  }
+  active_downloads_.erase(download->GetId());
 }
 
 bool DownloadManagerImpl::GenerateFileHash() {
   return delegate_ && delegate_->GenerateFileHash();
 }
 
 int DownloadManagerImpl::RemoveDownloadItems(
     const DownloadItemImplVector& pending_deletes) {
   if (pending_deletes.empty())
     return 0;
@@ skipping 10 matching lines @@
   }
   NotifyModelChanged();
   return static_cast<int>(pending_deletes.size());
 }
 
 void DownloadManagerImpl::DownloadRemoved(DownloadItemImpl* download) {
   if (!download ||
       downloads_.find(download->GetId()) == downloads_.end())
     return;
 
-  // TODO(benjhayden,rdsmith): Remove this.
-  if (!download->IsPersisted())
-    return;
-
-  // Make history update.
-  if (delegate_)
-    delegate_->RemoveItemFromPersistentStore(download);
-
   // Remove from our tables and delete.
   int downloads_count =
       RemoveDownloadItems(DownloadItemImplVector(1, download));
   DCHECK_EQ(1, downloads_count);
 }
 
 int DownloadManagerImpl::RemoveDownloadsBetween(base::Time remove_begin,
                                                 base::Time remove_end) {
-  if (delegate_)
-    delegate_->RemoveItemsFromPersistentStoreBetween(remove_begin, remove_end);
-
   DownloadItemImplVector pending_deletes;
   for (DownloadMap::const_iterator it = downloads_.begin();
       it != downloads_.end();
       ++it) {
     DownloadItemImpl* download = it->second;
-    if (download->IsPersisted() &&
-        download->GetStartTime() >= remove_begin &&
+    if (download->GetStartTime() >= remove_begin &&
         (remove_end.is_null() || download->GetStartTime() < remove_end) &&
         (download->IsComplete() || download->IsCancelled())) {
       AssertStateConsistent(download);
       download->NotifyRemoved();
       pending_deletes.push_back(download);
     }
   }
   return RemoveDownloadItems(pending_deletes);
 }
 
@@ skipping 26 matching lines @@
   // observers.
   observer->ModelChanged(this);
 }
 
 void DownloadManagerImpl::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 // Operations posted to us from the history service ----------------------------
 
-// The history service has retrieved all download entries. 'entries' contains
-// '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(
-        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);
-  }
+DownloadItem* DownloadManagerImpl::CreateDownloadItem(
+    const FilePath& path,
+    const GURL& url,
+    const GURL& referrer_url,
+    const base::Time& start_time,
+    const base::Time& end_time,
+    int64 received_bytes,
+    int64 total_bytes,
+    DownloadItem::DownloadState state,
+    bool opened) {
+  DownloadItemImpl* item = factory_->CreatePersistedItem(
+      this,
+      GetNextId(),
+      path,
+      url,
+      referrer_url,
+      start_time,
+      end_time,
+      received_bytes,
+      total_bytes,
+      state,
+      opened,
+      net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD));
+  DCHECK(!ContainsKey(downloads_, item->GetId()));
+  downloads_[item->GetId()] = item;
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, item));
+  VLOG(20) << __FUNCTION__ << "() download = " << item->DebugString(true);
   NotifyModelChanged();
-  CheckForHistoryFilesRemoval();
-}
-
-void DownloadManagerImpl::AddDownloadItemToHistory(DownloadItemImpl* download,
-                                                   int64 db_handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK_NE(DownloadItem::kUninitializedHandle, db_handle);
-  DCHECK(!download->IsPersisted());
-  download->SetDbHandle(db_handle);
-  download->SetIsPersisted();
-
-  download_stats::RecordHistorySize(history_size_);
-  // Not counting |download|.
-  ++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)) {
-    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
-// item when it's created so they can observe it directly. Are there any
-// clients that actually need to know when the item is added to the history?).
-void DownloadManagerImpl::OnDownloadItemAddedToPersistentStore(
-    DownloadItemImpl* item) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << item->GetDbHandle()
-           << " download_id = " << item->GetId()
-           << " download = " << item->DebugString(true);
-
-  // If the download is still in progress, try to complete it.
-  //
-  // Otherwise, download has been cancelled or interrupted before we've
-  // received the DB handle.  We post one final message to the history
-  // service so that it can be properly in sync with the DownloadItem's
-  // completion status, and also inform any observers so that they get
-  // more than just the start notification.
-  if (item->IsInProgress()) {
-    MaybeCompleteDownload(item);
-  } else {
-    DCHECK(item->IsCancelled());
-    active_downloads_.erase(item->GetId());
-    if (delegate_)
-      delegate_->UpdateItemInPersistentStore(item);
-    item->UpdateObservers();
-  }
+  return item;
 }
 
 void DownloadManagerImpl::ShowDownloadInBrowser(DownloadItemImpl* download) {
   // The 'contents' may no longer exist if the user closed the contents before
   // we get this start completion event.
   WebContents* content = download->GetWebContents();
 
   // If the contents no longer exists, we ask the embedder to suggest another
   // contents.
   if (!content && delegate_)
@@ skipping 15 matching lines @@
       ++count;
   }
   return count;
 }
 
 void DownloadManagerImpl::NotifyModelChanged() {
   FOR_EACH_OBSERVER(Observer, observers_, ModelChanged(this));
 }
 
 DownloadItem* DownloadManagerImpl::GetDownloadItem(int download_id) {
-  DownloadItem* download = GetDownload(download_id);
-  return (download && download->IsPersisted()) ? download : NULL;
+  return GetDownload(download_id);
 }
 
 DownloadItem* DownloadManagerImpl::GetDownload(int download_id) {
   return ContainsKey(downloads_, download_id) ? downloads_[download_id] : NULL;
 }
 
 DownloadItem* DownloadManagerImpl::GetActiveDownloadItem(int download_id) {
   if (ContainsKey(active_downloads_, download_id))
     return active_downloads_[download_id];
   return NULL;
@@ skipping 26 matching lines @@
     DownloadSet remainder;
     std::insert_iterator<DownloadSet> insert_it(remainder, remainder.begin());
     std::set_difference(all_sets[i]->begin(), all_sets[i]->end(),
                         all_downloads.begin(), all_downloads.end(),
                         insert_it);
     DCHECK(remainder.empty());
   }
 #endif
 }
 
-// SavePackage will call SavePageDownloadFinished upon completion/cancellation.
-// The history callback will call OnSavePageItemAddedToPersistentStore.
-// If the download finishes before the history callback,
-// OnSavePageItemAddedToPersistentStore calls SavePageDownloadFinished, ensuring
-// that the history event is update regardless of the order in which these two
-// events complete.
-// If something removes the download item from the download manager (Remove,
-// Shutdown) the result will be that the SavePage system will not be able to
-// properly update the download item (which no longer exists) or the download
-// history, but the action will complete properly anyway.  This may lead to the
-// history entry being wrong on a reload of chrome (specifically in the case of
-// Initiation -> History Callback -> Removal -> Completion), but there's no way
-// to solve that without canceling on Remove (which would then update the DB).
-
-void DownloadManagerImpl::OnSavePageItemAddedToPersistentStore(
-    DownloadItemImpl* item) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Finalize this download if it finished before the history callback.
-  if (!item->IsInProgress())
-    SavePageDownloadFinished(item);
-}
-
 void DownloadManagerImpl::SavePageDownloadFinished(
     content::DownloadItem* download) {
-  if (download->IsPersisted()) {
-    if (delegate_)
-      delegate_->UpdateItemInPersistentStore(download);
-    if (download->IsComplete())
-      content::NotificationService::current()->Notify(
-          content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
-          content::Source<DownloadManager>(this),
-          content::Details<DownloadItem>(download));
-  }
+  if (download->IsComplete())
+    content::NotificationService::current()->Notify(
+        content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
+        content::Source<DownloadManager>(this),
+        content::Details<DownloadItem>(download));
 }
 
 void DownloadManagerImpl::DownloadOpened(DownloadItemImpl* download) {
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
   int num_unopened = 0;
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
     if (item->IsComplete() &&
         !item->GetOpened())
       ++num_unopened;
   }
   download_stats::RecordOpensOutstanding(num_unopened);
 }
 
+// TODO(benjhayden) this should be a method on DI
 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
-  // before we receive the DownloadRenamedToIntermediateName() call.
-  if (delegate_) {
-    delegate_->AddItemToPersistentStore(download);
-  } else {
-    OnItemAddedToPersistentStore(download->GetId(),
-                                 DownloadItem::kUninitializedHandle);
-  }
+  download->UpdateObservers();
+  MaybeCompleteDownload(download);
 }
 
+// TODO(benjhayden) this should be a method on DI
 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 (delegate_) {
-    delegate_->UpdatePathForItemInPersistentStore(
-        download, download->GetFullPath());
-  }
+  download->UpdateObservers();
 }