diff options
author | benjhayden@chromium.org <benjhayden@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-08-03 03:07:49 +0000 |
---|---|---|
committer | benjhayden@chromium.org <benjhayden@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-08-03 03:07:49 +0000 |
commit | aadf7e165c67504e6170410ae734a67cbe9c96f5 (patch) | |
tree | 6ac22c46881bbe428ad4be5686dbe881b5f4d586 | |
parent | 2dd8dd827e37277cb23f4d2f81e2c4b420b7002b (diff) | |
download | chromium_src-aadf7e165c67504e6170410ae734a67cbe9c96f5.zip chromium_src-aadf7e165c67504e6170410ae734a67cbe9c96f5.tar.gz chromium_src-aadf7e165c67504e6170410ae734a67cbe9c96f5.tar.bz2 |
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() {} |