|
|
DescriptionUpdate 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 #Messages
Total messages: 80 (69 generated)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #2 (id:20001) 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...
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
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.
Description was changed from ========== Update "Most Visited" and "Recently Closed" category separately on demand. BUG=40407, 179576 ========== to ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. BUG=40407, 179576 ==========
Description was changed from ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. BUG=40407, 179576 ========== to ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. BUG=40407, 179576 ==========
Description was changed from ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. BUG=40407, 179576 ========== to ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. This CL also relieved 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, old JumpList will still be used. However, since the icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of the time, we'll still have icons for the other category if the OS notification step fails. That says, we'll lose about half of the icons instead of all of them. BUG=40407, 179576 ==========
Description was changed from ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. This CL also relieved 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, old JumpList will still be used. However, since the icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of the time, we'll still have icons for the other category if the OS notification step fails. That says, we'll lose about half of the icons instead of all of them. BUG=40407, 179576 ========== to ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. This CL also relieved 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, old JumpList will still be used. However, since the icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of the time, we'll still have icons for the other category if the OS notification step fails. That says, we'll lose about half of the icons instead of all of them, which is clearly better. BUG=40407, 179576 ==========
Description was changed from ========== Update different JumpList categories on demand JumpList class is an observer of the TopSites class as well as the TabRestoreService class. Each time TopSites gets updated or a tab is added/removed, the JumpList is updated. Chrome JumpList has two categories, i.e., "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 TopSites class has changes, it only affects "Most visited" category, while when TabRestoreService class has changes, it only affects "Recently closed". In this sense, we should update each category on demand rather than doing it in every JumpList update. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in one JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the JumpList slots assigned to these two categories. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to "Most visited" category and 4 to "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give "Most visited" 5 slots and "Recently closed" 3 slots out of 8. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. This CL also relieved 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, old JumpList will still be used. However, since the icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of the time, we'll still have icons for the other category if the OS notification step fails. That says, we'll lose about half of the icons instead of all of them, which is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category, while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category, while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category, while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category, while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. In each JumpList update, there're 10 old icons deleted followed by 10 new ones created. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org, isherman@chromium.org
I feel like it's better to land this CL before crrev.com/2831433003 (Time out jumplist update for very slow or busy OS). This makes it easier to interpret the UMA metric changes led by these changes. These two CLs have different focuses that I won't merge them into one. PTAL, thanks!
histograms.xml lgtm, thanks
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the number of JumpList slots assigned to these two categories which are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we assigned 6 to the "Most visited" category and 4 to the "Recently closed" category. However, the pre-defined "Tasks" category always takes 2 slots, resulting in there're actually 8 slots left. This CL fixes to give the "Most visited" category 5 slots and the "Recently closed" category 3 slots out of 8 in total. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of JumpList slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to both categories accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of slots available accordingly based on the presence of both categories. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of JumpList slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to both categories accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of slots available accordingly based on the presence of both categories. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5 per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number reduced from 24 to 10. This CL further reduced this number to 3~5 per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories are incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but JumpList background color showing up there which doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", this number 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After landing crrev.com/2816113002, i.e., "Fix to not create jumplist icon files that aren't used by shell", 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, this two CLs have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ==========
Some of these improvements seem fairly independent. Can any of them be split into their own CLs? https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:198: // https://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx hot tip: this can be reduced to https://msdn.microsoft.com/library/windows/desktop/dd378398.aspx https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:430: void JumpList::OnMostVisitedURLsAvailable( It seems that while the set of most visited sites may change often, there should be a lot of overlap in each update. The same may be true for recently closed. In the interest of reducing IO, it may make sense to cache the mapping of favicon identifier to image filename in the JumpList instance so that the work to fetch favicons, delete old ones, write new ones to disk, etc can all be skipped when not needed. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:450: data->icon_urls_.push_back(make_pair(url_string, link)); nit: std::make_pair https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:457: should_update_recent_closed_ = false; what if visited URLs arrive immediately after getting tabs from the tab restore service? won't this ignore the links/icons collected from the previous notification? same comment in TabRestoreServiceChanged. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:465: // if we have a pending handle request, cancel it here (it is out of date). nit: "handle" -> "favicon" https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:488: AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry), this looks pretty messed up: AddTab puts ShellLinkItems into temp_list, but puts icon urls directly into this->data_ (acquiring and releasing the mutex each time). this can lead to data inconsistency. it seems much, much more reliable to apply both changes to this->data atomically (as is done in OnMostVisitedURLsAvailable). https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:502: data->recently_closed_pages_ = temp_list; swap would be more efficient here so that no copies are made and so the old data from recently_closed_pages_ is deleted outside of the lock. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:652: base::TimeDelta::FromMilliseconds(kDelayForJumplistUpdateInMS), It seems that favicons are fetched as the most visited and recently closed lists are updated. These will be dropped on the floor if another similar update comes in within the kDelayForJumplistUpdate window, no? Would there not potentially be fewer favicon fetches if this fetching were moved to the end of this window?
Patchset #2 (id:160001) has been deleted
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] Besides, this CL fixes the subtle bug 714925, i.e., "Recently closed" category of JumpList won't show up until closing three websites instead of one". This is due to that the number of jumplist slots assigned to these two categories is incorrect. By default, the maximum number of items to display in JumpList is 10 per Windows. Previously, we considered there'are 10 available jumplist slots, and we assign these slots to each category accordingly. However, each category title, e.g., "Most visited" takes one slot. Therefore, we need to adjust the amount of available slots based on the presence of both categories. [Improvement 3] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576, 714925 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ==========
Description was changed from ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. [Improvement 1] Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by 83% per JumpList update. [Improvement 2] 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ========== to ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ==========
Patchset #4 (id:220001) has been deleted
Thanks for all the feedback comments! I've addressed most of them except two which I think are beyond the scope of this CL. I'll using other CLs for them. 1) Cache the mapping of favicon identifier to image filename in the JumpList instance so that the work to fetch favicons, delete old ones, write new ones to disk, etc can all be skipped when not needed. 2) Move the favicon fetching step to filter unnecessary favicon fetches. On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > Some of these improvements seem fairly independent. Can any of them be split > into their own CLs? I've move the change that fixes the number of JumpList slots assigned to each JumpList category to crrev.com/2844463003. gab@ gave me a LGTM, so I landed it and merged it to this CL already. If you have more comments on that change, I'll address them using another CL. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:198: // https://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > hot tip: this can be reduced to > https://msdn.microsoft.com/library/windows/desktop/dd378398.aspx Thanks! I've moved the change of this section to a separate CL crrev.com/2844463003/, which is also updated according to your suggestion. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:430: void JumpList::OnMostVisitedURLsAvailable( On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > It seems that while the set of most visited sites may change often, there should > be a lot of overlap in each update. The same may be true for recently closed. In > the interest of reducing IO, it may make sense to cache the mapping of favicon > identifier to image filename in the JumpList instance so that the work to fetch > favicons, delete old ones, write new ones to disk, etc can all be skipped when > not needed. I agree with the idea. I'll use another CL for this as it's out of scope of this CL. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:450: data->icon_urls_.push_back(make_pair(url_string, link)); On 2017/04/25 08:29:38, grt (UTC plus 2) wrote: > nit: std::make_pair Done. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:457: should_update_recent_closed_ = false; On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > what if visited URLs arrive immediately after getting tabs from the tab restore > service? won't this ignore the links/icons collected from the previous > notification? same comment in TabRestoreServiceChanged. You're right. The "TopSites Service" and the "TabRestore Service" can overwrite each other w.r.t these two boolean flags if their update are very close in time, typically 200 ms or even less according to the runtime and queue time of the jumplist update step. This rarely happens and it recovers easily if it does happen. I was thinking to pass these two flags from here all the way down to RunUpdateJumpList call, which could avoid mutual overwriting. But given the fact that it rarely happens, I think making these two flags global variables is fine. The code is also cleaner a bit due to this. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:465: // if we have a pending handle request, cancel it here (it is out of date). On 2017/04/25 08:29:38, grt (UTC plus 2) wrote: > nit: "handle" -> "favicon" Done. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:488: AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry), On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > this looks pretty messed up: AddTab puts ShellLinkItems into temp_list, but puts > icon urls directly into this->data_ (acquiring and releasing the mutex each > time). this can lead to data inconsistency. it seems much, much more reliable to > apply both changes to this->data atomically (as is done in > OnMostVisitedURLsAvailable). Done. Now JumpListData->data is updated entirely atomically. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:502: data->recently_closed_pages_ = temp_list; On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > swap would be more efficient here so that no copies are made and so the old data > from recently_closed_pages_ is deleted outside of the lock. This piece of code is gone after making JumpListData->data update entirely atomically. https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:652: base::TimeDelta::FromMilliseconds(kDelayForJumplistUpdateInMS), On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > It seems that favicons are fetched as the most visited and recently closed lists > are updated. These will be dropped on the floor if another similar update comes > in within the kDelayForJumplistUpdate window, no? Would there not potentially be > fewer favicon fetches if this fetching were moved to the end of this window? Yes, those favicons will be thrown away if another similar update comes within the kDelayForJumplistUpdate window. Moving the favicon fetching step involves quite a bit of code reordering and refactoring IMHO and it's beyond the scope of this CL anyway. I'll use another CL for this change too.
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 ========== 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 added/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 TopSites class has changes, it only affects the "Most visited" category; while when TabRestoreService class has changes, it only affects the "Recently closed". In this sense, we should update each category on demand rather than in every JumpList update even there's no change for that category. Initially, each JumpList update involves 24 icons' deletion and 24 icons' creation. After crrev.com/2816113002, i.e., "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 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, the old JumpList will still be used. However, since the old icons are deleted, there'll be nothing but background color showing up in the places where the icons should show up. This doesn't look nice. With this CL, since only one category is updated for most of 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 is clearly better. This CL changes the JumpList icons' related directory. Since the icon files for the two categories are dealt with separately, it's more efficient to put them in different folders rather in a single JumpListIcons folder. This CL introduces two new folders for this purpose: "JumpListIconsMostVisited" and "JumpListIconsRecentClosed". Since JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ========== to ========== 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 only affects the "Most visited" category; while when the TabRestoreService class has changes, it only affects the "Recently closed". 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) is 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 is uncommon, 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 is 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 in a single JumpListIcons folder. This CL introduces two new folders for this purpose: JumpListIconsMostVisited and JumpListIconsRecentClosed. Since the JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ==========
Description was changed from ========== 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 only affects the "Most visited" category; while when the TabRestoreService class has changes, it only affects the "Recently closed". 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) is 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 is uncommon, 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 is 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 in a single JumpListIcons folder. This CL introduces two new folders for this purpose: JumpListIconsMostVisited and JumpListIconsRecentClosed. Since the JumpListIcons folder is no longer needed, this CL posts a background task to delete it. BUG=40407, 179576 ========== to ========== 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 is uncommon, 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 ==========
Description was changed from ========== 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 is uncommon, 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 ========== to ========== 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 is uncommon, 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:430: void JumpList::OnMostVisitedURLsAvailable( On 2017/04/25 23:00:24, chengx wrote: > On 2017/04/25 08:29:39, grt (UTC plus 2) wrote: > > It seems that while the set of most visited sites may change often, there > should > > be a lot of overlap in each update. The same may be true for recently closed. > In > > the interest of reducing IO, it may make sense to cache the mapping of favicon > > identifier to image filename in the JumpList instance so that the work to > fetch > > favicons, delete old ones, write new ones to disk, etc can all be skipped when > > not needed. > > I agree with the idea. I'll use another CL for this as it's out of scope of > this CL. Agreed, this isn't the right CL for it. Thanks. https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:514: should_update_most_visited_ = false; do i understand correctly that the point of these two is to convey to UpdateJumpList that the corresponding list should be processed? it seems cleaner to put an "is dirty" bit in JumpListData next to each list that is set to true when the list is modified (OnMostVisitedURLsAvailable and TabRestoreServiceChanged) and then have RunUpdateJumpList extract the dirty bits when it copies the lists themselves and clear the dirty bits. if the update fails, you will want a way to re-apply the dirty bits to JumpListData. so maybe UpdateJumpList should return bool afterall so that RunUpdateJumpList could do something like: if (!UpdateJumpList(...)) { AutoLock l; if (most_visited_pages_dirty) data->most_visited_pages_dirty); if (local_re.... } i think this would avoid the problem where one type of update messes with the state for the other. https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:528: data->list_lock_.AssertAcquired(); https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:551: data->list_lock_.AssertAcquired();
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 #5 (id:260001) 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...
I've addressed all the comments in the new patch set. PTAL, thanks! https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:514: should_update_most_visited_ = false; On 2017/04/26 09:14:33, grt (UTC plus 2) wrote: > do i understand correctly that the point of these two is to convey to > UpdateJumpList that the corresponding list should be processed? it seems cleaner > to put an "is dirty" bit in JumpListData next to each list that is set to true > when the list is modified (OnMostVisitedURLsAvailable and > TabRestoreServiceChanged) and then have RunUpdateJumpList extract the dirty bits > when it copies the lists themselves and clear the dirty bits. if the update > fails, you will want a way to re-apply the dirty bits to JumpListData. so maybe > UpdateJumpList should return bool afterall so that RunUpdateJumpList could do > something like: > > if (!UpdateJumpList(...)) { > AutoLock l; > if (most_visited_pages_dirty) > data->most_visited_pages_dirty); > if (local_re.... > } > > i think this would avoid the problem where one type of update messes with the > state for the other. Awesome idea! I've updated the code accordingly. Thanks! https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:528: On 2017/04/26 09:14:33, grt (UTC plus 2) wrote: > data->list_lock_.AssertAcquired(); Done. https://codereview.chromium.org/2836873003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:551: On 2017/04/26 09:14:33, grt (UTC plus 2) wrote: > data->list_lock_.AssertAcquired(); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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 is uncommon, 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 ========== to ========== 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 is uncommon, 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 ==========
this is looking good. down to the final nits! https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:306: if (!jumplist_updater.CommitUpdate()) { nit: return jumplist_updater.CommitUpdate(); https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:365: : most_visited_pages_have_updates_(true), nit: prefer member initialization in the class definition. also please document why these initial values make sense (why true for this and false for the other?). https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:546: nit: omit blank line so that asserts are grouped together (below, too) https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:658: if (!waiting_for_icons) { nit: omit braces https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:694: // Post a task to delete JumpListIcons folder as it't no longer needed. it't -> it's https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:128: // AddTab creates a ShellLinkItem object from a tab and add it to |data|. nit: "Creates a ShellLinkItem..." https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:128: // AddTab creates a ShellLinkItem object from a tab and add it to |data|. nit: add -> adds https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:128: // AddTab creates a ShellLinkItem object from a tab and add it to |data|. better yet: // Adds a new ShellLinkItem for |tab| to |data| provided that doing so will not exceed |max_items|. similar for AddWindow below https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:130: JumpListData* data, nit: out params must follow all in params (https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering). please swap |data| and |max_items|. below, too.
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:300001) has been deleted
All the comments are addressed in the new patch set. PTAL, thanks! https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:306: if (!jumplist_updater.CommitUpdate()) { On 2017/04/27 09:05:25, grt (UTC plus 2) wrote: > nit: > return jumplist_updater.CommitUpdate(); Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:365: : most_visited_pages_have_updates_(true), On 2017/04/27 09:05:24, grt (UTC plus 2) wrote: > nit: prefer member initialization in the class definition. also please document > why these initial values make sense (why true for this and false for the > other?). Done. Now both of them are set to false by fault. I thought I needed to set most_visited_pages_have_updates_ flag to true to enable syncing with TopSites service when launching Chrome. However, I then realized there's no need to do this as syncing with TopSites service will set this flag to true automatically. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:546: On 2017/04/27 09:05:24, grt (UTC plus 2) wrote: > nit: omit blank line so that asserts are grouped together (below, too) Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:658: if (!waiting_for_icons) { On 2017/04/27 09:05:25, grt (UTC plus 2) wrote: > nit: omit braces Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:694: // Post a task to delete JumpListIcons folder as it't no longer needed. On 2017/04/27 09:05:24, grt (UTC plus 2) wrote: > it't -> it's Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:128: // AddTab creates a ShellLinkItem object from a tab and add it to |data|. On 2017/04/27 09:05:25, grt (UTC plus 2) wrote: > nit: add -> adds Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:128: // AddTab creates a ShellLinkItem object from a tab and add it to |data|. On 2017/04/27 09:05:25, grt (UTC plus 2) wrote: > nit: "Creates a ShellLinkItem..." Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:128: // AddTab creates a ShellLinkItem object from a tab and add it to |data|. On 2017/04/27 09:05:25, grt (UTC plus 2) wrote: > better yet: > // Adds a new ShellLinkItem for |tab| to |data| provided that doing so will > not exceed |max_items|. > > similar for AddWindow below Done. https://codereview.chromium.org/2836873003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:130: JumpListData* data, On 2017/04/27 09:05:25, grt (UTC plus 2) wrote: > nit: out params must follow all in params > (https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering). > please swap |data| and |max_items|. below, too. Done. Thanks for pointing me to this.
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 ========== 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 is uncommon, 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 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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 ========== to ========== 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 ==========
nice! lgtm.
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 Link to the patchset: https://codereview.chromium.org/2836873003/#ps320001 (title: "Fix nits")
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": 320001, "attempt_start_ts": 1493326427934270, "parent_rev": "bdc215987e3feb39a009ccb797bf751ba42311d6", "commit_rev": "63cfe1991cbd66f86963ed2e1afecb1c2ed5519e"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/63cfe1991cbd66f86963ed2e1afe... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:320001) as https://chromium.googlesource.com/chromium/src/+/63cfe1991cbd66f86963ed2e1afe... |