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

chrome/browser/download/download_status_updater.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 "chrome/browser/download/download_status_updater.h"
 
 #include <vector>
 
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 50 matching lines @@
   }
 
   UpdateAppIconDownloadProgress();
 }
 
 void DownloadStatusUpdater::ManagerGoingDown(
     content::DownloadManager* manager) {
   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.
 void DownloadStatusUpdater::OnDownloadUpdated(
     content::DownloadItem* download) {
   UpdateItem(download);
   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) {
 }
 
 void DownloadStatusUpdater::UpdateAppIconDownloadProgress() {
   float progress = 0;
   int download_count = 0;
   bool progress_known = GetProgress(&progress, &download_count);
   download_util::UpdateAppIconDownloadProgress(download_count,
                                                progress_known,
                                                progress);
 }
 
 // React to a transition that a download associated with one of our
 // download managers has made.  Our goal is to have only IN_PROGRESS
 // items on our set list, as they're the only ones that have relevance
 // to GetProgress() return values.
 void DownloadStatusUpdater::UpdateItem(content::DownloadItem* download) {
   if (download->GetState() == content::DownloadItem::IN_PROGRESS) {
     if (!ContainsKey(items_, download)) {
       items_.insert(download);
       download->AddObserver(this);
     }
   } else {
     if (ContainsKey(items_, download)) {
       items_.erase(download);
       download->RemoveObserver(this);
     }
   }
 }