If notifying the OS about the JumpList update fails, there'll be no icons showing up but the background color |
|||
Issue descriptionCurrently, notifying the OS about the JumpList update takes place after the old icon files in JumpListIcons folder are deleted. This order is critical as it can avoid the JumpList folder from getting accumulated endlessly. As a comparison, the previous implementation transfers the old icons from JumpListIcons folder to JumpListIconsOld folder temporarily (although they're not used anymore) rather than deleting them right away. This transfer process somehow led the two folders to grow endlessly and therefore caused severe JumpList performance issue. On the other hand, if the OS notification step fails which does happen for some users occasionally 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 makes the JumpList look awful. We need to fix this, or at least relieve this issue if fixing it entirely hurts the JumpList performance (runtime, disk IO, etc) too much. This situation can be simulated by manually deleting the icon files in the JumpListIcons folder, and then right clicking the Chrome icon in Window taskbar. The a JumpList without icons will show up.
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/63cfe1991cbd66f86963ed2e1afecb1c2ed5519e commit 63cfe1991cbd66f86963ed2e1afecb1c2ed5519e Author: chengx <chengx@chromium.org> Date: Thu Apr 27 21:05:09 2017 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} [modify] https://crrev.com/63cfe1991cbd66f86963ed2e1afecb1c2ed5519e/chrome/browser/win/jumplist.cc [modify] https://crrev.com/63cfe1991cbd66f86963ed2e1afecb1c2ed5519e/chrome/browser/win/jumplist.h
,
May 3 2017
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8127957d51be9220f6d5f64c637cd3a9d5566ea8 commit 8127957d51be9220f6d5f64c637cd3a9d5566ea8 Author: chengx <chengx@chromium.org> Date: Fri May 19 04:05:15 2017 Cache JumpList icons to avoid unnecessary creation and deletion Previously, when a JumpList category ("Mostly visited" or "Recently closed") is updated, new icons are created and written into the corresponding folder after all old icons in the same folder are deleted. However, there's usually a (big) overlap between the new and old icons. This means that we're deleting a lot of icons and then creating the same ones in each update, which is very inefficient. For example, JumpList can show at most 3 items for "Recently closed" category. Therefore, there are usually 3 icons existed for this JumpList category. When a Chrome tab gets closed, the icons for this category is updated. Previously, we delete all 3 existing icons and then create 3 new ones. However, the most efficient way is to create one icon (if it doesn't exist) only for this closed tab and delete the least recent one. If this icon exists already, we don't need to delete and create any icon files at all. To fix this, we introduce two maps to cache the URL-IconFilePath information of the two JumpList categories. With the help of these maps, we are able to avoid deleting and creating the same icons. The JumpList icon folders will be cleaned entirely only in the following situations: 1) The "Most visited" category updates for the first time after Chrome is launched. This actually happens right after Chrome is launched. 2) The "Recently closed" category updates for the first time after Chrome is launched. This happens when a first Chrome tab is closed. 3) The number of icons in the jumplist icon folders has exceeded the limit, which is currently set to 12 for "most visited" category and 6 for "recently closed" category, respectively. This is very important as it prevents the jumplist folders from getting bloated endlessly. Note that some sites change the favicon to indicate status. Since caching is done based on the URL rather than the image itself, the jumplist won't pick up these stats updates. Given the fact that this cache mechanism improves the performance nicely, this is an okay compromise. [Impact on UI side] 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 we're now only deleting the retired icons rather than all the old icons, we'll still have most of the old icons if the OS notification step fails, which makes the JumpList look much better in this situation. BUG= 40407 , 179576 , 715902 , 716115 Review-Url: https://codereview.chromium.org/2859193005 Cr-Commit-Position: refs/heads/master@{#473078} [modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist.cc [modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist.h [modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_file_util.cc [modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_file_util.h [modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_file_util_unittest.cc [modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_updater.h
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/531f38bd194f9d8a0dd98d78a15f82947d742cec commit 531f38bd194f9d8a0dd98d78a15f82947d742cec Author: chengx <chengx@chromium.org> Date: Thu Jun 29 18:44:20 2017 Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG= 40407 , 716115 , 736530 Review-Url: https://codereview.chromium.org/2941323002 Cr-Commit-Position: refs/heads/master@{#483439} [modify] https://crrev.com/531f38bd194f9d8a0dd98d78a15f82947d742cec/chrome/browser/win/jumplist.cc [modify] https://crrev.com/531f38bd194f9d8a0dd98d78a15f82947d742cec/chrome/browser/win/jumplist.h
,
Jun 29 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by chengx@chromium.org
, Apr 27 2017Components: UI Internals>PlatformIntegration
Labels: OS-Windows
Owner: chengx@chromium.org
Status: Assigned (was: Untriaged)