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

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();