Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18565002:
[FileSystem] Add another copy-or-move validation hook for post-write. (Closed)
|
Created:
7 years, 5 months ago by Greg Billock Modified:
7 years, 4 months ago CC:
tommycli, chromium-reviews, Lei Zhang, tzik+watch_chromium.org, darin-cc_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[FileSystem] Add another copy-or-move validation hook for post-write.
This is because any AV trigger needs to happen after the file is in
its final location.
R=kinuko@chromium.org, vandebo@chromium.org
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213267
Patch Set 1 #
Total comments: 14
Patch Set 2 : Simplify dest validation callbacks and delete dest file on error #
Total comments: 14
Patch Set 3 : Pass file ref to validator #
Total comments: 6
Patch Set 4 : Use second callback in validator #Patch Set 5 : Add test #
Total comments: 24
Patch Set 6 : Manage file_ref in calling code, remove from API. #
Total comments: 14
Patch Set 7 : Change validation method signature #Patch Set 8 : Beginnings of test for post-write validation failure #Patch Set 9 : Test fixes #
Total comments: 14
Patch Set 10 : Rebase #Patch Set 11 : Rebase #Patch Set 12 : Add test failure logging #Patch Set 13 : Fix flakey test #Messages
Total messages: 37 (0 generated)
https://codereview.chromium.org/18565002/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/18565002/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:134: // TODO(gbillock): Insert AV call here. Since we will have several different validators, which will all do the AV check, this should probably go into a base class that they can all derive the functionality from. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:215: FROM_HERE, base::Bind(result_callback, result_)); The test validator should control pre/post validation result separately so that both can be tested. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: if (operation_type_ == OPERATION_COPY) { Code is basically the same in both cases, combine. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:236: const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref) { You probably need to pass file_ref down so that the snapshot file isn't deleted. Though we think it won't actually be a snapshot file so maybe it's ok for the ShareableFileReference to go out of scope and check later on that the platform file is there. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:244: validator_->StartPostWriteValidation(platform_path, callback); Instead of passing |callback| down to this point, generate it here. You have access to operation_type_. Better yet, just use operation_type_ in DidPostCopy/MoveValidation and combine those two methods into one. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:258: // TODO(gbillock): Need to remove the destination file here? Yes
https://codereview.chromium.org/18565002/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/18565002/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:134: // TODO(gbillock): Insert AV call here. On 2013/07/02 18:43:05, vandebo wrote: > Since we will have several different validators, which will all do the AV check, > this should probably go into a base class that they can all derive the > functionality from. Yes. This is just a placeholder. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:215: FROM_HERE, base::Bind(result_callback, result_)); On 2013/07/02 18:43:05, vandebo wrote: > The test validator should control pre/post validation result separately so that > both can be tested. Done. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: if (operation_type_ == OPERATION_COPY) { On 2013/07/02 18:43:05, vandebo wrote: > Code is basically the same in both cases, combine. I see what you mean below. Yeah, that's a good plan. I'll check operation_type_ further down. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:236: const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref) { On 2013/07/02 18:43:05, vandebo wrote: > You probably need to pass file_ref down so that the snapshot file isn't deleted. > Though we think it won't actually be a snapshot file so maybe it's ok for the > ShareableFileReference to go out of scope and check later on that the platform > file is there. So the file_ref is what keeps the snapshot alive? So should that be an arg to the post-write validation method? Or should we just skip if we have a file_ref? I think the premise is that we'll have an empty one in cases where we want to run this validation step. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:244: validator_->StartPostWriteValidation(platform_path, callback); On 2013/07/02 18:43:05, vandebo wrote: > Instead of passing |callback| down to this point, generate it here. You have > access to operation_type_. Better yet, just use operation_type_ in > DidPostCopy/MoveValidation and combine those two methods into one. Done. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:258: // TODO(gbillock): Need to remove the destination file here? On 2013/07/02 18:43:05, vandebo wrote: > Yes Added the call, but I'm not sure what to do with errors. I think we want to ship back the validation error. Currently I just best-effort it.
https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:236: const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref) { On 2013/07/02 19:17:39, Greg Billock wrote: > On 2013/07/02 18:43:05, vandebo wrote: > > You probably need to pass file_ref down so that the snapshot file isn't > deleted. > > Though we think it won't actually be a snapshot file so maybe it's ok for the > > ShareableFileReference to go out of scope and check later on that the platform > > file is there. > > So the file_ref is what keeps the snapshot alive? So should that be an arg to > the post-write validation method? Or should we just skip if we have a file_ref? > I think the premise is that we'll have an empty one in cases where we want to > run this validation step. We don't want to validate files that need snapshoting, but that's specific to our use case. The file_ref should probably be passed on toe the validation step. https://codereview.chromium.org/18565002/diff/1/webkit/browser/fileapi/copy_o... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:258: // TODO(gbillock): Need to remove the destination file here? On 2013/07/02 19:17:39, Greg Billock wrote: > On 2013/07/02 18:43:05, vandebo wrote: > > Yes > > Added the call, but I'm not sure what to do with errors. I think we want to ship > back the validation error. Currently I just best-effort it. Yea, send back the validation error. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:197: explicit TestCopyOrMoveFileValidator(bool all_valid) Looks like you need another parameter here and several more test cases. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:222: base::Bind(&CopyOrMoveOperationDelegate::DidRemoveDestForError, Shouldn't this take the callback and invoke it after the remove is done? https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:244: callback.Run(error); Same here https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:286: // TODO(gbillock): What to do here? Currently nothing.... Remove it if you don't need it... but you do. You should save the error that put you on the path to here and run the callback with that, ignoring the error from Remove.
I'm a bit unclear about this check... at the time where we run the post-write validation, we've already written the file into the destination filesystem, and if Chrome finishes before validation runs we'll leave a file without validation. What does this check validate? https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:130: const fileapi::CopyOrMoveFileValidator::ResultCallback& result_callback) { ditto for ResultCallback https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:34: const fileapi::CopyOrMoveFileValidator::ResultCallback& nit: I think you can directly refer ResultCallback in a subclass. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_file_validator.h:33: // destination. Suitable for running AV checks. sorry, what does this 'AV' refer?
On 2013/07/03 04:53:07, kinuko wrote: > I'm a bit unclear about this check... at the time where we run the post-write > validation, we've already written the file into the destination filesystem, and > if Chrome finishes before validation runs we'll leave a file without validation. > What does this check validate? AV is Anti-Virus. The Windows AV-hook requires that the file have its final name (it does extension specific checks). So we have to run the AV-hook after the file has been moved. The security folks are ok with the corner cases that it's possible a file won't get AV checked because browser shutdown.
On 2013/07/03 05:00:29, vandebo wrote: > On 2013/07/03 04:53:07, kinuko wrote: > > I'm a bit unclear about this check... at the time where we run the post-write > > validation, we've already written the file into the destination filesystem, > and > > if Chrome finishes before validation runs we'll leave a file without > validation. > > What does this check validate? > > AV is Anti-Virus. The Windows AV-hook requires that the file have its final > name (it does extension specific checks). So we have to run the AV-hook after > the file has been moved. The security folks are ok with the corner cases that > it's possible a file won't get AV checked because browser shutdown. I see, thanks for the explanation. May I ask two more questions-- 1. is this validation necessary both for same-filesystem and cross-filesystem cases? 2. is it possible to do such a validation in CopyInForeignFile in FileUtil side? This one feels a bit less common than the regular validation path.
On 2013/07/03 10:11:28, kinuko wrote: > On 2013/07/03 05:00:29, vandebo wrote: > > On 2013/07/03 04:53:07, kinuko wrote: > > > I'm a bit unclear about this check... at the time where we run the > post-write > > > validation, we've already written the file into the destination filesystem, > > and > > > if Chrome finishes before validation runs we'll leave a file without > > validation. > > > What does this check validate? > > > > AV is Anti-Virus. The Windows AV-hook requires that the file have its final > > name (it does extension specific checks). So we have to run the AV-hook after > > the file has been moved. The security folks are ok with the corner cases that > > it's possible a file won't get AV checked because browser shutdown. > > I see, thanks for the explanation. May I ask two more questions-- > 1. is this validation necessary both for same-filesystem and cross-filesystem > cases? > 2. is it possible to do such a validation in CopyInForeignFile in FileUtil side? > This one feels a bit less common than the regular validation path. Shouldn't be necessary for same-filesystem. I think the code avoids that. I looked at the FileUtil location for this. I ended up putting it here because the other validation was here, so it kept the client code together at the same level. The real obstacle is getting the real FilePath for the destination file. That happens pretty far down in the guts of the copy and bubbling it up looked daunting, so I figured that this plan was the best since there are lots of FileUtil implementations and from the perspective of the operation delegate, it doesn't know which one is being used necessarily.
https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:130: const fileapi::CopyOrMoveFileValidator::ResultCallback& result_callback) { On 2013/07/03 04:53:08, kinuko wrote: > ditto for ResultCallback Done. https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/18565002/diff/5002/chrome/browser/media_galle... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:34: const fileapi::CopyOrMoveFileValidator::ResultCallback& On 2013/07/03 04:53:08, kinuko wrote: > nit: I think you can directly refer ResultCallback in a subclass. Done. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_file_validator.h:33: // destination. Suitable for running AV checks. On 2013/07/03 04:53:08, kinuko wrote: > sorry, what does this 'AV' refer? Anti-Virus. Will expand. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:197: explicit TestCopyOrMoveFileValidator(bool all_valid) On 2013/07/02 20:15:02, vandebo wrote: > Looks like you need another parameter here and several more test cases. Yes. I haven't added all that yet. Making sure this approach is what we want and I'll definitely add tests. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:222: base::Bind(&CopyOrMoveOperationDelegate::DidRemoveDestForError, On 2013/07/02 20:15:02, vandebo wrote: > Shouldn't this take the callback and invoke it after the remove is done? I want to return the validation error, not the remove-file error. I just left this best effort at this point. We'd need to change around a lot of code to return both, and I'm not sure it's worth it, since there's not much the client can do in that case. https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:244: callback.Run(error); On 2013/07/02 20:15:02, vandebo wrote: > Same here ditto https://codereview.chromium.org/18565002/diff/5002/webkit/browser/fileapi/cop... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:286: // TODO(gbillock): What to do here? Currently nothing.... On 2013/07/02 20:15:02, vandebo wrote: > Remove it if you don't need it... but you do. You should save the error that > put you on the path to here and run the callback with that, ignoring the error > from Remove. That's basically what's happening now. I should wait to return the error until post-delete-attempt, though, which is probably better -- that way the client doesn't potentially see the being-deleted file.
Thanks for the clarification. https://codereview.chromium.org/18565002/diff/15001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/18565002/diff/15001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:134: callback_ = result_callback; Can we reset callback_ when it becomes no longer necessary after the pre-copy validation (i.e. after OnFileOpen), and make sure it's already reset here with DCHECK? https://codereview.chromium.org/18565002/diff/15001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/15001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: if (validator_.get()) { nit: Can you add a comment here to clarify that validator_ must not exist if it's same-filesystem case? (Also maybe we could add DCHECK(!same_file_system_) here) Also it's very minor preferential issue, but can we swap this if/else like following so that the code flow looks similar to DidCreateSnapshot: if (!validator_.get()) { DidPostWriteValidation(...); return; } operation_runner()->CreateSnapshotFile(...); https://codereview.chromium.org/18565002/diff/15001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:218: // TODO(gbillock): need to check here that file_ref is null or anything? I think you can just bind the ref to the callback of StartPostWriteValidation so that it's kept until the validation is done.
https://codereview.chromium.org/18565002/diff/15001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/18565002/diff/15001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:134: callback_ = result_callback; On 2013/07/04 03:10:12, kinuko wrote: > Can we reset callback_ when it becomes no longer necessary after the pre-copy > validation (i.e. after OnFileOpen), and make sure it's already reset here with > DCHECK? OnFileOpen is forwarding that callback to the image decoder, and currently doesn't see when that finishes. I'll just use another callback for phase two. https://codereview.chromium.org/18565002/diff/15001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/15001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: if (validator_.get()) { On 2013/07/04 03:10:12, kinuko wrote: > nit: Can you add a comment here to clarify that validator_ must not exist if > it's same-filesystem case? (Also maybe we could add DCHECK(!same_file_system_) > here) Done. > > Also it's very minor preferential issue, but can we swap this if/else like > following so that the code flow looks similar to DidCreateSnapshot: > > if (!validator_.get()) { > DidPostWriteValidation(...); > return; > } > > operation_runner()->CreateSnapshotFile(...); Sure thing. Done. https://codereview.chromium.org/18565002/diff/15001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:218: // TODO(gbillock): need to check here that file_ref is null or anything? On 2013/07/04 03:10:12, kinuko wrote: > I think you can just bind the ref to the callback of StartPostWriteValidation so > that it's kept until the validation is done. Yeah. Steve and I figured this out I think. I'm just passing it through to the call. Docs say that scoped_refptr's will pass intact like that. (Bind has special mechanics for them.)
webkit/ lgtm https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:311: new TestCopyOrMoveFileValidatorFactory(true, false)); nit: add comment for the bool values here too?
https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:21: virtual void StartValidation( Should we rename this to StartPreWriteValidation() ? https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:50: fileapi::CopyOrMoveFileValidator::ResultCallback post_write_callback_; I'm pretty sure we can just use |callback_| for both stages. We can reset it when used and DCHECK that it is null before setting it. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator.h:39: // may not be present. Implementations should maintain the |file_ref| until Hmm, can the caller manage the lifetime of the file_ref so that the validators don't have to worry about it? https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:200: explicit TestCopyOrMoveFileValidator(bool all_valid, bool all_valid_write) all_valid -> pre_copy_valid all_valid_write -> post_copy_valid ? https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: // |validator_| must not exist in the same-filesystem case. 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? https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:225: AsWeakPtr(), error, callback)); nit: you could bind error to callback and pass it as a closure https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:233: AsWeakPtr(), url_pair, callback)); Yea, pass file_ref to DidPostWriteValidation so that the validator doesn't need to deal with it. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.h (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.h:79: const URLPair& url_pair, Does this chain work correctly when source is multiple files or directories?
Kinuko, this changes the API a bit. Look again? https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:21: virtual void StartValidation( On 2013/07/10 15:52:46, vandebo wrote: > Should we rename this to StartPreWriteValidation() ? Not opposed. https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:50: fileapi::CopyOrMoveFileValidator::ResultCallback post_write_callback_; On 2013/07/10 15:52:46, vandebo wrote: > I'm pretty sure we can just use |callback_| for both stages. We can reset it > when used and DCHECK that it is null before setting it. I agree we could, but I think this reads more straightforward. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator.h:39: // may not be present. Implementations should maintain the |file_ref| until On 2013/07/10 15:52:46, vandebo wrote: > Hmm, can the caller manage the lifetime of the file_ref so that the validators > don't have to worry about it? There was a good spot you found to manage this. Removing it. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:200: explicit TestCopyOrMoveFileValidator(bool all_valid, bool all_valid_write) On 2013/07/10 15:52:46, vandebo wrote: > all_valid -> pre_copy_valid > all_valid_write -> post_copy_valid ? Done. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:311: new TestCopyOrMoveFileValidatorFactory(true, false)); On 2013/07/10 04:42:19, kinuko wrote: > nit: add comment for the bool values here too? Done. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: // |validator_| must not exist in the same-filesystem case. On 2013/07/10 15:52:46, vandebo wrote: > 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? 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. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:225: AsWeakPtr(), error, callback)); On 2013/07/10 15:52:46, vandebo wrote: > nit: you could bind error to callback and pass it as a closure Yeah. I don't think that'd improve this, though. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:233: AsWeakPtr(), url_pair, callback)); On 2013/07/10 15:52:46, vandebo wrote: > Yea, pass file_ref to DidPostWriteValidation so that the validator doesn't need > to deal with it. I see. That's a good point. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.h (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.h:79: const URLPair& url_pair, On 2013/07/10 15:52:46, vandebo wrote: > Does this chain work correctly when source is multiple files or directories? I think so. multiple objects should be handled upstream, and this chain entered for each object. Kinuko, is that right?
https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator.h:35: // may not be present. Implementations should maintain the |file_ref| until This comment also needs to be updated? https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:113: src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, 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?
+1 to adding a recursive copy/move test https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:21: virtual void StartValidation( On 2013/07/11 22:54:35, Greg Billock wrote: > On 2013/07/10 15:52:46, vandebo wrote: > > Should we rename this to StartPreWriteValidation() ? > > Not opposed. But not enthusiastic either? https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:50: fileapi::CopyOrMoveFileValidator::ResultCallback post_write_callback_; On 2013/07/11 22:54:35, Greg Billock wrote: > On 2013/07/10 15:52:46, vandebo wrote: > > I'm pretty sure we can just use |callback_| for both stages. We can reset it > > when used and DCHECK that it is null before setting it. > > I agree we could, but I think this reads more straightforward. I'm not sure having two different members adds anything, but it does cost space... not going to push any harder though. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: // |validator_| must not exist in the same-filesystem case. On 2013/07/11 22:54:35, Greg Billock wrote: > On 2013/07/10 15:52:46, vandebo wrote: > > 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? > > 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. 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. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:225: AsWeakPtr(), error, callback)); On 2013/07/11 22:54:35, Greg Billock wrote: > On 2013/07/10 15:52:46, vandebo wrote: > > nit: you could bind error to callback and pass it as a closure > > Yeah. I don't think that'd improve this, though. I think it makes DidRemoveDestForError a bit more clear, but what ever. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:232: validator_->StartPostWriteValidation( 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. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:241: const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref, nit: file_ref isn't used in this context, so /*file ref*/ https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:291: callback.Run(prior_error); nit: VLOG if error != OK ?
https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/18565002/diff/24001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:21: virtual void StartValidation( On 2013/07/12 15:08:15, vandebo wrote: > On 2013/07/11 22:54:35, Greg Billock wrote: > > On 2013/07/10 15:52:46, vandebo wrote: > > > Should we rename this to StartPreWriteValidation() ? > > > > Not opposed. > > But not enthusiastic either? This stage seems more like validation. I can't think of a better name for post-write operation, though. It's a validation from the perspective of the fileapi code. I guess I'm hampered by thinking too much about what we anticipate doing in these methods. Let's rename it. https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/24001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:201: // |validator_| must not exist in the same-filesystem case. On 2013/07/12 15:08:15, vandebo wrote: > On 2013/07/11 22:54:35, Greg Billock wrote: > > On 2013/07/10 15:52:46, vandebo wrote: > > > 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? > > > > 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. > > 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. Done. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator.h:35: // may not be present. Implementations should maintain the |file_ref| until On 2013/07/12 05:00:19, kinuko wrote: > This comment also needs to be updated? Done. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:113: src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, On 2013/07/12 05:00:19, kinuko wrote: > 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? 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. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:232: validator_->StartPostWriteValidation( On 2013/07/12 15:08:15, vandebo wrote: > 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. Done. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:241: const scoped_refptr<webkit_blob::ShareableFileReference>& file_ref, On 2013/07/12 15:08:15, vandebo wrote: > nit: file_ref isn't used in this context, so /*file ref*/ Done. https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:291: callback.Run(prior_error); On 2013/07/12 15:08:15, vandebo wrote: > nit: VLOG if error != OK ? Done.
https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:113: src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, On 2013/07/12 18:02:19, Greg Billock wrote: > On 2013/07/12 05:00:19, kinuko wrote: > > 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? > > 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. 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).
https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:113: src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, 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: > On 2013/07/12 18:02:19, Greg Billock wrote: > > On 2013/07/12 05:00:19, kinuko wrote: > > > 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? > > > > 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. > > 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). >
https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:113: src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, On 2013/07/12 20:36:46, Greg Billock wrote: > 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? 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 ? > > 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: > > On 2013/07/12 18:02:19, Greg Billock wrote: > > > On 2013/07/12 05:00:19, kinuko wrote: > > > > 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? > > > > > > 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. > > > > 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). > > >
https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/37001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:113: src_root_, base::Bind(&CopyOrMoveOperationDelegate::DidFinishCopy, 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: > On 2013/07/12 20:36:46, Greg Billock wrote: > > 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? > > 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 ? > > > > > 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: > > > On 2013/07/12 18:02:19, Greg Billock wrote: > > > > On 2013/07/12 05:00:19, kinuko wrote: > > > > > 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? > > > > > > > > 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. > > > > > > 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). > > > > > >
I've updated the test to work now. Kinuko, this has somewhat surprising behavior -- a half-way done copy or move looks like it just leaves everything done in place and bails out upon encountering an error. This isn't crazy -- I can imagine situations where doing something different might result in data loss. Is that the idea, so we just drop everything and stop on error? I ratified that behavior in the test. It'd be nice to be able to put things back in the case of error, but perhaps we can't reliably do that.
On 2013/07/15 23:04:33, Greg Billock wrote: > I've updated the test to work now. > > Kinuko, this has somewhat surprising behavior -- a half-way done copy or move > looks like it just leaves everything done in place and bails out upon > encountering an error. This isn't crazy -- I can imagine situations where doing > something different might result in data loss. Is that the idea, so we just drop > everything and stop on error? I ratified that behavior in the test. It'd be nice > to be able to put things back in the case of error, but perhaps we can't > reliably do that. Yes, that's the intended behavior at least for now. Technically it can work either way, but some errors are not recoverable and it's hard to determine what to be returned if we get multiple different errors.
On 2013/07/17 02:47:24, kinuko wrote: > On 2013/07/15 23:04:33, Greg Billock wrote: > > I've updated the test to work now. > > > > Kinuko, this has somewhat surprising behavior -- a half-way done copy or move > > looks like it just leaves everything done in place and bails out upon > > encountering an error. This isn't crazy -- I can imagine situations where > doing > > something different might result in data loss. Is that the idea, so we just > drop > > everything and stop on error? I ratified that behavior in the test. It'd be > nice > > to be able to put things back in the case of error, but perhaps we can't > > reliably do that. > > Yes, that's the intended behavior at least for now. > Technically it can work either way, but some errors are not recoverable and it's > hard to determine what to be returned if we get multiple different errors. Thanks. Is this still lgtm? If so, feel free to hit the cq with it.
LGTM - clicking CQ. Will send a follow up CL to resolve nits. https://codereview.chromium.org/18565002/diff/65001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/18565002/diff/65001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:12: #include "webkit/common/blob/shareable_file_reference.h" nit: remove https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:187: explicit TestCopyOrMoveFileValidatorFactory(bool all_valid, nit: these args could use some help. Maybe an enum. https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:193: void CopyOrMoveOperationDelegate::DidFinishCopyDir( nit: What do you think of calling this DidFinishRecursiveCopyDir? https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:203: DCHECK_EQ(operation_type_, OPERATION_MOVE); nit: expectation first https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:230: DCHECK(!same_file_system_); nit: Move up and refine comment 221 https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:49: return new TestValidator(true, true, std::string("2")); nit: The args are all static, why not omit the functionality from TestValidator https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/te... File webkit/browser/fileapi/test_file_system_backend.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/te... webkit/browser/fileapi/test_file_system_backend.cc:185: // What purpose is this check serving? nit: remove/resolve this question
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/18565002/65001
Failed to apply patch for
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
Hunk #3 FAILED at 135.
Hunk #4 succeeded at 211 (offset 2 lines).
Hunk #5 succeeded at 488 (offset 2 lines).
1 out of 5 hunks FAILED -- saving rejects to file
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc.rej
Patch: webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
Index: webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
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
3eab0934a1ee2014ca532557f483c6f9c5b4c507..32662f428f1e496d49a2a8e7439b49377eabd284
100644
--- a/webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
+++ b/webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc
@@ -13,12 +13,14 @@
#include "base/stl_util.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "webkit/browser/fileapi/async_file_test_helper.h"
+#include "webkit/browser/fileapi/copy_or_move_file_validator.h"
#include "webkit/browser/fileapi/file_system_backend.h"
#include "webkit/browser/fileapi/file_system_context.h"
#include "webkit/browser/fileapi/file_system_operation.h"
#include "webkit/browser/fileapi/file_system_url.h"
#include "webkit/browser/fileapi/mock_file_system_context.h"
#include "webkit/browser/fileapi/test_file_set.h"
+#include "webkit/browser/fileapi/test_file_system_backend.h"
#include "webkit/browser/quota/mock_quota_manager.h"
#include "webkit/browser/quota/quota_manager.h"
#include "webkit/common/fileapi/file_system_util.h"
@@ -35,6 +37,61 @@ void ExpectOk(const GURL& origin_url,
ASSERT_EQ(base::PLATFORM_FILE_OK, error);
}
+class TestValidatorFactory : public CopyOrMoveFileValidatorFactory {
+ public:
+ // A factory that creates validators that accept everything or nothing.
+ TestValidatorFactory() {}
+ virtual ~TestValidatorFactory() {}
+
+ virtual CopyOrMoveFileValidator* CreateCopyOrMoveFileValidator(
+ const FileSystemURL& /*src_url*/,
+ const base::FilePath& /*platform_path*/) OVERRIDE {
+ return new TestValidator(true, true, std::string("2"));
+ }
+
+ private:
+ class TestValidator : public CopyOrMoveFileValidator {
+ public:
+ explicit TestValidator(bool pre_copy_valid,
+ bool post_copy_valid,
+ const std::string& reject_string)
+ : 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),
+ reject_string_(reject_string) {
+ }
+ virtual ~TestValidator() {}
+
+ 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 {
+ base::PlatformFileError result = write_result_;
+ std::string unsafe = dest_platform_path.AsUTF8Unsafe();
+ if (unsafe.find(reject_string_) != std::string::npos) {
+ result = base::PLATFORM_FILE_ERROR_SECURITY;
+ }
+ // Post the result since a real validator must do work asynchronously.
+ base::MessageLoop::current()->PostTask(
+ FROM_HERE, base::Bind(result_callback, result));
+ }
+
+ private:
+ base::PlatformFileError result_;
+ base::PlatformFileError write_result_;
+ std::string reject_string_;
+
+ DISALLOW_COPY_AND_ASSIGN(TestValidator);
+ };
+};
+
} // namespace
class CopyOrMoveOperationTestHelper {
@@ -78,6 +135,14 @@ class CopyOrMoveOperationTestHelper {
base::Bind(&ExpectOk));
mount_point_provider =
file_system_context_->GetFileSystemBackend(dest_type_);
+ if (dest_type_ == kFileSystemTypeTest) {
+ TestFileSystemBackend* test_provider =
+ static_cast<TestFileSystemBackend*>(mount_point_provider);
+ scoped_ptr<CopyOrMoveFileValidatorFactory> factory(
+ new TestValidatorFactory);
+ test_provider->set_require_copy_or_move_validator(true);
+ test_provider->InitializeCopyOrMoveFileValidatorFactory(factory.Pass());
+ }
mount_point_provider->OpenFileSystem(
origin_, dest_type_,
OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
@@ -152,10 +217,11 @@ class CopyOrMoveOperationTestHelper {
const test::TestCaseRecord* const test_cases,
size_t test_case_size) {
std::map<base::FilePath, const test::TestCaseRecord*> test_case_map;
- for (size_t i = 0; i < test_case_size; ++i)
+ for (size_t i = 0; i < test_case_size; ++i) {
test_case_map[
base::FilePath(test_cases[i].path).NormalizePathSeparators()] =
&test_cases[i];
+ }
std::queue<FileSystemURL> directories;
FileEntryList entries;
@@ -428,4 +494,38 @@ TEST(LocalFileSystemCopyOrMoveOperationTest, MoveDirectory)
{
ASSERT_EQ(src_increase, dest_increase);
}
+TEST(LocalFileSystemCopyOrMoveOperationTest,
+ MoveDirectoryFailPostWriteValidation) {
+ CopyOrMoveOperationTestHelper helper(GURL("http://foo"),
+ kFileSystemTypeTemporary,
+ kFileSystemTypeTest);
+ helper.SetUp();
+
+ FileSystemURL src = helper.SourceURL("a");
+ FileSystemURL dest = helper.DestURL("b");
+
+ // Set up a source directory.
+ ASSERT_EQ(base::PLATFORM_FILE_OK, helper.CreateDirectory(src));
+ ASSERT_EQ(base::PLATFORM_FILE_OK,
+ helper.SetUpTestCaseFiles(src,
+ test::kRegularTestCases,
+ test::kRegularTestCaseSize));
+
+ // Move it.
+ helper.Move(src, dest);
+
+ // Verify.
+ ASSERT_TRUE(helper.DirectoryExists(src));
+ ASSERT_TRUE(helper.DirectoryExists(dest));
+
+ test::TestCaseRecord kMoveDirResultCases[] = {
+ {false, FILE_PATH_LITERAL("file 0"), 38},
+ {false, FILE_PATH_LITERAL("file 3"), 0},
+ };
+
+ helper.VerifyTestCaseFiles(dest,
+ kMoveDirResultCases,
+ arraysize(kMoveDirResultCases));
+}
+
} // namespace fileapi
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/18565002/80001
Retried try job too often on mac_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/18565002/80001
Retried try job too often on mac_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
On 2013/07/18 19:58:49, I haz the power (commit-bot) wrote: > Retried try job too often on mac_rel for step(s) content_unittests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Uploaded a new patchset to try and get past this conflict. Want to watch the try jobs? Ought to be OK to merge I hope.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/18565002/102001
Retried try job too often on mac_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/18565002/145001
Message was sent while issue was closed.
Change committed as 213267
Message was sent while issue was closed.
Sent you a new CL for this: https://codereview.chromium.org/21097005 https://codereview.chromium.org/18565002/diff/65001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/18565002/diff/65001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:12: #include "webkit/common/blob/shareable_file_reference.h" On 2013/07/18 17:35:14, vandebo wrote: > nit: remove Done. https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator_unittest.cc:187: explicit TestCopyOrMoveFileValidatorFactory(bool all_valid, On 2013/07/18 17:35:14, vandebo wrote: > nit: these args could use some help. Maybe an enum. Done. https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:193: void CopyOrMoveOperationDelegate::DidFinishCopyDir( On 2013/07/18 17:35:14, vandebo wrote: > nit: What do you think of calling this DidFinishRecursiveCopyDir? Done. https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:203: DCHECK_EQ(operation_type_, OPERATION_MOVE); On 2013/07/18 17:35:14, vandebo wrote: > nit: expectation first Done. https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate.cc:230: DCHECK(!same_file_system_); On 2013/07/18 17:35:14, vandebo wrote: > nit: Move up and refine comment 221 I think this wants to remain after that clause -- validator will be not be null only for non-same-filesystem. It can also be null if there's no validation, but that doesn't make this assertion untrue. https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:49: return new TestValidator(true, true, std::string("2")); On 2013/07/18 17:35:14, vandebo wrote: > nit: The args are all static, why not omit the functionality from TestValidator You mean contain it there? https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/te... File webkit/browser/fileapi/test_file_system_backend.cc (right): https://codereview.chromium.org/18565002/diff/65001/webkit/browser/fileapi/te... webkit/browser/fileapi/test_file_system_backend.cc:185: // What purpose is this check serving? On 2013/07/18 17:35:14, vandebo wrote: > nit: remove/resolve this question removing it -- my guess is it is remnant test code |
