Add fileManagerPrivate.onCopyError event.

Add fileManagerPrivate.onCopyError event.

onCopyError event is dispatched when an error had happened during the operation.
The error is reported for each individual copy operation.

e.g. If you have /a/b/c and try to copy /a to /d, an error of copy file operation from /a/b/c to /d/b/c can be reported.

BUG=499642, 491046, 498087
TEST=none

Committed: https://crrev.com/77af6f93cbd761e70e254d6b353403266aeaa8bd
Cr-Commit-Position: refs/heads/master@{#335883}

[yawano, 2015-06-19 06:50:15]

yawano@chromium.org changed reviewers:
+ mtomasz@chromium.org, tzik@chromium.org

[yawano, 2015-06-19 06:50:16]

PTAL.

@mtomasz:
- files under chrome/browser/chromeos
- file_manager_private.idl

@tzik:
- files under storage/browser/fileapi
- files under content/browser/fileapi
- files under chrome/browser/sync_file_system

Thank you!

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/fil...
File storage/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/fil...
storage/browser/fileapi/file_system_operation.h:75: base::File::Error error)>
ErrorCallback;
@tzik: While I added this in http://crrev.com/1184343002, we are not using
ErrorCallback for now. Should we remove this?

If Yes, I'm going to add a patchset to remove it in this CL.
If No, I'm going to add a patchset to swap the |url| and |error| parameters of
ErrorCallback to keep consistent with CopyOrMoveErrorCallback.

I prefer to remove it since it's not used for now. WDYT?

[mtomasz, 2015-06-20 02:42:57]

https://codereview.chromium.org/1194783002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/file_manager_private.idl (right):

https://codereview.chromium.org/1194783002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/file_manager_private.idl:925: static void
onCopyError(long copyId, DOMString sourceURL, DOMString destionationURL);
nit: This line seems to long. Can we wrap?

https://codereview.chromium.org/1194783002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/file_manager_private.idl:927: static void
onCopyProgress(long copyId, CopyProgressStatus status);
CopyProgressStatus already passes an error. Why do we need a separate event?

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
File storage/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:733: const bool
continue_with_error,
Bools are hard to read. Can we made an enum, like enum ErrorBehavior {
  ERROR_BEHAVIOR_TERMINATE,
  ERROR_BEHAVIOR_CONTINUE
}?

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:781:
StartRecursiveOperationWithIgnoringError(src_root_, ErrorCallback(),
I'm thinking whether we can simplify this logic. Instead of having two different
code paths, we could make the progress callback have a return value. The
returned value would determine whether to terminate copying or continue. Would
it work?

[yawano, 2015-06-22 02:28:27]

Thank you for the review!

@mtomasz, @tzik: How about to integrate these two StartRecursiveOperation and
StartRecursiveOperationWithIgnoringError methods into one method to make it
simpler? Details are in the comment.

https://codereview.chromium.org/1194783002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/file_manager_private.idl (right):

https://codereview.chromium.org/1194783002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/file_manager_private.idl:927: static void
onCopyProgress(long copyId, CopyProgressStatus status);
Thank you! CopyProgressStatus event is enough. I'll remove the new
ErrorCallback.

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
File storage/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:781:
StartRecursiveOperationWithIgnoringError(src_root_, ErrorCallback(),
I don't like the idea that the progress callback returns a value. It seems to
make the behavior of recursive operation delegate more complex, i.e. by passing
the option at the start call, we can see how it works at a glance.

Because it takes a new error callback, I added
StartRecursiveOperationWithIgnoringError method. But since the new ErrorCallback
is not used for now, we can remove it and integrate these two methods into one
method.

StartRecursiveOperation(const FileSystemURL, const StatusCallback, const
ErrorBehavior).

@mtomasz, @tzik: WDYT?

[mtomasz, 2015-06-22 02:31:41]

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
File storage/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/1194783002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:781:
StartRecursiveOperationWithIgnoringError(src_root_, ErrorCallback(),
On 2015/06/22 02:28:27, yawano wrote:
> I don't like the idea that the progress callback returns a value. It seems to
> make the behavior of recursive operation delegate more complex, i.e. by
passing
> the option at the start call, we can see how it works at a glance.
> 
> Because it takes a new error callback, I added
> StartRecursiveOperationWithIgnoringError method. But since the new
ErrorCallback
> is not used for now, we can remove it and integrate these two methods into one
> method.
> 
> StartRecursiveOperation(const FileSystemURL, const StatusCallback, const
> ErrorBehavior).
> 
> @mtomasz, @tzik: WDYT?

SGTM

[yawano, 2015-06-23 02:18:43]

@tzik, @mtomasz: Removed ErrorCallback and changed to respond errors via
CopyProgressCallback. PTAL. Thank you!

[mtomasz, 2015-06-23 03:25:39]

In general lgtm. Is there any way we can add any test easily?

[yawano, 2015-06-23 03:45:33]

On 2015/06/23 03:25:39, mtomasz wrote:
> In general lgtm. Is there any way we can add any test easily?

For which test? For recursive operation delegate, we already have unittest of
continue error case. I should fix the unittest in this CL. -
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fi...

[yawano, 2015-06-23 11:21:46]

yawano@chromium.org changed reviewers:
+ sky@chromium.org

[yawano, 2015-06-23 11:21:46]

@sky: PTAL content/public/test/async_file_test_helper.cc. Thank you!

@tzik: Added significant amount of codes with new patch sets. PTAL. In addition
to the following files, PTAL also
chrome/browser/media_galleries/fileapi/native_media_file_util_unittest.cc. Thank
you!

- files under storage/browser/fileapi
- files under content/browser/fileapi
- files under chrome/browser/sync_file_system

[sky, 2015-06-23 16:28:04]

LGTM

[yawano, 2015-06-23 23:26:39]

On 2015/06/23 16:28:04, sky wrote:
> LGTM

@sky: Thank you!

[mtomasz, 2015-06-24 03:36:18]

https://codereview.chromium.org/1194783002/diff/40001/content/browser/fileapi...
File content/browser/fileapi/recursive_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/1194783002/diff/40001/content/browser/fileapi...
content/browser/fileapi/recursive_operation_delegate_unittest.cc:334:
TEST_F(RecursiveOperationDelegateTest, ContinueWithError) {
Can we add a similar test for ERROR_BEHAVIOR_TERMINATE to be sure that we don't
continue on error?

[yawano, 2015-06-24 03:38:48]

https://codereview.chromium.org/1194783002/diff/40001/content/browser/fileapi...
File content/browser/fileapi/recursive_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/1194783002/diff/40001/content/browser/fileapi...
content/browser/fileapi/recursive_operation_delegate_unittest.cc:334:
TEST_F(RecursiveOperationDelegateTest, ContinueWithError) {
We have one with the name of AbortWithError. It's above this method.

[mtomasz, 2015-06-24 03:44:31]

https://codereview.chromium.org/1194783002/diff/40001/content/browser/fileapi...
File content/browser/fileapi/recursive_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/1194783002/diff/40001/content/browser/fileapi...
content/browser/fileapi/recursive_operation_delegate_unittest.cc:334:
TEST_F(RecursiveOperationDelegateTest, ContinueWithError) {
On 2015/06/24 03:38:48, yawano wrote:
> We have one with the name of AbortWithError. It's above this method.

Ah, sorry I missed it! lgtm!

[tzik, 2015-06-24 05:01:24]

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
File storage/browser/fileapi/copy_or_move_operation_delegate.h (right):

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
storage/browser/fileapi/copy_or_move_operation_delegate.h:100: const
ErrorBehavior error_behavior,
nit: we usually pass enums by value without const.

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
File storage/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
storage/browser/fileapi/file_system_operation.h:128: enum ErrorBehavior {
ERROR_BEHAVIOR_TERMINATE, ERROR_BEHAVIOR_CONTINUE };
can we use ABORT/SKIP instead of TERMINATE/CONTINUE?

[yawano, 2015-06-24 06:02:22]

@tzik: PTAL again. Thank you!

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
File storage/browser/fileapi/copy_or_move_operation_delegate.h (right):

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
storage/browser/fileapi/copy_or_move_operation_delegate.h:100: const
ErrorBehavior error_behavior,
On 2015/06/24 05:01:24, tzik wrote:
> nit: we usually pass enums by value without const.

Done.

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
File storage/browser/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/1194783002/diff/80001/storage/browser/fileapi...
storage/browser/fileapi/file_system_operation.h:128: enum ErrorBehavior {
ERROR_BEHAVIOR_TERMINATE, ERROR_BEHAVIOR_CONTINUE };
On 2015/06/24 05:01:24, tzik wrote:
> can we use ABORT/SKIP instead of TERMINATE/CONTINUE?

Done.

[tzik, 2015-06-24 07:17:19]

lgtm

[yawano, 2015-06-24 07:17:50]

On 2015/06/24 07:17:19, tzik wrote:
> lgtm

Thank you!

[yawano, 2015-06-24 07:17:56]

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-06-24 07:17:56]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/1194783002/#ps100001
(title: "Remove const and rename enum.")

[commit-bot: I haz the power, 2015-06-24 07:18:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1194783002/100001

[commit-bot: I haz the power, 2015-06-24 07:22:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-24 07:23:48]

Patchset 6 (id:??) landed as
https://crrev.com/77af6f93cbd761e70e254d6b353403266aeaa8bd
Cr-Commit-Position: refs/heads/master@{#335883}