Reland DownloadItem::Observer::OnDownloadDestroyed() replaces DownloadItem::REMOVING

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index e0f441b949876d3d7db5aa3a5d29779ef988bc24..53ab80859f5cdaf5241c4b4b9bafaad6ad8bf870 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -298,10 +298,10 @@ void DownloadManagerImpl::Shutdown() {
   // 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);
+  downloads_to_delete.swap(downloads_);

[Randy Smith (Not in Mondays), 2012/07/11 17:55:37]

Is there a reason for moving this line?  (I don't care, but I don't want to miss
something I should review.)

[benjhayden, 2012/07/13 20:03:17]

~DownloadItemImpl() calls AssertStateConsistent() which checks that the download
is still in |downloads_|.

 
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
@@ -615,13 +615,6 @@ void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
 }
 
 void DownloadManagerImpl::AssertStateConsistent(DownloadItem* 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();
@@ -798,14 +791,15 @@ int DownloadManagerImpl::RemoveDownloadItems(
   if (pending_deletes.empty())
     return 0;
 
+  std::vector<int32> deleting_ids;
+
   // Delete from internal maps.
   for (DownloadVector::const_iterator it = pending_deletes.begin();
       it != pending_deletes.end();
       ++it) {
     DownloadItem* download = *it;
     DCHECK(download);
-    save_page_downloads_.erase(download->GetId());
-    downloads_.erase(download->GetId());
+    deleting_ids.push_back(download->GetId());
   }
 
   // Tell observers to refresh their views.
@@ -814,6 +808,14 @@ int DownloadManagerImpl::RemoveDownloadItems(
   // Delete the download items themselves.
   const int num_deleted = static_cast<int>(pending_deletes.size());
   STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
+
+  for (std::vector<int32>::const_iterator it = deleting_ids.begin();
+       it != deleting_ids.end();
+       ++it) {
+    save_page_downloads_.erase(*it);
+    downloads_.erase(*it);
+  }
+

[Randy Smith (Not in Mondays), 2012/07/11 17:55:37]

Why the shift?  This seems more complicated.  Simpler would be to just put a
"delete download" into the loop, above, and just delete everything at once.

[benjhayden, 2012/07/13 20:03:17]

Done.

   return num_deleted;
 }