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

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_item_impl.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 68 matching lines @@
 }
 
 const char* DebugDownloadStateString(DownloadItem::DownloadState state) {
   switch (state) {
     case DownloadItem::IN_PROGRESS:
       return "IN_PROGRESS";
     case DownloadItem::COMPLETE:
       return "COMPLETE";
     case DownloadItem::CANCELLED:
       return "CANCELLED";
-    case DownloadItem::REMOVING:
-      return "REMOVING";
     case DownloadItem::INTERRUPTED:
       return "INTERRUPTED";
     default:
       NOTREACHED() << "Unknown download state " << state;
       return "unknown";
   };
 }
 
 // Classes to null out request handle calls (for SavePage DownloadItems, which
 // may have, e.g., Cancel() called on them without it doing anything)
@@ skipping 208 matching lines @@
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   delegate_->Attach();
   Init(true /* actively downloading */,
        download_net_logs::SRC_SAVE_PAGE_AS);
 }
 
 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();
 }
 
 void DownloadItemImpl::AddObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   observers_.AddObserver(observer);
 }
 
 void DownloadItemImpl::RemoveObserver(Observer* observer) {
@@ skipping 317 matching lines @@
   // We have now been deleted.
 }
 
 void DownloadItemImpl::Remove() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   delegate_->AssertStateConsistent(this);
   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.
 }
 
 bool DownloadItemImpl::TimeRemaining(base::TimeDelta* remaining) const {
   if (total_bytes_ <= 0)
     return false;  // We never received the content_length for this download.
 
   int64 speed = CurrentSpeed();
   if (speed == 0)
@@ skipping 506 matching lines @@
   std::map<const void*, ExternalData*>::iterator it =
       external_data_map_.find(key);
 
   if (it == external_data_map_.end()) {
     external_data_map_[key] = data;
   } else if (it->second != data) {
     delete it->second;
     it->second = data;
   }
 }