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 34 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 SavePageExternalData : public DownloadItem::ExternalData {
+class SavePageData : public base::SupportsUserData::Data {
  public:
   // A spoonful of syntactic sugar.
   static bool Get(DownloadItem* item) {
-    return item->GetExternalData(kKey) != NULL;
+    return item->GetUserData(kKey) != NULL;
   }
 
-  explicit SavePageExternalData(DownloadItem* item) {
-    item->SetExternalData(kKey, this);
+  explicit SavePageData(DownloadItem* item) {
+    item->SetUserData(kKey, this);
   }
 
-  virtual ~SavePageExternalData() {}
+  virtual ~SavePageData() {}
 
  private:
   static const char kKey[];
 
-  DISALLOW_COPY_AND_ASSIGN(SavePageExternalData);
+  DISALLOW_COPY_AND_ASSIGN(SavePageData);
 };
 
-const char SavePageExternalData::kKey[] = "DownloadItem SavePageExternalData";
+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(new net::URLRequest(
       params->url(),
       NULL,
       params->resource_context()->GetRequestContext()));
@@ skipping 220 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.
 
-  // Copy downloads_ to separate container so as not to set off checks
-  // in DownloadItem destruction.
-  DownloadMap downloads_to_delete;
-  downloads_to_delete.swap(downloads_);
-
   active_downloads_.clear();
-  STLDeleteValues(&downloads_to_delete);
+  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;
     // Display Incognito downloads only in Incognito window, and vice versa.
     // The Incognito Downloads page will get the list of non-Incognito downloads
     // from its parent profile.
-    // TODO(benjhayden): Don't check IsPersisted().
     if (!download_item->IsTemporary() &&
-        download_item->IsPersisted() &&
         (browser_context_->IsOffTheRecord() == download_item->IsOtr()) &&
         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.";
@@ skipping 65 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 48 matching lines @@
 }
 
 DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
     const FilePath& main_file_path,
     const GURL& page_url,
     bool is_otr,
     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,
       is_otr,
       GetNextId(),
       mime_type,
       bound_net_log);
-
-  download->AddObserver(observer);
-
-  DCHECK(!ContainsKey(downloads_, download->GetId()));
-  downloads_[download->GetId()] = download;
-  DCHECK(!SavePageExternalData::Get(download));
-  new SavePageExternalData(download);
-  DCHECK(SavePageExternalData::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));
+  // TODO(benjhayden) XXX occam test showing SavePackage DownloadItems in ui
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(
+      this, download_item));
+  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 {
-  if (download->GetState() == DownloadItem::REMOVING) {
-    DCHECK(!ContainsKey(downloads_, download->GetId()));
-    DCHECK(!ContainsKey(active_downloads_, download->GetId()));
-    return;
-  }
-
-  // Should be in downloads_ if we're not REMOVING.
   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())
-    return false;
-
   return true;
 }
 
 // When SavePackage downloads MHTML to GData (see
 // SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
 // does for non-SavePackage downloads, but SavePackage downloads never satisfy
 // IsDownloadReadyForCompletion(). GDataDownloadObserver manually calls
 // DownloadItem::UpdateObservers() when the upload completes so that SavePackage
 // notices that the upload has completed and runs its normal Finish() pathway.
 // MaybeCompleteDownload() is never the mechanism by which SavePackage completes
@@ skipping 11 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;
 
   // Delete from internal maps.
   for (DownloadItemImplVector::const_iterator it = pending_deletes.begin();
-      it != pending_deletes.end();
-      ++it) {
+       it != pending_deletes.end();
+       ++it) {
     DownloadItemImpl* download = *it;
     DCHECK(download);
-    downloads_.erase(download->GetId());
+    int32 download_id = download->GetId();
+    delete download;
+    downloads_.erase(download_id);
   }
-
-  // Tell observers to refresh their views.
   NotifyModelChanged();
-
-  // Delete the download items themselves.
-  const int num_deleted = static_cast<int>(pending_deletes.size());
-  STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
-  return num_deleted;
+  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);
-
-  // All downloads visible to the user will be in the history,
-  // so scan that map.
   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);
 }
 
 int DownloadManagerImpl::RemoveDownloads(base::Time remove_begin) {
   return RemoveDownloadsBetween(remove_begin, base::Time());
 }
 
@@ skipping 28 matching lines @@
 }
 
 // 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);
   }
   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 (SavePageExternalData::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();
-  }
-}
-
 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_)
     content = delegate_->GetAlternativeWebContentsToNotifyForDownload();
 
@@ skipping 12 matching lines @@
     if (item->IsInProgress())
       ++count;
   }
   return count;
 }
 
 void DownloadManagerImpl::NotifyModelChanged() {
   FOR_EACH_OBSERVER(Observer, observers_, ModelChanged(this));
 }
 
+// TODO(benjhayden): Kill this method premailing XXX occam
 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();
 }
 
+// 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();
 }