Revert 113007 - DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
===================================================================
--- content/browser/download/download_manager_impl.cc	(revision 113083)
+++ content/browser/download/download_manager_impl.cc	(working copy)
@@ -101,14 +101,6 @@
   return id_factory_->GetNextId();
 }
 
-bool DownloadManagerImpl::ShouldOpenDownload(DownloadItem* item) {
-  return delegate_->ShouldOpenDownload(item);
-}
-
-bool DownloadManagerImpl::ShouldOpenFileBasedOnExtension(const FilePath& path) {
-  return delegate_->ShouldOpenFileBasedOnExtension(path);
-}
-
 void DownloadManagerImpl::Shutdown() {
   VLOG(20) << __FUNCTION__ << "()"
            << " shutdown_needed_ = " << shutdown_needed_;
@@ -142,7 +134,7 @@
       // The user hasn't accepted it, so we need to remove it
       // from the disk.  This may or may not result in it being
       // removed from the DownloadManager queues and deleted
-      // (specifically, DownloadManager::DownloadRemoved only
+      // (specifically, DownloadManager::RemoveDownload 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
@@ -325,7 +317,7 @@
   }
 }
 
-content::BrowserContext* DownloadManagerImpl::BrowserContext() const {
+content::BrowserContext* DownloadManagerImpl::BrowserContext() {
   return browser_context_;
 }
 
@@ -349,26 +341,6 @@
   active_downloads_[download_id] = download;
 }
 
-DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
-    const FilePath& main_file_path,
-    const GURL& page_url,
-    bool is_otr,
-    DownloadItem::Observer* observer) {
-  DownloadItem* download = new DownloadItemImpl(
-      this, main_file_path, page_url, is_otr, GetNextId());
-
-  download->AddObserver(observer);
-
-  DCHECK(!ContainsKey(save_page_downloads_, download->GetId()));
-  downloads_.insert(download);
-  save_page_downloads_[download->GetId()] = download;
-
-  // Will notify the observer in the callback.
-  delegate_->AddItemToPersistentStore(download);
-
-  return download;
-}
-
 void DownloadManagerImpl::ContinueDownloadWithPath(
     DownloadItem* download, const FilePath& chosen_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -399,8 +371,7 @@
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFileManager::RenameInProgressDownloadFile,
-                 file_manager_, download->GetGlobalId(),
-                 download_path));
+                 file_manager_, download->GetGlobalId(), download_path));
 
   download->Rename(download_path);
 
@@ -438,10 +409,10 @@
   download->OnAllDataSaved(size, hash);
   delegate_->OnResponseCompleted(download);
 
-  download->MaybeCompleteDownload();
+  MaybeCompleteDownload(download);
 }
 
-void DownloadManagerImpl::AssertStateConsistent(DownloadItem* download) const {
+void DownloadManagerImpl::AssertQueueStateConsistent(DownloadItem* download) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   if (download->GetState() == DownloadItem::REMOVING) {
     CHECK(!ContainsKey(downloads_, download));
@@ -460,7 +431,7 @@
   } else {
     // TODO(rdsmith): Somewhat painful; make sure to disable in
     // release builds after resolution of http://crbug.com/85408.
-    for (DownloadMap::const_iterator it = history_downloads_.begin();
+    for (DownloadMap::iterator it = history_downloads_.begin();
          it != history_downloads_.end(); ++it) {
       CHECK(it->second != download);
     }
@@ -540,12 +511,13 @@
   download->OnDownloadCompleting(file_manager_);
 }
 
-void DownloadManagerImpl::DownloadCompleted(DownloadItem* download) {
+void DownloadManagerImpl::DownloadCompleted(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadItem* download = GetDownloadItem(download_id);
   DCHECK(download);
   delegate_->UpdateItemInPersistentStore(download);
-  active_downloads_.erase(download->GetId());
-  AssertStateConsistent(download);
+  active_downloads_.erase(download_id);
+  AssertQueueStateConsistent(download);
 }
 
 void DownloadManagerImpl::OnDownloadRenamedToFinalName(
@@ -587,7 +559,7 @@
   download->Cancel(true);
 }
 
-void DownloadManagerImpl::DownloadCancelled(DownloadItem* download) {
+void DownloadManagerImpl::DownloadCancelledInternal(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
@@ -596,7 +568,7 @@
   RemoveFromActiveList(download);
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
-  AssertStateConsistent(download);
+  AssertQueueStateConsistent(download);
 
   if (file_manager_)
     download->OffThreadCancel(file_manager_);
@@ -688,12 +660,13 @@
   return num_deleted;
 }
 
-void DownloadManagerImpl::DownloadRemoved(DownloadItem* download) {
-  if (history_downloads_.find(download->GetDbHandle()) ==
-      history_downloads_.end())
+void DownloadManagerImpl::RemoveDownload(int64 download_handle) {
+  DownloadMap::iterator it = history_downloads_.find(download_handle);
+  if (it == history_downloads_.end())
     return;
 
   // Make history update.
+  DownloadItem* download = it->second;
   delegate_->RemoveItemFromPersistentStore(download);
 
   // Remove from our tables and delete.
@@ -715,7 +688,7 @@
     if (download->GetStartTime() >= remove_begin &&
         (remove_end.is_null() || download->GetStartTime() < remove_end) &&
         (download->IsComplete() || download->IsCancelled())) {
-      AssertStateConsistent(download);
+      AssertQueueStateConsistent(download);
       pending_deletes.push_back(download);
     }
   }
@@ -862,8 +835,7 @@
   largest_db_handle_in_history_ = 0;
 
   for (size_t i = 0; i < entries->size(); ++i) {
-    DownloadItem* download = new DownloadItemImpl(
-        this, GetNextId(), entries->at(i));
+    DownloadItem* download = new DownloadItemImpl(this, entries->at(i));
     // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
     CHECK(!ContainsKey(history_downloads_, download->GetDbHandle()));
     downloads_.insert(download);
@@ -1053,6 +1025,16 @@
 #endif
 }
 
+void DownloadManagerImpl::SavePageDownloadStarted(DownloadItem* download) {
+  DCHECK(!ContainsKey(save_page_downloads_, download->GetId()));
+  downloads_.insert(download);
+  save_page_downloads_[download->GetId()] = download;
+
+  // Add this entry to the history service.
+  // Additionally, the UI is notified in the callback.
+  delegate_->AddItemToPersistentStore(download);
+}
+
 // SavePackage will call SavePageDownloadFinished upon completion/cancellation.
 // The history callback will call OnSavePageItemAddedToPersistentStore.
 // If the download finishes before the history callback,
@@ -1109,7 +1091,7 @@
   }
 }
 
-void DownloadManagerImpl::DownloadOpened(DownloadItem* download) {
+void DownloadManagerImpl::MarkDownloadOpened(DownloadItem* download) {
   delegate_->UpdateItemInPersistentStore(download);
   int num_unopened = 0;
   for (DownloadMap::iterator it = history_downloads_.begin();