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

Issue 2836873003: Update different JumpList categories on demand (Closed)

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

Description

Update different JumpList categories on demand The JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is removed, the JumpList is updated. Chrome JumpList has two categories, namely "Most visited" and "Recently closed", whose icon files are updated altogether in a single JumpList update run. The icon files are updated in the way that old icons are deleted followed by new icons' creation. However, updating icons for both categories together is unnecessary. When the TopSites class has changes, it affects the "Most visited" category only; while when the TabRestoreService class has changes, it affects the "Recently closed" only. In this sense, we should update each category on demand rather than in every JumpList update even when there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002 (Fix to not create jumplist icon files that aren't used by shell) was landed, this number was reduced from 24*2 to 10*2. This CL further reduced this number to ~4*2 per JumpList update. As each icon file is 28 KB, these two CLs together have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. This CL also relieves the following issue. Currently, notifying the OS about the JumpList update takes place after the old icon files are deleted. This order is critical as it can avoid the JumpList folder from getting accumulated endlessly. On the other hand, if the OS notification step fails which does happen sometimes according to UMA data, the old JumpList will still be used. However, since the old icons have been deleted, there'll be nothing but the background color showing up where the icons should show up. This doesn't look good. With this CL, since only one category is updated for almost all the time, we'll still have icons for the other category if the OS notification step fails. That says, we'll still have about half of the icons instead of nothing, which's clearly better. This CL changes the JumpList related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in separate folders rather than in a single JumpListIcons folder. This CL introduces two new folders JumpListIconsMostVisited and JumpListIconsRecentClosed for this purpose. As the JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 715902, 716115 Review-Url: https://codereview.chromium.org/2836873003 Cr-Commit-Position: refs/heads/master@{#467788} Committed: https://chromium.googlesource.com/chromium/src/+/63cfe1991cbd66f86963ed2e1afecb1c2ed5519e

Patch Set 1 #

Total comments: 17

Patch Set 2 : Git pull to merge CL "Time out jumplist update for very slow or busy OS" #

Patch Set 3 : Git pull to merge CL "Fix the number of JumpList slots assigned to each JumpList category" #

Patch Set 4 : Address comments #

Total comments: 6

Patch Set 5 : Make the two types of Category update independent of each other #

Total comments: 18

Patch Set 6 : Fix nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+161 lines, -86 lines) Patch
M chrome/browser/win/jumplist.h View 1 2 3 4 5 3 chunks +29 lines, -12 lines 0 comments Download
M chrome/browser/win/jumplist.cc View 1 2 3 4 5 14 chunks +132 lines, -74 lines 0 comments Download

Messages

Total messages: 80 (69 generated)
chengx
I feel like it's better to land this CL before crrev.com/2831433003 (Time out jumplist update ...
3 years, 8 months ago (2017-04-24 23:56:55 UTC) #21
Ilya Sherman
histograms.xml lgtm, thanks
3 years, 8 months ago (2017-04-25 00:02:03 UTC) #22
grt (UTC plus 2)
Some of these improvements seem fairly independent. Can any of them be split into their ...
3 years, 8 months ago (2017-04-25 08:29:39 UTC) #39
chengx
Thanks for all the feedback comments! I've addressed most of them except two which I ...
3 years, 8 months ago (2017-04-25 23:00:24 UTC) #45
grt (UTC plus 2)
https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jumplist.cc#newcode430 chrome/browser/win/jumplist.cc:430: void JumpList::OnMostVisitedURLsAvailable( On 2017/04/25 23:00:24, chengx wrote: > On ...
3 years, 8 months ago (2017-04-26 09:14:33 UTC) #53
chengx
I've addressed all the comments in the new patch set. PTAL, thanks! https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc ...
3 years, 8 months ago (2017-04-27 01:18:47 UTC) #59
grt (UTC plus 2)
this is looking good. down to the final nits! https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jumplist.cc#newcode306 chrome/browser/win/jumplist.cc:306: ...
3 years, 7 months ago (2017-04-27 09:05:25 UTC) #63
chengx
All the comments are addressed in the new patch set. PTAL, thanks! https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc ...
3 years, 7 months ago (2017-04-27 17:21:11 UTC) #67
grt (UTC plus 2)
nice! lgtm.
3 years, 7 months ago (2017-04-27 20:46:07 UTC) #74
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/2836873003/320001
3 years, 7 months ago (2017-04-27 20:54:11 UTC) #77
commit-bot: I haz the power
3 years, 7 months ago (2017-04-27 21:05:27 UTC) #80
Message was sent while issue was closed.
Committed patchset #6 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/63cfe1991cbd66f86963ed2e1afe...

Powered by Google App Engine
This is Rietveld 408576698