[Resumption 8/11] Remove intermediate files when no longer necessary.

[Downloads] Remove intermediate files when no longer necessary.

When cancelling or removing an interrupted download, the associated
intermediate file should also be removed. That file is only necessary if
the download is going to be resumed.

BUG=7648
BUG=196806
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201883

[Randy Smith (Not in Mondays), 2013-05-15 15:34:56]

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
File content/browser/download/download_browsertest.cc (left):

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
content/browser/download/download_browsertest.cc:1418: 
I had thought it was convention to have a space between a function close and a
namespace close, but I can't find it in the style guide, so I'll raise the issue
and let you do what you want :-}.

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:422: // non-empty current_path_
that's the same as target_path_.
I'm slightly confused.  If the download was important from the history and is in
the interrupted state, shouldn't we remove the current_path_?  What's the
process by which we get into a state s.t. this extra code is needed?

Also, below in this file we're going to cancel the download; why doesn't the
code in Cancel for cancelling interrupted downloads take care of this case?

[asanka, 2013-05-21 21:09:02]

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
File content/browser/download/download_browsertest.cc (left):

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
content/browser/download/download_browsertest.cc:1418: 
On 2013/05/15 15:34:56, rdsmith wrote:
> I had thought it was convention to have a space between a function close and a
> namespace close, but I can't find it in the style guide, so I'll raise the
issue
> and let you do what you want :-}.

Added back. This was unintentional.

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:422: // non-empty current_path_
that's the same as target_path_.
On 2013/05/15 15:34:56, rdsmith wrote:
> I'm slightly confused.  If the download was important from the history and is
in
> the interrupted state, shouldn't we remove the current_path_?  What's the
> process by which we get into a state s.t. this extra code is needed?
> 
> Also, below in this file we're going to cancel the download; why doesn't the
> code in Cancel for cancelling interrupted downloads take care of this case?

https://codereview.chromium.org/11363222 introduced persisting the target and
current paths. Since previously we only persisted full_path, the migration
script set both target and current path to full_path.

A download that was imported from history might look like the following:
  { current_path: 'foo.crdownload',
    target_path: 'foo.crdownload',
    state: INTERRUPTED }

The problem is that that prior to downloads resumption, the intermediate file
would be deleted on interrupt with the exception of interruptions due to browser
crashes. So if 'foo.crdownload' exists, then its likely that it's associated
with another download. Removing it is probably the wrong thing to do.

Then again, a more recent download might look like:
  { current_path: 'foo.crdownload',
    target_path: 'foo',
    state: INTERRUPTED }

This download could suffer from the same problem (i.e. 'foo.crdownload' if it
exists, may be associated with a different download).

I've removed the check for now and we aggressively delete the intermediate path
on Cancel()/Remove().

Also, you're right that we can rely on the deletion logic in Cancel() here.

[Randy Smith (Not in Mondays), 2013-05-21 21:40:37]

LGTM with the DCHECK comment below.

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:422: // non-empty current_path_
that's the same as target_path_.
On 2013/05/21 21:09:02, asanka wrote:
> On 2013/05/15 15:34:56, rdsmith wrote:
> > I'm slightly confused.  If the download was important from the history and
is
> in
> > the interrupted state, shouldn't we remove the current_path_?  What's the
> > process by which we get into a state s.t. this extra code is needed?
> > 
> > Also, below in this file we're going to cancel the download; why doesn't the
> > code in Cancel for cancelling interrupted downloads take care of this case?
> 
> https://codereview.chromium.org/11363222 introduced persisting the target and
> current paths. Since previously we only persisted full_path, the migration
> script set both target and current path to full_path.
> 
> A download that was imported from history might look like the following:
>   { current_path: 'foo.crdownload',
>     target_path: 'foo.crdownload',
>     state: INTERRUPTED }
> 
> The problem is that that prior to downloads resumption, the intermediate file
> would be deleted on interrupt with the exception of interruptions due to
browser
> crashes. So if 'foo.crdownload' exists, then its likely that it's associated
> with another download. Removing it is probably the wrong thing to do.
> 
> Then again, a more recent download might look like:
>   { current_path: 'foo.crdownload',
>     target_path: 'foo',
>     state: INTERRUPTED }
> 
> This download could suffer from the same problem (i.e. 'foo.crdownload' if it
> exists, may be associated with a different download).
> 
> I've removed the check for now and we aggressively delete the intermediate
path
> on Cancel()/Remove().
> 
> Also, you're right that we can rely on the deletion logic in Cancel() here.

I hear you about the entries from before we started persisting current path. 
I'm ok with specifically checking for that, though I wince to have code that's
put in to handle old database entries--we should figure out a way to update the
database entry once so that the problem doesn't exist and remove the code. 
OTOH, I'm also good with just deleting the file and assuming that no user will
have that problem very often :-}.

With regard to a more recent download, I'm afraid that we sorta have to assume
that if there's an interrupted download with a non-empty() current_path in the
DB, that that path belongs to that download.  It might not, yes, but I can't
come up with a consistent way of thinking about it that allows for that and
allows for resuming downloads across history persistence.  Can you?

As noted above, I'm good with just getting rid of the check.

https://codereview.chromium.org/14958002/diff/11001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/14958002/diff/11001/content/browser/download/...
content/browser/download/download_manager_impl.cc:393:
item_iterator->second->IsCancelled()) {
(This is mostly just rambling--the final conclusion is that I'm ok with this
code given context.)

I wince at this check; it "feels" like it should be in the DownloadItem.  I
can't come up offhand with a better way to do it, though; when I try, my mind
goes to "well, why shouldn't you be able to resume (== restart) a cancelled
download?" and that's for the future :-}.  And when I think about it some more,
I also feel like the rest of this function should be inside of DownloadItemImpl,
and my memory is that I tried to do that when I was writing it and it didn't
work very well (probably around the DownloadFileFactory being a member of
DownloadManagerImpl.)  So there's probably not a good way to clean this up, and
it'll disappear when we get to restarting cancelled DownloadItems.  So I'll go
with it :-}.

https://codereview.chromium.org/14958002/diff/11001/content/browser/download/...
content/browser/download/download_manager_impl.cc:401:
DCHECK(download->IsInterrupted());
Won't this DCHECK fail in the cancelled case?

[asanka, 2013-05-22 22:55:52]

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14958002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:422: // non-empty current_path_
that's the same as target_path_.
On 2013/05/21 21:40:37, rdsmith wrote:
> On 2013/05/21 21:09:02, asanka wrote:
> > On 2013/05/15 15:34:56, rdsmith wrote:
> > > I'm slightly confused.  If the download was important from the history and
> is
> > in
> > > the interrupted state, shouldn't we remove the current_path_?  What's the
> > > process by which we get into a state s.t. this extra code is needed?
> > > 
> > > Also, below in this file we're going to cancel the download; why doesn't
the
> > > code in Cancel for cancelling interrupted downloads take care of this
case?
> > 
> > https://codereview.chromium.org/11363222 introduced persisting the target
and
> > current paths. Since previously we only persisted full_path, the migration
> > script set both target and current path to full_path.
> > 
> > A download that was imported from history might look like the following:
> >   { current_path: 'foo.crdownload',
> >     target_path: 'foo.crdownload',
> >     state: INTERRUPTED }
> > 
> > The problem is that that prior to downloads resumption, the intermediate
file
> > would be deleted on interrupt with the exception of interruptions due to
> browser
> > crashes. So if 'foo.crdownload' exists, then its likely that it's associated
> > with another download. Removing it is probably the wrong thing to do.
> > 
> > Then again, a more recent download might look like:
> >   { current_path: 'foo.crdownload',
> >     target_path: 'foo',
> >     state: INTERRUPTED }
> > 
> > This download could suffer from the same problem (i.e. 'foo.crdownload' if
it
> > exists, may be associated with a different download).
> > 
> > I've removed the check for now and we aggressively delete the intermediate
> path
> > on Cancel()/Remove().
> > 
> > Also, you're right that we can rely on the deletion logic in Cancel() here.
> 
> I hear you about the entries from before we started persisting current path. 
> I'm ok with specifically checking for that, though I wince to have code that's
> put in to handle old database entries--we should figure out a way to update
the
> database entry once so that the problem doesn't exist and remove the code. 
> OTOH, I'm also good with just deleting the file and assuming that no user will
> have that problem very often :-}.
> 
> With regard to a more recent download, I'm afraid that we sorta have to assume
> that if there's an interrupted download with a non-empty() current_path in the
> DB, that that path belongs to that download.  It might not, yes, but I can't
> come up with a consistent way of thinking about it that allows for that and
> allows for resuming downloads across history persistence.  Can you?
> 
> As noted above, I'm good with just getting rid of the check.

Yeah. I got rid of the check.

https://codereview.chromium.org/14958002/diff/11001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/14958002/diff/11001/content/browser/download/...
content/browser/download/download_manager_impl.cc:393:
item_iterator->second->IsCancelled()) {
On 2013/05/21 21:40:37, rdsmith wrote:
> (This is mostly just rambling--the final conclusion is that I'm ok with this
> code given context.)
> 
> I wince at this check; it "feels" like it should be in the DownloadItem.  I
> can't come up offhand with a better way to do it, though; when I try, my mind
> goes to "well, why shouldn't you be able to resume (== restart) a cancelled
> download?" and that's for the future :-}.  And when I think about it some
more,
> I also feel like the rest of this function should be inside of
DownloadItemImpl,
> and my memory is that I tried to do that when I was writing it and it didn't
> work very well (probably around the DownloadFileFactory being a member of
> DownloadManagerImpl.)  So there's probably not a good way to clean this up,
and
> it'll disappear when we get to restarting cancelled DownloadItems.  So I'll go
> with it :-}.

Noted :)

https://codereview.chromium.org/14958002/diff/11001/content/browser/download/...
content/browser/download/download_manager_impl.cc:401:
DCHECK(download->IsInterrupted());
On 2013/05/21 21:40:37, rdsmith wrote:
> Won't this DCHECK fail in the cancelled case?

If the download is cancelled, the previous 'if' block would return early and
we'd never get here. This DCHECK is to check that interrupted downloads are the
only kind that goes through here.

[Randy Smith (Not in Mondays), 2013-05-23 13:58:12]

Still LGTM :-}.

[commit-bot: I haz the power, 2013-05-23 14:36:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/14958002/17001

[commit-bot: I haz the power, 2013-05-23 20:56:38]

Change committed as 201883