Shift "commit point" for when a download will no longer accept cancels.

Shift "commit point" for when a download will no longer accept cancels.

This CL shifts the commit point for a download to just after the download 
file release has been dispatched.  The download remains IN_PROGRESS as 
far as the outside world is concerned, but at this point it is committed to 
continue to completion.

BUG=123998
R=benjhayden@chromium.org
R=asanka@chromium.org


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

[Randy Smith (Not in Mondays), 2012-09-18 21:58:55]

Ben, Asanka: Could you both take a look?  This pulls the "commit point" shift
out from the download file manager removal CL, and adds tests for the behavior
around that point.  Ben: Primary reviewer (you've seen it before), Asanka: UI
implications.  Thanks!

[benjhayden, 2012-09-19 15:44:35]

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:142:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
Seems like there should be a less magical way to do this. Why not
PostTaskAndReply?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:422: if (!state_ ==
COMPLETE_INTERNAL || file_externally_removed_)
s/!state_ ==/state_ !=/

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:463:
COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kInternalStateMap) ==
Could you put these up by the maps themselves?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:491: return (state_ ==
IN_PROGRESS_INTERNAL);
Have you inspected all callers of IsPartialDownload() to make sure that they
don't actually mean IsInProgress()?
And vice versa?
Could you remove IsPartialDownload() in this CL?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:996: if (!IsInProgress()) {
Why respell some IsInProgress()/IsComplete()/etc. calls using state_ directly,
but not all calls?
I'd prefer using state_ directly whenever possible for clarity.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:999: // The error doesn't matter
as the cause of download stoppaged
s/stoppaged/stoppage/

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1000: // will already have been
recorded.
Could you convert "the cause of download stoppage" to a DownloadInterruptReason
with more fidelity?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1080: if (!IsInProgress())
So, if I cancel a download *while* it's being renamed, current_path_ won't be
updated. Chrome will lose the file, and think it was externally removed.
Am I reading this right? Is that what you meant to do?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1210: if
(kInternalStateMap[old_state] != kInternalStateMap[state_])
Should you use the state maps in IsInProgress()/IsComplete()/etc. as well as
here so that they stay in sync?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1250: // static
What's the difference between a private static method and a function in the
anonymous namespace?
If a static method is private, does it make a noise?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1269: 
Mind deleting all these blank lines?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.h:216: // TODO(rdsmith): Put in
state variable for file name determination.
Why not go ahead and do this now?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.h:229: CANCELLED_INTERNAL,
I thought we wanted to merge this with interrupted? What would it take to do
that now?

[Randy Smith (Not in Mondays), 2012-09-19 20:46:39]

Many of my responses boil down to "If it's not centrally part of this CL, I'd
rather put it off, as this CL is blocking removing the DFM, which I want to get
in".  If you disagree with that basic point, ping me by IM or phone or in person
and let's talk.  (Disagreements about what's actually centrally part of this CL
I'm happy to have out in the context of the CL itself.)

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:142:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Seems like there should be a less magical way to do this. Why not
> PostTaskAndReply?

Because I wanted to be able to write a test to delay the callback under test
control, and I couldn't see a way to do that with PostTaskAndReply.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:422: if (!state_ ==
COMPLETE_INTERNAL || file_externally_removed_)
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> s/!state_ ==/state_ !=/

Whooops.  Done.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:463:
COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kInternalStateMap) ==
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Could you put these up by the maps themselves?

I'd like to, but they need to be in the DownloadItemImpl namespace to get access
to the list of internal states (it's private).  I didn't want to create a
function purely to do this, and this function seemed like the most appropriate
one to stick them into.   If you'd like me to put them in the first constructor,
I'm willing, but they'll still be a ways separated from the maps because of all
of the member variables we initialize.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:491: return (state_ ==
IN_PROGRESS_INTERNAL);
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Have you inspected all callers of IsPartialDownload() to make sure that they
> don't actually mean IsInProgress()?
> And vice versa?
> Could you remove IsPartialDownload() in this CL?

Argggh.  You triggerred my "not conceptually part of this CL, so I'd rather put
it off" subroutine, then I realized that this arguably *is* part of this CL.  I
think I'm going to say: There's a bug in this code in that it's not
functionality preserving, and I'm going to fix that (by making it check
COMPLETING as well), but that makes the definition false to the name of the
routine.  OTOH, the definition *is* false to the name of the routine anyway,
since we can finish downloading the data well before we switch out of
IN_PROGRESS (depending on user response).  On the third hand, the intended use
for IsPartialDownload() was to eventually return true for interrupted downloads
that could be resumed.  On the fourth hand, I'm inclined to get rid of *all* of
these probes and have consumers just check the state variable for what they want
to know anyway, but I haven't solidly come to that decision yet.

So: I'd rather not tug on this string (other than checking COMPLETING) at this
point.  That ok by you?

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:996: if (!IsInProgress()) {
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Why respell some IsInProgress()/IsComplete()/etc. calls using state_ directly,
> but not all calls?
> I'd prefer using state_ directly whenever possible for clarity.

Agreed.  I had thought I had replaced them all (I consider this respecting the
interface boundary--if DownloadItemImpl can't understand what it's own states
mean, we have a problem).  Thanks for calling this out.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:999: // The error doesn't matter
as the cause of download stoppaged
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> s/stoppaged/stoppage/

Done.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1000: // will already have been
recorded.
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Could you convert "the cause of download stoppage" to a
DownloadInterruptReason
> with more fidelity?

I'm slightly confused by your question.  My comment was intended to mean
"Whatever I pass to ODRTIN will be dropped on the floor, so it doesn't matter
what I pass".  But no reason not to pass the actual reason, since it's in
last_reason_.  Done.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1080: if (!IsInProgress())
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> So, if I cancel a download *while* it's being renamed, current_path_ won't be
> updated. Chrome will lose the file, and think it was externally removed.
> Am I reading this right? Is that what you meant to do?

Huh.  Good point; at a minimum we should have a comment here.  But I don't think
it matters.  If we're canceled at any time before we hit the commit point, we're
going to send a Cancel message over to the file thread, which will delete the
file.  So I don't think we care that we're not updating the file name.

Comment added.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1210: if
(kInternalStateMap[old_state] != kInternalStateMap[state_])
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Should you use the state maps in IsInProgress()/IsComplete()/etc. as well as
> here so that they stay in sync?

Could you put a few more words behind your argument?  I feel like they're doing
somewhat different things.  Or are you saying that they're both expressing the
state that should be exposed to the user, and if I made the checks in Is* use
the external states rather than the internals states, that would mean I don't
"leak" state I don't mean the users to see?  I agree with that argument
(although it strengthens my interest in getting rid of the Is* probes).  

Done.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1250: // static
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> What's the difference between a private static method and a function in the
> anonymous namespace?

I originally followed your suggestion and took it out, and the compiler was kind
enough to remind me why it's a static private: So that it has visibility into
the private typedef of DownloadInternalState.  I suppose I could just make it a
friend, but I'd rather make it a static private.

> If a static method is private, does it make a noise?

Only when you try to take it out of its class :-}.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.cc:1269: 
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Mind deleting all these blank lines?

I'll have you know, I have a deep, sentimental attachment to these blank lines!!
 We've been through a lot together!

(It can't be alcohol; I've haven't drunk today.  Lack of alcohol?)

Done.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.h:216: // TODO(rdsmith): Put in
state variable for file name determination.
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> Why not go ahead and do this now?

Not relevant to the focus of this CL, and after six reverts and four "peeled
off" CLs I'm feeling a little gun-shy about putting extra stuff into CLs :-}.

http://codereview.chromium.org/10950015/diff/4001/content/browser/download/do...
content/browser/download/download_item_impl.h:229: CANCELLED_INTERNAL,
On 2012/09/19 15:44:35, benjhayden_chromium wrote:
> I thought we wanted to merge this with interrupted? What would it take to do
> that now?

Shooting from the hip, 2-3 hours of coding and about double that in wall clock
time?  It's not at all a hard change, I'm just reluctant to do it as part of
this CL because of the stacked dependencies (i.e. I can't remove the DFM until I
get this in).

[benjhayden, 2012-09-19 21:06:05]

lgtm

[asanka, 2012-09-20 18:42:15]

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file.h:39: virtual
content::DownloadInterruptReason Initialize() = 0;
Nit: No content:: namespace qualifier needed in content namespace.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file_manager.cc:123:
download_file->AnnotateWithSourceInformation();
DownloadFileImpl::Detach() also does this.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file_manager_unittest.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:187: // |id| is the
download ID of the new download file.
Nit: No |id| parameter anymore.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file_unittest.cc:130: void
SetInterruptReasonCallback(bool* was_called,
Is this used?

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_item_impl.cc:123:
DownloadItemImpl::kInternalStateMap[] = {
A minor suggestion: Perhaps a switch based mapping function would be easier to
read?

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_item_impl.cc:463:
COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kInternalStateMap) ==
Nit: Perhaps keep the COMPILE_ASSERTs next to the array definitions?

[Randy Smith (Not in Mondays), 2012-09-21 21:58:01]

Asanka, PTAL?

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file.h:39: virtual
content::DownloadInterruptReason Initialize() = 0;
On 2012/09/20 18:42:15, asanka wrote:
> Nit: No content:: namespace qualifier needed in content namespace.

Done.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file_manager.cc:123:
download_file->AnnotateWithSourceInformation();
On 2012/09/20 18:42:15, asanka wrote:
> DownloadFileImpl::Detach() also does this.

Ooops; thanks.  Deleted.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file_manager_unittest.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:187: // |id| is the
download ID of the new download file.
On 2012/09/20 18:42:15, asanka wrote:
> Nit: No |id| parameter anymore.

Done.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_file_unittest.cc:130: void
SetInterruptReasonCallback(bool* was_called,
On 2012/09/20 18:42:15, asanka wrote:
> Is this used?

Apparently not.  I think I was a bit too fast and loose in the CL split :-{.

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_item_impl.cc:123:
DownloadItemImpl::kInternalStateMap[] = {
On 2012/09/20 18:42:15, asanka wrote:
> A minor suggestion: Perhaps a switch based mapping function would be easier to
> read?

Good idea.  I think it removes the pain around the COMPILE_ASSERT positioning
too (by removing the asserts).

http://codereview.chromium.org/10950015/diff/8002/content/browser/download/do...
content/browser/download/download_item_impl.cc:463:
COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kInternalStateMap) ==
On 2012/09/20 18:42:15, asanka wrote:
> Nit: Perhaps keep the COMPILE_ASSERTs next to the array definitions?

See above; now moot.

[asanka, 2012-09-21 22:09:37]

LGTM Thanks!

[commit-bot: I haz the power, 2012-09-24 15:00:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10950015/25002

[commit-bot: I haz the power, 2012-09-24 15:00:55]

Presubmit check for 10950015-25002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/shell
    content/public/test

Presubmit checks took 1.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Randy Smith (Not in Mondays), 2012-09-24 15:05:57]

Avi: Willing to give a quick skim and stamp for the non-content/browser/download
directories?

[Avi (use Gerrit), 2012-09-24 15:09:57]

lgtm

[commit-bot: I haz the power, 2012-09-24 15:55:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10950015/25002

[commit-bot: I haz the power, 2012-09-24 17:13:46]

Change committed as 158298