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

chrome/browser/download/download_status_updater.cc

Index: chrome/browser/download/download_status_updater.cc
diff --git a/chrome/browser/download/download_status_updater.cc b/chrome/browser/download/download_status_updater.cc
index 817e4184268926339bd599dc431f2ca5429911e2..a8dde9bc561020cd4e156d21df5ccf8791f5e5dd 100644
--- a/chrome/browser/download/download_status_updater.cc
+++ b/chrome/browser/download/download_status_updater.cc
@@ -68,8 +68,6 @@ void DownloadStatusUpdater::ManagerGoingDown(
   DCHECK(ContainsKey(managers_, manager));
   managers_.erase(manager);
   manager->RemoveObserver(this);
-  // Item removal will be handled in response to DownloadItem REMOVING
-  // notification (in the !IN_PROGRESS conditional branch in UpdateItem).
 }
 
 // Methods inherited from content::DownloadItem::Observer.
@@ -79,6 +77,15 @@ void DownloadStatusUpdater::OnDownloadUpdated(
   UpdateAppIconDownloadProgress();
 }
 
+void DownloadStatusUpdater::OnDownloadDestroyed(
+    content::DownloadItem* download) {
+  if (ContainsKey(items_, download)) {
+    items_.erase(download);
+    download->RemoveObserver(this);
+  }
+  UpdateAppIconDownloadProgress();

[Randy Smith (Not in Mondays), 2012/08/09 17:03:44]

I don't actually think you need this--OnDownloadDestroyed() is about a
transition of the object, not of the conceptual download, so there shouldn't be
any change that UAIDP cares about.  I don't object to it, just don't think it
needs to be here.

[benjhayden, 2012/08/09 18:35:06]

I don't know this code, but it appears that UAIDP uses items_ to calculate
everything, so failing to call UAIDP every time that items_ changes would cause
the app icon to fall out of sync.
Also, OnDownloadDestroyed is exactly analogous to REMOVING, so if it was
unnecessary to call UAIDP when REMOVING, then OnDownloadUpdated should have said
if (state != REMOVING). So, at worst, this is preserving existing functionality,
and at best this is necessary for keeping the app icon up to date. If this
functionality should not be preserved, then shouldn't that happen in a separate
CL?

[Randy Smith (Not in Mondays), 2012/08/09 20:27:32]

The major issue here is my distinction between the abstract download and the
concrete item.  Changes in the abstract download are reflected in the state. 
Destruction of the item is reflected via OnDownloadDestroyed(). 
UpdateAppIconDownloadProgress only cares about changes in the abstract download,
specifically in this case transitions out of IN_PROGRESS, which are caught
elsewhere.  I want to hook OnDownloadDestroyed() because I'm a paranoid person
and it's the notification "we're destroying the download", and we might
(accidentally or semantically) change the logic at some point so that the
download gets destroyed straight from IN_PROGRESS (though I think that would be
a bad idea) and I'd like to avoid use-after-free in that case.  

But it's not particularly important.  This code should never be called, so we
could conceivably put in a DCHECK.  But I wanted it there because
OnDownloadDestroyed() is how we officially notify that we're about to destroy
the download, and it strikes me as good programming to always release pointers
if held in OnDownloadDestroyed().  UpdateAppIconDownloadProgress() doesn't hurt,
and will help in some of the "can't happen" situations I'm targeting by asking
for this at all.

+}
+
 void DownloadStatusUpdater::OnDownloadOpened(content::DownloadItem* download) {
 }