Support removeRecursively and new copy/move behaviors for FileSystem API

Support removeRecursively and new copy/move behaviors added to the spec recently.

http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/1101.html
> For a move/copy of a file on top of existing file, or a directory on
> top of an existing empty directory, you get an automatic overwrite.
> A move/copy of a file on top of an existing directory, or of a
> directory on top of an existing file, will always fail.
> A move/copy of a file or directory on top of an existing non-empty
> directory will always fail.

BUG=32277
TEST=FileSystemOperationTest.*

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

[Kavita Kanetkar, 2010-10-04 23:35:10]

LGTM

Thanks for adding those test cases.

http://codereview.chromium.org/3567012/diff/1/2
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3567012/diff/1/2#newcode45
base/file_util_proxy.cc:45: // It is an error to copy/move an entroy to a
non-empty directory.
entry typo.

http://codereview.chromium.org/3567012/diff/1/2#newcode47
base/file_util_proxy.cc:47: // the file_util's Copy or Move method doesn't
perform overwrite
Is it worth making change in file_util instead here? Or file_util needs to stick
to OS-specific functionality?

http://codereview.chromium.org/3567012/diff/1/3
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3567012/diff/1/3#newcode109
base/file_util_proxy.h:109: // Deletes a file or empty directory.
So basically if dir is non-empty we rely on --recursive? Can we update comment?

http://codereview.chromium.org/3567012/diff/1/9
File chrome/common/file_system/webfilesystem_impl.cc (right):

http://codereview.chromium.org/3567012/diff/1/9#newcode55
chrome/common/file_system/webfilesystem_impl.cc:55: WebFileSystemCallbacks*
callbacks) {
indent

[kinuko, 2010-10-04 23:50:05]

Thanks!  Updated.

http://codereview.chromium.org/3567012/diff/1/2
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3567012/diff/1/2#newcode45
base/file_util_proxy.cc:45: // It is an error to copy/move an entroy to a
non-empty directory.
On 2010/10/04 23:35:10, Kavita Kanetkar wrote:
> entry typo.

Done.

http://codereview.chromium.org/3567012/diff/1/2#newcode47
base/file_util_proxy.cc:47: // the file_util's Copy or Move method doesn't
perform overwrite
On 2010/10/04 23:35:10, Kavita Kanetkar wrote:
> Is it worth making change in file_util instead here? Or file_util needs to
stick
> to OS-specific functionality?

Hmm it's scary to change file_util's methods as they are used everywhere...
instead added a TODO comment.

http://codereview.chromium.org/3567012/diff/1/3
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3567012/diff/1/3#newcode109
base/file_util_proxy.h:109: // Deletes a file or empty directory.
On 2010/10/04 23:35:10, Kavita Kanetkar wrote:
> So basically if dir is non-empty we rely on --recursive? Can we update
comment?

Done.

http://codereview.chromium.org/3567012/diff/1/9
File chrome/common/file_system/webfilesystem_impl.cc (right):

http://codereview.chromium.org/3567012/diff/1/9#newcode55
chrome/common/file_system/webfilesystem_impl.cc:55: WebFileSystemCallbacks*
callbacks) {
On 2010/10/04 23:35:10, Kavita Kanetkar wrote:
> indent

Done.

[ericu, 2010-10-05 01:14:03]

http://codereview.chromium.org/3567012/diff/6001/7001
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3567012/diff/6001/7001#newcode22
base/file_util_proxy.cc:22: // Neither |dest_file_path| nor its parent exist.
Do we need to check dest_file_path here? Isn't it enough to check the parent?

Also, perhaps you could add another easy-out: if dest_file_path doesn't exist,
return PLATFORM_FILE_OK and simplify the following tests.

http://codereview.chromium.org/3567012/diff/6001/7001#newcode40
base/file_util_proxy.cc:40: // It is an error to try to copy/move an entry into
its parent.
s/parent/child/ but I think the code is correct.
It's an error to copy/move into the parent without supplying a new name; where
is that checked?

http://codereview.chromium.org/3567012/diff/6001/7001#newcode50
base/file_util_proxy.cc:50: if (!file_util::Delete(dest_file_path, false /*
recursive */)) {
From the name of this function, I wouldn't have guessed that it would modify the
filesystem.  I can't think of a great name off the top of my head, so if you
can't either, please add a big warning comment about it.

http://codereview.chromium.org/3567012/diff/6001/7001#newcode285
base/file_util_proxy.cc:285: if (src_file_path_.value() ==
dest_file_path_.value()) {
Should we check for this before we try the copy?

http://codereview.chromium.org/3567012/diff/6001/7015
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/3567012/diff/6001/7015#newcode90
webkit/fileapi/file_system_operation_unittest.cc:90: // create temp and then
create non-existing file paths under it.
This comment isn't accurate.  Many tests don't use base_ at all.

http://codereview.chromium.org/3567012/diff/6001/7015#newcode229
webkit/fileapi/file_system_operation_unittest.cc:229:
EXPECT_FALSE(file_util::DirectoryExists(src_dir.path()));
I think it would be worthwhile to verify that the src_dir actually overwrote
dest_dir, as opposed to becoming a child of it [as would happen if one tried to
do this through the shell].

Same for the move copy test below.

[kinuko, 2010-10-05 02:29:39]

http://codereview.chromium.org/3567012/diff/6001/7001
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3567012/diff/6001/7001#newcode22
base/file_util_proxy.cc:22: // Neither |dest_file_path| nor its parent exist.
On 2010/10/05 01:14:03, ericu wrote:
> Do we need to check dest_file_path here? Isn't it enough to check the parent?
> 
> Also, perhaps you could add another easy-out: if dest_file_path doesn't exist,
> return PLATFORM_FILE_OK and simplify the following tests.

Done.

http://codereview.chromium.org/3567012/diff/6001/7001#newcode40
base/file_util_proxy.cc:40: // It is an error to try to copy/move an entry into
its parent.
On 2010/10/05 01:14:03, ericu wrote:
> s/parent/child/ but I think the code is correct.

Fixed.

> It's an error to copy/move into the parent without supplying a new name; where
> is that checked?

The caller should supply the full path of the destination path.  If src path is
"/a/b" and dest path is "/a" it means it's trying to overwrite the /a and it
must fail (since it's not empty).

http://codereview.chromium.org/3567012/diff/6001/7001#newcode50
base/file_util_proxy.cc:50: if (!file_util::Delete(dest_file_path, false /*
recursive */)) {
On 2010/10/05 01:14:03, ericu wrote:
> From the name of this function, I wouldn't have guessed that it would modify
the
> filesystem.  I can't think of a great name off the top of my head, so if you
> can't either, please add a big warning comment about it.

Good point, done.

http://codereview.chromium.org/3567012/diff/6001/7001#newcode285
base/file_util_proxy.cc:285: if (src_file_path_.value() ==
dest_file_path_.value()) {
On 2010/10/05 01:14:03, ericu wrote:
> Should we check for this before we try the copy?

We can do that, this situation will anyway fail in CopyDirectory though.  (We
usually defer those checks.)

http://codereview.chromium.org/3567012/diff/6001/7015
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/3567012/diff/6001/7015#newcode90
webkit/fileapi/file_system_operation_unittest.cc:90: // create temp and then
create non-existing file paths under it.
On 2010/10/05 01:14:03, ericu wrote:
> This comment isn't accurate.  Many tests don't use base_ at all.

I think I fixed most of them.  (The dir is for 'nonexisting' paths so "many
tests don't use it at all" still holds.)
Also I removed the word 'all' from the comment just in case.

http://codereview.chromium.org/3567012/diff/6001/7015#newcode229
webkit/fileapi/file_system_operation_unittest.cc:229:
EXPECT_FALSE(file_util::DirectoryExists(src_dir.path()));
On 2010/10/05 01:14:03, ericu wrote:
> I think it would be worthwhile to verify that the src_dir actually overwrote
> dest_dir, as opposed to becoming a child of it [as would happen if one tried
to
> do this through the shell].
> 
> Same for the move copy test below.

Done.

[ericu, 2010-10-05 02:46:31]

http://codereview.chromium.org/3567012/diff/3002/29015
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/3567012/diff/3002/29015#newcode118
webkit/fileapi/file_system_operation_unittest.cc:118:
operation()->Copy(src_dir.path(), dest_dir_path);
This was a move test, and now there's a copy here.

That comment up top made it look like base_ was supposed to be the parent
directory containing all test subdirectories.  The change here looks like the
opposite--you've moved the test directory out of base_.  I don't think we
necessarily need to use base_ at all; if CreateTemporary* works to create it, it
works just as well to create all the others.  I'm more inclined to fix the
comment [leaving base_ as just a convenient directory for multiple
nondestructive uses] than have you change lots of tests.

There's nothing wrong with the changes [other than the move->copy typo], but we
should fix the comment.

http://codereview.chromium.org/3567012/diff/3002/29015#newcode237
webkit/fileapi/file_system_operation_unittest.cc:237: // Make sure we've
overwriten but not moved the source under the |dest_dir|.
typo: "overwritten" [here and below].

[kinuko, 2010-10-05 03:16:11]

http://codereview.chromium.org/3567012/diff/3002/29015
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/3567012/diff/3002/29015#newcode118
webkit/fileapi/file_system_operation_unittest.cc:118:
operation()->Copy(src_dir.path(), dest_dir_path);
On 2010/10/05 02:46:32, ericu wrote:
> This was a move test, and now there's a copy here.

Ouch... fixed.

> That comment up top made it look like base_ was supposed to be the parent
> directory containing all test subdirectories.  The change here looks like the
> opposite--you've moved the test directory out of base_.

Most of the changes I made were necessary -- this one was creating a directory
under base_ that breaks the assumption of base_ (nondestructive uses only) and
the other one was trying to copy into nonexisting directory, that may throw
another error than the expected one depending on the check order.

> works just as well to create all the others.  I'm more inclined to fix the
> comment [leaving base_ as just a convenient directory for multiple
> nondestructive uses] than have you change lots of tests.

I fixed the comment -- interestingly I think I (and Kavita) wrote the original
comment to mean the same thing.  It wasn't meant to be used by *all* the tests
-- it is for building a 'nonexistent' paths, i.e. for non destructive uses only.

http://codereview.chromium.org/3567012/diff/3002/29015#newcode237
webkit/fileapi/file_system_operation_unittest.cc:237: // Make sure we've
overwriten but not moved the source under the |dest_dir|.
On 2010/10/05 02:46:32, ericu wrote:
> typo: "overwritten" [here and below].

Done.

[ericu, 2010-10-05 04:20:11]

LGTM