Fix up some tests for copy-or-move validator and nearby.

R=vandebo@chromium.org
BUG=None

Review URL: https://chromiumcodereview.appspot.com/21097005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215105 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 62 insertions, 32 deletions

diff --git a/webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc b/webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc
index 1190f7a..caecccf 100644
--- a/webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc
+++ b/webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc
@@ -180,31 +180,38 @@ class CopyOrMoveFileValidatorTestHelper {
   DISALLOW_COPY_AND_ASSIGN(CopyOrMoveFileValidatorTestHelper);
 };
 
+// For TestCopyOrMoveFileValidatorFactory
+enum Validity {
+  VALID,
+  PRE_WRITE_INVALID,
+  POST_WRITE_INVALID
+};
+
 class TestCopyOrMoveFileValidatorFactory
     : public CopyOrMoveFileValidatorFactory {
  public:
   // A factory that creates validators that accept everything or nothing.
-  explicit TestCopyOrMoveFileValidatorFactory(bool all_valid,
-                                              bool all_valid_write)
-      : all_valid_(all_valid),
-        all_valid_write_(all_valid_write) {}
+  // TODO(gbillock): switch args to enum or something
+  explicit TestCopyOrMoveFileValidatorFactory(Validity validity)
+      : validity_(validity) {}
   virtual ~TestCopyOrMoveFileValidatorFactory() {}
 
   virtual CopyOrMoveFileValidator* CreateCopyOrMoveFileValidator(
       const FileSystemURL& /*src_url*/,
       const base::FilePath& /*platform_path*/) OVERRIDE {
-    return new TestCopyOrMoveFileValidator(all_valid_, all_valid_write_);
+    return new TestCopyOrMoveFileValidator(validity_);
   }
 
  private:
   class TestCopyOrMoveFileValidator : public CopyOrMoveFileValidator {
    public:
-    explicit TestCopyOrMoveFileValidator(bool pre_copy_valid,
-                                         bool post_copy_valid)
-        : result_(pre_copy_valid ? base::PLATFORM_FILE_OK
-                                 : base::PLATFORM_FILE_ERROR_SECURITY),
-          write_result_(post_copy_valid ? base::PLATFORM_FILE_OK
-                                        : base::PLATFORM_FILE_ERROR_SECURITY) {
+    explicit TestCopyOrMoveFileValidator(Validity validity)
+        : result_(validity == VALID || validity == POST_WRITE_INVALID
+                  ? base::PLATFORM_FILE_OK
+                  : base::PLATFORM_FILE_ERROR_SECURITY),
+          write_result_(validity == VALID || validity == PRE_WRITE_INVALID
+                        ? base::PLATFORM_FILE_OK
+                        : base::PLATFORM_FILE_ERROR_SECURITY) {
     }
     virtual ~TestCopyOrMoveFileValidator() {}
 
@@ -230,8 +237,7 @@ class TestCopyOrMoveFileValidatorFactory
     DISALLOW_COPY_AND_ASSIGN(TestCopyOrMoveFileValidator);
   };
 
-  bool all_valid_;
-  bool all_valid_write_;
+  Validity validity_;
 
   DISALLOW_COPY_AND_ASSIGN(TestCopyOrMoveFileValidatorFactory);
 };
@@ -266,7 +272,7 @@ TEST(CopyOrMoveFileValidatorTest, AcceptAll) {
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
-      new TestCopyOrMoveFileValidatorFactory(true, true /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(VALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_OK);
@@ -279,7 +285,7 @@ TEST(CopyOrMoveFileValidatorTest, AcceptNone) {
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
-      new TestCopyOrMoveFileValidatorFactory(false, false /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(PRE_WRITE_INVALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
@@ -293,11 +299,11 @@ TEST(CopyOrMoveFileValidatorTest, OverrideValidator) {
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> reject_factory(
-      new TestCopyOrMoveFileValidatorFactory(false, false /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(PRE_WRITE_INVALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(reject_factory.Pass());
 
   scoped_ptr<CopyOrMoveFileValidatorFactory> accept_factory(
-      new TestCopyOrMoveFileValidatorFactory(true, true /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(VALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(accept_factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
@@ -310,8 +316,7 @@ TEST(CopyOrMoveFileValidatorTest, RejectPostWrite) {
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
-      // accept pre-copy, reject post-copy
-      new TestCopyOrMoveFileValidatorFactory(true, false));
+      new TestCopyOrMoveFileValidatorFactory(POST_WRITE_INVALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
diff --git a/webkit/browser/fileapi/copy_or_move_operation_delegate.cc b/webkit/browser/fileapi/copy_or_move_operation_delegate.cc
index 8746725..15e222d 100644
--- a/webkit/browser/fileapi/copy_or_move_operation_delegate.cc
+++ b/webkit/browser/fileapi/copy_or_move_operation_delegate.cc
@@ -110,9 +110,9 @@ void CopyOrMoveOperationDelegate::DidTryRemoveDestRoot(
   // and operation==MOVE case, probably we can just rename the root directory.
   // http://crbug.com/172187
   StartRecursiveOperation(
-      src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopyDir,
-                            AsWeakPtr(), src_root_,
-                            callback_));
+      src_root_,
+      base::Bind(&CopyOrMoveOperationDelegate::DidFinishRecursiveCopyDir,
+                 AsWeakPtr(), src_root_, callback_));
 }
 
 void CopyOrMoveOperationDelegate::CopyOrMoveFile(
@@ -190,7 +190,7 @@ void CopyOrMoveOperationDelegate::DidValidateFile(
   operation_runner()->CopyInForeignFile(platform_path, dest, callback);
 }
 
-void CopyOrMoveOperationDelegate::DidFinishCopyDir(
+void CopyOrMoveOperationDelegate::DidFinishRecursiveCopyDir(
     const FileSystemURL& src,
     const StatusCallback& callback,
     base::PlatformFileError error) {
@@ -200,7 +200,7 @@ void CopyOrMoveOperationDelegate::DidFinishCopyDir(
     return;
   }
 
-  DCHECK_EQ(operation_type_, OPERATION_MOVE);
+  DCHECK_EQ(OPERATION_MOVE, operation_type_);
 
   // Remove the source for finalizing move operation.
   operation_runner()->Remove(
diff --git a/webkit/browser/fileapi/copy_or_move_operation_delegate.h b/webkit/browser/fileapi/copy_or_move_operation_delegate.h
index 21cd942..3c40cae 100644
--- a/webkit/browser/fileapi/copy_or_move_operation_delegate.h
+++ b/webkit/browser/fileapi/copy_or_move_operation_delegate.h
@@ -75,7 +75,7 @@ class CopyOrMoveOperationDelegate
       const base::PlatformFileInfo& file_info,
       const base::FilePath& platform_path,
       base::PlatformFileError error);
-  void DidFinishCopyDir(
+  void DidFinishRecursiveCopyDir(
       const FileSystemURL& src,
       const StatusCallback& callback,
       base::PlatformFileError error);
diff --git a/webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc b/webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
index 95e73c6..183becd 100644
--- a/webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
+++ b/webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
@@ -46,6 +46,7 @@ class TestValidatorFactory : public CopyOrMoveFileValidatorFactory {
   virtual CopyOrMoveFileValidator* CreateCopyOrMoveFileValidator(
       const FileSystemURL& /*src_url*/,
       const base::FilePath& /*platform_path*/) OVERRIDE {
+    // Move arg management to TestValidator?
     return new TestValidator(true, true, std::string("2"));
   }
 
@@ -113,6 +114,15 @@ class CopyOrMoveOperationTestHelper {
   }
 
   void SetUp() {
+    SetUp(true, true);
+  }
+
+  void SetUpNoValidator() {
+    SetUp(true, false);
+  }
+
+  void SetUp(bool require_copy_or_move_validator,
+             bool init_copy_or_move_validator) {
     ASSERT_TRUE(base_.CreateUniqueTempDir());
     base::FilePath base_dir = base_.path();
     quota_manager_ =
@@ -138,8 +148,10 @@ class CopyOrMoveOperationTestHelper {
           static_cast<TestFileSystemBackend*>(backend);
       scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
           new TestValidatorFactory);
-      test_backend->set_require_copy_or_move_validator(true);
-      test_backend->InitializeCopyOrMoveFileValidatorFactory(factory.Pass());
+      test_backend->set_require_copy_or_move_validator(
+          require_copy_or_move_validator);
+      if (init_copy_or_move_validator)
+        test_backend->InitializeCopyOrMoveFileValidatorFactory(factory.Pass());
     }
     backend->OpenFileSystem(origin_, dest_type_,
                             OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
@@ -529,4 +541,22 @@ TEST(LocalFileSystemCopyOrMoveOperationTest,
                              arraysize(kMoveDirResultCases));
 }
 
+TEST(LocalFileSystemCopyOrMoveOperationTest, CopySingleFileNoValidator) {
+  CopyOrMoveOperationTestHelper helper(GURL("http://foo"),
+                                       kFileSystemTypeTemporary,
+                                       kFileSystemTypeTest);
+  helper.SetUpNoValidator();
+
+  FileSystemURL src = helper.SourceURL("a");
+  FileSystemURL dest = helper.DestURL("b");
+
+  // Set up a source file.
+  ASSERT_EQ(base::PLATFORM_FILE_OK, helper.CreateFile(src, 10));
+
+  // The copy attempt should fail with a security error -- getting
+  // the factory returns a security error, and the copy operation must
+  // respect that.
+  ASSERT_EQ(base::PLATFORM_FILE_ERROR_SECURITY, helper.Copy(src, dest));
+}
+
 }  // namespace fileapi
diff --git a/webkit/browser/fileapi/test_file_system_backend.cc b/webkit/browser/fileapi/test_file_system_backend.cc
index 569800f..584e2e3 100644
--- a/webkit/browser/fileapi/test_file_system_backend.cc
+++ b/webkit/browser/fileapi/test_file_system_backend.cc
@@ -185,11 +185,6 @@ TestFileSystemBackend::GetCopyOrMoveFileValidatorFactory(
 
 void TestFileSystemBackend::InitializeCopyOrMoveFileValidatorFactory(
     scoped_ptr<CopyOrMoveFileValidatorFactory> factory) {
-  // What purpose is this check serving?
-  if (!require_copy_or_move_validator_) {
-    DCHECK(!factory);
-    return;
-  }
   if (!copy_or_move_file_validator_factory_)
     copy_or_move_file_validator_factory_ = factory.Pass();
 }