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

Make cancel remove cancelled download from active queues at time of cancel.
Also add various tests required or enabled by this change.

This changes two aspects of Cancel semantics (for downloads, not save page): 
a) Cancel can now be called anytime during the lifetime of a download, and
b) if it is called before the history callback occurs, the download will be removed from the system (no where to store it persistently while waiting)

BUG=85408
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101510

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102126

[Randy Smith (Not in Mondays), 2011-09-10 01:15:19]

PTAL.  This CL was originally uploaded as
http://codereview.chromium.org/7294013, but work on that got back burned for (at
the time) higher priority work, and there had been enough code churn when I got
back to it that I had to regenerate the CL.  

ahendrickson: Primary reviewer (everything except for chrome/browser/net)
benjhayden: I want to give you a chance to comment on the queue rewriting in
download_manager.cc, but if you want to give it a pass or go over it lightly,t
hat's ok.
eroman: chrome/browser/net (additions to *slow_download_job*).

Notes:

* This CL is presented as a diff from http://codereview.chromium.org/7859001 and
will be landed after that oen is.

* Most files are only touched because of changing the DownloadItem::Cancel()
signature.  I'm not going to try and get OWNER approval for those changes
specifically (unless someone wants to jump on me for that decision, which is why
it's front and center).  Given that the last time I tried to land this change,
it passed the trybots and failed on the main tree, I'm planning to dcommit and
watch anwyay, so satisfying the commit bots is a non-issue.

* The semantic change in the issue description around cancel before history
insertion implying remove can be fixed after Ben's upcoming queue rewrite;
that'll separate "persistent for the lifetime of the browser" from "persistent
on disk" and allow us to keep cancelled downloads around even if the history
system hasn't gotten back to us.

* I'll note that I changed the interrupt pathway to parallel the cancel
pathway--I've been wanting to do that for some weeks now, and since I was in the
code anyway.... :-}.

PTAL.

[eroman, 2011-09-10 01:53:50]

LGTM on browser/net/*

http://codereview.chromium.org/7796014/diff/4001/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.h (right):

http://codereview.chromium.org/7796014/diff/4001/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.h:4: // This class simulates a
slow download.  This used in a UI test to test the
nit: "This is used" (not from your change).

[Randy Smith (Not in Mondays), 2011-09-10 16:14:38]

http://codereview.chromium.org/7796014/diff/4001/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.h (right):

http://codereview.chromium.org/7796014/diff/4001/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.h:4: // This class simulates a
slow download.  This used in a UI test to test the
On 2011/09/10 01:53:50, eroman wrote:
> nit: "This is used" (not from your change).

Done.

[ahendrickson, 2011-09-11 15:57:01]

LGTM, with some nits.

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.h:184: // |size| is the amount of data
received so far, and |os_error| is the error
Nit: |net_error| (or change it to |error|, as it will eventually use download
errors).

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_manager.cc:588: DownloadMap::iterator it =
history_downloads_.find(download->db_handle());
I don't think we need this iterator here any more . . .

http://codereview.chromium.org/7796014/diff/7029/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:36: if (url.spec() ==
URLRequestSlowDownloadJob::kFinishDownloadUrl)
Here and in other parts of the file:  Why did you get rid of
|LowerCaseEqualsASCII| in favor of a plain |==|?

[Randy Smith (Not in Mondays), 2011-09-12 18:00:27]

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.h:184: // |size| is the amount of data
received so far, and |os_error| is the error
On 2011/09/11 15:57:02, ahendrickson wrote:
> Nit: |net_error| (or change it to |error|, as it will eventually use download
> errors).

Done.

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_manager.cc:588: DownloadMap::iterator it =
history_downloads_.find(download->db_handle());
On 2011/09/11 15:57:02, ahendrickson wrote:
> I don't think we need this iterator here any more . . .

Whooops.  Done.

http://codereview.chromium.org/7796014/diff/7029/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:36: if (url.spec() ==
URLRequestSlowDownloadJob::kFinishDownloadUrl)
On 2011/09/11 15:57:02, ahendrickson wrote:
> Here and in other parts of the file:  Why did you get rid of
> |LowerCaseEqualsASCII| in favor of a plain |==|?

In response to a comment of Eric's on the original CL; see
http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ....

[benjhayden, 2011-09-12 18:13:37]

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:324: void
DownloadItem::StopInternal(DownloadState target_state) {
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;
    ...
  }
}

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:327: DCHECK(target_state == CANCELLED
|| target_state == INTERRUPTED);
Should this be CHECK?

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:330: DCHECK(IsInProgress());
Should this be CHECK?

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:368: // May be triggered by user
action or because of various kinds of error.
What kinds of errors would go through here instead of Interrupted?

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:531:
download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
Want to do this in StopInternal()?

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_manager.cc:509:
DCHECK(ContainsKey(active_downloads_, download->id()));
CHECK?

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

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:324: void
DownloadItem::StopInternal(DownloadState target_state) {
On 2011/09/12 18:13:38, b s h wrote:
> 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?

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. 

> 
> 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;
>     ...
>   }
> }

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:327: DCHECK(target_state == CANCELLED
|| target_state == INTERRUPTED);
On 2011/09/12 18:13:38, b s h wrote:
> Should this be CHECK?

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).

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:330: DCHECK(IsInProgress());
On 2011/09/12 18:13:38, b s h wrote:
> Should this be CHECK?

See above.

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:368: // May be triggered by user
action or because of various kinds of error.
On 2011/09/12 18:13:38, b s h wrote:
> What kinds of errors would go through here instead of Interrupted?

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).

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:531:
download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
On 2011/09/12 18:13:38, b s h wrote:
> Want to do this in StopInternal()?

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.

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_manager.cc:509:
DCHECK(ContainsKey(active_downloads_, download->id()));
On 2011/09/12 18:13:38, b s h wrote:
> CHECK?

Done.

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

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/7796014/diff/7029/content/browser/download/dow...
content/browser/download/download_item.cc:327: DCHECK(target_state == CANCELLED
|| target_state == INTERRUPTED);
On 2011/09/12 19:43:23, rdsmith wrote:
> On 2011/09/12 18:13:38, b s h wrote:
> > Should this be CHECK?
> 
> 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).

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

[benjhayden, 2011-09-12 19:57:55]

LGTM Thanks!

[jennb, 2011-09-12 20:52:48]

LGTM for the panels change.

[Randy Smith (Not in Mondays), 2011-09-20 15:50:53]

Andy, PTAL?  I'd recommend you look at the PS14->15 changes; 13->14 was just the
merge up to LKGR.  I don't believe anyone else needs to do a re-review.  I'd
welcome a drive-by from Pawel on these if he has the bandwidth, but I'm not
going to ask because I'm not sure he does.

This change was landed, and then immediately reverted because it showed up
failures on the main waterfall that hadn't shown up in try runs.  I've tried to
adapt to what I believe those failures are (I'm working a bit blind here). 
Links to failing builds:

http://build.chromium.org/p/chromium/builders/XP%20Tests%20%281%29/builds/7730
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%281%29/builds/...
http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%285%29/b...
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%28dbg%29%284%2...
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%28dbg%29%285%2...

My conclusion on the problems with interrupts were that the failure from the
URLRequestJob hadn't had a chance to propagate through the system, so I put in
infrastructure to chase it through from the IO thread.  My conclusion on the
65535 error from the shell dialogs check was that the shell dialog hadn't had a
chance to be created before the test was terminated; rather than try to fix that
race, I interposed a delegate that behaves properly (for the individual test)
but doesn't throw up a file selection dialog.

[ahendrickson, 2011-09-20 18:22:32]

Changes from PS14->15 LGTM.

Note: I couldn't open any of the links to the failing builds.

Andy

On 2011/09/20 15:50:53, rdsmith wrote:
> Andy, PTAL?  I'd recommend you look at the PS14->15 changes; 13->14 was just
the
> merge up to LKGR.  I don't believe anyone else needs to do a re-review.  I'd
> welcome a drive-by from Pawel on these if he has the bandwidth, but I'm not
> going to ask because I'm not sure he does.
> 
> This change was landed, and then immediately reverted because it showed up
> failures on the main waterfall that hadn't shown up in try runs.  I've tried
to
> adapt to what I believe those failures are (I'm working a bit blind here). 
> Links to failing builds:
> 
>
http://build.chromium.org/p/chromium/builders/XP%2520Tests%2520%25281%2529/bu...
>
http://build.chromium.org/p/chromium/builders/Vista%2520Tests%2520%25281%2529...
>
http://build.chromium.org/p/chromium/builders/XP%2520Tests%2520%2528dbg%2529%...
>
http://build.chromium.org/p/chromium/builders/Vista%2520Tests%2520%2528dbg%25...
>
http://build.chromium.org/p/chromium/builders/Vista%2520Tests%2520%2528dbg%25...
> 
> My conclusion on the problems with interrupts were that the failure from the
> URLRequestJob hadn't had a chance to propagate through the system, so I put in
> infrastructure to chase it through from the IO thread.  My conclusion on the
> 65535 error from the shell dialogs check was that the shell dialog hadn't had
a
> chance to be created before the test was terminated; rather than try to fix
that
> race, I interposed a delegate that behaves properly (for the individual test)
> but doesn't throw up a file selection dialog.

[Randy Smith (Not in Mondays), 2011-09-20 18:26:47]

On 2011/09/20 18:22:32, ahendrickson wrote:
> Changes from PS14->15 LGTM.
> 
> Note: I couldn't open any of the links to the failing builds.

Argg.  For some reason the initial % of the %20s was replaced by a %25.  I'll
try repasting them here, but they may just be unusual with this transformation:

http://build.chromium.org/p/chromium/builders/XP%20Tests%20%281%29/builds/7730
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%281%29/builds/...
http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%285%29/b...
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%28dbg%29%284%2...
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%28dbg%29%285%2...


> 
> Andy
> 
> On 2011/09/20 15:50:53, rdsmith wrote:
> > Andy, PTAL?  I'd recommend you look at the PS14->15 changes; 13->14 was just
> the
> > merge up to LKGR.  I don't believe anyone else needs to do a re-review.  I'd
> > welcome a drive-by from Pawel on these if he has the bandwidth, but I'm not
> > going to ask because I'm not sure he does.
> > 
> > This change was landed, and then immediately reverted because it showed up
> > failures on the main waterfall that hadn't shown up in try runs.  I've tried
> to
> > adapt to what I believe those failures are (I'm working a bit blind here). 
> > Links to failing builds:
> > 
> >
>
http://build.chromium.org/p/chromium/builders/XP%252520Tests%252520%2525281%2...
> >
>
http://build.chromium.org/p/chromium/builders/Vista%252520Tests%252520%252528...
> >
>
http://build.chromium.org/p/chromium/builders/XP%252520Tests%252520%252528dbg...
> >
>
http://build.chromium.org/p/chromium/builders/Vista%252520Tests%252520%252528...
> >
>
http://build.chromium.org/p/chromium/builders/Vista%252520Tests%252520%252528...
> > 
> > My conclusion on the problems with interrupts were that the failure from the
> > URLRequestJob hadn't had a chance to propagate through the system, so I put
in
> > infrastructure to chase it through from the IO thread.  My conclusion on the
> > 65535 error from the shell dialogs check was that the shell dialog hadn't
had
> a
> > chance to be created before the test was terminated; rather than try to fix
> that
> > race, I interposed a delegate that behaves properly (for the individual
test)
> > but doesn't throw up a file selection dialog.