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

webkit/browser/fileapi/copy_or_move_operation_delegate.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/browser/fileapi/copy_or_move_operation_delegate.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "webkit/browser/fileapi/copy_or_move_file_validator.h"
 #include "webkit/browser/fileapi/file_system_context.h"
@@ skipping 93 matching lines @@
     callback_.Run(error);
     return;
   }
 
   // Start to process the source directory recursively.
   // TODO(kinuko): This could be too expensive for same_file_system_==true
   // and operation==MOVE case, probably we can just rename the root directory.
   // http://crbug.com/172187
   StartRecursiveOperation(
       src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy,
-                            AsWeakPtr(), src_root_, callback_));
+                            AsWeakPtr(), URLPair(src_root_, dest_root_),
+                            callback_));
 }
 
 void CopyOrMoveOperationDelegate::CopyOrMoveFile(const URLPair& url_pair,
                                             const StatusCallback& callback) {
   // Same filesystem case.
   if (same_file_system_) {
     if (operation_type_ == OPERATION_MOVE) {
       operation_runner()->MoveFileLocal(url_pair.src, url_pair.dest, callback);
     } else {
       operation_runner()->CopyFileLocal(url_pair.src, url_pair.dest, callback);
     }
     return;
   }
 
   // Cross filesystem case.
   // Perform CreateSnapshotFile, CopyInForeignFile and then calls
   // 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(),
                  url_pair, copy_callback));
 }
 
 void CopyOrMoveOperationDelegate::DidCreateSnapshot(
     const URLPair& url_pair,
     const StatusCallback& callback,
     base::PlatformFileError error,
@@ skipping 37 matching lines @@
     base::PlatformFileError error) {
   if (error != base::PLATFORM_FILE_OK) {
     callback.Run(error);
     return;
   }
 
   operation_runner()->CopyInForeignFile(platform_path, dest, callback);
 }
 
 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.

[vandebo (ex-Chrome), 2013/07/10 15:52:46]

This comment is odd.  I think it means |validator_| will be null in the
same-filesystem case.  But will we even hit this code path in the
same-filesystem case?

[Greg Billock, 2013/07/11 22:54:35]

Yes, we can hit it same-filesystem, but we won't have set validator_ at all --
we bail out in CopyOrMoveFile above.

Aren't "must not exist" and "must be NULL" equivalent? I can change it.

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

validator_ would work in the same filesystem case, we decided that we don't need
it though.  So saying |validator_| will be NULL in the same file system case, or
when the dest filesystem doesn't do validation would be more accurate.

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

Done.

+  if (!validator_.get()) {
+    DidPostWriteValidation(url_pair, callback, 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));

[vandebo (ex-Chrome), 2013/07/10 15:52:46]

nit: you could bind error to callback and pass it as a closure

[Greg Billock, 2013/07/11 22:54:35]

Yeah. I don't think that'd improve this, though.

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

I think it makes DidRemoveDestForError a bit more clear, but what ever.

+    return;
+  }
+
+  DCHECK(validator_.get());
+  validator_->StartPostWriteValidation(
+      platform_path, file_ref,
+      base::Bind(&CopyOrMoveOperationDelegate::DidPostWriteValidation,
+                 AsWeakPtr(), url_pair, callback));

[vandebo (ex-Chrome), 2013/07/10 15:52:46]

Yea, pass file_ref to DidPostWriteValidation so that the validator doesn't need
to deal with it.

[Greg Billock, 2013/07/11 22:54:35]

I see. That's a good point.

+}
+
+void CopyOrMoveOperationDelegate::DidPostWriteValidation(
+    const URLPair& url_pair,
+    const StatusCallback& callback,
+    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;
+  }
 
   DCHECK_EQ(OPERATION_MOVE, operation_type_);
 
   // Remove the source for finalizing move operation.
   operation_runner()->Remove(
-      src, true /* recursive */,
+      url_pair.src, true /* recursive */,
       base::Bind(&CopyOrMoveOperationDelegate::DidRemoveSourceForMove,
                  AsWeakPtr(), callback));
 }
 
 void CopyOrMoveOperationDelegate::DidRemoveSourceForMove(
     const StatusCallback& callback,
     base::PlatformFileError error) {
   if (error == base::PLATFORM_FILE_ERROR_NOT_FOUND)
     error = base::PLATFORM_FILE_OK;
   callback.Run(error);
 }
 
 FileSystemURL CopyOrMoveOperationDelegate::CreateDestURL(
     const FileSystemURL& src_url) const {
   DCHECK_EQ(src_root_.type(), src_url.type());
   DCHECK_EQ(src_root_.origin(), src_url.origin());
 
   base::FilePath relative = dest_root_.virtual_path();
   src_root_.virtual_path().AppendRelativePath(src_url.virtual_path(),
                                               &relative);
   return file_system_context()->CreateCrackedFileSystemURL(
       dest_root_.origin(),
       dest_root_.mount_type(),
       relative);
 }
 
+void CopyOrMoveOperationDelegate::DidRemoveDestForError(
+    base::PlatformFileError prior_error,
+    const StatusCallback& callback,
+    base::PlatformFileError error) {
+  callback.Run(prior_error);
+}
+
 }  // namespace fileapi