diff options
Diffstat (limited to 'content/browser/download')
-rw-r--r-- | content/browser/download/download_item_impl.cc | 11 | ||||
-rw-r--r-- | content/browser/download/download_item_impl.h | 3 | ||||
-rw-r--r-- | content/browser/download/download_item_impl_unittest.cc | 43 | ||||
-rw-r--r-- | content/browser/download/download_manager_impl.cc | 34 | ||||
-rw-r--r-- | content/browser/download/drag_download_file.cc | 14 | ||||
-rw-r--r-- | content/browser/download/drag_download_file.h | 2 | ||||
-rw-r--r-- | content/browser/download/save_package.cc | 11 | ||||
-rw-r--r-- | content/browser/download/save_package.h | 3 |
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(); |