Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2564343002:
Mark duplicate files even when scan result found no new files. (Closed)
|
Created:
4 years ago by yamaguchi Modified:
3 years, 11 months ago CC:
chromium-reviews, tfarina, yamaguchi+watch_chromium.org, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMark duplicate files even when scan result found no new files.
Downside of this change is that it'll rerun the latest scan after
marking the duplicate files. Though, the second scan is faster than
the first one as it'll not find any new files, thus can skip
de-duplication by content hashes. It may increase the scan time
especially when having large number of files in a media and doing the
"manual copy to Drive" operation like noted in the bug comment.
https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16
BUG=476684
TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/8336e28a7e8d7c64cb8c96912d874eb0483b2668
Cr-Commit-Position: refs/heads/master@{#439444}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Avoid recursive call to checkState_. #
Total comments: 2
Messages
Total messages: 26 (17 generated)
Description was changed from ========== Mark duplicate files when scan result found only content duplicates. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 ========== to ========== Mark duplicate files when scan result found only content duplicates. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@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...
Description was changed from ========== Mark duplicate files when scan result found only content duplicates. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files when scan result found only content duplicates. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 TEST=manual test as noted in the bug. Confirm content dup files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Mark duplicate files when scan result found only content duplicates. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 TEST=manual test as noted in the bug. Confirm content dup files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 TEST=manual test as noted in the bug. Confirm content dup files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll run history duplicate scan again after marking the duplicate files. BUG=476684 TEST=manual test as noted in the bug. Confirm content dup files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files, but it is faster than the first one as it'd not find any new files. BUG=476684 TEST=manual test as noted in the bug. Confirm content dup files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files, but it is faster than the first one as it'd not find any new files. BUG=476684 TEST=manual test as noted in the bug. Confirm content dup files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files and can skip de-duplication by content hash. BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files and can skip de-duplication by content hash. BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
PTAL.
https://codereview.chromium.org/2564343002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/2564343002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/import_controller.js:385: this.execute_(); As discussed offline, I will revise this to avoid the recursion described in the comment here.
ptal https://codereview.chromium.org/2564343002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/2564343002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/import_controller.js:287: * by calling "update" on this class. Note: importer.ImportController.update doesn't exist now. I found it in the old revision. https://codereview.chromium.org/792233009/diff/200001/ui/file_manager/file_ma... https://codereview.chromium.org/2564343002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/import_controller.js:385: this.execute_(); On 2016/12/19 03:30:33, yamaguchi wrote: > As discussed offline, I will revise this to avoid the recursion described in the > comment here. Done.
lgtm
The CQ bit was checked by yamaguchi@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": 20001, "attempt_start_ts": 1482137985973640,
"parent_rev": "c9ec98ca22345bf9e6a8bd3e8ae50795bf3e0bed", "commit_rev":
"07334adbfccf6d13b5ac616cf8eb8f0942d684f3"}
Message was sent while issue was closed.
Description was changed from ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2564343002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2564343002 ========== to ========== Mark duplicate files even when scan result found no new files. Downside of this change is that it'll rerun the latest scan after marking the duplicate files. Though, the second scan is faster than the first one as it'll not find any new files, thus can skip de-duplication by content hashes. It may increase the scan time especially when having large number of files in a media and doing the "manual copy to Drive" operation like noted in the bug comment. https://bugs.chromium.org/p/chromium/issues/detail?id=476684#c16 BUG=476684 TEST=manual test as noted in the bug. Confirm content-duplicate files marked as backed up after scan. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/8336e28a7e8d7c64cb8c96912d874eb0483b2668 Cr-Commit-Position: refs/heads/master@{#439444} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8336e28a7e8d7c64cb8c96912d874eb0483b2668 Cr-Commit-Position: refs/heads/master@{#439444}
Message was sent while issue was closed.
smckay@chromium.org changed reviewers: + smckay@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2564343002/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/2564343002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:395: this.startImportTask_(); Seems like this might result in some false reports of imports. Can we break up startImportTask_ to avoid this? Also, it looks like startImportTask_ will ultimately call onImportFinished_ which itself calls checkState_ again. So there appears to be risk of an infinite loop.
Message was sent while issue was closed.
https://codereview.chromium.org/2564343002/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/2564343002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:395: this.startImportTask_(); On 2016/12/27 17:10:58, Steve McKay wrote: > Seems like this might result in some false reports of imports. Can we break up > startImportTask_ to avoid this? I agree. I found it's importer.MediaImportHandler.ImportTask that sends reports to the tracker. Therefore I think a possible fix would be to stop reusing ImportTask but establish a different type of Task (e.g. importer.MediaImportHandler.MarkDuplicatesTask) sharing part of the code. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/background/... Having said that, this code path will no longer be used after the change to skip content-duplicate check at the scan phase. It's because history-duplicate is not included in the getDuplicateFileEntrie(). https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/background/... I think we need to cleanup code around this. > > Also, it looks like startImportTask_ will ultimately call onImportFinished_ > which itself calls checkState_ again. So there appears to be risk of an infinite > loop. Thanks for finding it out. I agree that we should have a separate code path to avoid such risk. |
