Make cancel remove cancelled download from active queues at time of cancel.

content/browser/download/download_item.cc

 // Copyright (c) 2011 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.h"
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 303 matching lines @@
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   received_bytes_ = bytes_so_far;
 
   // If we've received more data than we were expecting (bad server info?),
   // revert to 'unknown size mode'.
   if (received_bytes_ > total_bytes_)
     total_bytes_ = 0;
 }
 
+void DownloadItem::StopInternal(DownloadState target_state) {

[benjhayden, 2011/09/12 18:13:38]

Would you rather do this stuff inside TransitionTo, so that we don't have to
remember that StopInternal is like TransitionTo, but with some other stuff, and
that if we're going to TransitionTo cancelled/interrupted, then we should call
StopInternal instead?

Generalizing further, if you like:
void DI::PreTransitionToCancelled() {...}
void DI::PostTransitionToCancelled() {...}
void DI::PreTransitionToCompleted() {...}
void DI::PostTransitionToCompleted() {...}
...
void DI::TransitionTo(DownloadState target_state) {
  switch (target_state) {
    case CANCELLED: PreTransitionToCancelled(); break;
    ...
  }
  state_ = target_state;
  UpdateObservers();
  switch (target_state) {
    case CANCELLED: PostTransitionToCancelled(); break;
    ...
  }
}

[Randy Smith (Not in Mondays), 2011/09/12 19:43:23]

Hmmm.  I'm torn.  I had originally conceptualized TransitionTo() as being a very
specific helper function, not a "take care of doing everything required by this
state transition".  But I certainly see your point.  Ok. 

+  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(target_state == CANCELLED || target_state == INTERRUPTED);

[benjhayden, 2011/09/12 18:13:38]

Should this be CHECK?

[Randy Smith (Not in Mondays), 2011/09/12 19:43:23]

Moot (since I'm nuking StopInternal) but I'm not inclined to make things DCHECKs
that aren't directly related to the areas we think the crashers were in, as I
think they have a reasonable chance of tripping things that aren't actually bugs
(e.g. I tripped a recent DCHECK on the drag-and-drop pathway when I wrote a new
test).  We do want to know about such things, but I think the right way is to
write more tests, not to use the field as a testbed (unless we have to, e.g.
crashers).

[Randy Smith (Not in Mondays), 2011/09/12 19:44:36]

Sorry, mistyped--I meant "I'm not inclined to make things CHECKs".

+
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
+  DCHECK(IsInProgress());

[benjhayden, 2011/09/12 18:13:38]

Should this be CHECK?

[Randy Smith (Not in Mondays), 2011/09/12 19:43:23]

See above.

+
+  TransitionTo(target_state);
+  download_manager_->DownloadStopped(this);
+  StopProgressTimer();
+}
+
 void DownloadItem::StartProgressTimer() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   update_timer_.Start(FROM_HERE,
                       base::TimeDelta::FromMilliseconds(kUpdateTimeMs), this,
                       &DownloadItem::UpdateObservers);
 }
 
 void DownloadItem::StopProgressTimer() {
@@ skipping 11 matching lines @@
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!IsInProgress()) {
     NOTREACHED();
     return;
   }
   UpdateSize(bytes_so_far);
   UpdateObservers();
 }
 
-// Triggered by a user action.
-void DownloadItem::Cancel(bool update_history) {
+// May be triggered by user action or because of various kinds of error.

[benjhayden, 2011/09/12 18:13:38]

What kinds of errors would go through here instead of Interrupted?

[Randy Smith (Not in Mondays), 2011/09/12 19:43:23]

I think this comment was based on a previous incarnation of the source, in which
the DownloadFileManager was sometimes remotely canceling (rather than using
OnDownloadError).  I've removed the comment, and also killed
DownloadManager::CancelDownload() (which isn't needed if nothing off-thread
needs to cancel a download).

+void DownloadItem::Cancel() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
 
-  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (!IsPartialDownload()) {
-    // Small downloads might be complete before this method has
-    // a chance to run.
+  // Small downloads might be complete before we have a chance to run.
+  if (!IsInProgress())
     return;
-  }
+
+  StopInternal(CANCELLED);
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
-  TransitionTo(CANCELLED);
-  StopProgressTimer();
-  if (update_history)
-    download_manager_->DownloadCancelledInternal(this);
+  // History insertion is the point at which we have finalized download
+  // details and persist them if something goes wrong.  Before history
+  // insertion, interrupt or cancel results in download removal.
+  if (db_handle() == DownloadItem::kUninitializedHandle) {
+    download_manager_->RemoveDownload(this);
+    // We are now deleted; no further code should be executed on this
+    // object.
+  }
 }
 
 void DownloadItem::MarkAsComplete() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   TransitionTo(COMPLETE);
 }
 
@@ skipping 63 matching lines @@
 }
 
 void DownloadItem::UpdateTarget() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (state_info_.target_name.value().empty())
     state_info_.target_name = full_path_.BaseName();
 }
 
-void DownloadItem::Interrupted(int64 size, net::Error net_error) {
+void DownloadItem::Interrupt(int64 size, net::Error net_error) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
 
+  // Small downloads might be complete before we have a chance to run.
   if (!IsInProgress())
     return;
 
+  UpdateSize(size);
   last_error_ = net_error;
-  UpdateSize(size);
-  StopProgressTimer();
+
+  StopInternal(INTERRUPTED);
+
   download_stats::RecordDownloadInterrupted(net_error,
                                             received_bytes_,
                                             total_bytes_);
-  TransitionTo(INTERRUPTED);
+
+  // History insertion is the point at which we have finalized download
+  // details and persist them if something goes wrong.  Before history
+  // insertion, interrupt or cancel results in download removal.
+  if (db_handle() == DownloadItem::kUninitializedHandle) {
+    download_manager_->RemoveDownload(this);
+    // We are now deleted; no further code should be executed on this
+    // object.
+  }
 }
 
 void DownloadItem::Delete(DeleteReason reason) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   switch (reason) {
     case DELETE_DUE_TO_USER_DISCARD:
       UMA_HISTOGRAM_ENUMERATION("Download.UserDiscard", GetDangerType(),
                                 DANGEROUS_TYPE_MAX);
@@ skipping 10 matching lines @@
       NewRunnableFunction(&DeleteDownloadedFile, full_path_));
   Remove();
   // We have now been deleted.
 }
 
 void DownloadItem::Remove() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   download_manager_->AssertQueueStateConsistent(this);
-  Cancel(true);
+  if (IsInProgress()) {
+    StopInternal(CANCELLED);
+    download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);

[benjhayden, 2011/09/12 18:13:38]

Want to do this in StopInternal()?

[Randy Smith (Not in Mondays), 2011/09/12 19:43:23]

I'm disinclined.  I'd have to have a conditional (and minimizing conditionals is
a useful, if low priority, rule of thumb) and now that I've merged StopInternal
into TransitionTo, it messes with the concept of TransitionTo (which is to do
things associated with specific state transitions).  We're trying to capture the
number of times that the user did something that cancelled a download; not all
transitions to CANCELLED will necessarily be in that class.  Certainly not after
we nuke the INTERRUPTED state.

+  }
   download_manager_->AssertQueueStateConsistent(this);
+  download_stats::RecordDownloadCount(download_stats::REMOVED_COUNT);
 
-  TransitionTo(REMOVING);
-  download_manager_->RemoveDownload(db_handle_);
+  download_manager_->RemoveDownload(this);
   // We have now been deleted.
 }
 
 bool DownloadItem::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)
     return false;
@@ skipping 282 matching lines @@
         state_info_.target_name.value().c_str(),
         full_path().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
 
   description += " }";
 
   return description;
 }