|
|
DescriptionRemove JumpListIconsOld directory and set upper limit for delete attempts
Current implementation for jumplist involves deleting JumpListIconOld
followed by moving JumpListIcons to JumpListIconsOld. In addition to
that holding icons in JumpListIconsOld has little benefits, this method
is problematic as we assume file's deletion and move are perfect while
they are not. There are certain unexpected situations where these steps
can fail, which has been proved by UMA metrics. This CL removes any
existing file operations related to JumpListIconsOld. A background
task is added to delete JumpListIconsOld for all users.
Also in one jumplist icons update, JumpListIconsOld and JumpListIcons
folders are tried to be deleted, which is a step with heavy IO involved.
Given that the UMA metrics are showing there are still quite a number
of updates taking a very long time, it is skeptical that base::Delete
has issues in some situations, e.g., when antivirus software is scanning
the icon files. It can be the case that base::Delete marks the files
deleted yet they are still there, which causes the files to be read in
every jumplist update.
A method to handle this case is to set an upper limit for the amount
of jumplist icons deleted as well as the amount of attempt failures
per update. This can avoid the jumplist file operations from taking a
very long time. Typically, there are less than 30 jumplist icons in
JumpListIcons(Old) folder, so setting the upper limit to be 100 is a
reasonable choice. This ensures a number of icons can be deleted per
update for users whose jumpilst folders are bloated without holding
the IO thread for an excessive long time.
Customized delete methods for JumpListIcons(Old) directories are created,
which allows limiting the time spent on file deletion and recording all
the possible failure reasons. These methods are put in new files named
jumplist_file_util.{h,cc}. Unit tests for these methods are also created.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2752063002
Cr-Commit-Position: refs/heads/master@{#459297}
Committed: https://chromium.googlesource.com/chromium/src/+/2eefdd30dac56219587f39e35eac276444470401
Patch Set 1 #Patch Set 2 : Remove JumpListIconsOld related file operations. #
Total comments: 26
Patch Set 3 : Address comments. #
Total comments: 38
Patch Set 4 : Add jumplist_util_file* and unit tests, plus address other comments. #
Total comments: 30
Patch Set 5 : Address feedback comments. #
Total comments: 4
Patch Set 6 : Fix win_clang, update code comments. #
Total comments: 18
Patch Set 7 : Fix nits. #Patch Set 8 : Log JumpListIcons folder empty status to UMA. #Patch Set 9 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into jumplistdeletesetupperā¦ #
Messages
Total messages: 135 (113 generated)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Set upper limit for jumplist icons deleted per update. BUG=40407, 179576 ========== to ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric which shows there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete is not doing correctly in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted while they are not, which causes the files (can be a lot when the folder is corrupted) to be read per jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operation from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
Description was changed from ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric which shows there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete is not doing correctly in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted while they are not, which causes the files (can be a lot when the folder is corrupted) to be read per jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operation from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric which shows there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete is not doing correctly in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted while they are not, which causes the files (can be a lot when the folder is corrupted) to be read per jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operation from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
Description was changed from ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric which shows there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete is not doing correctly in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted while they are not, which causes the files (can be a lot when the folder is corrupted) to be read per jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operation from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete is not doing correctly in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
Hi Greg, I am trying to evaluate how base::Delete affects the jumplist update using this CL. PTAL~
Description was changed from ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete is not doing correctly in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
It's looking more and more like the original design of this code was imperfect. Rather than continue to apply bailing wire and duct tape to it, perhaps now is a good time to rethink everything. As I understand it, we have a JumpListIcons directory into which Chrome periodically spews icons and then notifies the shell somehow. Rather than dealing with renaming and deleting directories (which is clearly tricky on Windows), why don't we switch to something more simple and direct? I haven't studied the code, so the following may not work out of the box, but how about something like this: Say we want to create N new icons that should replace all pre-existing icons. to do so, we: - Build a std::vector O of all files in JumpListIcons. - If O.size() > some cap C, delete some random N old icons -- hopefully we never need to do this, but consider this the "avoid explosion" safeguard. - Write the new icons into JumpListIcons. - Notify the shell. - Post a low-priority background task to delete all files in O. Potential flaws and their respective mitigations: F1: Chrome could crash before the delete takes place. M1: The next update will clean up the old items. F2: Chrome could continually crash before the post-update delete ever takes place. M2: Some future update will delete at least some of the files before writing new ones once the cap C is reached. F3: When the cap C is reached, Chrome could delete icons that are still referenced by the shell. M3: Capture the file creation time when building O, sort it by age, and always delete starting from the oldest file. This is still imperfect, but has a high likelihood of preserving active icons in most cases. F4: We discover that F3 is truly a problem and M3 doesn't adequately address it. M4: Ask the shell to tell us which icons are in use and avoid deleting those during the pre-update purge.
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...
Thanks for the detailed suggestion! I have some comments inline with yours below. On 2017/03/16 08:50:46, grt (UTC plus 1) wrote: > It's looking more and more like the original design of this code was imperfect. > Rather than continue to apply bailing wire and duct tape to it, perhaps now is a > good time to rethink everything. > > As I understand it, we have a JumpListIcons directory into which Chrome > periodically spews icons and then notifies the shell somehow. Rather than > dealing with renaming and deleting directories (which is clearly tricky on > Windows), why don't we switch to something more simple and direct? I haven't > studied the code, so the following may not work out of the box, but how about > something like this: In more details, we have two folders namely JumpListIcons and JumpListIconsOld. In one Jumplist icons' update, we first try to delete JumpListIconsOld, and then rename JumpListIcons to JumpListIconsOld, and then create a new empty folder JumpListIcons. Following this, we write all the new jumplist icons into folder JumpListIcons, and then notify the shell. As you may have noticed, I have made some steps above take place conditional on the previous ones. For example, if deletion of JumpListIconsOld fails, no need to rename JumpListIcons to JumpListIconsOld. > Say we want to create N new icons that should replace all pre-existing icons. to > do so, we: > - Build a std::vector O of all files in JumpListIcons. > - If O.size() > some cap C, delete some random N old icons -- hopefully we never > need to do this, but consider this the "avoid explosion" safeguard. > - Write the new icons into JumpListIcons. > - Notify the shell. > - Post a low-priority background task to delete all files in O. Regarding to the 1st step "build a std::vector...", do you mean loading all the jumplist icons (gfx::ImageSkia) into memory and build a vector for them? IMHO, I don't think this is feasible for users whose JumpListIcons(Old) folders are corrupted/bloated already because of the heavy IO. If you mean just counting the number of icons in the folder, that still takes quite a bit of time and then there is not much difference comparing to the current implementation. Regarding to the 2nd step "If O.size() > some cap C...", I assume you mean deleting the N old icons directly in the folder not the vector, then the potential failure is that "delete some random N old icons" can fail. This is what we suspected, i.e., base::Delete is imperfect, or it can mark icons "deleted" even though the icons are not deleted, etc. If you are saying loading all the icons into a vector, and delete random N in the vector if its size is greater than cap C, and write it back, then again, I don't think this is feasible for users who already have a lot of icons in the JumpListIcons folder. I think if base::Delete works perfectly, the current jumplist implementation should not have such a big performance issue. > Potential flaws and their respective mitigations: > F1: Chrome could crash before the delete takes place. > M1: The next update will clean up the old items. The current implementation also tries to delete the old icons in every update. I still think the issue is base::Delete itself. > F2: Chrome could continually crash before the post-update delete ever takes > place. > M2: Some future update will delete at least some of the files before writing new > ones once the cap C is reached. > F3: When the cap C is reached, Chrome could delete icons that are still > referenced by the shell. > M3: Capture the file creation time when building O, sort it by age, and always > delete starting from the oldest file. This is still imperfect, but has a high > likelihood of preserving active icons in most cases. > > F4: We discover that F3 is truly a problem and M3 doesn't adequately address it. > M4: Ask the shell to tell us which icons are in use and avoid deleting those > during the pre-update purge. I agree we need to preserve active icons. I was focusing on deleting the icons effectively and thought I could put this aside temporarily.
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have used UMA metrics extensively trying to find the root cause and monitoring the changes (good or bad) my previous CLs have brought. There are two UMA metrics I am using right now, one is UMA histogram that records those JumpListIcons(Old) folders' operation results, and the other one is the running time of whole jumplist update. After analyzing them together, I started to suspect base::Delete as well. One fact is that full validation of any CL trying to improve this requires the CL to hit stable branch. Canary has a different JumpListIcon(Old) folder from Dev/Beta/Stable branches. In addition, there are a lot more users with Stable branch than others, therefore most of the users having this trouble are in Stable branch. I am stilling waiting for M57 stable branch fully released to see how much benefit my CLs landed last year can bring. Such a pain! I agree with you that starting a new design is something need to consider now. On the other hand, I am eager to understand which step in the jumplist update is the true nightmare. As the UMA metric is no longer reliable in telling if base::Delete works okay now. I have 2 more CLs (including this one) I want to land to see what happens. Apart from this CL, the other one is that I want to get rid of JumpListIconsOld in the update, as I don't see any of its usage excepting for holding the old icons temporarily. While trying these 2 CLs, I will create a google doc for discussing the new design and shared it with you and maybe others. How do you think please? Thanks Greg!
On 2017/03/16 18:17:33, chengx wrote: > Thanks for the detailed suggestion! I have some comments inline with yours > below. > > On 2017/03/16 08:50:46, grt (UTC plus 1) wrote: > > It's looking more and more like the original design of this code was > imperfect. > > Rather than continue to apply bailing wire and duct tape to it, perhaps now is > a > > good time to rethink everything. > > > > As I understand it, we have a JumpListIcons directory into which Chrome > > periodically spews icons and then notifies the shell somehow. Rather than > > dealing with renaming and deleting directories (which is clearly tricky on > > Windows), why don't we switch to something more simple and direct? I haven't > > studied the code, so the following may not work out of the box, but how about > > something like this: > > In more details, we have two folders namely JumpListIcons and JumpListIconsOld. > In one Jumplist icons' update, we first try to delete JumpListIconsOld, and then > rename JumpListIcons to JumpListIconsOld, and then create a new empty folder > JumpListIcons. Following this, we write all the new jumplist icons into folder > JumpListIcons, and then notify the shell. As you may have noticed, I have made > some steps above take place conditional on the previous ones. For example, if > deletion of JumpListIconsOld fails, no need to rename JumpListIcons to > JumpListIconsOld. > > > Say we want to create N new icons that should replace all pre-existing icons. > to > > do so, we: > > - Build a std::vector O of all files in JumpListIcons. > > - If O.size() > some cap C, delete some random N old icons -- hopefully we > never > > need to do this, but consider this the "avoid explosion" safeguard. > > - Write the new icons into JumpListIcons. > > - Notify the shell. > > - Post a low-priority background task to delete all files in O. > > Regarding to the 1st step "build a std::vector...", do you mean loading all the > jumplist icons (gfx::ImageSkia) into memory and build a vector for them? No, just enumerate the items in the directory and stuff their paths into a vector. > IMHO, I > don't think this is feasible for users whose JumpListIcons(Old) folders are > corrupted/bloated already because of the heavy IO. If you mean just counting the > number of icons in the folder, that still takes quite a bit of time and then > there is not much difference comparing to the current implementation. > > Regarding to the 2nd step "If O.size() > some cap C...", I assume you mean > deleting the N old icons directly in the folder not the vector, then the > potential failure is that "delete some random N old icons" can fail. This is > what we suspected, i.e., base::Delete is imperfect, or it can mark icons > "deleted" even though the icons are not deleted, etc. The old code is flawed because it assumes that: 1) moving a directory is error-free 2) deleting a directory and its contents is error-free It appears to me that you are pushing a boulder uphill trying to make this work. I think that any "two-directory" solution (such as the current one) is likely to have worse failure modes than one that works in a single directory. As for deleting a file, you can tell Windows to delete a single file with extremely high confidence with the following: // Delete |path| at some point in the future. bool will_be_deleted = base::File(path, (base::File::FLAG_OPEN | base::File::FLAG_READ | base::File::FLAG_DELETE_ON_CLOSE | base::File::FLAG_SHARE_DELETE)).IsValid(); when |will_be_deleted| is true, you are guaranteed that the file will go away at some point in the future as long as the machine doesn't lose power or bluescreen. The file will vanish even if Chrome crashes immediately afterward. What you can't do is rely on |will_be_deleted| = true meaning that you can create a new file with the same name or delete/move the directory containing the file -- another process may still have the file open. > If you are saying loading > all the icons into a vector, and delete random N in the vector if its size is > greater than cap C, and write it back, then again, I don't think this is > feasible for users who already have a lot of icons in the JumpListIcons folder. No, I wasn't proposing this. The vector was just to know which old files in the dir need to be deleted after the new files are written. Another approach would be: - ask the shell for the list of "active" icons - delete anything in the dir that isn't in the above list - write the new icons - notify the shell - delete the files in the dir in the above list You could even split the first two tasks into their own thing that can be run in the background via BrowserThread::PostAfterStartupTask so that Chrome quietly sneaks off in the bg and harvests old icon files when there's no jumplist update needed. > I think if base::Delete works perfectly, the current jumplist implementation > should not have such a big performance issue. Except that moving/deleting directories is fragile. It always will be. There will always be things outside of your control that could interfere with moving and deleting directories like this. Deleting single files without the directory swap will be more reliable. > > Potential flaws and their respective mitigations: > > F1: Chrome could crash before the delete takes place. > > M1: The next update will clean up the old items. > > The current implementation also tries to delete the old icons in every update. I > still think the issue is base::Delete itself. > > > F2: Chrome could continually crash before the post-update delete ever takes > > place. > > M2: Some future update will delete at least some of the files before writing > new > > ones once the cap C is reached. > > > F3: When the cap C is reached, Chrome could delete icons that are still > > referenced by the shell. > > M3: Capture the file creation time when building O, sort it by age, and always > > delete starting from the oldest file. This is still imperfect, but has a high > > likelihood of preserving active icons in most cases. > > > > F4: We discover that F3 is truly a problem and M3 doesn't adequately address > it. > > M4: Ask the shell to tell us which icons are in use and avoid deleting those > > during the pre-update purge. > > I agree we need to preserve active icons. I was focusing on deleting the icons > effectively and thought I could put this aside temporarily.
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...
On 2017/03/17 08:44:38, grt (UTC plus 1) wrote: > The old code is flawed because it assumes that: > 1) moving a directory is error-free > 2) deleting a directory and its contents is error-free I agree. We should get rid of the directory swap strategy. Also, I don't see any benefit brought by JumpListIconsOld. > Say we want to create N new icons that should replace all pre-existing icons. to > do so, we: > 1) Build a std::vector O of all files in JumpListIcons. > 2) If O.size() > some cap C, delete some random N old icons -- hopefully we never > need to do this, but consider this the "avoid explosion" safeguard. > 3) Write the new icons into JumpListIcons. > 4) Notify the shell. > 5) Post a low-priority background task to delete all files in O. I think the current implementation specifically doesn't work for users whose jumplist folders are bloated. Since an icon file is named using 4 hex characters, there can be at most 16*16*16*16=65536 icons in the folders. Some users have reported this limit has been reached in their machines. Also, past experience is that when there are thousands+ of icons files already in the jumplist folder, everything slows done, e.g., creation of a single icon. This can be due to that the machine needs to find a distinct name for the icon file newly created. Therefore, when we know the deletion of JumplistIcons folder fails, we should simply stop writing new icons into the folder and exit early. We can post a low-priority background task to try to delete later on though, as step 5 above. Also, when there are already thousands+ of icon files in the JumplistIcons folder, enumerating all of them already can take a fair long time. I think base::DeleteFileRecursive enumerates all the files followed by deleting each of them. However, base::DeleteFileRecursive allows early return when it tries to delete a file but fails, which is a good thing. However, my concern now is that the method thinks it is deleting all the files successfully while it is not in some situations. Therefore, it is not returning early but enumerating all the files per update. That is why I try limit the maximum number of icon files the method should try to delete per update. >Another approach would > be: > > 1) ask the shell for the list of "active" icons > 2) delete anything in the dir that isn't in the above list > 3) write the new icons > 4) notify the shell > 5) delete the files in the dir in the above list > > You could even split the first two tasks into their own thing that can be run in > the background via BrowserThread::PostAfterStartupTask so that Chrome quietly > sneaks off in the bg and harvests old icon files when there's no jumplist update > needed. > > > I think if base::Delete works perfectly, the current jumplist implementation > > should not have such a big performance issue. For this method, say we have M (can be a few thousand) icon files in the JumpListIcons folder, and typical 8 active icons used by shell, then step 2 requires O(8*M) operations in the loop. I think this may take even longer time for users whose jumplist folders are bloated now. ================================================================ I have proposed a approach in the new patch set. Specifically, the two directory swap approach is removed. The main steps are as below, where you can also find in the code: 1) Try to delete all the content (but up to 100 icon files) in the JumpListIcons folder. Raw directory is kept, so no need to re-create it later on. 2) If step 1 fails, post a low-priority background task trying to delete again, and exit. 3) If step 1 succeeds, write new icons into this folder and notify the shell 4) If JumpListIconsOld folder exists, post a low-priority background task trying to delete it. For users without jumplist issue: there should be no impacts to JumpListIcons folder as there are only typically less than 30 icons in it. The JumpListIconsOld folder will be deleted. For users with jumplist issue: a) If they have trouble in JumpListIcons, typically step 1 can fail. However, due to the 100 icon files' limit, it'll just take a short time for this step. The folder won't have new icons written in until all old ones get removed in the future. b) If they have trouble in JumpListIconsOld, then their jumplist should work well with shell. The content in JumpListIconsOld will be deleted gradually using the background task, or not. However, it won't freeze Chrome anymore. c) If they have trouble in both, we are focusing on JumpListIcons for now as it is much more important. Once it is fixed, the code will try to delete JumpListIconsOld . Major concern: 1) The icons that are used by the shell are deleted, and no new icons are generate to update them. Re: we don't need to keep old icons even no new ones are generated, because they are obsolete anyway when tabs are closed. For example, for "recently closed tabs", anytime a tab is closed, the old icon is obsolete. Again, I think the effort to retain these 8 effective icons is too much as we need to enumerate all the icons in the folder. Also, as I mentioned, full validation of any jumplist CL awaits its hitting stable branch. I feel like we can incrementally checking in the fix, as we can get some data from Canary users on a daily basis. With the data, we can definitely have a better understanding of where the problem is. I have checked in some fixes in the past months, and there are indeed some improvements according to UMA. I am confident removing the directory swap can help.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
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 ========== Set upper limit for the amount of jumplist icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Remove jumplist directory swap and set upper limit for icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
Description was changed from ========== Remove jumplist directory swap and set upper limit for icons deleted per update In one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Remove jumplist directory swap and set upper limit for icons deleted per update Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
Description was changed from ========== Remove jumplist directory swap and set upper limit for icons deleted per update Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A walk around of this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Remove jumplist directory swap and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. While holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There can certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
Description was changed from ========== Remove jumplist directory swap and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. While holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There can certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Remove JumplistIconsOld directory and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. While holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There can certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumplistIconsOld directory and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. While holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There can certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Remove JumplistIconsOld directory and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. In addition to that holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There are certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any existing file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove JumplistIconsOld directory and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. In addition to that holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There are certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any existing file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. UMA metric will be monitored. If things are going unexpectedly, this CL will be reverted. BUG=40407, 179576 ========== to ========== Remove JumplistIconsOld directory and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. In addition to that holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There are certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any existing file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. BUG=40407, 179576 ==========
this is a good start. the various delete helper functions need some unit tests. histograms.xml needs to be updated, too, no? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:245: int upper_limit = INT_MAX) { it's confusing for a caller to invoke DeleteFileRecursiveWithUpperLimit with three args, since "WithUpperLimit" is meaningless in that case. can you do something other than the default arg here? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:262: if (recursive && (!DeleteFileRecursiveWithUpperLimit(current, pattern, |file_deleted| should not be incremented if IsDirectory && !recursive since nothing is deleted, no? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:265: return false; nit: braces around this since the conditional spans multiple lines https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:267: return false; why not keep going trying to delete other files in this case (without bumping |file_deleted|)? should this not be a best-effort delete? i think it makes sense to return false if not all files were deleted, but is there a reason to bail out on the first failure? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:278: // sub-directory. are there ever sub-directories in JumplistIcons{,Old}? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:280: bool recursive, don't overly generalize this -- the only caller passes |recursive| = true, so make the function do exactly that -- a recursive delete. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:290: // Handle any path with wildcards. remove wildcard support here -- it's unused https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:310: // but save the row directory. The raw directory is deleted if row -> raw https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:370: SUCCEED = 0, SUCCEED and FAILED are very generic. do you want to capture: - deleted all files - deleted max files allowed - failed to delete all requested files - ? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:377: uint32_t folder_delete_status = FolderDeleteResult::SUCCEED; FolderDeleteResult isn't an "enum class", so no need to qualify. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:391: folder_delete_status |= FolderDeleteResult::FAILED; is folder_delete_status a bitfield or just a variable? if the former, please change the type to "enum FolderDeleteResult : uint32_t" and give FAILED an explicit value. if not, leave the enum type as-is, but still give FAILED an explicit value since it's used in a histogram. it's also good practice to add a comment to the enum saying something like: // Used for UMA. Do not delete entries, and keep in sync with histograms.xml. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:392: BrowserThread::PostAfterStartupTask( PostAfterStartupTask is only needed if you're posting a task during startup that you want to run sometime later. in this case, you can post the task using the new base/task_scheduler API. beware that you may get a request to update the jumplist while this task is of running. think about what sort of synchronization is needed. for example, should the next jumplist update wait until this delete task from a previous run has completed? otherwise could you have a race where thread 1 is deleting files created by thread 2? https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:417: BrowserThread::PostAfterStartupTask( comments above apply here as well. does it suffice to post this task once during the lifetime of a browser process? if the task runs as TaskPriority::BACKGROUND with TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, it can quietly do its job in the bg and never needs to be posted more than once. if you're concerned about it hogging a thread for a very, very long time, you could have it chunk its work by, for example, deleting 100 files then reposting itself so that other work can be done before the next 100 are deleted.
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...
isherman@ PTAL histograms.xml~ grt@ Thanks for all the suggestions! I have updated the patch set and replied to your comments inline. Jumplist code is not well covered by unit tests as in bug 498987. The fact that the jumplist performance bug doesn't seem to be caught by any unit tests and the temporary methods I added change quickly decreases the priority of adding unittests to them. So I think it is maybe better to add the test suite after the root cause of this bug is clear. As the discussion is going wild and future work is definitely needed, I am creating a google doc to put existing findings/discussion, and to allow future discussion in a more convenient manner. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:245: int upper_limit = INT_MAX) { On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > it's confusing for a caller to invoke DeleteFileRecursiveWithUpperLimit with > three args, since "WithUpperLimit" is meaningless in that case. can you do > something other than the default arg here? I renamed this method to DeleteFileRecursive, removed input parameter |upper_limit|, but added a boolean input parameter |has_upper_limit|. If it is set to true, the upper limit is set to |kMaxIconFilesDeletedPerUpdate|. Since these methods are only used here, I think not over-generalizing them is preferred as you suggested. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:262: if (recursive && (!DeleteFileRecursiveWithUpperLimit(current, pattern, On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > |file_deleted| should not be incremented if IsDirectory && !recursive since > nothing is deleted, no? I have removed the |recursive| input parameter, so now (IsDirectory && !recursive) is always false and the condition you mentioned is gone. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:265: return false; On 2017/03/20 09:15:37, grt (UTC plus 1) wrote: > nit: braces around this since the conditional spans multiple lines Done. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:267: return false; On 2017/03/20 09:15:37, grt (UTC plus 1) wrote: > why not keep going trying to delete other files in this case (without bumping > |file_deleted|)? should this not be a best-effort delete? i think it makes sense > to return false if not all files were deleted, but is there a reason to bail out > on the first failure? I think it just made sense if it returns false when deletion of any file fails. If it returned true after 100 files are deleted, there was no way to know if there was a failure in deleting files at all. Now since I have recorded all the delete status, not returning false right away should not lose any information. I have updated the logic that when there is an upper limit set for deletion, not return intermediately but keep trying deleting files until the upper limit is hit. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:278: // sub-directory. On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > are there ever sub-directories in JumplistIcons{,Old}? There should not be sub-dir in JumplistIcos{,Old} according to the code logic, and I have not seen users reported this. I added "... any of its sub-directory." above for the method alone as it does handle subdirectories, not for JumplistIcons folders. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:280: bool recursive, On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > don't overly generalize this -- the only caller passes |recursive| = true, so > make the function do exactly that -- a recursive delete. Sure, the input parameter |recursive| is gone now. I tend to keep |has_upper_limit| although currently all the calls set it to true. I will use it soon I think. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:290: // Handle any path with wildcards. On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > remove wildcard support here -- it's unused Done. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:310: // but save the row directory. The raw directory is deleted if On 2017/03/20 09:15:37, grt (UTC plus 1) wrote: > row -> raw Done. Sorry for the typo. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:370: SUCCEED = 0, On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > SUCCEED and FAILED are very generic. do you want to capture: > - deleted all files > - deleted max files allowed > - failed to delete all requested files > - ? This enum is updated. It has all the possible failure modes now. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:377: uint32_t folder_delete_status = FolderDeleteResult::SUCCEED; On 2017/03/20 09:15:37, grt (UTC plus 1) wrote: > FolderDeleteResult isn't an "enum class", so no need to qualify. Done. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:391: folder_delete_status |= FolderDeleteResult::FAILED; On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > is folder_delete_status a bitfield or just a variable? if the former, please > change the type to "enum FolderDeleteResult : uint32_t" and give FAILED an > explicit value. if not, leave the enum type as-is, but still give FAILED an > explicit value since it's used in a histogram. it's also good practice to add a > comment to the enum saying something like: > // Used for UMA. Do not delete entries, and keep in sync with histograms.xml. In this CL, it should be changed to a variable. So I will leave the enum type as it is. I tend to list all failure modes in the enum rather than a single "FAILED". Also, comment added as what you suggested. For this |folder_delete_status| variable, it should be int instead of uint now. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:392: BrowserThread::PostAfterStartupTask( On 2017/03/20 09:15:36, grt (UTC plus 1) wrote: > PostAfterStartupTask is only needed if you're posting a task during startup that > you want to run sometime later. in this case, you can post the task using the > new base/task_scheduler API. > > beware that you may get a request to update the jumplist while this task is of > running. think about what sort of synchronization is needed. for example, should > the next jumplist update wait until this delete task from a previous run has > completed? otherwise could you have a race where thread 1 is deleting files > created by thread 2? Thanks for the reminder. This is something that needs to be well considered. Making the next jumplist update wait until this background task from previous run finishes, or invoking another icon creation task after the background delete task finishes, seems not optimal either way. Maybe try to use another "JumpListIconsNew" directory to hold the icons, while deleting the two existing folders using two background tasks. I will create a google doc and write down all these methods for discussion. But now, I would prefer to not posting this background delete task for JumpListIcons directory. This CL alone will allow us to evaluate the effects of removing the JumpListIconsOld directory related operations. Also, I have added UMA to record the detailed delete status for both JumpListIcons{,Old} folders, which should give us more information to decide which way to go later on. https://codereview.chromium.org/2752063002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:417: BrowserThread::PostAfterStartupTask( On 2017/03/20 09:15:37, grt (UTC plus 1) wrote: > comments above apply here as well. does it suffice to post this task once during > the lifetime of a browser process? if the task runs as TaskPriority::BACKGROUND > with TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, it can quietly do its job in > the bg and never needs to be posted more than once. if you're concerned about it > hogging a thread for a very, very long time, you could have it chunk its work > by, for example, deleting 100 files then reposting itself so that other work can > be done before the next 100 are deleted. Thanks for pointing to these APIs and traits that shall be used. I did have concern that the job would hog a thread for a very long time. As suggested, I have set the upper limit (i.e., 100) for the amount of files allowed to be deleted per update. Since the deletion here is for the JumpListIconsOld directory, there will be no deletion/creation race condition happening here. Self-invoking deletion of 100 files periodically sounds a good idea. However, since it is suspicious that ::DeleteFile can mark file as deleted while it is not, I am reluctant to self-invoke the deletion task. Moreover, as this JumpListIconsOld folder is no longer used, we can allow its size to go down gradually instead of all of a sudden. This is a trade-off indeed, and I prefer just trying to delete 100 files per update.
chengx@chromium.org changed reviewers: + isherman@chromium.org
isherman@ for histograms.xml please!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove JumplistIconsOld directory and set upper limit for icons deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. In addition to that holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There are certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any existing file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. In addition to that holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There are certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any existing file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumplistIconOld followed by moving JumplistIcons to JumplistIconsOld. In addition to that holding icons in JumplistIconsOld has little benefit, this method is problematic as we assume file deletion and move are perfect which they are not. There are certain unexpected situations when these steps fail, and this has been proved by UMA metric. This CL removes any existing file operation related to JumplistIconsOld in jumplist update but adds a low-priority background task to delete JumplistIconsOld until it is completely gone. Also in one jumplist icons update, JumplistIconOld and JumplistIcon folders are tried to be deleted, which is the only step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the folder. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumplistIcon(Old) folder, so setting the upper limit to be 100 won't affect the jumplist update functionality for normal users at all. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
i'm concerned about this approach. i see that you're copying code from base so that you can customize it to gather the metrics you need and limit the time spent deleting files. since you're not adding unittest coverage for these utility functions, the burden is on you and me to be absolutely certain that they are doing what you intend and that they don't regress. this is not so good. i think you should drive for the absolute simplest code possible (e.g., don't have code for a recursive delete if there are no subdirectories) and have unittest coverage for these new utility functions. you could introduce a simple jumplist_file_util_win{.cc,.h,_unittest.cc} set of files for these. i'm not suggesting that you add missing tests for the existing jumplist logic, but that you add it for the new functionality here. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:268: bool DeleteFileRecursive(const base::FilePath& path, if this is only called to do recursive deletes on directories, then i think DeleteDirectoryRecursive is more clear. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:271: int* delete_status) { int* -> FolderDeleteResult* https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:275: int file_deleted = 0, file_fail_to_deted = 0; nit: one definition per line, please https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:275: int file_deleted = 0, file_fail_to_deted = 0; file_fail_to_deted -> failure_count https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:275: int file_deleted = 0, file_fail_to_deted = 0; file_deleted -> attempt_count https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:299: file_fail_to_deted++; since you don't decrement file_deleted here, it's really a count of the # of attempts rather than the number of successes, so the doc comment is incorrect. also, what do you think about counting the # of items overall rather than just the # of files? also, what does the status mean when you have nested directories? if the max # of files are deleted three levels deep, should the iteration in the dirs above continue or not? https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:304: if (has_upper_limit && file_deleted > kMaxIconFilesDeletedPerUpdate) { ? file_deleted >= kMaxIconFilesDeletedPerUpdate ? https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:333: int* delete_status) { int* -> FolderDeleteResult* https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:355: // If |path| is a file, simply delete it. this is never expected to be hit, but i agree that it makes sense for this to be here. please add a comment mentioning that it's unexpected. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:380: int* delete_status) { int* -> FolderDeleteResult* https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:422: UMA_HISTOGRAM_ENUMERATION(histogram_name, *delete_status, END); the UMA macros cannot be used with a generated histogram name like this; see https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&.... you'll need to use base::LinearHistogram::FactoryGet and Add the sample manually. alternatively, use "WinJumplist.DeleteStatusJumpListIconsOld" directly here. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:448: int folder_delete_status = SUCCEED; int -> FolderDeleteResult https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:481: int jumplisticons_old_delete_status = SUCCEED; move the definition of this |delete_status| variable into DeleteDirectoryAndLogResults. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:483: base::PostTaskWithTraits( since a new task is posted for each jumplist icon update, you may end up with multiple tasks running in parallel. you can avoid this by using base::CreateSequencedTaskRunnerWithTraits once to create a SequencedTaskRunner, then posting this task to that runner each time. there will still be N cleanup jobs for N jumplist icon updates, but they will always run in sequence. perhaps JumpList can create this SequencedTaskRunner in its ctor and pass a reference to it to this function.
https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:416: // UMA_HISTOGRAM_ENUMERATION. This comment literally just repeats the code, which is not useful. For a brief function comment like this, I'd recommend something like "Deletes the directory at |path| and records the result to UMA." (Admittedly that's still pretty much literally repeating the code, but at least it rephrases it into something slightly more human-natural-language-friendly. In general, I'm not a huge fan of doc strings for simple internal methods like these, but it does seem to be the prevailing Chromium style to include them.) https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:420: std::string histogram_name) { Why are you passing this string by copy? https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:422: UMA_HISTOGRAM_ENUMERATION(histogram_name, *delete_status, END); On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > the UMA macros cannot be used with a generated histogram name like this; see > https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&.... > you'll need to use base::LinearHistogram::FactoryGet and Add the sample > manually. alternatively, use "WinJumplist.DeleteStatusJumpListIconsOld" directly > here. You can also use UmaHistogramEnumeration() from histogram_functions.h. But, yes, if this method is only used to record a single histogram, then there's no reason to pass in the name as a parameter. https://codereview.chromium.org/2752063002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752063002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79864: + Please refer to JumplisticonsDeleteCategory for more information. There is no need for the second sentence here -- it doesn't mean anything in the context of the dashboard, which is where this will be viewed. Ditto below. https://codereview.chromium.org/2752063002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:97628: + <int value="4" label="Fail to delete maximum files allowed"/> I don't understand the difference between these two buckets, nor what either of them means individually. Please try to clarify these labels. (Note: I'm not necessarily asking you to write longer text; if you can find a brief way to clearly label these, that would be best.)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #4 (id:120001) has been deleted
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and record all the possible failure reasons. These methods are put new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and record all the possible failure reasons. These methods are put new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
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 ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Patchset #4 (id:140001) has been deleted
I have addressed all the comments in the new patch set. PTAL~ grt@: I have moved all the file operation related methods I added to jumplist_file_util.*. I also created unit tests for them. However, I didn't test the case when there are more than 100 files in a directory, because it involves quite a lot of IO to create and delete those files. The delete method has also been simplified to tightly fit the JumpListIcons case. For example, it is not doing recursive deletion now as there shouldn't be subdirectories. Please see the new patch set for detailed information. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:268: bool DeleteFileRecursive(const base::FilePath& path, On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > if this is only called to do recursive deletes on directories, then i think > DeleteDirectoryRecursive is more clear. Done. Renamed to "DeleteDirectoryRecursive". https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:271: int* delete_status) { On 2017/03/21 08:40:27, grt (UTC plus 1) wrote: > int* -> FolderDeleteResult* Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:275: int file_deleted = 0, file_fail_to_deted = 0; On 2017/03/21 08:40:27, grt (UTC plus 1) wrote: > nit: one definition per line, please Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:275: int file_deleted = 0, file_fail_to_deted = 0; On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > file_fail_to_deted -> failure_count Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:275: int file_deleted = 0, file_fail_to_deted = 0; On 2017/03/21 08:40:27, grt (UTC plus 1) wrote: > file_deleted -> attempt_count I will rename it to |success_count|. I think if we set an upper limit for |success_count|, we will know that 100 files are deleted if the method returns true. However, if we set upper limit for |attempt_count|, we won't know how many files are deleted even when the method returns true, since we don't know what the |failure_count| is. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:299: file_fail_to_deted++; On 2017/03/21 08:40:27, grt (UTC plus 1) wrote: > since you don't decrement file_deleted here, it's really a count of the # of > attempts rather than the number of successes, so the doc comment is incorrect. > also, what do you think about counting the # of items overall rather than just > the # of files? > > also, what does the status mean when you have nested directories? if the max # > of files are deleted three levels deep, should the iteration in the dirs above > continue or not? You are right. It's actually a count of the # of attempts. I have fixed the code to record the count of the # of successful deletes. Also, since there shouldn't be sub-directories in JumpListIcons{,Old} directories, I think in this CL we'd better just not do recursive deletion. On the other hand, if there are sub-directories, then we have bigger troubles as the maximum # of icon files is not 65536 anymore. Instead, it can be much bigger than that. So I would propose to just delete maximum files (not dirs) allowed. If a sub-dir is detected, then we use |delete_status| to record that sub-dir does exist. We don't try to delete the stb-dir, and we don't exit the method right away. Instead, we just increment |failure_count|. After gathering the information from UMA using this CL, we will know if sub-dir exists and come up with possible solutions for that. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:304: if (has_upper_limit && file_deleted > kMaxIconFilesDeletedPerUpdate) { On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > ? file_deleted >= kMaxIconFilesDeletedPerUpdate ? Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:333: int* delete_status) { On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > int* -> FolderDeleteResult* Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:355: // If |path| is a file, simply delete it. On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > this is never expected to be hit, but i agree that it makes sense for this to be > here. please add a comment mentioning that it's unexpected. Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:380: int* delete_status) { On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > int* -> FolderDeleteResult* Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:416: // UMA_HISTOGRAM_ENUMERATION. On 2017/03/21 20:56:44, Ilya Sherman wrote: > This comment literally just repeats the code, which is not useful. For a brief > function comment like this, I'd recommend something like "Deletes the directory > at |path| and records the result to UMA." (Admittedly that's still pretty much > literally repeating the code, but at least it rephrases it into something > slightly more human-natural-language-friendly. In general, I'm not a huge fan > of doc strings for simple internal methods like these, but it does seem to be > the prevailing Chromium style to include them.) Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:420: std::string histogram_name) { On 2017/03/21 20:56:45, Ilya Sherman wrote: > Why are you passing this string by copy? This parameter is removed in the new patch set. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:422: UMA_HISTOGRAM_ENUMERATION(histogram_name, *delete_status, END); On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > the UMA macros cannot be used with a generated histogram name like this; see > https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&.... > you'll need to use base::LinearHistogram::FactoryGet and Add the sample > manually. alternatively, use "WinJumplist.DeleteStatusJumpListIconsOld" directly > here. I have changed to use "WinJumplist.DeleteStatusJumpListIconsOld" directly. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:422: UMA_HISTOGRAM_ENUMERATION(histogram_name, *delete_status, END); On 2017/03/21 20:56:44, Ilya Sherman wrote: > On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > > the UMA macros cannot be used with a generated histogram name like this; see > > > https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&.... > > you'll need to use base::LinearHistogram::FactoryGet and Add the sample > > manually. alternatively, use "WinJumplist.DeleteStatusJumpListIconsOld" > directly > > here. > > You can also use UmaHistogramEnumeration() from histogram_functions.h. But, > yes, if this method is only used to record a single histogram, then there's no > reason to pass in the name as a parameter. The name parameter is removed. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:448: int folder_delete_status = SUCCEED; On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > int -> FolderDeleteResult Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:481: int jumplisticons_old_delete_status = SUCCEED; On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > move the definition of this |delete_status| variable into > DeleteDirectoryAndLogResults. Done. https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:483: base::PostTaskWithTraits( On 2017/03/21 08:40:26, grt (UTC plus 1) wrote: > since a new task is posted for each jumplist icon update, you may end up with > multiple tasks running in parallel. you can avoid this by using > base::CreateSequencedTaskRunnerWithTraits once to create a SequencedTaskRunner, > then posting this task to that runner each time. there will still be N cleanup > jobs for N jumplist icon updates, but they will always run in sequence. perhaps > JumpList can create this SequencedTaskRunner in its ctor and pass a reference to > it to this function. Thanks for the suggestion. I have added a member nameed |sequenced_task_runner_| to JumpList class, initialized in JumpList class's constructor, and passed its const ref to this method. https://codereview.chromium.org/2752063002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752063002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79864: + Please refer to JumplisticonsDeleteCategory for more information. On 2017/03/21 20:56:45, Ilya Sherman wrote: > There is no need for the second sentence here -- it doesn't mean anything in the > context of the dashboard, which is where this will be viewed. Ditto below. Done. https://codereview.chromium.org/2752063002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:97628: + <int value="4" label="Fail to delete maximum files allowed"/> On 2017/03/21 20:56:45, Ilya Sherman wrote: > I don't understand the difference between these two buckets, nor what either of > them means individually. Please try to clarify these labels. (Note: I'm not > necessarily asking you to write longer text; if you can find a brief way to > clearly label these, that would be best.) Alright, I have renamed those two labels. Basically, I am trying to delete 100 icon files in a big icon pool using maximum 200 attempts. Label 3 (now 4) means that we can delete 100 files with more than 100 attempts but less than 200 attempts. Therefore, there are some failures and the failure count is less than 100. Label 4 (now 5) means that we have attempted 200 times, but more than half of them fail. Therefore, the goal of deleting 100 files is not achieved.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
i don't think that creating 101 files in a unittest is too much IO. if it turns out that it is, you could make the limit an in-param so that the test can work with 5 files while the impl works with 100. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:50: // If it successfully deletes |kMaxIconFilesDeletedPerUpdate| files without i think this is more clear with both limit checks in one block: if (has_upper_limit) { if (success_count >= kMaxIconFilesDeletedPerUpdate) { // The desired max number of files have been deleted. if (!failure_count) return true; *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY; return false; } if (failure_count >= kMaxIconFilesDeletedPerUpdate) { // The desired max number of failures have been hit. *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY; return false; } } https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:55: *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY; this reads to me like "we failed because we perfectly deleted the max # of files. wdyt of FAIL_DELETE_MAX_FILES_WITH_ERRORS, which sounds more to me like "failed because we delete the max # of files but had some errors mixed in". https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:62: *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY; FAIL_MAX_DELETE_FAILURES -> "we failed because we hit the max # of tolerable failures"? https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:69: bool DeleteDirectoryContent(const base::FilePath& path, return FolderDeleteResult rather than bool (which seems to be ignored). callers can compare it with SUCCEED if they care. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:72: base::ThreadRestrictions::AssertIOAllowed(); this function seems to assume that |delete_status| is SUCCEED on entry. please either DCHECK that this is true, or explicitly set it. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:111: void DeleteDirectory(const base::FilePath& path, return FolderDeleteResult rather than void w/ out param https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:157: static_cast<int>(delete_status), END); since FolderDeleteResult is an old-school enum and not an enum class, is this static_cast needed? https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:51: bool has_upper_limit, it doesn't look like any callers pass false for has_upper_limit on any of these functions -- do you need it? https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util_unittest.cc (right): https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:15: // Random text to write into a file. nit: blank line before this and before the close of the namespace on line 27 https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:16: const wchar_t file_content[] = L"I'm random context."; constexpr wchar_t kFileContent[] = ... https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:21: std::wofstream file; ASSERT_EQ(contents.length(), base::WriteFile(filename, contents.data(), contents.length())); https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:30: public: protected https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:36: base::ScopedTempDir temp_dir; put these two lines in a SetUp() override in the test harness and introduce a protected method for tests to get temp_dir.GetPath() https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:42: CreateTextFile(file_name, file_content); ASSERT_NO_FATAL_FAILURE(CreateTextFile(...)); here and elsewhere https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:43: ASSERT_TRUE(PathExists(file_name)); move this assert into CreateTextFile
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...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #5 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All the feedback comments are addressed in the new patch set. PTAL, thanks! https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:50: // If it successfully deletes |kMaxIconFilesDeletedPerUpdate| files without On 2017/03/22 12:07:21, grt (UTC plus 1) wrote: > i think this is more clear with both limit checks in one block: > if (has_upper_limit) { > if (success_count >= kMaxIconFilesDeletedPerUpdate) { > // The desired max number of files have been deleted. > if (!failure_count) > return true; > *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY; > return false; > } > if (failure_count >= kMaxIconFilesDeletedPerUpdate) { > // The desired max number of failures have been hit. > *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY; > return false; > } > } Agreed and updated accordingly. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:55: *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY; On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > this reads to me like "we failed because we perfectly deleted the max # of > files. wdyt of FAIL_DELETE_MAX_FILES_WITH_ERRORS, which sounds more to me like > "failed because we delete the max # of files but had some errors mixed in". Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:62: *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY; On 2017/03/22 12:07:21, grt (UTC plus 1) wrote: > FAIL_MAX_DELETE_FAILURES -> "we failed because we hit the max # of tolerable > failures"? Sounds good. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:69: bool DeleteDirectoryContent(const base::FilePath& path, On 2017/03/22 12:07:21, grt (UTC plus 1) wrote: > return FolderDeleteResult rather than bool (which seems to be ignored). callers > can compare it with SUCCEED if they care. Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:72: base::ThreadRestrictions::AssertIOAllowed(); On 2017/03/22 12:07:21, grt (UTC plus 1) wrote: > this function seems to assume that |delete_status| is SUCCEED on entry. please > either DCHECK that this is true, or explicitly set it. Changed to explicitly set it. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:111: void DeleteDirectory(const base::FilePath& path, On 2017/03/22 12:07:21, grt (UTC plus 1) wrote: > return FolderDeleteResult rather than void w/ out param Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:157: static_cast<int>(delete_status), END); On 2017/03/22 12:07:21, grt (UTC plus 1) wrote: > since FolderDeleteResult is an old-school enum and not an enum class, is this > static_cast needed? You are right. It is not needed. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:51: bool has_upper_limit, On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > it doesn't look like any callers pass false for has_upper_limit on any of these > functions -- do you need it? It is not needed and has been removed. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util_unittest.cc (right): https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:15: // Random text to write into a file. On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > nit: blank line before this and before the close of the namespace on line 27 Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:16: const wchar_t file_content[] = L"I'm random context."; On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > constexpr wchar_t kFileContent[] = ... Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:21: std::wofstream file; On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > ASSERT_EQ(contents.length(), base::WriteFile(filename, contents.data(), > contents.length())); Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:30: public: On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > protected Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:36: base::ScopedTempDir temp_dir; On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > put these two lines in a SetUp() override in the test harness and introduce a > protected method for tests to get temp_dir.GetPath() Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:42: CreateTextFile(file_name, file_content); On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > ASSERT_NO_FATAL_FAILURE(CreateTextFile(...)); here and elsewhere Done. https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:43: ASSERT_TRUE(PathExists(file_name)); On 2017/03/22 12:07:22, grt (UTC plus 1) wrote: > move this assert into CreateTextFile Done.
Metrics LGTM https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:119: delete_status, END); FWIW, you might want to add histogram test coverage along with other test coverage, using base::HistogramTester. (Note: There's probably not much point in testing just this method in isolation, since it's pretty much just internal API; but if you can test the code that wraps this code, that might be appropriate.) https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:18: // File name's length exceeds MAX_PATH. This won't happen. Why are there enum entries that won't happen? Do you mean "This shouldn't happen"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefit, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps fail, which has been proved by UMA metric. This CL removes any existing file operations related to JumpListIconsOld in jumplist update. This CL also adds a background task to delete JumpListIconsOld. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given the UMA metric showing there are still quite a number of updates taking a long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files (can be a lot when the folder is corrupted) to be read in every jumplist update. A strategy for this potential case is to set an upper limit for the amount of jumplist icons deleted per update. This can avoid the jumplist IO operations from taking a long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
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 ========== Remove JumpListIconsOld directory and set upper limit for files deleted per update Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limits for files deleted and attempt failures Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Patchset #6 (id:220001) has been deleted
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...
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...
Patchset #6 (id:240001) has been deleted
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...
Patchset #6 (id:260001) has been deleted
Description was changed from ========== Remove JumpListIconsOld directory and set upper limits for files deleted and attempt failures Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limits for file delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limits for file delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits. i look forward to seeing how this performs. i think it should work out better than the directory dance. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:20: #include "base/task_scheduler/post_task.h" move to .cc file https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:33: forward declare SequencedTaskRunner in namespace base here and #include the sequenced_task_runner.h in the .cc file https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:49: if (failure_count) return failure_count ? FAIL_DELETE_MAX_FILES_WITH_ERRORS : delete_status; https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:70: if (path.value().length() >= MAX_PATH) { nit: omit braces https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:108: delete_status = FAIL_REMOVE_RAW_DIRECTORY; nit: braces around this due to multi-line conditional https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:11: const int kMaxFilesDeleted = 100; nit: kMaxFilesToDelete or kFileDeleteLimit? https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:48: const int max_file_deleted); nit: no const on primitive in params in decls like this. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util_unittest.cc (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:35: public: make this protected: and remove protected: on line 38
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 ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is highly skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ==========
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:48: const int max_file_deleted); On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > nit: no const on primitive in params in decls like this. To clarify/expand - 'const int' is meaningless in a declaration because it is irrelevant to the caller. It can be reasonable to use 'const int' for function parameters in the function *definition*, where it has meaning for the implementation. Because the const is not applicable to the caller the 'mismatch' is legal. In other cases the const is meaningful - understanding the difference is important.
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...
All feedback comments are addressed in the new patch set. Now the CL is ready for checking in. Thanks for all your help! https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:119: delete_status, END); On 2017/03/22 22:24:00, Ilya Sherman wrote: > FWIW, you might want to add histogram test coverage along with other test > coverage, using base::HistogramTester. (Note: There's probably not much point > in testing just this method in isolation, since it's pretty much just internal > API; but if you can test the code that wraps this code, that might be > appropriate.) I would probably not add histogram test for this internal API now. Thanks for your suggestion though. https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:18: // File name's length exceeds MAX_PATH. This won't happen. On 2017/03/22 22:24:00, Ilya Sherman wrote: > Why are there enum entries that won't happen? Do you mean "This shouldn't > happen"? Yea, I mean "shouldn't happen". I have updated it to avoid confusion. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:20: #include "base/task_scheduler/post_task.h" On 2017/03/23 11:24:57, grt (UTC plus 1) wrote: > move to .cc file Done. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:33: On 2017/03/23 11:24:57, grt (UTC plus 1) wrote: > forward declare SequencedTaskRunner in namespace base here and #include the > sequenced_task_runner.h in the .cc file Done. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:49: if (failure_count) On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > return failure_count ? FAIL_DELETE_MAX_FILES_WITH_ERRORS : delete_status; Done. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:70: if (path.value().length() >= MAX_PATH) { On 2017/03/23 11:24:57, grt (UTC plus 1) wrote: > nit: omit braces Done. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.cc:108: delete_status = FAIL_REMOVE_RAW_DIRECTORY; On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > nit: braces around this due to multi-line conditional Done. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:11: const int kMaxFilesDeleted = 100; On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > nit: kMaxFilesToDelete or kFileDeleteLimit? Changed to kFileDeleteLimit. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:48: const int max_file_deleted); On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > nit: no const on primitive in params in decls like this. Done. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util.h:48: const int max_file_deleted); On 2017/03/23 18:53:26, brucedawson wrote: > On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > > nit: no const on primitive in params in decls like this. > > To clarify/expand - 'const int' is meaningless in a declaration because it is > irrelevant to the caller. It can be reasonable to use 'const int' for function > parameters in the function *definition*, where it has meaning for the > implementation. > > Because the const is not applicable to the caller the 'mismatch' is legal. > > In other cases the const is meaningful - understanding the difference is > important. Acknowledged. https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_file_util_unittest.cc (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_file_util_unittest.cc:35: public: On 2017/03/23 11:24:58, grt (UTC plus 1) wrote: > make this protected: and remove protected: on line 38 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2752063002/#ps290009 (title: "Log JumpListIcons folder empty status to UMA.")
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
Failed to apply patch for chrome/browser/win/jumplist.cc: While running git apply --index -p1; error: patch failed: chrome/browser/win/jumplist.cc:27 error: chrome/browser/win/jumplist.cc: patch does not apply Patch: chrome/browser/win/jumplist.cc Index: chrome/browser/win/jumplist.cc diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc index 680db16a6a490256bf10dd17a699b6806db5152a..43d7d833ddfb5ce87311295dd17da93282cedab1 100644 --- a/chrome/browser/win/jumplist.cc +++ b/chrome/browser/win/jumplist.cc @@ -5,18 +5,18 @@ #include "chrome/browser/win/jumplist.h" #include <Shlwapi.h> -#include <windows.h> #include "base/bind.h" #include "base/bind_helpers.h" -#include "base/callback_helpers.h" #include "base/command_line.h" #include "base/files/file_util.h" #include "base/macros.h" #include "base/metrics/histogram_macros.h" #include "base/path_service.h" +#include "base/sequenced_task_runner.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" +#include "base/task_scheduler/post_task.h" #include "base/threading/thread.h" #include "base/threading/thread_restrictions.h" #include "base/trace_event/trace_event.h" @@ -27,6 +27,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sessions/tab_restore_service_factory.h" #include "chrome/browser/shell_integration_win.h" +#include "chrome/browser/win/jumplist_file_util.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" @@ -229,28 +230,13 @@ bool UpdateJumpList(const wchar_t* app_id, return true; } -// Renames the directory |from_dir| to |to_path|. This method fails if any -// process has a handle open in |from_dir| or if |to_path| exists. Base::Move() -// tries to rename a file and if this fails, it tries copy-n-delete; This -// RenameDirectory method only does the rename part. -bool RenameDirectory(const base::FilePath& from_path, - const base::FilePath& to_path) { - base::ThreadRestrictions::AssertIOAllowed(); - if (from_path.ReferencesParent() || to_path.ReferencesParent()) - return false; - if (from_path.value().length() >= MAX_PATH || - to_path.value().length() >= MAX_PATH) { - return false; - } - return MoveFileEx(from_path.value().c_str(), to_path.value().c_str(), 0) != 0; -} - // Updates the jumplist, once all the data has been fetched. void RunUpdateOnFileThread( IncognitoModePrefs::Availability incognito_availability, const std::wstring& app_id, const base::FilePath& icon_dir, - base::RefCountedData<JumpListData>* ref_counted_data) { + base::RefCountedData<JumpListData>* ref_counted_data, + const scoped_refptr<base::SequencedTaskRunner>& sequenced_task_runner) { JumpListData* data = &ref_counted_data->data; ShellLinkItemList local_most_visited_pages; ShellLinkItemList local_recently_closed_pages; @@ -267,73 +253,25 @@ void RunUpdateOnFileThread( local_recently_closed_pages = data->recently_closed_pages_; } - // Delete the directory which contains old icon files, rename the current - // icon directory, and create a new directory which contains new JumpList - // icon files. - base::FilePath icon_dir_old = icon_dir.DirName().Append( - icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old")); - - enum FolderOperationResult { - SUCCESS = 0, - DELETE_DEST_FAILED = 1 << 0, - RENAME_FAILED = 1 << 1, - DELETE_SRC_CONTENT_FAILED = 1 << 2, - DELETE_SRC_DIR_FAILED = 1 << 3, - CREATE_SRC_FAILED = 1 << 4, - // This value is beyond the sum of all bit fields above and - // should remain last (shifted by one more than the last value) - END = 1 << 5 - }; - - // This variable records the status of three folder operations. - uint32_t folder_operation_status = FolderOperationResult::SUCCESS; - - base::ScopedClosureRunner log_operation_status_when_done(base::Bind( - [](uint32_t* folder_operation_status_ptr) { - UMA_HISTOGRAM_ENUMERATION( - "WinJumplist.DetailedFolderResultsDeleteUpdated", - *folder_operation_status_ptr, FolderOperationResult::END); - }, - base::Unretained(&folder_operation_status))); - - // If deletion of |icon_dir_old| fails, do not rename |icon_dir| to - // |icon_dir_old|, instead, delete |icon_dir| directly to avoid bloating - // |icon_dir_old| by moving more things to it. - if (!base::DeleteFile(icon_dir_old, true)) { - folder_operation_status |= FolderOperationResult::DELETE_DEST_FAILED; - // If deletion of any item in |icon_dir| fails, exit early. If deletion of - // all the items succeeds while only deletion of the dir fails, it is okay - // to proceed. This skips creating the same directory and updating jumplist - // icons to avoid bloating the JumplistIcons folder. - if (!base::DeleteFile(icon_dir, true)) { - if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) { - folder_operation_status |= - FolderOperationResult::DELETE_SRC_CONTENT_FAILED; - return; - } - folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED; - } - } else if (!RenameDirectory(icon_dir, icon_dir_old)) { - // If RenameDirectory() fails, delete |icon_dir| to avoid file accumulation - // in this directory, which can eventually lead the folder to be huge. - folder_operation_status |= FolderOperationResult::RENAME_FAILED; - // If deletion of any item in |icon_dir| fails, exit early. If deletion of - // all the items succeeds while only deletion of the dir fails, it is okay - // to proceed. This skips creating the same directory and updating jumplist - // icons to avoid bloating the JumplistIcons folder. - if (!base::DeleteFile(icon_dir, true)) { - if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) { - folder_operation_status |= - FolderOperationResult::DELETE_SRC_CONTENT_FAILED; - return; - } - folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED; - } - } - - // If CreateDirectory() fails, exit early. - if (!base::CreateDirectory(icon_dir)) { - folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED; + // Delete the contents in JumpListIcons directory and log the delete status + // to UMA. + FolderDeleteResult delete_status = + DeleteDirectoryContent(icon_dir, kFileDeleteLimit); + + UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIcons", + delete_status, END); + + // If JumpListIcons directory is not empty, skip jumplist update and return + // early. If the directory doesn't exist which shouldn't though, try to create + // a new JumpListIcons directory. If the creation fails, return early. + if (base::DirectoryExists(icon_dir)) { + DirectoryEmptyStatus empty_status = + ::PathIsDirectoryEmpty(icon_dir.value().c_str()) ? EMPTY : NON_EMPTY; + UMA_HISTOGRAM_ENUMERATION("WinJumplist.EmptyStatusJumpListIcons", + empty_status, EMPTY_STATUS_END); + if (empty_status == NON_EMPTY) + return; + } else if (!base::CreateDirectory(icon_dir)) { return; } @@ -349,6 +287,17 @@ void RunUpdateOnFileThread( // with it. UpdateJumpList(app_id.c_str(), local_most_visited_pages, local_recently_closed_pages, incognito_availability); + + // Post a background task to delete JumpListIconsOld folder if it exists and + // log the delete results to UMA. + base::FilePath icon_dir_old = icon_dir.DirName().Append( + icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old")); + + if (base::DirectoryExists(icon_dir_old)) { + sequenced_task_runner->PostTask( + FROM_HERE, base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old, + kFileDeleteLimit)); + } } } // namespace @@ -363,6 +312,12 @@ JumpList::JumpList(Profile* profile) profile_(profile), jumplist_data_(new base::RefCountedData<JumpListData>), task_id_(base::CancelableTaskTracker::kBadTaskId), + sequenced_task_runner_(base::CreateSequencedTaskRunnerWithTraits( + base::TaskTraits() + .WithPriority(base::TaskPriority::BACKGROUND) + .WithShutdownBehavior( + base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) + .MayBlock())), weak_ptr_factory_(this) { DCHECK(Enabled()); // To update JumpList when a tab is added or removed, we add this object to @@ -661,11 +616,9 @@ void JumpList::DeferredRunUpdate() { BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, - base::Bind(&RunUpdateOnFileThread, - incognito_availability, - app_id_, - icon_dir_, - base::RetainedRef(jumplist_data_))); + base::Bind(&RunUpdateOnFileThread, incognito_availability, app_id_, + icon_dir_, base::RetainedRef(jumplist_data_), + sequenced_task_runner_)); } void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2752063002/#ps330001 (title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src into jumplistdeletesetupperā¦")
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": 330001, "attempt_start_ts": 1490314483064370, "parent_rev": "07b41129c2000598bba17282207731e8946a4031", "commit_rev": "2eefdd30dac56219587f39e35eac276444470401"}
Message was sent while issue was closed.
Description was changed from ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 ========== to ========== Remove JumpListIconsOld directory and set upper limit for delete attempts Current implementation for jumplist involves deleting JumpListIconOld followed by moving JumpListIcons to JumpListIconsOld. In addition to that holding icons in JumpListIconsOld has little benefits, this method is problematic as we assume file's deletion and move are perfect while they are not. There are certain unexpected situations where these steps can fail, which has been proved by UMA metrics. This CL removes any existing file operations related to JumpListIconsOld. A background task is added to delete JumpListIconsOld for all users. Also in one jumplist icons update, JumpListIconsOld and JumpListIcons folders are tried to be deleted, which is a step with heavy IO involved. Given that the UMA metrics are showing there are still quite a number of updates taking a very long time, it is skeptical that base::Delete has issues in some situations, e.g., when antivirus software is scanning the icon files. It can be the case that base::Delete marks the files deleted yet they are still there, which causes the files to be read in every jumplist update. A method to handle this case is to set an upper limit for the amount of jumplist icons deleted as well as the amount of attempt failures per update. This can avoid the jumplist file operations from taking a very long time. Typically, there are less than 30 jumplist icons in JumpListIcons(Old) folder, so setting the upper limit to be 100 is a reasonable choice. This ensures a number of icons can be deleted per update for users whose jumpilst folders are bloated without holding the IO thread for an excessive long time. Customized delete methods for JumpListIcons(Old) directories are created, which allows limiting the time spent on file deletion and recording all the possible failure reasons. These methods are put in new files named jumplist_file_util.{h,cc}. Unit tests for these methods are also created. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2752063002 Cr-Commit-Position: refs/heads/master@{#459297} Committed: https://chromium.googlesource.com/chromium/src/+/2eefdd30dac56219587f39e35eac... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:330001) as https://chromium.googlesource.com/chromium/src/+/2eefdd30dac56219587f39e35eac... |