Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865133003:
Defer syncing TopSites with history until the first tab closure (Closed)
DescriptionDefer syncing TopSites with history until the first tab closure
This change defers syncing TopSites with history until the first tab
closure and ignores any TopSites updates that arrive before that time.
It also makes the category titles ever-present in the JumpList and
removes the old balancing strategy that used to show more most visited
items when there were fewer than 3 recently closed items.
The JumpList will now retain its items from a previous launch until
the first tab closure, making it more useful early on in the browser's
lifetime. Also, there will be no unnecessary TopSites syncs now if an
user launches Chrome but shuts it down before opening any tabs. After
that initial tab closure, all old recently closed items are lost as it
is not possible to query Windows for them. Removal of the balancing
strategy trims out waste resulting from fetching 9 items initially,
which are then trimmed down as tabs are closed.
BUG=721484, 721486
Review-Url: https://codereview.chromium.org/2865133003
Cr-Commit-Position: refs/heads/master@{#473747}
Committed: https://chromium.googlesource.com/chromium/src/+/55c71f0ff307dfc3b3e561a2f72e90bb3df05393
Patch Set 1 #Patch Set 2 : Delay the history sync until 3 tabs are closed rather than 3 recently closed category updates are scheduled #
Total comments: 2
Patch Set 3 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into cancelfirstmostvisited… #Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Address comments from grt@ #
Total comments: 4
Patch Set 6 : Address comments form gab@ #Patch Set 7 : Git pull and fix merge conflicts #Messages
Total messages: 104 (89 generated)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Delay forced sync with history service for JumpList BUG= ========== to ========== Delay forced sync with history service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG= ==========
Description was changed from ========== Delay forced sync with history service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG= ========== to ========== Delay forced sync with history service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ==========
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Description was changed from ========== Delay forced sync with history service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced sync with history/TopSites service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Delay forced sync with history service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced sync with history/TopSites service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ==========
Patchset #1 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Delay forced sync with history/TopSites service for JumpList Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay the forced sync with the history service for JumpList until 5+ tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay updating jumplist "most visited" category after launching Chrome Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay updating jumplist "most visited" category until 3 tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Delay updating jumplist "most visited" category after launching Chrome Currently, once Chrome is launched, JumpList is forced to sync with the history service to update its "most visited" category. This results in deleting 9 old jumplist icons followed by creating 9 new ones. Since the JumpList class itself is an observer of the history service, its "most visited" category gets updated once getting notified there's update regularly. This indicates that the 9 icons deleted are very likely to have a big overlap with the 9 new ones. Besides, once 3+ tabs get opened and closed after Chrome is launched, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. In the worst case, if a user likes to open and close only one tab in a Chrome launching and shutting-down cycle, then most of the icon files' IO due to the history sync is wasted. To fix this, we delay updating jumplist "most visited" category until 3 tabs are opened and closed after Chrome is launched. If the "most visited" category already gets updated when observing the history service has updates before the delay finishes, the forced sync is now unnecessary therefore won't be executed anymore. Moreover, since we now have 3+ tabs opened and closed, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4 icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced syncing jumplist with history service after launching Chrome Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If removing this call, opening the first open will also triggers JumpList::DeferredTopSitesChanged() to update the "most visited" category right away. This is nice as the jumplist is ensured to be up-to-date starting from the very beginning. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome is closed after 3+ tabs are opened and closed) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more newly created icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they'll be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from last using Chrome. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay updating jumplist "most visited" category until "recently closed" category is updated for 3 times. There can be more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Delay forced syncing jumplist with history service after launching Chrome Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If removing this call, opening the first open will also triggers JumpList::DeferredTopSitesChanged() to update the "most visited" category right away. This is nice as the jumplist is ensured to be up-to-date starting from the very beginning. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome is closed after 3+ tabs are opened and closed) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more newly created icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they'll be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from last using Chrome. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay updating jumplist "most visited" category until "recently closed" category is updated for 3 times. There can be more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced syncing jumplist with history service once Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If removing this call, opening the first open will also triggers JumpList::DeferredTopSitesChanged() to update the "most visited" category right away. This is nice as the jumplist is ensured to be up-to-date starting from the very beginning. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome is closed after 3+ tabs are opened and closed) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more newly created icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they'll be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from last using Chrome. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay updating jumplist "most visited" category until "recently closed" category is updated for 3 times. There can be more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Delay forced syncing jumplist with history service once Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If removing this call, opening the first open will also triggers JumpList::DeferredTopSitesChanged() to update the "most visited" category right away. This is nice as the jumplist is ensured to be up-to-date starting from the very beginning. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome is closed after 3+ tabs are opened and closed) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more newly created icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they'll be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from last using Chrome. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay updating jumplist "most visited" category until "recently closed" category is updated for 3 times. There can be more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced syncing jumplist with history service once Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Delay forced syncing jumplist with history service once Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until the "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ==========
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 #1 (id:80001) has been deleted
chengx@chromium.org changed reviewers: + gab@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm probably not fully understanding the issue, perhaps a dedicated bug with screenshots would help? As-is this feels fairly arbitrary? A timeout seems better than an absolute number if the issue is to help with startup performance? If the problem is that we end up over generating Most Visited only to throw them away maybe we should just cap both categories at 5 and 3 instead of trying to load balance when one is empty? Feels like Most Visited should just keep its old value on startup until a tab is closed? In all cases, seems like this is starting to call for a UX discussion with chrome-ui-review@?
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until the "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=40407, 179576, 715902 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until the "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until the "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until the "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=721484, 721486 ==========
I've filed bugs 721484 and 721486 to describe the two issues mentioned in the CL description. You can find the links to these 2 bugs at the bottom of the CL description. Also please see my inline comments/replies to yours below. On 2017/05/11 15:02:52, gab wrote: > I'm probably not fully understanding the issue, perhaps a dedicated bug with > screenshots would help? Re: dedicated bugs have been filed with screenshots attached. > As-is this feels fairly arbitrary? Re: This is not arbitrary though. > A timeout seems better than an absolute number if the issue is to help with > startup performance? Re: An absolute number needs to be used here, because we need to make sure at least 3 tabs are closed so that we have 3 "recently closed" items to show now. This means we now only need to create 5 "most visited" items instead of 9. > If the problem is that we end up over generating Most Visited only to throw them > away maybe we should just cap both categories at 5 and 3 instead of trying to > load balance when one is empty? Re: I've removed the load balance code already, so now we cap both categories at 5 and 3. > Feels like Most Visited should just keep its old > value on startup until a tab is closed? Re: True. I am just keeping Most Visited to be not updated until 3 (instead of 1) tabs are closed. This ensures we always have 8 items to show in jumplist. Btw, after Chrome launches, the "most visited" list and "recently closed" list are both empty. It's expensive to get old ones from the shell/OS, so the two lists now contain nothing. Only updating most visited via history sync or updating recently closed alone will result in only one category showing up in the jumplist. This is how jumplist update works with the OS, which I cannot modify. Therefore, we need to wait until a few tabs closed to make recently closed list full (having 3 items) before syncing with history service to make most visited list full (having 5 items). Once both of the list are full, we can start a jumplist update. > In all cases, seems like this is starting to call for a UX discussion with > chrome-ui-review@? Re: I don't think so though. The issue is obvious. Please see bug 721486.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until the "recently closed" category is updated for 3 times. There are more than 3 tabs opened and closed due to the "delay and coalesce" strategy. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until 3 tabs are closed. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=721484, 721486 ==========
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 ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This update for "most visited" category removes the "recently closed" category's information from Chrome's last run. UI-wise, there will be only "most visited" category in the jumplist panel. To fix this, we delay syncing jumplist "most visited" category with the history service until 3 tabs are closed. Therefore, we are only creating 5 instead of 9 jumplist icons for the "most visited" category. This avoids wasting those 4+ icons as mentioned above. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update updates jumplist items used by the OS for both categories at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. To fix these two issues, we delay syncing jumplist "most visited" category with the history service until 3 tabs are closed. BUG=721484, 721486 ==========
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 ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned to the "recently closed" category. If the user opens and closes less than 3 tabs, then more than 4 newly icons are wasted. In the worst case, if the user shuts down Chrome before opening any tabs, all these 9 new icons are wasted as they will be deleted once Chrome is launched again. 2) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update updates jumplist items used by the OS for both categories at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. To fix these two issues, we delay syncing jumplist "most visited" category with the history service until 3 tabs are closed. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. Since there're no "recently closed" tabs now, all the available jumplist slots are assigned to "most visited" category, resulting in 9 icons are created. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this sync with the history service is completely wasted. 2) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update updates jumplist items used by the OS for both categories at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it. This can avoid wasting 4 icons shortly. However, this doesn't To fix these two issues, we delay syncing jumplist "most visited" category with the history service until 3 tabs are closed. BUG=721484, 721486 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service to update its "most visited" category before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. Since there're no "recently closed" tabs now, all the available jumplist slots are assigned to "most visited" category, resulting in 9 icons are created. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this sync with the history service is completely wasted. 2) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update updates jumplist items used by the OS for both categories at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it. This can avoid wasting 4 icons shortly. However, this doesn't To fix these two issues, we delay syncing jumplist "most visited" category with the history service until 3 tabs are closed. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened/closed to update its "most visited" category. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is very expensive as it causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. Since there're no "recently closed" tabs now, all the available jumplist slots are assigned to "most visited" category, resulting in 9 icons are created. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this sync with the history service is completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update run updates both jumplist categories' items used by the OS at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from Chrome's last run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" category has less than 3 items. This can avoid wasting 4 icons. For issue 2 and 3, we can delay syncing jumplist with the history service until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. So in this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened/closed to update its "most visited" category. This is done by calling history::TopSites::SyncWithHistory(). If this function called is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is very expensive as it causes the following issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. Since there're no "recently closed" tabs now, all the available jumplist slots are assigned to "most visited" category, resulting in 9 icons are created. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this sync with the history service is completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update run updates both jumplist categories' items used by the OS at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from Chrome's last run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" category has less than 3 items. This can avoid wasting 4 icons. For issue 2 and 3, we can delay syncing jumplist with the history service until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. So in this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites::SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is very expensive as it causes the following issues. Moreover, since the top 5 "most visited" items usually remains the same, forcing a history sync has no benefits for most of the time. Given the following performance/UI issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. Since there're no "recently closed" tabs now, all the available jumplist slots are assigned to "most visited" category, resulting in 9 icons are created. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this sync with the history service is completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update run updates both jumplist categories' items used by the OS at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from Chrome's last run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" category has less than 3 items. This can avoid wasting 4 icons. For issue 2 and 3, we can delay syncing jumplist with the history service until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. So in this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites::SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is very expensive as it causes the following issues. Moreover, since the top 5 "most visited" items usually remains the same, forcing a history sync has no benefits for most of the time. Given the following performance/UI issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got opened and closed in the last run) right away, followed by creating 9 new ones. Since there're no "recently closed" tabs now, all the available jumplist slots are assigned to "most visited" category, resulting in 9 icons are created. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will become useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this sync with the history service is completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from Chrome's last run. This is because one jumplist update run updates both jumplist categories' items used by the OS at the same time, per its implementation on Windows side. Since there're no tabs closed yet, the new "recently closed" item list is still empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from Chrome's last run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" category has less than 3 items. This can avoid wasting 4 icons. For issue 2 and 3, we can delay syncing jumplist with the history service until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. So in this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites::SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is very expensive as it causes the following performance/UI issues. Moreover, since the top 5 "most visited" items usually remain unchanged, forcing a history sync has no benefits for most of the time. Given the following issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got closed in the last run) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync is completely unnecessary. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last Chrome run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issue 2 and 3, we can delay the history sync until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the history sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. In this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites::SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is very expensive as it causes the following performance/UI issues. Moreover, since the top 5 "most visited" items usually remain unchanged, forcing a history sync has no benefits for most of the time. Given the following issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got closed in the last run) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync is completely unnecessary. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last Chrome run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issue 2 and 3, we can delay the history sync until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the history sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. In this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. Moreover, since the top 5 "most visited" items usually remain unchanged, forcing a history sync has no benefits for most of the time. Given the following issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got closed in the last run) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync is completely unnecessary. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last Chrome run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the history sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. In this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ==========
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #2 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. Moreover, since the top 5 "most visited" items usually remain unchanged, forcing a history sync has no benefits for most of the time. Given the following issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got closed in the last run) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync is completely unnecessary. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last Chrome run as they are still the latest ones. [How to fix them] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the history sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. In this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. Moreover, since the top 5 "most visited" items usually remain unchanged, forcing a history sync has no benefits for most of the time. Given the following issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got closed in the last run) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync is completely unnecessary. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last Chrome run as they are still the latest ones. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the history sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. In this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ==========
Hi Gabriel, I've updated the CL description to make it clearer. Hope it helps. Thanks! On 2017/05/11 19:14:32, chengx wrote: > I've filed bugs 721484 and 721486 to describe the two issues mentioned in the CL > description. You can find the links to these 2 bugs at the bottom of the CL > description. Also please see my inline comments/replies to yours below. > > On 2017/05/11 15:02:52, gab wrote: > > I'm probably not fully understanding the issue, perhaps a dedicated bug with > > screenshots would help? > Re: dedicated bugs have been filed with screenshots attached. > > > As-is this feels fairly arbitrary? > Re: This is not arbitrary though. > > > A timeout seems better than an absolute number if the issue is to help with > > startup performance? > Re: An absolute number needs to be used here, because we need to make sure at > least 3 tabs are closed so that we have 3 "recently closed" items to show now. > This means we now only need to create 5 "most visited" items instead of 9. > > > If the problem is that we end up over generating Most Visited only to throw > them > > away maybe we should just cap both categories at 5 and 3 instead of trying to > > load balance when one is empty? > Re: I've removed the load balance code already, so now we cap both categories at > 5 and 3. > > > Feels like Most Visited should just keep its old > > value on startup until a tab is closed? > Re: True. I am just keeping Most Visited to be not updated until 3 (instead of > 1) tabs are closed. This ensures we always have 8 items to show in jumplist. > Btw, after Chrome launches, the "most visited" list and "recently closed" list > are both empty. It's expensive to get old ones from the shell/OS, so the two > lists now contain nothing. Only updating most visited via history sync or > updating recently closed alone will result in only one category showing up in > the jumplist. This is how jumplist update works with the OS, which I cannot > modify. Therefore, we need to wait until a few tabs closed to make recently > closed list full (having 3 items) before syncing with history service to make > most visited list full (having 5 items). Once both of the list are full, we can > start a jumplist update. > > > In all cases, seems like this is starting to call for a UX discussion with > > chrome-ui-review@? > Re: I don't think so though. The issue is obvious. Please see bug 721486.
chengx@chromium.org changed reviewers: + grt@chromium.org
Adding grt@ for suggestions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not a fan of piling on ints/bools logic, this is how we end up with logic that looks like startup_browser_creator_impl.cc... I'd prefer a fixed set of slots for Most Visited and another fixed number for Recently Closed. I also think we need to update Recently Closed after first tab closed (otherwise this feature is just weird... if a user relies on being able to re-open via this feature it now becomes flaky after startup (it won't be obvious to a user why it's sometimes there sometimes not...), and if no one relies on that then we shouldn't have it at all... so..!). Why do we have to restart Recently Closed? Can't we use existing history when we get the first tab closed notification? That wouldn't force a sync on startup so it's win-win? https://codereview.chromium.org/2865133003/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2865133003/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:656: MulDiv(user_max_items_adjusted, kMostVisited, kTotal); Can we just make this 5 and recently closed 3. i.e. stop trying to load-balance?
While we can't ask the OS to tell us what is in the jumplist, we should be able to track it ourselves. If the last N closed tabs is already in the history DB (it must be, since RestoreTab 5 times restores the last 5 closed tabs, no?), then can we not pull that out for the first jumplist update?
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is closed to update its "most visited" category. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. Moreover, since the top 5 "most visited" items usually remain unchanged, forcing a history sync has no benefits for most of the time. Given the following issues it leads to, we should have a better strategy for it. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5 if Chrome was closed after 3+ tabs got closed in the last run) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync is completely unnecessary. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last Chrome run as they are still the latest ones. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until 1 tab is closed. After this update, the jumplist panel will have 5 most visited items and 1 recently closed item. We can also choose to delay the history sync until 3 tabs are closed. This ensures there're always 3 items in the "recently closed" category, which looks a bit nicer. It also addresses issue 1 naturally. In this CL, we choose to wait for 3 tabs being closed to start the history sync. BUG=721484, 721486 ========== to ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed. After that, the jumplist will have 5 most visited items and 1 recently closed item. BUG=721484, 721486 ==========
Patchset #4 (id:200001) has been deleted
Hi Greg and Gabriel, please see my replies to your comments below. ================================== ====== From gab@ and grt@ ======== ================================== On 2017/05/12 18:13:36, gab wrote: > Why do we have to restart Recently Closed? Can't we use existing history when we > get the first tab closed notification? That wouldn't force a sync on startup so > it's win-win? On 2017/05/15 11:39:22, grt (UTC plus 2) wrote: > While we can't ask the OS to tell us what is in the jumplist, we should be able > to track it ourselves. If the last N closed tabs is already in the history DB > (it must be, since RestoreTab 5 times restores the last 5 closed tabs, no?), > then can we not pull that out for the first jumplist update? I think you're talking about the same thing. Yes, we could restore the tabs from last session using TabRestoreServie::LoadTabsFromLastSession(). However, it's a bit expensive as disk IO must be involved. Also, there's no existing API in TabRestoreService to do this. I could implement it but I doubt if the efforts are worthwhile. Also, this seems to me that it's a new small jumplist feature as it was never there. If you think this is something we really need to have, I'll try the implementation in another CL. Btw, I've changed the old jumplist from the last session to stay until the first tab is closed in the current session, rather than disappearing at the very beginning of the current session. ================================= ======= From gab@ =============== ================================= On 2017/05/12 18:13:36, gab wrote: > I'd prefer a fixed set of slots for Most Visited and another fixed number for > Recently Closed. I've limited the items in these two categories to 5 and 3. They can be less if Windows decreases the default value which is 10 now, so I made the code still adjustable to this possible change. Load balancing between the 2 categories is stopped. > I also think we need to update Recently Closed after first tab closed (otherwise > this feature is just weird... if a user relies on being able to re-open via this > feature it now becomes flaky after startup (it won't be obvious to a user why > it's sometimes there sometimes not...), and if no one relies on that then we > shouldn't have it at all... so..!). Sounds reasonable. I've changed to update Recently Closed after first tab closed in a new session. https://codereview.chromium.org/2865133003/diff/160001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2865133003/diff/160001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:656: MulDiv(user_max_items_adjusted, kMostVisited, kTotal); On 2017/05/12 18:13:35, gab wrote: > Can we just make this 5 and recently closed 3. i.e. stop trying to load-balance? I stopped load-balancing, and limited these two categories to 5 items and 3 items.
Description was changed from ========== Delay forced syncing jumplist with history service after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed. After that, the jumplist will have 5 most visited items and 1 recently closed item. BUG=721484, 721486 ========== to ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed. After that, the jumplist will have 5 most visited items and 1 recently closed item. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, this is expensive as it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed. After that, the jumplist will have 5 most visited items and 1 recently closed item. BUG=721484, 721486 ========== to ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed. After that, the jumplist will have 5 most visited items and 1 recently closed item. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed. After that, the jumplist will have 5 most visited items and 1 recently closed item. BUG=721484, 721486 ========== to ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to still have the "recently closed" items from the last session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ========== to ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to keep the "recently closed" items from the last session until we have new items from the current session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to keep the "recently closed" items from the last session until we have new items from the current session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ========== to ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to keep the "recently closed" items from the last session until we have new items from the current session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ==========
Description was changed from ========== Delay forced history sync for jumplist after Chrome launches Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to keep the "recently closed" items from the last session until we have new items from the current session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ========== to ========== Delay forced history sync for jumplist after Chrome is launched Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to keep the "recently closed" items from the last session until we have new items from the current session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ==========
I find it a bit hard to follow the CL description. It seems to explain problems with a different possible implementation that are resolved in this CL (the "Fix the issues" section). In general, I think CL descriptions are most clear when structured like so: Describe the new behavior in this CL without discussions of what happened in the past. Explain the new behavior relative to the old behavior with appropriate tradeoff discussions. For example (feel free to borrow/steal, but be sure to add any info I left out): Preserve recently closed items in the JumpList until the first tab closure. This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. The JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. https://codereview.chromium.org/2865133003/diff/220001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2865133003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:549: if (updates_to_skip_ > 0) { please add DCHECK(CalledOnValidThread()); since this must run on the UI thread. same for DeferredTopSitesChanged, please. https://codereview.chromium.org/2865133003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:554: // Force to do a TopSite history sync when closing a first tab in one session. nit: "Force a TopSite..."
Description was changed from ========== Delay forced history sync for jumplist after Chrome is launched Currently, once Chrome is launched, JumpList is forced to sync with the history (TopSites) service before any tab is opened. This is done by calling history::TopSites:: SyncWithHistory(). If this function call is removed, opening the first tab immediately triggers JumpList::DeferredTopSitesChanged(), which also syncs the "most visited" category with the history service. This is nice as the jumplist is ensured to be up-to-date once Chrome is launched. However, it causes the following performance/UI issues. 1) This results in deleting all old jumplist icons for "most visited" category (typically 5) right away, followed by creating 9 new ones. It is 9 new icons instead of 5 because there're no "recently closed" items now, and all the available jumplist slots are assigned to "most visited" category. If the user opens and closes 3+ tabs later, 4 of the 9 new icons will be useless and deleted right away, as 4 jumplist slots (including 1 for "recently closed" category title) are now assigned back to the "recently closed" category. 2) If the user launches Chrome but shuts it down before opening any tabs, this history sync and the following jumplist update are completely wasted. 3) This history sync for the "most visited" category also wipes out the "recently closed" items from the last Chrome run. This is because one jumplist update cycle replaces items used by the OS with the current ones for both categories, per its implementation on Windows's side. Since there're no tabs closed yet, the current "recently closed" item list is empty. Therefore, after this history sync, there'll be no "recently closed" category in the jumplist panel. It would be nice to keep the "recently closed" items from the last session until we have new items from the current session. [Fix the issues] For issue 1, we can avoid creating 9 icons if we cap 5 items for "most visited" category instead of giving all available slots to it when "recently closed" list has less than 3 items. This can avoid wasting 4 icons. For issues 2 and 3, we can delay the history sync until the first tab is closed in a session. BUG=721484, 721486 ========== to ========== Preserve recently closed items in the JumpList until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. Thanks to deferring the TopSites sync, the JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ==========
Description was changed from ========== Preserve recently closed items in the JumpList until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. Thanks to deferring the TopSites sync, the JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ========== to ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. Thanks to deferring the TopSites sync, the JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ==========
Description was changed from ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. Thanks to deferring the TopSites sync, the JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ========== to ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. Thanks to deferring the TopSites sync, the JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ==========
Description was changed from ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. Thanks to deferring the TopSites sync, the JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ========== to ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. The JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Greg, thanks a lot for your suggestions on CL description. I was amazed by your succinct example description, so I borrowed most of it. There is only one point not covered by your example description. I added it up. https://codereview.chromium.org/2865133003/diff/220001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2865133003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:549: if (updates_to_skip_ > 0) { On 2017/05/18 09:35:14, grt (UTC plus 2) wrote: > please add > DCHECK(CalledOnValidThread()); > since this must run on the UI thread. same for DeferredTopSitesChanged, please. Done. https://codereview.chromium.org/2865133003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:554: // Force to do a TopSite history sync when closing a first tab in one session. On 2017/05/18 09:35:14, grt (UTC plus 2) wrote: > nit: "Force a TopSite..." Done.
Super! Thanks for the updates. LGTM.
lgtm w/ nit and comment about a simplification I'd like to see. https://codereview.chromium.org/2865133003/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2865133003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:536: // category from last session stay longer. s/stay/to stay/ https://codereview.chromium.org/2865133003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:674: // category. I think we should just do 5/3 always, no MulDiv either way. If one category has less we just don't get to 9, that's it.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (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...
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 #7 (id:300001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Gab, I've addressed your comments in the new patch set. Thanks! https://codereview.chromium.org/2865133003/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2865133003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:536: // category from last session stay longer. On 2017/05/19 15:13:26, gab wrote: > s/stay/to stay/ Done. https://codereview.chromium.org/2865133003/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:674: // category. On 2017/05/19 15:13:27, gab wrote: > I think we should just do 5/3 always, no MulDiv either way. If one category has > less we just don't get to 9, that's it. Done. I also made some other changes accordingly.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2865133003/#ps320001 (title: "Git pull and fix merge conflicts")
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": 1495496180462650,
"parent_rev": "1ba8b6855fc458da0a571203c2b4b9233f756e22", "commit_rev":
"55c71f0ff307dfc3b3e561a2f72e90bb3df05393"}
Message was sent while issue was closed.
Description was changed from ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. The JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 ========== to ========== Defer syncing TopSites with history until the first tab closure This change defers syncing TopSites with history until the first tab closure and ignores any TopSites updates that arrive before that time. It also makes the category titles ever-present in the JumpList and removes the old balancing strategy that used to show more most visited items when there were fewer than 3 recently closed items. The JumpList will now retain its items from a previous launch until the first tab closure, making it more useful early on in the browser's lifetime. Also, there will be no unnecessary TopSites syncs now if an user launches Chrome but shuts it down before opening any tabs. After that initial tab closure, all old recently closed items are lost as it is not possible to query Windows for them. Removal of the balancing strategy trims out waste resulting from fetching 9 items initially, which are then trimmed down as tabs are closed. BUG=721484, 721486 Review-Url: https://codereview.chromium.org/2865133003 Cr-Commit-Position: refs/heads/master@{#473747} Committed: https://chromium.googlesource.com/chromium/src/+/55c71f0ff307dfc3b3e561a2f72e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001) as https://chromium.googlesource.com/chromium/src/+/55c71f0ff307dfc3b3e561a2f72e... |
