Adds callbacks to notify progress into Copy's API.

Adds callbacks to notify progress into Copy's API.

Copy operation will support progress update. This CL adds the callbacks to
report the progress update.

BUG=278038
TEST=Ran unit_tests

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

[hidehiko, 2013-09-06 08:38:58]

Thank you for your review in advance,
- hidehiko

[kinuko, 2013-09-06 09:55:25]

Looks good, I have a high-level question.

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
File webkit/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
webkit/browser/fileapi/file_system_operation.h:182: typedef
base::Callback<void(int64 size)> CopyFileProgressCallback;
Question: how difficult would it be (in terms of cost+complexity etc) to add the
same/similar progress callback to Move operation?  It feels a bit weird that
these two have different signature.

[hidehiko, 2013-09-06 10:06:15]

Thank you for your review.

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
File webkit/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
webkit/browser/fileapi/file_system_operation.h:182: typedef
base::Callback<void(int64 size)> CopyFileProgressCallback;
On 2013/09/06 09:55:25, kinuko wrote:
> Question: how difficult would it be (in terms of cost+complexity etc) to add
the
> same/similar progress callback to Move operation?  It feels a bit weird that
> these two have different signature.

IMHO, it's a bit complicated especially for directory move (recursively).
"Move" may be implemented as just updating the root's metadata. In such a case,
generating progress would be much different from one for the copy-after-delete.
Thus, we need to add more concept to the progress.

[kinuko, 2013-09-09 03:46:30]

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
File webkit/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
webkit/browser/fileapi/file_system_operation.h:182: typedef
base::Callback<void(int64 size)> CopyFileProgressCallback;
On 2013/09/06 10:06:16, hidehiko wrote:
> On 2013/09/06 09:55:25, kinuko wrote:
> > Question: how difficult would it be (in terms of cost+complexity etc) to add
> the
> > same/similar progress callback to Move operation?  It feels a bit weird that
> > these two have different signature.
> 
> IMHO, it's a bit complicated especially for directory move (recursively).
> "Move" may be implemented as just updating the root's metadata.
> In such a case,
> generating progress would be much different from one for the
copy-after-delete.
> Thus, we need to add more concept to the progress.

Would it need more effort than adding one more progress type, if not firing any
progress event?  In cross-filesystem move we basically need to fall back to
recursive copy + remove, in which case I feel it's more natural to fire progress
callbacks.

Another question-- if the copy fails even before processing one file no progress
events should be fired?  (Asking this because OWP JS APIs tend to have multiple
progress event types and expects UA to fire at least one progress event at the
end of the operation (like 'fooend' for 'foo' operation) even in error case)  In
any case could we add a comment about the behavior?

[hidehiko, 2013-09-09 04:48:14]

PTAL?

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
File webkit/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/24030002/diff/1/webkit/browser/fileapi/file_s...
webkit/browser/fileapi/file_system_operation.h:182: typedef
base::Callback<void(int64 size)> CopyFileProgressCallback;
On 2013/09/09 03:46:31, kinuko wrote:
> On 2013/09/06 10:06:16, hidehiko wrote:
> > On 2013/09/06 09:55:25, kinuko wrote:
> > > Question: how difficult would it be (in terms of cost+complexity etc) to
add
> > the
> > > same/similar progress callback to Move operation?  It feels a bit weird
that
> > > these two have different signature.
> > 
> > IMHO, it's a bit complicated especially for directory move (recursively).
> > "Move" may be implemented as just updating the root's metadata.
> > In such a case,
> > generating progress would be much different from one for the
> copy-after-delete.
> > Thus, we need to add more concept to the progress.
> 
> Would it need more effort than adding one more progress type, if not firing
any
> progress event?  In cross-filesystem move we basically need to fall back to
> recursive copy + remove, in which case I feel it's more natural to fire
progress
> callbacks.
> 
> Another question-- if the copy fails even before processing one file no
progress
> events should be fired?  (Asking this because OWP JS APIs tend to have
multiple
> progress event types and expects UA to fire at least one progress event at the
> end of the operation (like 'fooend' for 'foo' operation) even in error case) 
In
> any case could we add a comment about the behavior?

Added comments about future extension and on error.

[kinuko, 2013-09-09 06:47:17]

Ok, lgtm

https://codereview.chromium.org/24030002/diff/7001/webkit/browser/fileapi/fil...
File webkit/browser/fileapi/file_system_operation_runner.h (right):

https://codereview.chromium.org/24030002/diff/7001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_operation_runner.h:68: // |dest_url| as
needed.
Can you add a comment about |progress_callback|?  (Just 'see
file_system_operation.h for details' is fine)

https://codereview.chromium.org/24030002/diff/7001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_operation_runner.h:198: //
ditto.

[kinaba, 2013-09-09 08:27:52]

lgtm

[hidehiko, 2013-09-09 11:49:54]

Thank you for your review. Sending to CQ.

https://codereview.chromium.org/24030002/diff/7001/webkit/browser/fileapi/fil...
File webkit/browser/fileapi/file_system_operation_runner.h (right):

https://codereview.chromium.org/24030002/diff/7001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_operation_runner.h:68: // |dest_url| as
needed.
On 2013/09/09 06:47:17, kinuko wrote:
> Can you add a comment about |progress_callback|?  (Just 'see
> file_system_operation.h for details' is fine)

Done.

https://codereview.chromium.org/24030002/diff/7001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_operation_runner.h:198: //
On 2013/09/09 06:47:17, kinuko wrote:
> ditto.

Done.

[commit-bot: I haz the power, 2013-09-09 11:50:08]

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

[commit-bot: I haz the power, 2013-09-09 14:18:55]

Change committed as 222024