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

content/browser/download/download_item.cc

Index: content/browser/download/download_item.cc
diff --git a/content/browser/download/download_item.cc b/content/browser/download/download_item.cc
index 080987289601b82facb9fbacb99319b1b820f7ad..2868e341fffcacddfa96a8e0ee80138824c7537d 100644
--- a/content/browser/download/download_item.cc
+++ b/content/browser/download/download_item.cc
@@ -321,6 +321,19 @@ void DownloadItem::UpdateSize(int64 bytes_so_far) {
     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));
@@ -352,24 +365,28 @@ void DownloadItem::Update(int64 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);
-  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() {
@@ -453,20 +470,32 @@ void DownloadItem::UpdateTarget() {
     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;
 
-  last_error_ = net_error;
   UpdateSize(size);
-  StopProgressTimer();
+  last_error_ = net_error;
+
+  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) {
@@ -497,11 +526,14 @@ void DownloadItem::Remove() {
   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.
 }