fileapi: Pass virtual path to copy validator.

fileapi: Pass virtual path to copy validator.

Copy validator factory needs to know the virtual path because the platform
path may have a different file extension; this is true for obfuscated file
util.

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

[vandebo (ex-Chrome), 2013-04-17 23:50:48]

https://codereview.chromium.org/14225022/diff/1/webkit/fileapi/cross_operatio...
File webkit/fileapi/cross_operation_delegate.h (right):

https://codereview.chromium.org/14225022/diff/1/webkit/fileapi/cross_operatio...
webkit/fileapi/cross_operation_delegate.h:69: const Request& request,
In order to pass virtual_path into the copy validator, we need FileSystemURL
src.  Adding that parameter pushes this method over the base::Bind argument
limit.

src, dest, callback are used in most of these methods, so I pulled it out into a
struct and used it in these private methods.  Happy to do a different solution
if there's one that you prefer.

[kinuko, 2013-04-18 03:32:00]

https://chromiumcodereview.appspot.com/14225022/diff/1/webkit/fileapi/copy_or...
File webkit/fileapi/copy_or_move_file_validator.h (right):

https://chromiumcodereview.appspot.com/14225022/diff/1/webkit/fileapi/copy_or...
webkit/fileapi/copy_or_move_file_validator.h:34: const base::FilePath&
virtual_path,
Does it make sense if we just pass the source FileSystemURL instead of
virtual_path here? (The term 'virtual_path' is a bit overloaded and may not be
always obvious what it really means)

https://chromiumcodereview.appspot.com/14225022/diff/1/webkit/fileapi/cross_o...
File webkit/fileapi/cross_operation_delegate.h (right):

https://chromiumcodereview.appspot.com/14225022/diff/1/webkit/fileapi/cross_o...
webkit/fileapi/cross_operation_delegate.h:54: Request(const FileSystemURL& src,
const FileSystemURL& dest,
nit: can we break the line after 'src,'

https://chromiumcodereview.appspot.com/14225022/diff/1/webkit/fileapi/cross_o...
webkit/fileapi/cross_operation_delegate.h:69: const Request& request,
On 2013/04/17 23:50:48, vandebo wrote:
> In order to pass virtual_path into the copy validator, we need FileSystemURL
> src.  Adding that parameter pushes this method over the base::Bind argument
> limit.
> 
> src, dest, callback are used in most of these methods, so I pulled it out into
a
> struct and used it in these private methods.  Happy to do a different solution
> if there's one that you prefer.

This looks fine, the name Request sounds a bit unclear to me though. ...I can't
come up with a clean/great name either, but how about naming it FilePair or
CurrentFilePair (indicates current working src/dest parameters) and let it have
only src and dest? (If saving one param works for this patch)

[vandebo (ex-Chrome), 2013-04-19 01:07:18]

Thanks for the review.  If you're happy with this patch set, feel free to check
the CQ box.

https://codereview.chromium.org/14225022/diff/1/webkit/fileapi/cross_operatio...
File webkit/fileapi/cross_operation_delegate.h (right):

https://codereview.chromium.org/14225022/diff/1/webkit/fileapi/cross_operatio...
webkit/fileapi/cross_operation_delegate.h:54: Request(const FileSystemURL& src,
const FileSystemURL& dest,
On 2013/04/18 03:32:00, kinuko wrote:
> nit: can we break the line after 'src,'

With just src and dest, they fit on one line.  Do you still want to break here?

https://codereview.chromium.org/14225022/diff/1/webkit/fileapi/cross_operatio...
webkit/fileapi/cross_operation_delegate.h:69: const Request& request,
On 2013/04/18 03:32:00, kinuko wrote:
> On 2013/04/17 23:50:48, vandebo wrote:
> > In order to pass virtual_path into the copy validator, we need FileSystemURL
> > src.  Adding that parameter pushes this method over the base::Bind argument
> > limit.
> > 
> > src, dest, callback are used in most of these methods, so I pulled it out
into
> a
> > struct and used it in these private methods.  Happy to do a different
solution
> > if there's one that you prefer.
> 
> This looks fine, the name Request sounds a bit unclear to me though. ...I
can't
> come up with a clean/great name either, but how about naming it FilePair or
> CurrentFilePair (indicates current working src/dest parameters) and let it
have
> only src and dest? (If saving one param works for this patch)

Done.

Named URLPair.

Since it's just the two values now, I also only used it where we need both
values.

[kinuko, 2013-04-19 01:16:32]

lgtm!

[commit-bot: I haz the power, 2013-04-19 01:25:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/14225022/5001

[commit-bot: I haz the power, 2013-04-19 04:01:37]

Change committed as 195089