Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570693002:
Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic (Closed)
DescriptionFix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic
JumpListIcons(Old) folders are getting unexpectively huge for some users.
This is caused by file operation failures.
After analyzing those two folders' operation metrics by landing several changes,
we find that there are a few issues that need to be fixed.
1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target
dir string contains source dir string. Please see detailed comments in the code,
2) When it fails to delete folder |JumpListIconOld|, we should not move
folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can
get unexpectedly huge eventually.
3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we
should still delete |JumpListIcon| to avoid endless file accumulation in it.
To fix the first issue, IsParent() function is used in file_util_win.cc to
replace the old logic. I have tested it in a previously checked-in CL.
The gathered user data confirms its correctness. It can be found in line
370 of the old version of jumplist.cc in this CL. In that version, I copied
and changed a few file operation functions from //base for debugging and
testing purposes.
We still log one kind of user data for verification and further analysis.
This CL also reverts several changes I made for debugging purpose.
BUG=40407, 179576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/76561121d20d72e2d54a9071fe30b1db23b128ad
Cr-Commit-Position: refs/heads/master@{#438778}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Mark retired metrics as obselete rather than deleting them #
Total comments: 2
Patch Set 3 : Fix deletion logic for JumpListIcon folder #
Total comments: 19
Patch Set 4 : Update comments and fix some nits #
Total comments: 4
Patch Set 5 : Update some comments #
Total comments: 5
Messages
Total messages: 74 (57 generated)
Description was changed from ========== Format xml Fix base::CopyDirectory() and force to delete JumpListIcon folder Merge branch 'master' of https://chromium.googlesource.com/chromium/src into fixjumplistfoldermassive remove unnecessary included header files Fix a few more nits More UMA record and fix some nits Format xml file Mark old histogram as obselete Format xml file Log more detailed user data for base::move() operation BUG= ========== to ========== Format xml Fix base::CopyDirectory() and force to delete JumpListIcon folder Merge branch 'master' of https://chromium.googlesource.com/chromium/src into fixjumplistfoldermassive remove unnecessary included header files Fix a few more nits More UMA record and fix some nits Format xml file Mark old histogram as obselete Format xml file Log more detailed user data for base::move() operation BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by chengx@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 ========== Format xml Fix base::CopyDirectory() and force to delete JumpListIcon folder Merge branch 'master' of https://chromium.googlesource.com/chromium/src into fixjumplistfoldermassive remove unnecessary included header files Fix a few more nits More UMA record and fix some nits Format xml file Mark old histogram as obselete Format xml file Log more detailed user data for base::move() operation BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those folders' operation (delete+move+creation) metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those folders' operation (delete+move+creation) metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those folders' operation (delete+move+creation) metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those folders' operation (delete+move+creation) metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metric by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. In thi CL, we still log user data for verification and further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld}, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) when it fails to move |JumpListIcon| folder to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move |JumpListIcon| folder to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move |JumpListIcon| folder to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
chengx@chromium.org changed reviewers: + brucedawson@chromium.org, dcheng@chromium.org, gab@chromium.org, isherman@chromium.org
This CL is ready for review. @Ilya, could you PTAL the metric? @Gabriel, could you PTAL jumplist.cc? @Daniel, could you PTAL file_util_win.cc? Thanks!
Are you sure you want to remove the metrics now? It might be better to leave them in the code in case the problem is still not resolved. I'm not sure there is any hurry to remove them. https://codereview.chromium.org/2570693002/diff/1/base/files/file_util_win.cc File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/1/base/files/file_util_win.cc... base/files/file_util_win.cc:189: if (real_to_path.value().size() >= real_from_path.value().size() && beloe -> below Can you also add a comment explaining what the check is trying to prevent? The original code should have had such a comment I think.
https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:74767: - enum="JumplistIconsFolderMoveCategory"> Please mark this metric as <obsolete> rather than removing its metadata from this file. https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:74753: + enum="DetailedJumplisticonsfolderCategory"> nit: The previous enum name was better. It's helpful for scanning if the feature name is listed first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chengx@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 ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 steps that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation function from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation function from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation function from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation function from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
On 2016/12/13 00:35:45, brucedawson wrote: > Are you sure you want to remove the metrics now? It might be better to leave > them in the code in case the problem is still not resolved. I'm not sure there > is any hurry to remove them. I kept one of the original two metrics. For the one deleted, it can no longer be recorded as it can not be simply done in jumplist.cc. It was recorded by moving a lot of functions from //base to jumplist.cc. Now we are making changes in //base, and we typically don't want to add UMA code in //base.
This patch addressed the comments from Bruce and Ilya. https://codereview.chromium.org/2570693002/diff/1/base/files/file_util_win.cc File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/1/base/files/file_util_win.cc... base/files/file_util_win.cc:189: if (real_to_path.value().size() >= real_from_path.value().size() && On 2016/12/13 00:35:45, brucedawson wrote: > beloe -> below > > Can you also add a comment explaining what the check is trying to prevent? The > original code should have had such a comment I think. I have fixed the typo, and added more comments to this change. Besides, the usage of IsParent() function has been tested in a previously checked-in CL. It was in jumplist.cc line 370, if you look at its old version in this CL. https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:74767: - enum="JumplistIconsFolderMoveCategory"> On 2016/12/13 00:40:51, Ilya Sherman wrote: > Please mark this metric as <obsolete> rather than removing its metadata from > this file. Done. https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2570693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:74753: + enum="DetailedJumplisticonsfolderCategory"> On 2016/12/13 00:40:51, Ilya Sherman wrote: > nit: The previous enum name was better. It's helpful for scanning if the > feature name is listed first. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2570693002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2570693002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74753: - enum="JumplistIconsDetailedFolderMoveCategory"> It still looks like you've removed the metadata for this histogram. Please mark it as <obsolete> instead. (Or am I misreading the diff somehow?)
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are 2 issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 3) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise the latter can get unexpectedly huge eventually. 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 3) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise the latter can get unexpectedly huge eventually. 2) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise the latter can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
https://codereview.chromium.org/2570693002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2570693002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74753: - enum="JumplistIconsDetailedFolderMoveCategory"> On 2016/12/13 06:20:07, Ilya Sherman wrote: > It still looks like you've removed the metadata for this histogram. Please mark > it as <obsolete> instead. (Or am I misreading the diff somehow?) Sorry I thought this could be removed. Now I have added it back and marked as obsolete.
The CQ bit was checked by chengx@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 ========== Fix base::CopyDirectory() and force to delete JumpListIcon folder even if its move fails JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise the latter can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM % comments https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74780: + any of these file operations is suspected to be related to a known issue. Throughout this paragraph, please separate sentences with spaces. https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74780: + any of these file operations is suspected to be related to a known issue. Is there a bug on file for the known issue, that you could link to from this description?
lgtm w/ comments, this is great, thanks! https://codereview.chromium.org/2570693002/diff/40001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:184: // contains the other, e.g. C:\\JumpListIcon and C:\\JumpListIconOld. s/JumpListIcon/Foo/ in example (since this is in //base) also no need to escape the \ in a comment :) https://codereview.chromium.org/2570693002/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:187: // "return false" below to execute. No need to describe history here IMO, just say: // Note: it's important to use IsParent() here as string comparison would result in a false positive on, e.g. C:\Foo versus C:\FooOld. https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: // |icon_dir_old|, instead, delete |icon_dir| directly. "(...) directly to avoid bloating |icon_dir_old| by moving more things to it: http://crbug.com/179576/" https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:263: if (!base::Move(icon_dir, icon_dir_old)) { Combine with else {} into an else if () {} https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:265: // If Move() fails, we force to delete |icon_dir| to avoide file s/avoide/avoid/ https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:266: // accumulation Avoid "we", e.g.: // If Move() fails, delete |icon_dir| to avoid file accumulation: http://crbug.com/179576. https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74803: + <obsolete> Should matching enums be marked as obsolete? (I forget..)
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
https://codereview.chromium.org/2570693002/diff/40001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:184: // contains the other, e.g. C:\\JumpListIcon and C:\\JumpListIconOld. On 2016/12/13 21:31:21, gab wrote: > s/JumpListIcon/Foo/ in example (since this is in //base) > > also no need to escape the \ in a comment :) Done. https://codereview.chromium.org/2570693002/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:187: // "return false" below to execute. On 2016/12/13 21:31:21, gab wrote: > No need to describe history here IMO, just say: > > // Note: it's important to use IsParent() here as string comparison would result > in a false positive on, e.g. C:\Foo versus C:\FooOld. Done. https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: // |icon_dir_old|, instead, delete |icon_dir| directly. On 2016/12/13 21:31:21, gab wrote: > "(...) directly to avoid bloating |icon_dir_old| by moving more things to it: > http://crbug.com/179576/%22 I will just remove |...| https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:263: if (!base::Move(icon_dir, icon_dir_old)) { On 2016/12/13 21:31:21, gab wrote: > Combine with else {} into an else if () {} Done. https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:265: // If Move() fails, we force to delete |icon_dir| to avoide file On 2016/12/13 21:31:21, gab wrote: > s/avoide/avoid/ Done. https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:266: // accumulation On 2016/12/13 21:31:21, gab wrote: > Avoid "we", e.g.: > > // If Move() fails, delete |icon_dir| to avoid file accumulation: > http://crbug.com/179576. Done. https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74780: + any of these file operations is suspected to be related to a known issue. On 2016/12/13 21:08:32, Ilya Sherman wrote: > Throughout this paragraph, please separate sentences with spaces. Done. I think it is the python format script that did that. https://codereview.chromium.org/2570693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74803: + <obsolete> On 2016/12/13 21:31:22, gab wrote: > Should matching enums be marked as obsolete? (I forget..) Done.
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.
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is be caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the 1st issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. You can find it in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. It can be found in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
+grt who's digging into the fallouts of base::Move https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: // |icon_dir_old|, instead, delete |icon_dir| directly. On 2016/12/13 23:00:40, chengx wrote: > On 2016/12/13 21:31:21, gab wrote: > > "(...) directly to avoid bloating |icon_dir_old| by moving more things to it: > > http://crbug.com/179576/%22 > > I will just remove |...| No, you need the |...| around vars per style. What I meant is to add justification as to why this is required. https://codereview.chromium.org/2570693002/diff/60001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/60001/base/files/file_util_wi... base/files/file_util_win.cc:181: // result nit: wrapping https://codereview.chromium.org/2570693002/diff/60001/base/files/file_util_wi... base/files/file_util_win.cc:182: // in a false negative on, e.g. C:\bar\Foo versus C:\bar\FooOld. s/on, e.g./, e.g. on/
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
@Daniel, could you please take a look at this CL? https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2570693002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: // |icon_dir_old|, instead, delete |icon_dir| directly. On 2016/12/14 15:04:23, gab wrote: > On 2016/12/13 23:00:40, chengx wrote: > > On 2016/12/13 21:31:21, gab wrote: > > > "(...) directly to avoid bloating |icon_dir_old| by moving more things to > it: > > > http://crbug.com/179576/%22 > > > > I will just remove |...| > > No, you need the |...| around vars per style. > > What I meant is to add justification as to why this is required. Sorry, I thought you wanted me to replace |...| with (...) https://codereview.chromium.org/2570693002/diff/60001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/60001/base/files/file_util_wi... base/files/file_util_win.cc:181: // result On 2016/12/14 15:04:23, gab wrote: > nit: wrapping Done. https://codereview.chromium.org/2570693002/diff/60001/base/files/file_util_wi... base/files/file_util_win.cc:182: // in a false negative on, e.g. C:\bar\Foo versus C:\bar\FooOld. On 2016/12/14 15:04:23, gab wrote: > s/on, e.g./, e.g. on/ Done.
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.
lgtm (sorry for the review latency)
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2570693002/#ps80001 (title: "Update some comments")
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": 1481788202962110,
"parent_rev": "8f68e26db1bd8d0bf0060f38c047297b2ee0670d", "commit_rev":
"8e784f8f16e682d80567227268fd8f5d674121d6"}
Message was sent while issue was closed.
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. It can be found in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. It can be found in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2570693002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. It can be found in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2570693002 ========== to ========== Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic JumpListIcons(Old) folders are getting unexpectively huge for some users. This is caused by file operation failures. After analyzing those two folders' operation metrics by landing several changes, we find that there are a few issues that need to be fixed. 1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target dir string contains source dir string. Please see detailed comments in the code, 2) When it fails to delete folder |JumpListIconOld|, we should not move folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can get unexpectedly huge eventually. 3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we should still delete |JumpListIcon| to avoid endless file accumulation in it. To fix the first issue, IsParent() function is used in file_util_win.cc to replace the old logic. I have tested it in a previously checked-in CL. The gathered user data confirms its correctness. It can be found in line 370 of the old version of jumplist.cc in this CL. In that version, I copied and changed a few file operation functions from //base for debugging and testing purposes. We still log one kind of user data for verification and further analysis. This CL also reverts several changes I made for debugging purpose. BUG=40407,179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/76561121d20d72e2d54a9071fe30b1db23b128ad Cr-Commit-Position: refs/heads/master@{#438778} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/76561121d20d72e2d54a9071fe30b1db23b128ad Cr-Commit-Position: refs/heads/master@{#438778}
Message was sent while issue was closed.
post-commit drive-by. please address in a followup CL. thanks. https://codereview.chromium.org/2570693002/diff/80001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2570693002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:180: // Note: it's important to use IsParent() here as string comparison would nit: i think this comment is unnecessary. the FilePath API should be used because it's the right thing. otherwise, why is this one use of IsParent so important that this comment is justified in comparison to every other use of IsParent in the codebase? https://codereview.chromium.org/2570693002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:182: if (real_to_path.value().size() >= real_from_path.value().size() && this "optimization" seems wrong to me -- either a FilePath is treated like a string (as was with the old code), or it's treated as a FilePath (as it is with the call to IsParent). i think it's a mistake to mix the two like this. https://codereview.chromium.org/2570693002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:183: real_from_path.IsParent(real_to_path)) { please make this same fix in file_util_posix https://codereview.chromium.org/2570693002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2570693002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:254: int folder_operation_status = FolderOperationResult::SUCCESS; nit: always use an unsigned type to hold a bitfield -- uint32_t would be good here https://codereview.chromium.org/2570693002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:256: if (base::PathExists(icon_dir_old) && !base::DeleteFile(icon_dir_old, true)) { base::PathExists is unnecessary -- DeleteFile returns true if the item didn't exist (and is documented as such). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
