[FileSystem] Add another copy-or-move validation hook for post-write.

webkit/browser/fileapi/copy_or_move_operation_delegate.cc

Index: webkit/browser/fileapi/copy_or_move_operation_delegate.cc
diff --git a/webkit/browser/fileapi/copy_or_move_operation_delegate.cc b/webkit/browser/fileapi/copy_or_move_operation_delegate.cc
index 8b706902b5c84396c675b884a01422d75f638b4b..29791b7bbbc4d8ec8c456a09c999755ae3bff5a7 100644
--- a/webkit/browser/fileapi/copy_or_move_operation_delegate.cc
+++ b/webkit/browser/fileapi/copy_or_move_operation_delegate.cc
@@ -111,7 +111,8 @@ void CopyOrMoveOperationDelegate::DidTryRemoveDestRoot(
   // http://crbug.com/172187
   StartRecursiveOperation(
       src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy,

[kinuko, 2013/07/12 05:00:19]

I overlooked this in my previous review, but when DidFinishCopy is called for
this case (and only for this case) the given url_pair contains directories, and
we won't want to run PostWriteValidation.  I think you need to separate out this
case, maybe splitting DidFinishCopy into two or passing another flag.

Also it might be better to add a test for recursive directory /w multiple files
case?

[Greg Billock, 2013/07/12 18:02:19]

OK. I'll add a directory-specific handler for this. Does the MoveDirectory test
already qualify? It looks to me like this moves a directory recursively with
multiples files in it. The test validator just always passes, so we're just
using what ought to be equivalent to the old code path in the test.

[vandebo (ex-Chrome), 2013/07/12 18:51:48]

Probably want a complementary test that fails verify at some middle point so
that you can ensure the code does what you expect (I don't know what to expect).

[Greg Billock, 2013/07/12 20:36:46]

validation is per-file at present, so you'd expect the move/copy to work except
for the one failed file. Should this work differently do you think?

I agree the behavior is currently somewhat difficult to follow, since there are
lots of cases combined (dir/file, move/copy). They share enough that it's
sensible to combine them, but it does have this result.

On 2013/07/12 18:51:48, vandebo wrote:

[vandebo (ex-Chrome), 2013/07/12 21:06:58]

We only get one result code, so do you say it worked, or failed if only some of
the files worked?  If you are doing a move, do you delete all the originals, the
originals that were ok, or none of the originals ?

[Greg Billock, 2013/07/12 22:06:32]

Yeah, the test shows this: it deletes the entire dest dir, and bails out on the
first error. Is that what we want? (Uploaded current state; this does not pass,
but let's figure out what we want to happen for post-write failure first.)

On 2013/07/12 21:06:58, vandebo wrote:

-                            AsWeakPtr(), src_root_, callback_));
+                            AsWeakPtr(), URLPair(src_root_, dest_root_),
+                            callback_));
 }
 
 void CopyOrMoveOperationDelegate::CopyOrMoveFile(const URLPair& url_pair,
@@ -131,7 +132,7 @@ void CopyOrMoveOperationDelegate::CopyOrMoveFile(const URLPair& url_pair,
   // copy_callback which removes the source file if operation_type == MOVE.
   StatusCallback copy_callback =
       base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, AsWeakPtr(),
-                 url_pair.src, callback);
+                 url_pair, callback);
   operation_runner()->CreateSnapshotFile(
       url_pair.src,
       base::Bind(&CopyOrMoveOperationDelegate::DidCreateSnapshot, AsWeakPtr(),
@@ -189,11 +190,65 @@ void CopyOrMoveOperationDelegate::DidValidateFile(
 }
 
 void CopyOrMoveOperationDelegate::DidFinishCopy(
-    const FileSystemURL& src,
+    const URLPair& url_pair,
     const StatusCallback& callback,
     base::PlatformFileError error) {
-  if (error != base::PLATFORM_FILE_OK ||
-      operation_type_ == OPERATION_COPY) {
+  if (error != base::PLATFORM_FILE_OK) {
+    callback.Run(error);
+    return;
+  }
+
+  // |validator_| must not exist in the same-filesystem case.
+  if (!validator_.get()) {
+    scoped_refptr<webkit_blob::ShareableFileReference> file_ref;
+    DidPostWriteValidation(url_pair, callback, file_ref,
+                           base::PLATFORM_FILE_OK);
+    return;
+  }
+
+  DCHECK(!same_file_system_);
+  operation_runner()->CreateSnapshotFile(
+      url_pair.dest,
+      base::Bind(&CopyOrMoveOperationDelegate::DoPostWriteValidation,
+                 AsWeakPtr(), url_pair, callback));
+}
+
+void CopyOrMoveOperationDelegate::DoPostWriteValidation(
+    const URLPair& url_pair,
+    const StatusCallback& callback,
+    base::PlatformFileError error,
+    const base::PlatformFileInfo& file_info,
+    const base::FilePath& platform_path,
+    const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref) {
+  if (error != base::PLATFORM_FILE_OK) {
+    operation_runner()->Remove(
+        url_pair.dest, true,
+        base::Bind(&CopyOrMoveOperationDelegate::DidRemoveDestForError,
+                   AsWeakPtr(), error, callback));
+    return;
+  }
+
+  DCHECK(validator_.get());
+  validator_->StartPostWriteValidation(

[vandebo (ex-Chrome), 2013/07/12 15:08:15]

Probably should add a comment about keeping file_ref alive until after
PostWriteValidation finishes, otherwise someone might come along later are
remove the "unused" argument.

[Greg Billock, 2013/07/12 18:02:19]

Done.

+      platform_path,
+      base::Bind(&CopyOrMoveOperationDelegate::DidPostWriteValidation,
+                 AsWeakPtr(), url_pair, callback, file_ref));
+}
+
+void CopyOrMoveOperationDelegate::DidPostWriteValidation(
+    const URLPair& url_pair,
+    const StatusCallback& callback,
+    const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref,

[vandebo (ex-Chrome), 2013/07/12 15:08:15]

nit: file_ref isn't used in this context, so /*file ref*/

[Greg Billock, 2013/07/12 18:02:19]

Done.

+    base::PlatformFileError error) {
+  if (error != base::PLATFORM_FILE_OK) {
+    operation_runner()->Remove(
+        url_pair.dest, true,
+        base::Bind(&CopyOrMoveOperationDelegate::DidRemoveDestForError,
+                   AsWeakPtr(), error, callback));
+    return;
+  }
+
+  if (operation_type_ == OPERATION_COPY) {
     callback.Run(error);
     return;
   }
@@ -202,7 +257,7 @@ void CopyOrMoveOperationDelegate::DidFinishCopy(
 
   // Remove the source for finalizing move operation.
   operation_runner()->Remove(
-      src, true /* recursive */,
+      url_pair.src, true /* recursive */,
       base::Bind(&CopyOrMoveOperationDelegate::DidRemoveSourceForMove,
                  AsWeakPtr(), callback));
 }
@@ -229,4 +284,11 @@ FileSystemURL CopyOrMoveOperationDelegate::CreateDestURL(
       relative);
 }
 
+void CopyOrMoveOperationDelegate::DidRemoveDestForError(
+    base::PlatformFileError prior_error,
+    const StatusCallback& callback,
+    base::PlatformFileError error) {
+  callback.Run(prior_error);

[vandebo (ex-Chrome), 2013/07/12 15:08:15]

nit: VLOG if error != OK ?

[Greg Billock, 2013/07/12 18:02:19]

Done.

+}
+
 }  // namespace fileapi