summaryrefslogtreecommitdiffstats
path: root/content/browser/download
diff options
context:
space:
mode:
authorbenjhayden@chromium.org <benjhayden@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2012-08-03 03:07:49 +0000
committerbenjhayden@chromium.org <benjhayden@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2012-08-03 03:07:49 +0000
commitaadf7e165c67504e6170410ae734a67cbe9c96f5 (patch)
tree6ac22c46881bbe428ad4be5686dbe881b5f4d586 /content/browser/download
parent2dd8dd827e37277cb23f4d2f81e2c4b420b7002b (diff)
downloadchromium_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
Diffstat (limited to 'content/browser/download')
-rw-r--r--content/browser/download/download_item_impl.cc11
-rw-r--r--content/browser/download/download_item_impl.h3
-rw-r--r--content/browser/download/download_item_impl_unittest.cc43
-rw-r--r--content/browser/download/download_manager_impl.cc34
-rw-r--r--content/browser/download/drag_download_file.cc14
-rw-r--r--content/browser/download/drag_download_file.h2
-rw-r--r--content/browser/download/save_package.cc11
-rw-r--r--content/browser/download/save_package.h3
8 files changed, 74 insertions, 47 deletions
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();