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

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

R=vandebo@chromium.org
BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215105

[vandebo (ex-Chrome), 2013-07-31 18:45:06]

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right):

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:187: ALL_VALID,
How about VALID, PRE_WRITE_INVALID, and POST_WRITE_INVALID ?

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:209: explicit
TestCopyOrMoveFileValidator(bool pre_copy_valid,
propagate enum down.

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
File webkit/browser/fileapi/test_file_system_backend.cc (left):

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
webkit/browser/fileapi/test_file_system_backend.cc:189: if
(!require_copy_or_move_validator_) {
Since this is unused, completely remove it from the file.

[Greg Billock, 2013-07-31 19:01:53]

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right):

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:187: ALL_VALID,
On 2013/07/31 18:45:06, vandebo wrote:
> How about VALID, PRE_WRITE_INVALID, and POST_WRITE_INVALID ?

Went with just "INVALID" for the second.

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:209: explicit
TestCopyOrMoveFileValidator(bool pre_copy_valid,
On 2013/07/31 18:45:06, vandebo wrote:
> propagate enum down.

Done.

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
File webkit/browser/fileapi/test_file_system_backend.cc (left):

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
webkit/browser/fileapi/test_file_system_backend.cc:189: if
(!require_copy_or_move_validator_) {
On 2013/07/31 18:45:06, vandebo wrote:
> Since this is unused, completely remove it from the file.

It's used further down.

[vandebo (ex-Chrome), 2013-07-31 20:27:57]

LGTM after nits.

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
File webkit/browser/fileapi/test_file_system_backend.cc (left):

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
webkit/browser/fileapi/test_file_system_backend.cc:189: if
(!require_copy_or_move_validator_) {
On 2013/07/31 19:01:54, Greg Billock wrote:
> On 2013/07/31 18:45:06, vandebo wrote:
> > Since this is unused, completely remove it from the file.
> 
> It's used further down.

Not really - initialized to false and never changed.

https://codereview.chromium.org/21097005/diff/4001/webkit/browser/fileapi/cop...
File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right):

https://codereview.chromium.org/21097005/diff/4001/webkit/browser/fileapi/cop...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:212:
write_result_(validity == VALID
nit: I think write_result_ should be OK when validity == INVALID to ensure that
it's the prewrite validity check that's catching it.

[Greg Billock, 2013-07-31 21:57:58]

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
File webkit/browser/fileapi/test_file_system_backend.cc (left):

https://codereview.chromium.org/21097005/diff/1/webkit/browser/fileapi/test_f...
webkit/browser/fileapi/test_file_system_backend.cc:189: if
(!require_copy_or_move_validator_) {
On 2013/07/31 20:27:57, vandebo wrote:
> On 2013/07/31 19:01:54, Greg Billock wrote:
> > On 2013/07/31 18:45:06, vandebo wrote:
> > > Since this is unused, completely remove it from the file.
> > 
> > It's used further down.
> 
> Not really - initialized to false and never changed.

As discussed, there's a missing test here -- making sure the copy operation
fails security if it finds a bad return from the factory getter.

The other test we worried about (return OK + validator=null --> no validation
needed and copy should complete) is already getting covered.

https://codereview.chromium.org/21097005/diff/4001/webkit/browser/fileapi/cop...
File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right):

https://codereview.chromium.org/21097005/diff/4001/webkit/browser/fileapi/cop...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:212:
write_result_(validity == VALID
On 2013/07/31 20:27:57, vandebo wrote:
> nit: I think write_result_ should be OK when validity == INVALID to ensure
that
> it's the prewrite validity check that's catching it.

Done. I think that makes the other names better, so changed to PRE_WRITE_INVALID
as well.

[vandebo (ex-Chrome), 2013-07-31 22:43:22]

LGTM with comment.

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/te...
File webkit/browser/fileapi/test_file_system_backend.h (right):

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/te...
webkit/browser/fileapi/test_file_system_backend.h:69: // Reset the
CopyOrMoveFileValidatorFactory to empty.
Not used.

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

lgtm + one nit

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/co...
File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right):

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/co...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:190: };
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]

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/co...
File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right):

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/co...
webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:190: };
On 2013/08/01 07:37:24, kinuko wrote:
> 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)

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.

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/te...
File webkit/browser/fileapi/test_file_system_backend.h (right):

https://codereview.chromium.org/21097005/diff/11001/webkit/browser/fileapi/te...
webkit/browser/fileapi/test_file_system_backend.h:69: // Reset the
CopyOrMoveFileValidatorFactory to empty.
On 2013/07/31 22:43:23, vandebo wrote:
> Not used.

Done.

[commit-bot: I haz the power, 2013-08-01 16:23:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/21097005/21001

[commit-bot: I haz the power, 2013-08-01 16:31:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-08-01 16:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/21097005/21001

[commit-bot: I haz the power, 2013-08-01 21:11:24]

Change committed as 215105