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

webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc

 // Copyright 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 "base/basictypes.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/message_loop/message_loop.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ skipping 165 matching lines @@
   FileSystemURL copy_dest_;
   FileSystemURL move_src_;
   FileSystemURL move_dest_;
 
   DISALLOW_COPY_AND_ASSIGN(CopyOrMoveFileValidatorTestHelper);
 };
 
 class TestCopyOrMoveFileValidatorFactory
     : public CopyOrMoveFileValidatorFactory {
  public:
+  enum Validity {
+    VALID,
+    PRE_WRITE_INVALID,
+    POST_WRITE_INVALID
+  };

[kinuko, 2013/08/01 07:37:24]

nit: since we're in a test code, it might make the code prettier if we define
this enum outside this class but in an anonymous namespace?  Then you can remove
all TestCopyOrMoveFileValidatorFactory:: prefixes.

Also if you separately handle pre- and post-write validity it might feel more
natural to use bitset value?

enum Validity {
  PRE_WRITE_VALID = 0x1,
  POST_WRITE_VALID = 0x2,
};

TestCopyOrMoveFileValidator(int validity)
    : result_((validity & PRE_WRITE_VALID) ? OK : ERROR_SECURITY),
      write_result_((validity & POST_WRITE_VALID) ? OK : ERROR_SECURITY)

[Greg Billock, 2013/08/01 16:23:05]

sounds good. I'll keep the enum to move complexity away from test code into the
fixture, but I put it at the top level to reduce name chewiness. Looks fine.

+
   // 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(
+        TestCopyOrMoveFileValidatorFactory::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() {}
 
     virtual void StartPreWriteValidation(
         const ResultCallback& result_callback) OVERRIDE {
       // Post the result since a real validator must do work asynchronously.
       base::MessageLoop::current()->PostTask(
           FROM_HERE, base::Bind(result_callback, result_));
     }
 
     virtual void StartPostWriteValidation(
         const base::FilePath& dest_platform_path,
         const ResultCallback& result_callback) OVERRIDE {
       // Post the result since a real validator must do work asynchronously.
       base::MessageLoop::current()->PostTask(
           FROM_HERE, base::Bind(result_callback, write_result_));
     }
 
    private:
     base::PlatformFileError result_;
     base::PlatformFileError write_result_;
 
     DISALLOW_COPY_AND_ASSIGN(TestCopyOrMoveFileValidator);
   };
 
-  bool all_valid_;
-  bool all_valid_write_;
+  Validity validity_;
 
   DISALLOW_COPY_AND_ASSIGN(TestCopyOrMoveFileValidatorFactory);
 };
 
 }  // namespace
 
 TEST(CopyOrMoveFileValidatorTest, NoValidatorWithinSameFSType) {
   // Within a file system type, validation is not expected, so it should
   // work for kWithValidatorType without a validator set.
   CopyOrMoveFileValidatorTestHelper helper(GURL("http://foo"),
@@ skipping 14 matching lines @@
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
   helper.MoveTest(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 TEST(CopyOrMoveFileValidatorTest, AcceptAll) {
   CopyOrMoveFileValidatorTestHelper helper(GURL("http://foo"),
                                            kNoValidatorType,
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
-      new TestCopyOrMoveFileValidatorFactory(true, true /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(
+          TestCopyOrMoveFileValidatorFactory::VALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_OK);
   helper.MoveTest(base::PLATFORM_FILE_OK);
 }
 
 TEST(CopyOrMoveFileValidatorTest, AcceptNone) {
   CopyOrMoveFileValidatorTestHelper helper(GURL("http://foo"),
                                            kNoValidatorType,
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
-      new TestCopyOrMoveFileValidatorFactory(false, false /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(
+          TestCopyOrMoveFileValidatorFactory::PRE_WRITE_INVALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
   helper.MoveTest(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 TEST(CopyOrMoveFileValidatorTest, OverrideValidator) {
   // Once set, you can not override the validator.
   CopyOrMoveFileValidatorTestHelper helper(GURL("http://foo"),
                                            kNoValidatorType,
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> reject_factory(
-      new TestCopyOrMoveFileValidatorFactory(false, false /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(
+          TestCopyOrMoveFileValidatorFactory::PRE_WRITE_INVALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(reject_factory.Pass());
 
   scoped_ptr<CopyOrMoveFileValidatorFactory> accept_factory(
-      new TestCopyOrMoveFileValidatorFactory(true, true /*accept_all*/));
+      new TestCopyOrMoveFileValidatorFactory(
+          TestCopyOrMoveFileValidatorFactory::VALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(accept_factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
   helper.MoveTest(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 TEST(CopyOrMoveFileValidatorTest, RejectPostWrite) {
   CopyOrMoveFileValidatorTestHelper helper(GURL("http://foo"),
                                            kNoValidatorType,
                                            kWithValidatorType);
   helper.SetUp();
   scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
-      // accept pre-copy, reject post-copy
-      new TestCopyOrMoveFileValidatorFactory(true, false));
+      new TestCopyOrMoveFileValidatorFactory(
+          TestCopyOrMoveFileValidatorFactory::POST_WRITE_INVALID));
   helper.SetMediaCopyOrMoveFileValidatorFactory(factory.Pass());
 
   helper.CopyTest(base::PLATFORM_FILE_ERROR_SECURITY);
   helper.MoveTest(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 }  // namespace fileapi