Revert 102126 - Make cancel remove cancelled download from active queues at time of cancel.

content/browser/download/download_manager.cc

Index: content/browser/download/download_manager.cc
===================================================================
--- content/browser/download/download_manager.cc	(revision 102135)
+++ content/browser/download/download_manager.cc	(working copy)
@@ -112,7 +112,8 @@
       // from all queues.
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
-      download->Cancel();
+      download->Cancel(false);
+      delegate_->UpdateItemInPersistentStore(download);
     }
   }
 
@@ -274,7 +275,7 @@
     TabContents* contents = request_handle.GetTabContents();
 
     // |id_ptr| will be deleted in either FileSelected() or
-    // FileSelectionCanceled().
+    // FileSelectionCancelled().
     int32* id_ptr = new int32;
     *id_ptr = download_id;
 
@@ -504,19 +505,23 @@
   delegate_->UpdatePathForItemInPersistentStore(item, full_path);
 }
 
-void DownloadManager::DownloadStopped(DownloadItem* download) {
+void DownloadManager::CancelDownload(int32 download_id) {
+  DownloadItem* download = GetActiveDownload(download_id);
+  // A cancel at the right time could remove the download from the
+  // |active_downloads_| map before we get here.
+  if (!download)
+    return;
+
+  download->Cancel(true);
+}
+
+void DownloadManager::DownloadCancelledInternal(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  CHECK(ContainsKey(active_downloads_, download->id()));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  in_progress_.erase(download->id());
-  active_downloads_.erase(download->id());
-  UpdateDownloadProgress();  // Reflect removal from in_progress_.
-  if (download->db_handle() != DownloadItem::kUninitializedHandle)
-    delegate_->UpdateItemInPersistentStore(download);
-
+  RemoveFromActiveList(download);
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
   AssertQueueStateConsistent(download);
@@ -538,7 +543,9 @@
            << " size = " << size
            << " download = " << download->DebugString(true);
 
-  download->Interrupt(size, error);
+  RemoveFromActiveList(download);
+  download->Interrupted(size, error);
+  download->OffThreadCancel(file_manager_);
 }
 
 DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) {
@@ -555,6 +562,20 @@
   return download;
 }
 
+void DownloadManager::RemoveFromActiveList(DownloadItem* 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->db_handle() != DownloadItem::kUninitializedHandle) {
+    in_progress_.erase(download->id());
+    active_downloads_.erase(download->id());
+    UpdateDownloadProgress();  // Reflect removal from in_progress_.
+    delegate_->UpdateItemInPersistentStore(download);
+  }
+}
+
 void DownloadManager::UpdateDownloadProgress() {
   delegate_->DownloadProgressUpdated();
 }
@@ -584,10 +605,15 @@
   return num_deleted;
 }
 
-void DownloadManager::RemoveDownload(DownloadItem* download) {
-  // Make history update.  Ignores if db_handle isn't in history.
-  delegate_->RemoveItemFromPersistentStore(download->db_handle());
+void DownloadManager::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.
   int downloads_count = RemoveDownloadItems(DownloadVector(1, download));
   DCHECK_EQ(1, downloads_count);
@@ -736,7 +762,12 @@
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  download->Cancel();
+  // TODO(ahendrickson) -- This currently has no effect, as the download is
+  // not put on the active list until the file selection is complete.  Need
+  // to put it on the active list earlier in the process.
+  RemoveFromActiveList(download);
+
+  download->OffThreadCancel(file_manager_);
 }
 
 // Operations posted to us from the history service ----------------------------
@@ -807,22 +838,8 @@
                                                            int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download) {
-    // The download was cancelled while the persistent store was entering it.
-    // We resolve this race by turning around and deleting it in the
-    // persistent store (implicitly treating it as a failure in download
-    // initiation, which is appropriate as the only places the cancel could
-    // have come from were in resolving issues (like the file name) which
-    // we need to have resolved for persistent store insertion).
-
-    // Make sure we haven't already been shutdown (the callback raced
-    // with shutdown), as that would mean that we no longer have access
-    // to the persistent store.  In that case, the history will be cleaned up
-    // on next persistent store query.
-    if (shutdown_needed_)
-      delegate_->RemoveItemFromPersistentStore(db_handle);
+  if (!download)
     return;
-  }
 
   VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
            << " download_id = " << download_id
@@ -838,10 +855,27 @@
 
   CHECK(!ContainsKey(history_downloads_, db_handle));
 
-  CHECK(download->IsInProgress());
   AddDownloadItemToHistory(download, db_handle);
 
-  MaybeCompleteDownload(download);
+  // 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 (download->IsInProgress()) {
+    MaybeCompleteDownload(download);
+  } else {
+    // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
+    // is fixed.
+    CHECK(download->IsCancelled())
+        << " download = " << download->DebugString(true);
+    in_progress_.erase(download_id);
+    active_downloads_.erase(download_id);
+    delegate_->UpdateItemInPersistentStore(download);
+    download->UpdateObservers();
+  }
 }
 
 void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
@@ -880,20 +914,8 @@
   return NULL;
 }
 
-void DownloadManager::GetInProgressDownloads(
-    std::vector<DownloadItem*>* result) {
-  DCHECK(result);
-
-  for (DownloadMap::iterator it = active_downloads_.begin();
-       it != active_downloads_.end(); ++it) {
-    result->push_back(it->second);
-  }
-}
-
 DownloadItem* DownloadManager::GetActiveDownloadItem(int download_id) {
-  if (!ContainsKey(active_downloads_, download_id))
-    return NULL;
-
+  DCHECK(ContainsKey(active_downloads_, download_id));
   DownloadItem* download = active_downloads_[download_id];
   DCHECK(download != NULL);
   return download;