Extract CopyOrMoveFile operation into classes.

Extract CopyOrMoveFile operation into classes.

To prepare implementing ProgressCallbak, Cancel and stream based copy/move
operation, this CL extracts main file-copying logic into classes.

BUG=279287, 278038, 278036
TEST=Ran content_unittests

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

[hidehiko, 2013-09-09 15:14:08]

Thank you for your review in advance,
- hidehiko

[kinuko, 2013-09-09 16:12:38]

lgtm + some nits

looks nicer, thanks!

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

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:19: // Needs to define
the interface in before the each implementation define below.
nit: ditto, this comment's probably not necessary

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:68: // Espacially for
cross file system copy/move operation, this class creates
nit: Especially -> Specifically ?  (Since it's specific impl for cross-fs case)

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:70: // validate the
created file, and remove source file for move (noop for copy).
nit: validate -> validates, run -> runs, remove -> removes

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:78: const
FileSystemURL&  dest_url,
nit: extra space

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:110: // TODO(kinuko):
Otherwise create a FileStreamReader to perform a copy/move.
nit: maybe we can remove this comment now?

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:120: // Runs pre write
validation.
nit: Runs -> Run

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:155: // |validator_|
is NULL in when the destination filesystem does not do
nit: 'in when' -> when

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:174: dest_url_, true,
nit: can you add /* recursive */ after true while you're there?  (I know you
just copy-pasted the original code)

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate.h (right):

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.h:26: // Implementation
of Copy file.
nit: this comment is probably not necessary (not adding much info)

[hidehiko, 2013-09-09 16:55:34]

Thank you for your quick review! Sending to CQ.

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

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:19: // Needs to define
the interface in before the each implementation define below.
On 2013/09/09 16:12:38, kinuko wrote:
> nit: ditto, this comment's probably not necessary

Removed.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:68: // Espacially for
cross file system copy/move operation, this class creates
On 2013/09/09 16:12:38, kinuko wrote:
> nit: Especially -> Specifically ?  (Since it's specific impl for cross-fs
case)

Done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:70: // validate the
created file, and remove source file for move (noop for copy).
On 2013/09/09 16:12:38, kinuko wrote:
> nit: validate -> validates, run -> runs, remove -> removes

Done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:78: const
FileSystemURL&  dest_url,
On 2013/09/09 16:12:38, kinuko wrote:
> nit: extra space

Oops done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:110: // TODO(kinuko):
Otherwise create a FileStreamReader to perform a copy/move.
On 2013/09/09 16:12:38, kinuko wrote:
> nit: maybe we can remove this comment now?

Done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:120: // Runs pre write
validation.
On 2013/09/09 16:12:38, kinuko wrote:
> nit: Runs -> Run

Done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:155: // |validator_|
is NULL in when the destination filesystem does not do
On 2013/09/09 16:12:38, kinuko wrote:
> nit: 'in when' -> when

Done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:174: dest_url_, true,
On 2013/09/09 16:12:38, kinuko wrote:
> nit: can you add /* recursive */ after true while you're there?  (I know you
> just copy-pasted the original code)

Done.

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate.h (right):

https://codereview.chromium.org/23621026/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.h:26: // Implementation
of Copy file.
On 2013/09/09 16:12:38, kinuko wrote:
> nit: this comment is probably not necessary (not adding much info)

Removed.

[commit-bot: I haz the power, 2013-09-09 16:55:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23621026/2001

[commit-bot: I haz the power, 2013-09-09 20:40:32]

Change committed as 222083