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

Reviewers:
Ben Goodger: chrome/browser/ui/views/download/download_item_view.*, chrome/browser/plugin_installer.*, chrome/browser/automation/automation_provider.cc
rdsmith, asanka: all
Nico (thakis): chrome/browser/ui/cocoa/download/download_item_mac.*
Achuith: chrome/browser/chromeos/gdata/gdata_download_observer.*
Evan (estade): chrome/browser/ui/gtk/download/download_item_gtk.*, chrome/browser/ui/webui/downloads_dom_handler.*
Scott (sky): chrome/browser/history/history_unittest.cc
Aaron (aa): chrome/browser/extensions/webstore_installer.*

Add DownloadItem::Observer::OnDownloadRemoved() to signal when a download is being removed from history.
Make chrome.downloads.onErased trigger from DownloadItem::Observer::OnDownloadRemoved() so that extensions don't think that all downloads are being erased just because the browser is closing.

Review URL: https://chromiumcodereview.appspot.com/10704026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149794 0039d316-1c4b-4281-b951-d872f2087c98

33 files changed, 180 insertions, 155 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index b2ea3ff..78f5740 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -304,7 +304,6 @@ DictionaryValue* AutomationProvider::GetDictionaryFromDownloadItem(
   std::map<DownloadItem::DownloadState, std::string> state_to_string;
   state_to_string[DownloadItem::IN_PROGRESS] = std::string("IN_PROGRESS");
   state_to_string[DownloadItem::CANCELLED] = std::string("CANCELLED");
-  state_to_string[DownloadItem::REMOVING] = std::string("REMOVING");
   state_to_string[DownloadItem::INTERRUPTED] = std::string("INTERRUPTED");
   state_to_string[DownloadItem::COMPLETE] = std::string("COMPLETE");
 
diff --git a/chrome/browser/chromeos/gdata/gdata_download_observer.cc b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
index c6299d0..684d888 100644
--- a/chrome/browser/chromeos/gdata/gdata_download_observer.cc
+++ b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
@@ -363,7 +363,6 @@ void GDataDownloadObserver::OnDownloadUpdated(DownloadItem* download) {
 
     // TODO(achuith): Stop the pending upload and delete the file.
     case DownloadItem::CANCELLED:
-    case DownloadItem::REMOVING:
     case DownloadItem::INTERRUPTED:
       RemovePendingDownload(download);
       break;
@@ -375,6 +374,10 @@ void GDataDownloadObserver::OnDownloadUpdated(DownloadItem* download) {
   DVLOG(1) << "Number of pending downloads=" << pending_downloads_.size();
 }
 
+void GDataDownloadObserver::OnDownloadDestroyed(DownloadItem* download) {
+  RemovePendingDownload(download);
+}
+
 void GDataDownloadObserver::AddPendingDownload(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
diff --git a/chrome/browser/chromeos/gdata/gdata_download_observer.h b/chrome/browser/chromeos/gdata/gdata_download_observer.h
index 51bb00b..5799336 100644
--- a/chrome/browser/chromeos/gdata/gdata_download_observer.h
+++ b/chrome/browser/chromeos/gdata/gdata_download_observer.h
@@ -91,7 +91,7 @@ class GDataDownloadObserver : public content::DownloadManager::Observer,
 
   // DownloadItem overrides.
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE {}
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // Adds/Removes |download| to pending_downloads_.
   // Also start/stop observing |download|.
diff --git a/chrome/browser/download/download_item_model.cc b/chrome/browser/download/download_item_model.cc
index b636377..b7912e9 100644
--- a/chrome/browser/download/download_item_model.cc
+++ b/chrome/browser/download/download_item_model.cc
@@ -128,8 +128,6 @@ string16 DownloadItemModel::GetStatusText() {
     case DownloadItem::CANCELLED:
       status_text = l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_CANCELLED);
       break;
-    case DownloadItem::REMOVING:
-      break;
     case DownloadItem::INTERRUPTED:
       reason = download_->GetLastReason();
       status_text = InterruptReasonStatusMessage(reason);
diff --git a/chrome/browser/download/download_path_reservation_tracker.cc b/chrome/browser/download/download_path_reservation_tracker.cc
index cec8173..2f0e3d8 100644
--- a/chrome/browser/download/download_path_reservation_tracker.cc
+++ b/chrome/browser/download/download_path_reservation_tracker.cc
@@ -36,7 +36,7 @@ class DownloadItemObserver : public DownloadItem::Observer {
 
   // DownloadItem::Observer
   virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(DownloadItem* download) OVERRIDE;
 
   DownloadItem& download_item_;
 
@@ -89,10 +89,6 @@ void DownloadItemObserver::OnDownloadUpdated(DownloadItem* download) {
     case DownloadItem::CANCELLED:
       // We no longer need the reservation if the download is being removed.
 
-    case DownloadItem::REMOVING:
-      // Ditto, but this case shouldn't happen in practice. We should have
-      // received another notification beforehand.
-
     case DownloadItem::INTERRUPTED:
       // The download filename will need to be re-generated when the download is
       // restarted. Holding on to the reservation now would prevent the name
@@ -108,10 +104,11 @@ void DownloadItemObserver::OnDownloadUpdated(DownloadItem* download) {
   }
 }
 
-void DownloadItemObserver::OnDownloadOpened(DownloadItem* download) {
-  // We shouldn't be tracking reservations for a download that has been
-  // externally opened. The tracker should have detached itself when the
-  // download was complete.
+void DownloadItemObserver::OnDownloadDestroyed(DownloadItem* download) {
+  // This shouldn't happen. We should catch either COMPLETE, CANCELLED, or
+  // INTERRUPTED first.
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, revoke_callback_);
+  delete this;
 }
 
 }  // namespace
diff --git a/chrome/browser/download/download_path_reservation_tracker_unittest.cc b/chrome/browser/download/download_path_reservation_tracker_unittest.cc
index aceaf3b..0abe714 100644
--- a/chrome/browser/download/download_path_reservation_tracker_unittest.cc
+++ b/chrome/browser/download/download_path_reservation_tracker_unittest.cc
@@ -33,8 +33,8 @@ class FakeDownloadItem : public MockDownloadItem {
   explicit FakeDownloadItem()
       : state_(IN_PROGRESS) {
   }
-  ~FakeDownloadItem() {
-    SetState(REMOVING);
+  virtual ~FakeDownloadItem() {
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
     EXPECT_EQ(0u, observers_.size());
   }
   virtual void AddObserver(Observer* observer) OVERRIDE {
diff --git a/chrome/browser/download/download_test_observer.cc b/chrome/browser/download/download_test_observer.cc
index 3f5dc5f..dd98422 100644
--- a/chrome/browser/download/download_test_observer.cc
+++ b/chrome/browser/download/download_test_observer.cc
@@ -85,17 +85,15 @@ bool DownloadTestObserver::IsFinished() const {
           wait_count_);
 }
 
-void DownloadTestObserver::OnDownloadUpdated(DownloadItem* download) {
-  // The REMOVING state indicates that the download is being destroyed.
+void DownloadTestObserver::OnDownloadDestroyed(DownloadItem* download) {
   // Stop observing.  Do not do anything with it, as it is about to be gone.
-  if (download->GetState() == DownloadItem::REMOVING) {
-    DownloadSet::iterator it = downloads_observed_.find(download);
-    ASSERT_TRUE(it != downloads_observed_.end());
-    downloads_observed_.erase(it);
-    download->RemoveObserver(this);
-    return;
-  }
+  DownloadSet::iterator it = downloads_observed_.find(download);
+  ASSERT_TRUE(it != downloads_observed_.end());
+  downloads_observed_.erase(it);
+  download->RemoveObserver(this);
+}
 
+void DownloadTestObserver::OnDownloadUpdated(DownloadItem* download) {
   // Real UI code gets the user's response after returning from the observer.
   if (download->GetSafetyState() == DownloadItem::DANGEROUS &&
       !ContainsKey(dangerous_downloads_seen_, download->GetId())) {
@@ -268,17 +266,15 @@ void DownloadTestFlushObserver::ModelChanged(DownloadManager* manager) {
   CheckDownloadsInProgress(true);
 }
 
-void DownloadTestFlushObserver::OnDownloadUpdated(DownloadItem* download) {
-  // The REMOVING state indicates that the download is being destroyed.
+void DownloadTestFlushObserver::OnDownloadDestroyed(DownloadItem* download) {
   // Stop observing.  Do not do anything with it, as it is about to be gone.
-  if (download->GetState() == DownloadItem::REMOVING) {
-    DownloadSet::iterator it = downloads_observed_.find(download);
-    ASSERT_TRUE(it != downloads_observed_.end());
-    downloads_observed_.erase(it);
-    download->RemoveObserver(this);
-    return;
-  }
+  DownloadSet::iterator it = downloads_observed_.find(download);
+  ASSERT_TRUE(it != downloads_observed_.end());
+  downloads_observed_.erase(it);
+  download->RemoveObserver(this);
+}
 
+void DownloadTestFlushObserver::OnDownloadUpdated(DownloadItem* download) {
   // No change in DownloadItem set on manager.
   CheckDownloadsInProgress(false);
 }
diff --git a/chrome/browser/download/download_test_observer.h b/chrome/browser/download/download_test_observer.h
index 76792a4..160c80d 100644
--- a/chrome/browser/download/download_test_observer.h
+++ b/chrome/browser/download/download_test_observer.h
@@ -60,7 +60,7 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
 
   // content::DownloadItem::Observer
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE {}
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // content::DownloadManager::Observer
   virtual void ModelChanged(content::DownloadManager* manager) OVERRIDE;
@@ -202,7 +202,7 @@ class DownloadTestFlushObserver
 
   // DownloadItem observer methods.
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE {}
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
  protected:
   friend class base::RefCountedThreadSafe<DownloadTestFlushObserver>;
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index 10f2045..b5a6a2e 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -506,8 +506,6 @@ DictionaryValue* CreateDownloadItemValue(DownloadItem* download, int id) {
       file_value->SetString("state", "DANGEROUS");
     else
       file_value->SetString("state", "COMPLETE");
-  } else if (download->GetState() == DownloadItem::REMOVING) {
-    file_value->SetString("state", "REMOVING");
   } else {
     NOTREACHED() << "state undefined";
   }
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index bbfe75d..9ccd22c 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -139,7 +139,6 @@ const char* kStateStrings[] = {
   kStateInProgress,
   kStateComplete,
   kStateInterrupted,
-  NULL,
   kStateInterrupted,
 };
 COMPILE_ASSERT(arraysize(kStateStrings) == DownloadItem::MAX_DOWNLOAD_STATE,
@@ -164,7 +163,6 @@ const char* StateString(DownloadItem::DownloadState state) {
   DCHECK(state >= 0);
   DCHECK(state < static_cast<DownloadItem::DownloadState>(
       arraysize(kStateStrings)));
-  DCHECK(state != DownloadItem::REMOVING);
   return kStateStrings[state];
 }
 
@@ -820,24 +818,31 @@ ExtensionDownloadsEventRouter::OnChangedStat::~OnChangedStat() {
     UMA_HISTOGRAM_PERCENTAGE("Download.OnChanged", (fires * 100 / total));
 }
 
-void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
+void ExtensionDownloadsEventRouter::OnDownloadDestroyed(DownloadItem* item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   int download_id = item->GetId();
-  if (item->GetState() == DownloadItem::REMOVING) {
-    // The REMOVING state indicates that this item is being erased.
-    // Let's unregister as an observer so that we don't see any more updates
-    // from it, dispatch the onErased event, and remove its json and is
-    // OnChangedStat from our maps.
-    downloads_.erase(download_id);
-    item->RemoveObserver(this);
-    DispatchEvent(extensions::event_names::kOnDownloadErased,
-                  base::Value::CreateIntegerValue(download_id));
-    delete item_jsons_[download_id];
-    item_jsons_.erase(download_id);
-    delete on_changed_stats_[download_id];
-    on_changed_stats_.erase(download_id);
+  downloads_.erase(download_id);
+  item->RemoveObserver(this);
+  delete item_jsons_[download_id];
+  item_jsons_.erase(download_id);
+  delete on_changed_stats_[download_id];
+  on_changed_stats_.erase(download_id);
+}
+
+void ExtensionDownloadsEventRouter::OnDownloadRemoved(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!profile_)
     return;
-  }
+  int download_id = item->GetId();
+  DispatchEvent(extensions::event_names::kOnDownloadErased,
+                base::Value::CreateIntegerValue(download_id));
+}
+
+void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!profile_)
+    return;
+  int download_id = item->GetId();
 
   base::DictionaryValue* old_json = item_jsons_[download_id];
   scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(item));
@@ -886,10 +891,6 @@ void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
   item_jsons_[download_id]->Swap(new_json.get());
 }
 
-void ExtensionDownloadsEventRouter::OnDownloadOpened(DownloadItem* item) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-}
-
 void ExtensionDownloadsEventRouter::OnDownloadCreated(
     DownloadManager* manager, DownloadItem* download_item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -913,6 +914,7 @@ void ExtensionDownloadsEventRouter::ManagerGoingDown(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   manager_->RemoveObserver(this);
   manager_ = NULL;
+  profile_ = NULL;
 }
 
 void ExtensionDownloadsEventRouter::DispatchEvent(
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.h b/chrome/browser/extensions/api/downloads/downloads_api.h
index 9461dd4..178ff7a 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.h
+++ b/chrome/browser/extensions/api/downloads/downloads_api.h
@@ -226,7 +226,8 @@ class ExtensionDownloadsEventRouter : public content::DownloadManager::Observer,
 
   // content::DownloadItem::Observer
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadRemoved(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // Used for testing.
   struct DownloadsNotificationSource {
diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
index 1dfb39f..13d9852 100644
--- a/chrome/browser/extensions/webstore_installer.cc
+++ b/chrome/browser/extensions/webstore_installer.cc
@@ -296,10 +296,6 @@ void WebstoreInstaller::OnDownloadUpdated(DownloadItem* download) {
     case DownloadItem::INTERRUPTED:
       ReportFailure(kDownloadInterruptedError);
       break;
-    case DownloadItem::REMOVING:
-      download_item_->RemoveObserver(this);
-      download_item_ = NULL;
-      break;
     case DownloadItem::COMPLETE:
       // Wait for other notifications if the download is really an extension.
       if (!download_crx_util::IsExtensionDownload(*download))
@@ -311,8 +307,10 @@ void WebstoreInstaller::OnDownloadUpdated(DownloadItem* download) {
   }
 }
 
-void WebstoreInstaller::OnDownloadOpened(DownloadItem* download) {
+void WebstoreInstaller::OnDownloadDestroyed(DownloadItem* download) {
   CHECK_EQ(download_item_, download);
+  download_item_->RemoveObserver(this);
+  download_item_ = NULL;
 }
 
 void WebstoreInstaller::StartDownload(const FilePath& file) {
diff --git a/chrome/browser/extensions/webstore_installer.h b/chrome/browser/extensions/webstore_installer.h
index aaf95fb..8636c5b 100644
--- a/chrome/browser/extensions/webstore_installer.h
+++ b/chrome/browser/extensions/webstore_installer.h
@@ -137,7 +137,7 @@ class WebstoreInstaller :public content::NotificationObserver,
 
   // DownloadItem::Observer implementation:
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // Starts downloading the extension to |file_path|.
   void StartDownload(const FilePath& file_path);
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index a83c595..7bf7be9 100644
--- a/chrome/browser/history/history_unittest.cc
+++ b/chrome/browser/history/history_unittest.cc
@@ -323,7 +323,7 @@ TEST_F(HistoryTest, ClearBrowsingData_Downloads) {
   EXPECT_EQ(0U, downloads.size());
 
   // Keep track of these as we need to update them later during the test.
-  DownloadID in_progress, removing;
+  DownloadID in_progress;
 
   // Create one with a 0 time.
   EXPECT_NE(0, AddDownload(DownloadItem::COMPLETE, Time()));
@@ -336,23 +336,22 @@ TEST_F(HistoryTest, ClearBrowsingData_Downloads) {
   EXPECT_NE(0, in_progress = AddDownload(DownloadItem::IN_PROGRESS, month_ago));
   EXPECT_NE(0, AddDownload(DownloadItem::CANCELLED, month_ago));
   EXPECT_NE(0, AddDownload(DownloadItem::INTERRUPTED, month_ago));
-  EXPECT_NE(0, removing = AddDownload(DownloadItem::REMOVING, month_ago));
 
   // Test to see if inserts worked.
   db_->QueryDownloads(&downloads);
-  EXPECT_EQ(9U, downloads.size());
+  EXPECT_EQ(8U, downloads.size());
 
   // Try removing from current timestamp. This should delete the one in the
   // future and one very recent one.
   db_->RemoveDownloadsBetween(now, Time());
   db_->QueryDownloads(&downloads);
-  EXPECT_EQ(7U, downloads.size());
+  EXPECT_EQ(6U, downloads.size());
 
   // Try removing from two months ago. This should not delete items that are
   // 'in progress' or in 'removing' state.
   db_->RemoveDownloadsBetween(now - TimeDelta::FromDays(60), Time());
   db_->QueryDownloads(&downloads);
-  EXPECT_EQ(3U, downloads.size());
+  EXPECT_EQ(2U, downloads.size());
 
   // Download manager converts to TimeT, which is lossy, so we do the same
   // for comparison.
@@ -364,9 +363,6 @@ TEST_F(HistoryTest, ClearBrowsingData_Downloads) {
   EXPECT_EQ(DownloadItem::IN_PROGRESS, downloads[1].state);
   EXPECT_EQ(month_ago_lossy.ToInternalValue(),
             downloads[1].start_time.ToInternalValue());
-  EXPECT_EQ(DownloadItem::REMOVING, downloads[2].state);
-  EXPECT_EQ(month_ago_lossy.ToInternalValue(),
-            downloads[2].start_time.ToInternalValue());
 
   // Change state so we can delete the downloads.
   DownloadPersistentStoreInfo data;
@@ -377,7 +373,6 @@ TEST_F(HistoryTest, ClearBrowsingData_Downloads) {
   data.db_handle = in_progress;
   EXPECT_TRUE(db_->UpdateDownload(data));
   data.state = DownloadItem::CANCELLED;
-  data.db_handle = removing;
   EXPECT_TRUE(db_->UpdateDownload(data));
 
   // Try removing from Time=0. This should delete all.
diff --git a/chrome/browser/plugin_installer.cc b/chrome/browser/plugin_installer.cc
index f795662..bcfbecc 100644
--- a/chrome/browser/plugin_installer.cc
+++ b/chrome/browser/plugin_installer.cc
@@ -150,11 +150,6 @@ void PluginInstaller::OnDownloadUpdated(DownloadItem* download) {
       DownloadCancelled();
       break;
     }
-    case DownloadItem::REMOVING: {
-      DCHECK_EQ(INSTALLER_STATE_DOWNLOADING, state_);
-      state_ = INSTALLER_STATE_IDLE;
-      break;
-    }
     case DownloadItem::INTERRUPTED: {
       content::DownloadInterruptReason reason = download->GetLastReason();
       DownloadError(content::InterruptReasonDebugString(reason));
@@ -168,7 +163,10 @@ void PluginInstaller::OnDownloadUpdated(DownloadItem* download) {
   download->RemoveObserver(this);
 }
 
-void PluginInstaller::OnDownloadOpened(DownloadItem* download) {
+void PluginInstaller::OnDownloadDestroyed(DownloadItem* download) {
+  DCHECK_EQ(INSTALLER_STATE_DOWNLOADING, state_);
+  state_ = INSTALLER_STATE_IDLE;
+  download->RemoveObserver(this);
 }
 
 void PluginInstaller::AddObserver(PluginInstallerObserver* observer) {
diff --git a/chrome/browser/plugin_installer.h b/chrome/browser/plugin_installer.h
index 00a2af1..bd2606d 100644
--- a/chrome/browser/plugin_installer.h
+++ b/chrome/browser/plugin_installer.h
@@ -48,8 +48,7 @@ class PluginInstaller : public content::DownloadItem::Observer {
   virtual ~PluginInstaller();
 
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   void AddObserver(PluginInstallerObserver* observer);
   void RemoveObserver(PluginInstallerObserver* observer);
diff --git a/chrome/browser/ui/cocoa/download/download_item_mac.h b/chrome/browser/ui/cocoa/download/download_item_mac.h
index 4ead6a5..2403ac5 100644
--- a/chrome/browser/ui/cocoa/download/download_item_mac.h
+++ b/chrome/browser/ui/cocoa/download/download_item_mac.h
@@ -37,6 +37,7 @@ class DownloadItemMac : content::DownloadItem::Observer {
   // content::DownloadItem::Observer implementation
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
   virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   BaseDownloadItemModel* download_model() { return download_model_.get(); }
 
diff --git a/chrome/browser/ui/cocoa/download/download_item_mac.mm b/chrome/browser/ui/cocoa/download/download_item_mac.mm
index b5dc9b6..3a622b4 100644
--- a/chrome/browser/ui/cocoa/download/download_item_mac.mm
+++ b/chrome/browser/ui/cocoa/download/download_item_mac.mm
@@ -46,9 +46,6 @@ void DownloadItemMac::OnDownloadUpdated(content::DownloadItem* download) {
   }
 
   switch (download->GetState()) {
-    case DownloadItem::REMOVING:
-      [item_controller_ remove];  // We're deleted now!
-      break;
     case DownloadItem::COMPLETE:
       if (download->GetAutoOpened()) {
         [item_controller_ remove];  // We're deleted now!
@@ -69,6 +66,10 @@ void DownloadItemMac::OnDownloadUpdated(content::DownloadItem* download) {
   }
 }
 
+void DownloadItemMac::OnDownloadDestroyed(content::DownloadItem* download) {
+  [item_controller_ remove];  // We're deleted now!
+}
+
 void DownloadItemMac::OnDownloadOpened(content::DownloadItem* download) {
   DCHECK_EQ(download, download_model_->download());
   [item_controller_ downloadWasOpened];
diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.cc b/chrome/browser/ui/gtk/download/download_item_gtk.cc
index 35118a2..b10831c 100644
--- a/chrome/browser/ui/gtk/download/download_item_gtk.cc
+++ b/chrome/browser/ui/gtk/download/download_item_gtk.cc
@@ -326,9 +326,6 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download) {
   }
 
   switch (download->GetState()) {
-    case DownloadItem::REMOVING:
-      parent_shelf_->RemoveDownloadItem(this);  // This will delete us!
-      return;
     case DownloadItem::CANCELLED:
       StopDownloadProgress();
       gtk_widget_queue_draw(progress_area_.get());
@@ -372,6 +369,11 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download) {
   UpdateStatusLabel(status_text_);
 }
 
+void DownloadItemGtk::OnDownloadDestroyed(DownloadItem* download) {
+  parent_shelf_->RemoveDownloadItem(this);
+  // This will delete us!
+}
+
 void DownloadItemGtk::AnimationProgressed(const ui::Animation* animation) {
   if (animation == &complete_animation_) {
     gtk_widget_queue_draw(progress_area_.get());
diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.h b/chrome/browser/ui/gtk/download/download_item_gtk.h
index cc0e96b..9b0b33f 100644
--- a/chrome/browser/ui/gtk/download/download_item_gtk.h
+++ b/chrome/browser/ui/gtk/download/download_item_gtk.h
@@ -50,6 +50,7 @@ class DownloadItemGtk : public content::DownloadItem::Observer,
 
   // content::DownloadItem::Observer implementation.
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
   virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
 
   // ui::AnimationDelegate implementation.
diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc
index cd9a253..988f4dc 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -297,9 +297,6 @@ void DownloadItemView::OnDownloadUpdated(DownloadItem* download) {
         StopDownloadProgress();
         LoadIcon();
         break;
-      case DownloadItem::REMOVING:
-        parent_->RemoveDownloadView(this);  // This will delete us!
-        return;
       default:
         NOTREACHED();
     }
@@ -320,6 +317,10 @@ void DownloadItemView::OnDownloadUpdated(DownloadItem* download) {
   parent()->SchedulePaint();
 }
 
+void DownloadItemView::OnDownloadDestroyed(DownloadItem* download) {
+  parent_->RemoveDownloadView(this);  // This will delete us!
+}
+
 void DownloadItemView::OnDownloadOpened(DownloadItem* download) {
   disabled_while_opening_ = true;
   SetEnabled(false);
diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h
index ba49d29..081f0fc 100644
--- a/chrome/browser/ui/views/download/download_item_view.h
+++ b/chrome/browser/ui/views/download/download_item_view.h
@@ -75,9 +75,10 @@ class DownloadItemView : public views::ButtonListener,
   // Returns the DownloadItem model object belonging to this item.
   content::DownloadItem* download() const { return download_; }
 
-  // DownloadObserver method
+  // DownloadItem::Observer methods
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
   virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // Overridden from views::View:
   virtual void Layout() OVERRIDE;
diff --git a/chrome/browser/ui/webui/downloads_dom_handler.cc b/chrome/browser/ui/webui/downloads_dom_handler.cc
index 514ae7a..3c9f22d 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.cc
+++ b/chrome/browser/ui/webui/downloads_dom_handler.cc
@@ -179,14 +179,6 @@ void DownloadsDOMHandler::OnDownloadUpdated(content::DownloadItem* download) {
   if (it == download_items_.end())
     return;
 
-  if (download->GetState() == content::DownloadItem::REMOVING) {
-    (*it)->RemoveObserver(this);
-    *it = NULL;
-    // A later ModelChanged() notification will change the WebUI's
-    // view of the downloads list.
-    return;
-  }
-
   const int id = static_cast<int>(it - download_items_.begin());
 
   ListValue results_value;
@@ -194,6 +186,17 @@ void DownloadsDOMHandler::OnDownloadUpdated(content::DownloadItem* download) {
   web_ui()->CallJavascriptFunction("downloadUpdated", results_value);
 }
 
+void DownloadsDOMHandler::OnDownloadDestroyed(
+    content::DownloadItem* download) {
+  download->RemoveObserver(this);
+  OrderedDownloads::iterator it = std::find(download_items_.begin(),
+                                            download_items_.end(),
+                                            download);
+  *it = NULL;
+  // A later ModelChanged() notification will change the WebUI's
+  // view of the downloads list.
+}
+
 // A download has started or been deleted. Query our DownloadManager for the
 // current set of downloads.
 void DownloadsDOMHandler::ModelChanged(content::DownloadManager* manager) {
diff --git a/chrome/browser/ui/webui/downloads_dom_handler.h b/chrome/browser/ui/webui/downloads_dom_handler.h
index 2cd3ec4..d57df0a 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.h
+++ b/chrome/browser/ui/webui/downloads_dom_handler.h
@@ -33,7 +33,7 @@ class DownloadsDOMHandler : public content::WebUIMessageHandler,
 
   // content::DownloadItem::Observer interface
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE { }
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // content::DownloadManager::Observer interface
   virtual void ModelChanged(content::DownloadManager* manager) OVERRIDE;
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index e7eb3f3..cdabec15 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -87,8 +87,6 @@ const char* DebugDownloadStateString(DownloadItem::DownloadState state) {
       return "COMPLETE";
     case DownloadItem::CANCELLED:
       return "CANCELLED";
-    case DownloadItem::REMOVING:
-      return "REMOVING";
     case DownloadItem::INTERRUPTED:
       return "INTERRUPTED";
     default:
@@ -299,8 +297,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
 
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  TransitionTo(REMOVING);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
   STLDeleteContainerPairSecondPointers(
       external_data_map_.begin(), external_data_map_.end());
   delegate_->AssertStateConsistent(this);
@@ -654,11 +651,15 @@ void DownloadItemImpl::Remove() {
   Cancel(true);
   delegate_->AssertStateConsistent(this);
 
-  TransitionTo(REMOVING);
+  NotifyRemoved();
   delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }
 
+void DownloadItemImpl::NotifyRemoved() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadRemoved(this));
+}
+
 bool DownloadItemImpl::TimeRemaining(base::TimeDelta* remaining) const {
   if (total_bytes_ <= 0)
     return false;  // We never received the content_length for this download.
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 1b4a334..da652e7 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -115,6 +115,9 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   // Called by SavePackage to set the total number of bytes on the item.
   virtual void SetTotalBytes(int64 total_bytes);
 
+  // Notify observers that this item is being removed by the user.
+  virtual void NotifyRemoved();
+
   // Overridden from DownloadItem.
   virtual void AddObserver(DownloadItem::Observer* observer) OVERRIDE;
   virtual void RemoveObserver(DownloadItem::Observer* observer) OVERRIDE;
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index 502e426..af7ec6b 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -131,16 +131,42 @@ class DownloadItemTest : public testing::Test {
  public:
   class MockObserver : public DownloadItem::Observer {
    public:
-    explicit MockObserver(DownloadItem* item) : item_(item), updated_(false) {
+    explicit MockObserver(DownloadItem* item)
+      : item_(item),
+        removed_(false),
+        destroyed_(false),
+        updated_(false) {
       item_->AddObserver(this);
     }
-    ~MockObserver() { item_->RemoveObserver(this); }
+
+    virtual ~MockObserver() {
+      if (item_) item_->RemoveObserver(this);
+    }
+
+    virtual void OnDownloadRemoved(DownloadItem* download) {
+      removed_ = true;
+    }
 
     virtual void OnDownloadUpdated(DownloadItem* download) {
       updated_ = true;
     }
 
-    virtual void OnDownloadOpened(DownloadItem* download) { }
+    virtual void OnDownloadOpened(DownloadItem* download) {
+    }
+
+    virtual void OnDownloadDestroyed(DownloadItem* download) {
+      destroyed_ = true;
+      item_->RemoveObserver(this);
+      item_ = NULL;
+    }
+
+    bool CheckRemoved() {
+      return removed_;
+    }
+
+    bool CheckDestroyed() {
+      return destroyed_;
+    }
 
     bool CheckUpdated() {
       bool was_updated = updated_;
@@ -150,6 +176,8 @@ class DownloadItemTest : public testing::Test {
 
    private:
     DownloadItem* item_;
+    bool removed_;
+    bool destroyed_;
     bool updated_;
   };
 
@@ -302,12 +330,21 @@ TEST_F(DownloadItemTest, NotificationAfterDelete) {
   ASSERT_TRUE(observer.CheckUpdated());
 }
 
+TEST_F(DownloadItemTest, NotificationAfterDestroyed) {
+  DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+  MockObserver observer(item);
+
+  DestroyDownloadItem(item);
+  ASSERT_TRUE(observer.CheckDestroyed());
+}
+
 TEST_F(DownloadItemTest, NotificationAfterRemove) {
   DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
   MockObserver observer(item);
 
   item->Remove();
   ASSERT_TRUE(observer.CheckUpdated());
+  ASSERT_TRUE(observer.CheckRemoved());
 }
 
 TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index c1e983e..e64283f 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -321,13 +321,9 @@ void DownloadManagerImpl::Shutdown() {
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
 
-  // 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);
+  STLDeleteValues(&downloads_);
+  downloads_.clear();
 
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
@@ -617,13 +613,6 @@ void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
 
 void DownloadManagerImpl::AssertStateConsistent(
     DownloadItemImpl* 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();
@@ -782,20 +771,16 @@ int DownloadManagerImpl::RemoveDownloadItems(
 
   // Delete from internal maps.
   for (DownloadItemImplVector::const_iterator it = pending_deletes.begin();
-      it != pending_deletes.end();
-      ++it) {
+       it != pending_deletes.end();
+       ++it) {
     DownloadItemImpl* download = *it;
     DCHECK(download);
-    downloads_.erase(download->GetId());
+    int32 download_id = download->GetId();
+    delete download;
+    downloads_.erase(download_id);
   }
-
-  // Tell observers to refresh their views.
   NotifyModelChanged();
-
-  // Delete the download items themselves.
-  const int num_deleted = static_cast<int>(pending_deletes.size());
-  STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
-  return num_deleted;
+  return static_cast<int>(pending_deletes.size());
 }
 
 void DownloadManagerImpl::DownloadRemoved(DownloadItemImpl* download) {
@@ -822,8 +807,6 @@ int DownloadManagerImpl::RemoveDownloadsBetween(base::Time remove_begin,
   if (delegate_)
     delegate_->RemoveItemsFromPersistentStoreBetween(remove_begin, remove_end);
 
-  // All downloads visible to the user will be in the history,
-  // so scan that map.
   DownloadItemImplVector pending_deletes;
   for (DownloadMap::const_iterator it = downloads_.begin();
       it != downloads_.end();
@@ -834,6 +817,7 @@ int DownloadManagerImpl::RemoveDownloadsBetween(base::Time remove_begin,
         (remove_end.is_null() || download->GetStartTime() < remove_end) &&
         (download->IsComplete() || download->IsCancelled())) {
       AssertStateConsistent(download);
+      download->NotifyRemoved();
       pending_deletes.push_back(download);
     }
   }
diff --git a/content/browser/download/drag_download_file.cc b/content/browser/download/drag_download_file.cc
index 38f0df0..2d76e6f 100644
--- a/content/browser/download/drag_download_file.cc
+++ b/content/browser/download/drag_download_file.cc
@@ -196,8 +196,7 @@ void DragDownloadFile::ModelChanged(DownloadManager* manager) {
 
 void DragDownloadFile::OnDownloadUpdated(content::DownloadItem* download) {
   AssertCurrentlyOnUIThread();
-  if (download->IsCancelled() ||
-      (download->GetState() == DownloadItem::REMOVING)) {
+  if (download->IsCancelled()) {
     RemoveObservers();
     DownloadCompleted(false);
   } else if (download->IsComplete()) {
@@ -207,6 +206,17 @@ void DragDownloadFile::OnDownloadUpdated(content::DownloadItem* download) {
   // Ignore other states.
 }
 
+// If the download completes or is cancelled, then OnDownloadUpdated() will
+// handle it and RemoveObserver() so that OnDownloadDestroyed is never called.
+// OnDownloadDestroyed is only called if OnDownloadUpdated() does not detect
+// completion or cancellation (in which cases it removes this observer).
+// TODO(benjhayden): Try to change this to NOTREACHED()?
+void DragDownloadFile::OnDownloadDestroyed(content::DownloadItem* download) {
+  AssertCurrentlyOnUIThread();
+  RemoveObservers();
+  DownloadCompleted(false);
+}
+
 void DragDownloadFile::AssertCurrentlyOnDragThread() {
   // Only do the check on Windows where two threads are involved.
 #if defined(OS_WIN)
diff --git a/content/browser/download/drag_download_file.h b/content/browser/download/drag_download_file.h
index 96a259b..19b771f 100644
--- a/content/browser/download/drag_download_file.h
+++ b/content/browser/download/drag_download_file.h
@@ -63,7 +63,7 @@ class DragDownloadFile
   // content::DownloadItem::Observer methods.
   // Called on UI thread.
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE { }
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
  private:
   // Called on drag-and-drop thread (Windows).
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index 675529b..7d41c65 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -1358,15 +1358,8 @@ void SavePackage::StopObservation() {
   download_manager_ = NULL;
 }
 
-void SavePackage::OnDownloadUpdated(DownloadItem* download) {
-  DCHECK(download_);
-  DCHECK(download_ == download);
-  DCHECK(download_manager_);
-
-  // Check for removal.
-  if (download_->GetState() == DownloadItem::REMOVING) {
-    StopObservation();
-  }
+void SavePackage::OnDownloadDestroyed(DownloadItem* download) {
+  StopObservation();
 }
 
 void SavePackage::FinalizeDownloadEntry() {
diff --git a/content/browser/download/save_package.h b/content/browser/download/save_package.h
index 7f2bebb..aaa4377 100644
--- a/content/browser/download/save_package.h
+++ b/content/browser/download/save_package.h
@@ -145,8 +145,7 @@ class CONTENT_EXPORT SavePackage
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
   // content::DownloadItem::Observer implementation.
-  virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE {}
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
 
   // Update the download history of this item upon completion.
   void FinalizeDownloadEntry();
diff --git a/content/public/browser/download_item.h b/content/public/browser/download_item.h
index 20dc06b..9c91128 100644
--- a/content/public/browser/download_item.h
+++ b/content/public/browser/download_item.h
@@ -61,10 +61,6 @@ class CONTENT_EXPORT DownloadItem {
     // Download has been cancelled.
     CANCELLED,
 
-    // This state indicates that the download item is about to be destroyed,
-    // and observers seeing this state should release all references.
-    REMOVING,
-
     // This state indicates that the download has been interrupted.
     INTERRUPTED,
 
@@ -102,10 +98,18 @@ class CONTENT_EXPORT DownloadItem {
   // to receive updates to the download's status.
   class CONTENT_EXPORT Observer {
    public:
-    virtual void OnDownloadUpdated(DownloadItem* download) = 0;
+    virtual void OnDownloadUpdated(DownloadItem* download) {}
 
     // Called when a downloaded file has been opened.
-    virtual void OnDownloadOpened(DownloadItem* download) = 0;
+    virtual void OnDownloadOpened(DownloadItem* download) {}
+
+    // Called when the user removes a download.
+    virtual void OnDownloadRemoved(DownloadItem* download) {}
+
+    // Called when the download is being destroyed. This happens after
+    // every OnDownloadRemoved() as well as when the DownloadManager is going
+    // down.
+    virtual void OnDownloadDestroyed(DownloadItem* download) {}
 
    protected:
     virtual ~Observer() {}