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

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index c2c6941a288990af490387f7e0941cc3ddb7ab59..3e8e4cd840066823e7f5b01a14f5b6bac2063b4d 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -86,8 +86,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:
@@ -316,11 +314,9 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate,
 
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  TransitionTo(REMOVING);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestructed(this));

[Randy Smith (Not in Mondays), 2012/06/29 19:34:54]

nit, suggestion: Destroyed.

[benjhayden, 2012/07/10 16:58:57]

Done.

   STLDeleteContainerPairSecondPointers(
       external_data_map_.begin(), external_data_map_.end());
-  delegate_->AssertStateConsistent(this);

[Randy Smith (Not in Mondays), 2012/06/29 19:34:54]

Why?  I could imagine us yanking out the queue state consistent assertions
sometime around now, but that seems like it should be part of a single CL, and
not done piecemeal.

[benjhayden, 2012/07/10 16:58:57]

Done.

   delegate_->Detach();
 }
 
@@ -658,7 +654,7 @@ void DownloadItemImpl::Remove() {
   Cancel(true);
   delegate_->AssertStateConsistent(this);
 
-  TransitionTo(REMOVING);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadRemoved(this));

[Randy Smith (Not in Mondays), 2012/06/29 19:34:54]

I'm torn about this.  This *does* seem like a state.  A very short lived state,
but a state that describes an actual transition in the state of the DownloadItem
in terms of how it's being seen by the consumer.  But I realize that it's also
somewhat similar to the REMOVING case (thus the "torn").  How strongly do you
feel about expressing this through a new API rather than through a new state?

[benjhayden, 2012/07/10 16:58:57]

When we talked about this, we were talking about these odd things that you
called "feelings". Let's define "state" and "event" so that we can talk about
our "feelings" more clearly. Here're my ideas, please suggest alternatives or
refinements.

State: a period of time, typically lasting longer than a single MessageLoop flop
(loop? turn?), during which certain operations may or may not be performed,
bounded by externally-driven events

Event: an instantaneous change in a particular piece of state

I think we can agree that [destroyed] is a state. It is indefinite and so longer
than a flop. No operations can be performed on the download while it is
[destroyed]. It is bounded on the left by OnDownloadDestroyed, and on the right
by OnUniverseIcyDeath.

I don't think that there's an externally-driven event that transitions from
REMOVING to [destroyed]. That process appears to be automatic. Sure, I can
imagine a GHOST state, but that is not the same as the concept that you are
proposing for REMOVING. If and when we have need for a GHOST state, it will make
sense as a state.
In addition to being automatic, the transition from REMOVING to [destroyed]
seems to be effectively instantaneous, in contrast with the "longer than a
single flop" clause.
I am also unaware of any operations that may be performed on the download item
between OnDownloadRemoved and OnDownloadDestroyed. The operations that may be
performed during REMOVING seem to be the same operations that may be performed
while the download is [destroyed]. You can't even delete a REMOVING download,
because that happens automatically.

So it seems to me that REMOVING fails all of the criteria for a State, but I
fully expect to learn something new and surprising about how the downloads
system works that will change this analysis, or have shown to me a better
definition for State or Event.

   delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }