Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912033002:
Continue CopyTask when some files are unreadable. (Closed)
|
Created:
3 years, 6 months ago by tetsui Modified:
3 years, 6 months ago Reviewers:
fukino CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionContinue CopyTask when some files are unreadable.
Previously, in case of copying multiple files, CopyTask was aborted when
some files return errors during the copy. With this CL, the CopyTask
will skip the unreadable files and continue, while making separate
progress center item so that the remaining copy progress is still
visible.
BUG=499642
TEST=manually with fileSystemProvider extension to mock broken files.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2912033002
Cr-Commit-Position: refs/heads/master@{#476949}
Committed: https://chromium.googlesource.com/chromium/src/+/cc2b31f0e4fe164ec05ab2788356d15006f96c8f
Patch Set 1 #Patch Set 2 : Only generate single error progress center item. #Patch Set 3 : Add consecutive failure limit. #
Total comments: 2
Patch Set 4 : Keep successCallback and errorCallback behavior. #
Total comments: 6
Patch Set 5 : Do not run deleteOriginals when we have lastError. #Messages
Total messages: 36 (23 generated)
Description was changed from ========== Continue CopyTask when some files are unreadable. Previously, in case of copying multiple files, CopyTask was aborted when some files return errors during the copy. With this CL, the CopyTask will skip the unreadable files and continue, while making separate progress center item so that the remaining copy progress is still visible. BUG=499642 TEST=manually with fileSystemProvider extension to mock broken files. ========== to ========== Continue CopyTask when some files are unreadable. Previously, in case of copying multiple files, CopyTask was aborted when some files return errors during the copy. With this CL, the CopyTask will skip the unreadable files and continue, while making separate progress center item so that the remaining copy progress is still visible. BUG=499642 TEST=manually with fileSystemProvider extension to mock broken files. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tetsui@chromium.org changed reviewers: + fukino@chromium.org
fukino@: PTAL. Thanks!
self-review: * What if many files return errors? (progress center might be flooded) * The shown error in progress center is not useful enough, because the name of the file for the error is not included. * if it is better to have an unit test and how to do that (I personally have the fileSystemProvider extension to manually test this.) * I felt a bit hacky generating new task id
The CQ bit was checked by tetsui@chromium.org
The CQ bit was unchecked by tetsui@chromium.org
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look! I resolved the consecutive failure issue and the flooded progress center issue. http://crbug.com/499642 BTW, the CopyTask generates empty file for failed item from the original code. Maybe it should also be fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2912033002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2912033002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:856: successCallback(); It seems weird to call successCallback() when the length of consecutive errors exceeds the limit. I would keep the following statement true: "errorCallback is called if and only if the operation task fails." If we accept some copy errors during this task and the task fails only on excessive errors, maybe we should call errorCallback on the excessive errors and use progressCallback (or a new dedicated callback) to notify that some files are not copied during the task.
https://codereview.chromium.org/2912033002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2912033002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:856: successCallback(); On 2017/06/02 07:02:40, fukino wrote: > It seems weird to call successCallback() when the length of consecutive errors > exceeds the limit. > > I would keep the following statement true: > "errorCallback is called if and only if the operation task fails." > > If we accept some copy errors during this task and the task fails only on > excessive errors, maybe we should call errorCallback on the excessive errors and > use progressCallback (or a new dedicated callback) to notify that some files are > not copied during the task. I changed to call errorCallback with lastError at the end of the task. It seems to be consistent with the suggestion of the initial implementation here. https://bugs.chromium.org/p/chromium/issues/detail?id=499642#c28 Error notification during the task seems not be included in the suggestion at this point. Let's discuss offline later.
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:853: this.processedBytes = this.calcProcessedBytes_(); Is it safe to add bytes for un-copyable files? https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:868: errorCallback(lastError); Could you file a bug to report the detail of skipped files in the progress center?
https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:853: this.processedBytes = this.calcProcessedBytes_(); On 2017/06/05 04:04:37, fukino wrote: > Is it safe to add bytes for un-copyable files? calcProcessedBytes_ works properly with un-copyable files, because only processedBytes of the last file is used. https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:868: errorCallback(lastError); On 2017/06/05 04:04:37, fukino wrote: > Could you file a bug to report the detail of skipped files in the progress > center? Done. crbug.com/729462
https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:866: deleteOriginals(); we should not delete original files when we have lastError.
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2912033002/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:866: deleteOriginals(); On 2017/06/05 05:10:59, fukino wrote: > we should not delete original files when we have lastError. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. Thanks!
On 2017/06/05 05:24:18, fukino wrote: > lgtm. Thanks! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tetsui@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496641763700050,
"parent_rev": "4707d1724a8a2e4db9bc78715fe9b21fb95b10f7", "commit_rev":
"cc2b31f0e4fe164ec05ab2788356d15006f96c8f"}
Message was sent while issue was closed.
Description was changed from ========== Continue CopyTask when some files are unreadable. Previously, in case of copying multiple files, CopyTask was aborted when some files return errors during the copy. With this CL, the CopyTask will skip the unreadable files and continue, while making separate progress center item so that the remaining copy progress is still visible. BUG=499642 TEST=manually with fileSystemProvider extension to mock broken files. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Continue CopyTask when some files are unreadable. Previously, in case of copying multiple files, CopyTask was aborted when some files return errors during the copy. With this CL, the CopyTask will skip the unreadable files and continue, while making separate progress center item so that the remaining copy progress is still visible. BUG=499642 TEST=manually with fileSystemProvider extension to mock broken files. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2912033002 Cr-Commit-Position: refs/heads/master@{#476949} Committed: https://chromium.googlesource.com/chromium/src/+/cc2b31f0e4fe164ec05ab2788356... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/cc2b31f0e4fe164ec05ab2788356... |
