Run copy operations in parallel when destinations are different volumes.

Run copy operations in parallel when destinations are different volumes.

BUG=430651
TEST=FileManagerJsTest.FileOperationManagerTest

Committed: https://crrev.com/5116df2c1b32aca1ca8da6839b3868271e5223c9
Cr-Commit-Position: refs/heads/master@{#325201}

[yawano, 2015-04-14 11:14:45]

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

[yawano, 2015-04-14 11:14:46]

PTAL. Thank you!

[hirono, 2015-04-15 03:18:12]

lgtm!

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/file_operation_manager.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager.js:29:
this.runningCopyTasks_ = {};
Please add comments about what is the key.

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager.js:266:
continue;
Should we remove the invalid copy task?

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File
ui/file_manager/file_manager/background/js/file_operation_manager_unittest.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager_unittest.js:70:
function waitUntil(condition) {
We have similar function in unittest_util.js

https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...

[yawano, 2015-04-15 03:47:36]

Thank you for the review!

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/file_operation_manager.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager.js:266:
continue;
I think we shouldn't silently remove invalid copy task. getVolumeInfo may return
null when the volume is unmounted after the task is added.

1. Current implementation: keeps the task in the list until a user explicitly
remove it by canceling it.
2. Another option: When it fails to get volume info, remove the task with
calling error callback.

WDYT?

[yawano, 2015-04-15 04:04:07]

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/file_operation_manager.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager.js:29:
this.runningCopyTasks_ = {};
On 2015/04/15 03:18:12, hirono wrote:
> Please add comments about what is the key.

Done.

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File
ui/file_manager/file_manager/background/js/file_operation_manager_unittest.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager_unittest.js:70:
function waitUntil(condition) {
On 2015/04/15 03:18:12, hirono wrote:
> We have similar function in unittest_util.js
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...

Done.

[hirono, 2015-04-15 04:25:00]

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/file_operation_manager.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager.js:266:
continue;
On 2015/04/15 03:47:36, yawano wrote:
> I think we shouldn't silently remove invalid copy task. getVolumeInfo may
return
> null when the volume is unmounted after the task is added.
> 
> 1. Current implementation: keeps the task in the list until a user explicitly
> remove it by canceling it.
> 2. Another option: When it fails to get volume info, remove the task with
> calling error callback.
> 
> WDYT?

I prefer option 2. I imagined the situation where we start 10 copy to USB, and
then remove the USB volume. The first copy task (that has already started)
raises an error, but remaining tasks are never removed from progress center
unless user cancel them.

[yawano, 2015-04-15 04:46:13]

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/file_operation_manager.js
(right):

https://codereview.chromium.org/1085823002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/file_operation_manager.js:266:
continue;
That makes sense. I'm going to add patch set to implement option 2. Thank you!

[yawano, 2015-04-15 05:53:59]

Added a patch set to fail with an error when volume info is not available. PTAL.
Thank you!

[hirono, 2015-04-15 06:25:19]

Thanks, still lgtm!

[yawano, 2015-04-15 06:34:30]

On 2015/04/15 06:25:19, hirono wrote:
> Thanks, still lgtm!

Thank you!

[yawano, 2015-04-15 06:34:35]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-04-15 06:35:09]

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

[commit-bot: I haz the power, 2015-04-15 07:04:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-15 07:05:44]

Patchset 4 (id:??) landed as
https://crrev.com/5116df2c1b32aca1ca8da6839b3868271e5223c9
Cr-Commit-Position: refs/heads/master@{#325201}