Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(220)

Issue 2752063002: Remove JumpListIconsOld directory and set upper limit for delete attempts (Closed)

Created:
3 years, 9 months ago by chengx
Modified:
3 years, 9 months ago
CC:
chromium-reviews, brucedawson
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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ā€¦ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+397 lines, -91 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/win/jumplist.h View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/win/jumplist.cc View 1 2 3 4 5 6 7 8 7 chunks +44 lines, -91 lines 0 comments Download
A chrome/browser/win/jumplist_file_util.h View 1 2 3 4 5 6 7 1 chunk +80 lines, -0 lines 0 comments Download
A chrome/browser/win/jumplist_file_util.cc View 1 2 3 4 5 6 1 chunk +115 lines, -0 lines 0 comments Download
A chrome/browser/win/jumplist_file_util_unittest.cc View 1 2 3 4 5 6 1 chunk +103 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 5 chunks +45 lines, -0 lines 0 comments Download

Messages

Total messages: 135 (113 generated)
chengx
Hi Greg, I am trying to evaluate how base::Delete affects the jumplist update using this ...
3 years, 9 months ago (2017-03-15 21:22:48 UTC) #9
grt (UTC plus 2)
It's looking more and more like the original design of this code was imperfect. Rather ...
3 years, 9 months ago (2017-03-16 08:50:46 UTC) #11
chengx
Thanks for the detailed suggestion! I have some comments inline with yours below. On 2017/03/16 ...
3 years, 9 months ago (2017-03-16 18:17:33 UTC) #14
chengx
I have used UMA metrics extensively trying to find the root cause and monitoring the ...
3 years, 9 months ago (2017-03-16 18:44:55 UTC) #18
grt (UTC plus 2)
On 2017/03/16 18:17:33, chengx wrote: > Thanks for the detailed suggestion! I have some comments ...
3 years, 9 months ago (2017-03-17 08:44:38 UTC) #19
chengx
On 2017/03/17 08:44:38, grt (UTC plus 1) wrote: > The old code is flawed because ...
3 years, 9 months ago (2017-03-18 00:20:02 UTC) #22
grt (UTC plus 2)
this is a good start. the various delete helper functions need some unit tests. histograms.xml ...
3 years, 9 months ago (2017-03-20 09:15:37 UTC) #34
chengx
isherman@ PTAL histograms.xml~ grt@ Thanks for all the suggestions! I have updated the patch set ...
3 years, 9 months ago (2017-03-21 01:37:11 UTC) #37
chengx
isherman@ for histograms.xml please!
3 years, 9 months ago (2017-03-21 01:38:02 UTC) #39
grt (UTC plus 2)
i'm concerned about this approach. i see that you're copying code from base so that ...
3 years, 9 months ago (2017-03-21 08:40:27 UTC) #54
Ilya Sherman
https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2752063002/diff/100001/chrome/browser/win/jumplist.cc#newcode416 chrome/browser/win/jumplist.cc:416: // UMA_HISTOGRAM_ENUMERATION. This comment literally just repeats the code, ...
3 years, 9 months ago (2017-03-21 20:56:45 UTC) #55
chengx
I have addressed all the comments in the new patch set. PTAL~ grt@: I have ...
3 years, 9 months ago (2017-03-22 06:46:28 UTC) #67
grt (UTC plus 2)
i don't think that creating 101 files in a unittest is too much IO. if ...
3 years, 9 months ago (2017-03-22 12:07:22 UTC) #70
chengx
All the feedback comments are addressed in the new patch set. PTAL, thanks! https://codereview.chromium.org/2752063002/diff/160001/chrome/browser/win/jumplist_file_util.cc File ...
3 years, 9 months ago (2017-03-22 21:16:37 UTC) #76
Ilya Sherman
Metrics LGTM https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jumplist_file_util.cc File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2752063002/diff/200001/chrome/browser/win/jumplist_file_util.cc#newcode119 chrome/browser/win/jumplist_file_util.cc:119: delete_status, END); FWIW, you might want to ...
3 years, 9 months ago (2017-03-22 22:24:00 UTC) #77
grt (UTC plus 2)
lgtm with nits. i look forward to seeing how this performs. i think it should ...
3 years, 9 months ago (2017-03-23 11:24:58 UTC) #99
brucedawson
https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jumplist_file_util.h File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2752063002/diff/280001/chrome/browser/win/jumplist_file_util.h#newcode48 chrome/browser/win/jumplist_file_util.h:48: const int max_file_deleted); On 2017/03/23 11:24:58, grt (UTC plus ...
3 years, 9 months ago (2017-03-23 18:53:26 UTC) #107
chengx
All feedback comments are addressed in the new patch set. Now the CL is ready ...
3 years, 9 months ago (2017-03-23 21:16:12 UTC) #110
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752063002/290009
3 years, 9 months ago (2017-03-23 21:35:11 UTC) #115
commit-bot: I haz the power
Failed to apply patch for chrome/browser/win/jumplist.cc: While running git apply --index -p1; error: patch failed: ...
3 years, 9 months ago (2017-03-23 21:59:22 UTC) #117
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752063002/330001
3 years, 9 months ago (2017-03-24 00:15:25 UTC) #132
commit-bot: I haz the power
3 years, 9 months ago (2017-03-24 00:25:28 UTC) #135
Message was sent while issue was closed.
Committed patchset #9 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/2eefdd30dac56219587f39e35eac...

Powered by Google App Engine
This is Rietveld 408576698