Rename the download to its final name only after the download is finished

Rename the download to its final name only after the download is finished

BUG=27687, 28928
TEST=DownloadManagerTest.DownloadRenameTest

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

[kinuko, 2010-07-02 21:20:16]

I think something like this should work.  Can you take a look at it?

[Nico, 2010-07-02 21:26:24]

Drive-by: ".download" is a known extension of what safari uses for its
downloaded files on OS X (behind the scenes, a .download file is a bundle, which
is mostly a directory on os x). Maybe .crdownload?

[Avi (use Gerrit), 2010-07-02 21:44:37]

This will likely fix bug 28928. When you commit this, can you mark that as fixed
so it gets tested?

[kinuko, 2010-07-02 23:41:52]

On 2010/07/02 21:26:24, Nico wrote:
> Drive-by: ".download" is a known extension of what safari uses for its
> downloaded files on OS X (behind the scenes, a .download file is a bundle,
which
> is mostly a directory on os x). Maybe .crdownload?

Changed .download to .crdownload.

In this patch .crdownload file is used just as a placeholder - we can get rid of
the file if people will be still happy with having an empty file (kept empty)
with the final filename while downloading.  Seems like this is what FireFox
does.

On 2010/07/02 21:44:37, Avi wrote:
> This will likely fix bug 28928. When you commit this, can you mark that as
fixed
> so it gets tested?

I (blindly) added 28928 to the BUG= line.

[Lei Zhang, 2010-07-05 21:17:25]

On 2010/07/02 21:20:16, Kinuko Yasuda wrote:
> I think something like this should work.  Can you take a look at it?

I tried this out and found a couple issues:

a) For a non-dangerous download foo.zip without any user prompts, we download to
a temporary file, and create a dummy foo.zip.crdownload. When the download
finishes, we rename the temporary file to foo.zip and delete foo.zip.crdownload.
Wouldn't it be easier to download to foo.zip.crdownload and rename it to foo.zip
upon completion?

b) For the download in case (a): While the download is in progress, if we cancel
the download, we will call DownloadFile::Cancel(), which deletes |full_path_|,
but |full_path_| contains the path to the temporary file, so this would forget
to delete foo.zip.crdownload.

[kinuko, 2010-07-08 22:47:34]

Thanks for your comments,

On 2010/07/05 21:17:25, Lei Zhang wrote:
> a) For a non-dangerous download foo.zip without any user prompts, we download
to
> a temporary file, and create a dummy foo.zip.crdownload. When the download
> finishes, we rename the temporary file to foo.zip and delete
foo.zip.crdownload.
> Wouldn't it be easier to download to foo.zip.crdownload and rename it to
foo.zip
> upon completion?

I've been wondering if I should do this.  Currently we start downloading files
right after we get notified from I/O thread and then determine/check the final
writable/unique path.  To start with foo.zip.crdownload we need to reverse the
order and slightly delay downloading.  It won't make a big difference in
downloading time but I'm not strongly convinced to do so yet.  Also for save-as
cases we'll have both .crdownload and a temporary file anyway.  How do you
think?

> b) For the download in case (a): While the download is in progress, if we
cancel
> the download, we will call DownloadFile::Cancel(), which deletes |full_path_|,
> but |full_path_| contains the path to the temporary file, so this would forget
> to delete foo.zip.crdownload.

Good catch, I made a fix.

[kinuko, 2010-07-08 23:00:19]

On 2010/07/08 22:47:34, Kinuko Yasuda wrote:
> Thanks for your comments,
> 
> On 2010/07/05 21:17:25, Lei Zhang wrote:
> > a) For a non-dangerous download foo.zip without any user prompts, we
download
> to
> > a temporary file, and create a dummy foo.zip.crdownload. When the download
> > finishes, we rename the temporary file to foo.zip and delete
> foo.zip.crdownload.
> > Wouldn't it be easier to download to foo.zip.crdownload and rename it to
> foo.zip
> > upon completion?
> 
> I've been wondering if I should do this.  Currently we start downloading files
> right after we get notified from I/O thread and then determine/check the final
> writable/unique path.  To start with foo.zip.crdownload we need to reverse the
> order and slightly delay downloading.  It won't make a big difference in
> downloading time but I'm not strongly convinced to do so yet.  Also for
save-as
> cases we'll have both .crdownload and a temporary file anyway.  How do you
> think?

Alternatively we can rename the file twice for large files.
E.g. start downloading in a temporary file -> (determine the final name) ->
rename to the final name if it's finished, otherwise rename to
foo.zip.crdownload -> rename again to the final name.

The second rename would be required only for very large files and it wouldn't
add extra overhead.

> > b) For the download in case (a): While the download is in progress, if we
> cancel
> > the download, we will call DownloadFile::Cancel(), which deletes
|full_path_|,
> > but |full_path_| contains the path to the temporary file, so this would
forget
> > to delete foo.zip.crdownload.
> 
> Good catch, I made a fix.

[Lei Zhang, 2010-07-08 23:10:49]

On 2010/07/08 23:00:19, Kinuko Yasuda wrote:
> Alternatively we can rename the file twice for large files.
> E.g. start downloading in a temporary file -> (determine the final name) ->
> rename to the final name if it's finished, otherwise rename to
> foo.zip.crdownload -> rename again to the final name.

I'm in favor of this. I don't think we should delay downloading until the user
has selected a filename.

[kinuko, 2010-07-09 00:37:16]

Updated the patch to make it rename twice.
It still does create an empty .crdownload just to avoid allocating the same name
to multiple downloads (before we actually rename).

On 2010/07/08 23:10:49, Lei Zhang wrote:
> On 2010/07/08 23:00:19, Kinuko Yasuda wrote:
> > Alternatively we can rename the file twice for large files.
> > E.g. start downloading in a temporary file -> (determine the final name) ->
> > rename to the final name if it's finished, otherwise rename to
> > foo.zip.crdownload -> rename again to the final name.
> 
> I'm in favor of this. I don't think we should delay downloading until the user
> has selected a filename.

Just to be clear, I meant non-user-interaction cases.  The download manager
automatically determines the file name but chrome starts downloading even
without waiting it.

[Lei Zhang, 2010-07-15 01:52:14]

A few more issues:

c) If I cancel a download that's in progress, it tries to delete
'foo.crdownload' and also 'foo.crdownload.crdownload' .

d) If you accept a dangerous download, and then cancel it before it finishes, it
shows up as say, foo.exe in the download dialog. If you restart the browser, it
then shows up as 'unconfirmed NNNN.crdownload'.

http://codereview.chromium.org/2877008/diff/37001/38004
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/2877008/diff/37001/38004#newcode234
chrome/browser/download/download_manager.cc:234: if (safety_state_ ==
DownloadItem::SAFE && !save_as_)
This looks like a response to issue (b) I posted earlier, but is this still
needed in patch set 6? It looks like DownloadFile::Cancel() now deletes
foo.crdownload, and that makes this redundant.

[kinuko, 2010-07-15 22:02:43]

Thanks for reviewing.

On 2010/07/15 01:52:14, Lei Zhang wrote:
> c) If I cancel a download that's in progress, it tries to delete
> 'foo.crdownload' and also 'foo.crdownload.crdownload' .

Fixed.

> d) If you accept a dangerous download, and then cancel it before it finishes,
it
> shows up as say, foo.exe in the download dialog. If you restart the browser,
it
> then shows up as 'unconfirmed NNNN.crdownload'.

I think this has been an existing issue.  Can we open a separate bug for this?

http://codereview.chromium.org/2877008/diff/37001/38004
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/2877008/diff/37001/38004#newcode234
chrome/browser/download/download_manager.cc:234: if (safety_state_ ==
DownloadItem::SAFE && !save_as_)
On 2010/07/15 01:52:15, Lei Zhang wrote:
> This looks like a response to issue (b) I posted earlier, but is this still
> needed in patch set 6? It looks like DownloadFile::Cancel() now deletes
> foo.crdownload, and that makes this redundant.

Fixed.  Actually since now we renames to .crdownload we didn't need this even in
DownloadFile::Cancel().

[Lei Zhang, 2010-07-16 21:06:48]

I filed bug 49220 for problem (d).

http://codereview.chromium.org/2877008/diff/54001/4002
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/2877008/diff/54001/4002#newcode131
chrome/browser/download/download_file.cc:131: FilePath crdownload =
download_util::GetCrDownloadPath(new_path);
This works for the case where the download is in progress. But in the case where
the download is still in progress, |new_path| is foo.crdownload, so end up
deleting foo.crdownload.crdownload.

[Lei Zhang, 2010-07-16 21:07:25]

Correction: This works for the case where the download is -finished-. But in the
case where the download is still in progress, |new_path| is foo.crdownload, so
end up deleting foo.crdownload.crdownload.

[kinuko, 2010-07-16 23:30:46]

http://codereview.chromium.org/2877008/diff/54001/4002
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/2877008/diff/54001/4002#newcode131
chrome/browser/download/download_file.cc:131: FilePath crdownload =
download_util::GetCrDownloadPath(new_path);
On 2010/07/16 21:06:48, Lei Zhang wrote:
> This works for the case where the download is in progress. But in the case
where
> the download is still in progress, |new_path| is foo.crdownload, so end up
> deleting foo.crdownload.crdownload.

Fixed.

I added another flag to OnFinalDownloadName for the erroneous .crdownload
deletion.
So iiuc now the method can be called in 4 different cases:

1. tmp -> foo.crdownload  (is_final_rename=false, need_delete_crdownload=false)
2. tmp -> foo  (is_final_rename=true, need_delete_crdownload=true)
3. foo.crdownload -> foo  (is_final_rename=true, need_delete_crdownload=false)
4. tmp -> unconfirmed.xxx.crdownload  (is_final_rename=true,
need_delete_crdownload=false)
(No cases for is_final_rename=false, need_delete_crdownload=true)

[Nico, 2010-07-16 23:32:47]

Drive-by

On 2010/07/16 23:30:46, Kinuko Yasuda wrote:
> http://codereview.chromium.org/2877008/diff/54001/4002
> File chrome/browser/download/download_file.cc (right):
> 
> http://codereview.chromium.org/2877008/diff/54001/4002#newcode131
> chrome/browser/download/download_file.cc:131: FilePath crdownload =
> download_util::GetCrDownloadPath(new_path);
> On 2010/07/16 21:06:48, Lei Zhang wrote:
> > This works for the case where the download is in progress. But in the case
> where
> > the download is still in progress, |new_path| is foo.crdownload, so end up
> > deleting foo.crdownload.crdownload.
> 
> Fixed.
> 
> I added another flag to OnFinalDownloadName for the erroneous .crdownload
> deletion.
> So iiuc now the method can be called in 4 different cases:
> 
> 1. tmp -> foo.crdownload  (is_final_rename=false,
need_delete_crdownload=false)
> 2. tmp -> foo  (is_final_rename=true, need_delete_crdownload=true)
> 3. foo.crdownload -> foo  (is_final_rename=true, need_delete_crdownload=false)
> 4. tmp -> unconfirmed.xxx.crdownload  (is_final_rename=true,
> need_delete_crdownload=false)
> (No cases for is_final_rename=false, need_delete_crdownload=true)

Can you add unit tests for these cases?

[kinuko, 2010-07-19 21:29:37]

On 2010/07/16 23:32:47, Nico wrote:
> > 1. tmp -> foo.crdownload  (is_final_rename=false,
> need_delete_crdownload=false)
> > 2. tmp -> foo  (is_final_rename=true, need_delete_crdownload=true)
> > 3. foo.crdownload -> foo  (is_final_rename=true,
need_delete_crdownload=false)
> > 4. tmp -> unconfirmed.xxx.crdownload  (is_final_rename=true,
> > need_delete_crdownload=false)
> > (No cases for is_final_rename=false, need_delete_crdownload=true)
> 
> Can you add unit tests for these cases?

I added a unit test for possible download/rename sequences.

[Lei Zhang, 2010-07-20 00:05:39]

LGTM

http://codereview.chromium.org/2877008/diff/75001/51012
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/2877008/diff/75001/51012#newcode808
chrome/browser/download/download_manager_unittest.cc:808: // download is owned
by DownloadFileManager.
nit: |download|