Implement DownloadFile::Rename() for posix. Downloads work on linux!

Implement DownloadFile::Rename() for posix. Downloads work on linux!


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

[Evan Stade, 2009-03-13 00:39:01]

well, in a very limited sense. Try this: navigate to a web page and right click
on an image, choose save as, save button, voila.

[tony, 2009-03-13 06:16:13]

Paul might know why the delete is there. LGTM.

http://codereview.chromium.org/46020/diff/1/2
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/46020/diff/1/2#newcode110
Line 110: return false;
If the copy fails, should you try to delete?

[Mark Larson, 2009-03-13 07:22:44]

http://codereview.chromium.org/46020/diff/1/2
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/46020/diff/1/2#newcode107
Line 107: // TODO(estade): copying sucks for large downloads. crbug.com/8737
Does it make sense to have file_util::Move implement the Copy-Delete when rename
fails?

Huan just implemented this in file_util_win to handle the same 'rename fails
across volumes' case:
http://src.chromium.org/viewvc/chrome?view=rev&revision=11460

[Evan Stade, 2009-03-13 18:57:39]

http://codereview.chromium.org/46020/diff/1/2
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/46020/diff/1/2#newcode107
Line 107: // TODO(estade): copying sucks for large downloads. crbug.com/8737
On 2009/03/13 07:22:44, Mark Larson wrote:
> Does it make sense to have file_util::Move implement the Copy-Delete when
rename
> fails?

yea, it would be good to match windows. Thanks for the pointer. But I don't get
why he exposed a CopyAndDeleteDirectory function. When will that ever be used? I
also don't get the unit test. Perhaps he exposed it for the unit test. But
Copy() and Delete() are already tested, so why go out of your way to test
CopyAndDelete()?

http://codereview.chromium.org/46020/diff/1/2#newcode110
Line 110: return false;
On 2009/03/13 06:16:13, tony wrote:
> If the copy fails, should you try to delete?

I guess it doesn't matter much since the old file is stored in a temporary
directory. If you don't delete, maybe the user can muck around in /tmp/ and
recover some of the data. Do you feel strongly either way?

[tony, 2009-03-13 19:05:54]

I don't feel strongly either way, but it seems to be what the windows code is
doing.  The windows code and non-windows code should be doing the same thing.

On 2009/03/13 18:57:39, estade wrote:
> http://codereview.chromium.org/46020/diff/1/2
> File chrome/browser/download/download_file.cc (right):
> 
> http://codereview.chromium.org/46020/diff/1/2#newcode107
> Line 107: // TODO(estade): copying sucks for large downloads. crbug.com/8737
> On 2009/03/13 07:22:44, Mark Larson wrote:
> > Does it make sense to have file_util::Move implement the Copy-Delete when
> rename
> > fails?
> 
> yea, it would be good to match windows. Thanks for the pointer. But I don't
get
> why he exposed a CopyAndDeleteDirectory function. When will that ever be used?
I
> also don't get the unit test. Perhaps he exposed it for the unit test. But
> Copy() and Delete() are already tested, so why go out of your way to test
> CopyAndDelete()?
> 
> http://codereview.chromium.org/46020/diff/1/2#newcode110
> Line 110: return false;
> On 2009/03/13 06:16:13, tony wrote:
> > If the copy fails, should you try to delete?
> 
> I guess it doesn't matter much since the old file is stored in a temporary
> directory. If you don't delete, maybe the user can muck around in /tmp/ and
> recover some of the data. Do you feel strongly either way?

[Evan Stade, 2009-03-13 19:11:50]

> I don't feel strongly either way, but it seems to be what the windows code is
> doing.  The windows code and non-windows code should be doing the same thing.

hm? The windows code returns early if the move fails and doesn't delete the
original file. Or are you talking about the file_util function? There, too, the
delete only occurs on a successful copy. Or am I missing something?

[tony, 2009-03-13 19:25:51]

On 2009/03/13 19:11:50, estade wrote:
> > I don't feel strongly either way, but it seems to be what the windows code
is
> > doing.  The windows code and non-windows code should be doing the same
thing.
> 
> hm? The windows code returns early if the move fails and doesn't delete the
> original file. Or are you talking about the file_util function? There, too,
the
> delete only occurs on a successful copy. Or am I missing something?

Oh, I see.  Then we can probably remove the delete call from the windows code
(as your todo suggests).  Let's do that then.  LGTM.