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 "net/url_request/url_request_context.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 101 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();
@@ skipping 84 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();
 }
 
 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));
-
-  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);

[Randy Smith (Not in Mondays), 2012/09/25 18:09:20]

You got rid of blank lines that I personally feel like improve readability, but
allowing people to evaluate a chunk of related lines at the same time. 
Re-insert?

[benjhayden, 2012/09/25 20:22:18]

Done.

+  DCHECK(!ContainsKey(downloads_, download_item->GetId()));
+  downloads_[download_item->GetId()] = download_item;
+  DCHECK(!SavePageData::Get(download_item));
+  new SavePageData(download_item);

[Randy Smith (Not in Mondays), 2012/09/25 18:09:20]

I don't see this being used anywhere in this file anymore.  Delete?

[benjhayden, 2012/09/25 20:22:18]

Done.

+  DCHECK(SavePageData::Get(download_item));
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(
+      this, download_item));
+  OnDownloadTargetDetermined(

[Randy Smith (Not in Mondays), 2012/09/25 18:09:20]

So.  This starts a long cascade inside of DownloadItemImpl
(OnTargetDownloadDetermined-> OnDownloadRenamedToIntermediateName ->
DMI::OnDownloadRenamedToIntermediateName -> MaybeCompleteDownload()).  Maybe it
works without you tweaking it (if so, probably because MaybeCompleteDownload is
missing some attribute needed for continuation), or maybe you can get it to
work.  But I'd rather we not engage that cascade at all--it's oriented at
regular downloads, not save page downloads.

I'm not sure how to avoid it, but the way I'd be tempted to try is to modify the
SavePackage DownloadItem constructor so that it naturally sets up the variables
you want setup from the beginning, and don't go anywhere near the Dangerous
Machinery of the regular download item cascade.

[benjhayden, 2012/09/25 20:22:18]

Done.

[Randy Smith (Not in Mondays), 2012/09/26 15:56:12]

I don't see any modifications to the SavePackage download item constructor. 
What did you do in place of this?  Or did you decide that it wasn't needed after
all?

+      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 the intermediate filename rename hasn't occurred, don't complete the
+  // download.
+  if (download->GetTargetFilePath().DirName() !=
+      download->GetFullPath().DirName())
+    return false;
+
+  // If the target filename hasn't been determined, don't complete the download.
+  if (download->GetTargetFilePath().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());
 }
 
 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;
     DCHECK(download);
     int32 download_id = download->GetId();
     delete download;
     downloads_.erase(download_id);
   }
   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;
 }
 
 void DownloadManagerImpl::GetAllDownloads(DownloadVector* downloads) {
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     downloads->push_back(it->second);
@@ skipping 33 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);
-  }
-}
-
 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) MaybeCompleteDownload should be moved into DownloadItemImpl.
 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);
-  }
+  MaybeCompleteDownload(download);
 }
-
-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());
-  }
-}