Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2941323002:
Delete the right JumpList icons conditional on shell notification (Closed)
|
Created:
3 years, 6 months ago by chengx Modified:
3 years, 5 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, brucedawson Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelete the right JumpList icons conditional on shell notification
This CL decides which JumpList icons to delete based on the results of
shell notification. That is, if the shell notification fails or times
out, we delete the newly created icons as the old icons are still
going to be used by the shell. Previously, we delete old icons before
notifying the shell about the update. If the notification fails, the
old JumpList is still used but their icons are already gone. In this
case, there will be no icons showing up but the background color. By
moving the icon deletion step after shell notification and introducing
the dependency, this issue is fixed.
In addition, this change makes it possible to cancel more updates
where shell notification is detected to be slow. In more details, if
AddCustomCategory API call in the shell notification process times
out, we can now abort this update to avoid calling CommitUpdate which
is very likely to be slow as well. This was impossible before this CL.
BUG=40407, 716115, 736530
Review-Url: https://codereview.chromium.org/2941323002
Cr-Commit-Position: refs/heads/master@{#483439}
Committed: https://chromium.googlesource.com/chromium/src/+/531f38bd194f9d8a0dd98d78a15f82947d742cec
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update members of UpdateResults, use recently_closed_* rather than recent_closed_* #
Total comments: 10
Patch Set 3 : A much easier way of doing this, address comments #
Total comments: 18
Patch Set 4 : Address comments, record timeout info for failed steps in shell notification #
Total comments: 2
Patch Set 5 : Revert timeout related changes #
Total comments: 8
Patch Set 6 : Address comments. #Messages
Total messages: 93 (79 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:160001) 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...
Description was changed from ========== Delete icon files after notifying the Shell BUG= ========== to ========== Delete right icon files based on the commit result of JumpList update BUG=40407, 716115 ==========
Description was changed from ========== Delete right icon files based on the commit result of JumpList update BUG=40407, 716115 ========== to ========== Delete right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce a map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ==========
Description was changed from ========== Delete right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce a map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ========== to ========== Delete right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce a "IconAssociation" map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Delete right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce a "IconAssociation" map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce a "IconAssociation" map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
PTAL, thanks! I renamed quite a few methods and variables to make them a bit more succinct.
FYI Bruce. I remember once you asked me if it's possible to delete icons after notifying the OS. After the jumplist massive performance bug is fixed, this is finally going to happen.
https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:556: auto update_results = base::MakeUnique<UpdateResults>(); meta comment: this object does more than just carry results back to the JumpList instance. what differentiates the two lists above from the data being put into |update_results|? perhaps this should become UpdateTransaction if it has inputs and outputs. also, does it make sense for JumpList to create an instance of this struct in its ctor and have that one instance get bounced between the JL and the updating thread rather than creating it anew for each update? https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:92: URLIconCache recent_closed_icons; i prefer "recently" over "recent" since these are for the tabs that were recently closed. could you revert the variable renamings? it makes the CL harder to review.
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #3 (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...
Patchset #2 (id:240001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've addressed the comments in the new patch set. PTAL, thanks! https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:556: auto update_results = base::MakeUnique<UpdateResults>(); On 2017/06/20 10:29:18, grt (UTC plus 2) wrote: > meta comment: this object does more than just carry results back to the JumpList > instance. what differentiates the two lists above from the data being put into > |update_results|? perhaps this should become UpdateTransaction if it has inputs > and outputs. The two lists above, i.e., *_pages_, are not updated by the updating thread, so they are merely inputs. I've also removed *_should_update from |update_results| as they are also read-only in the updating thread. I added them there there because I wanted to reduce the number of params passed by a few methods. However, this seems to be confusing. The data being put into |update_results| now include *_icons and *_icons_assoc. They are all updated by the updating thread, which means they are indeed "update results". So I will keep the struct's name as UpdateResults. > also, does it make sense for JumpList to create an instance of this struct in > its ctor and have that one instance get bounced between the JL and the updating > thread rather than creating it anew for each update? I feel it reads better with the current impl though. https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:92: URLIconCache recent_closed_icons; On 2017/06/20 10:29:18, grt (UTC plus 2) wrote: > i prefer "recently" over "recent" since these are for the tabs that were > recently closed. could you revert the variable renamings? it makes the CL harder > to review. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:210003) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:556: auto update_results = base::MakeUnique<UpdateResults>(); On 2017/06/20 19:33:59, chengx wrote: > On 2017/06/20 10:29:18, grt (UTC plus 2) wrote: > > meta comment: this object does more than just carry results back to the > JumpList > > instance. what differentiates the two lists above from the data being put into > > |update_results|? perhaps this should become UpdateTransaction if it has > inputs > > and outputs. > > The two lists above, i.e., *_pages_, are not updated by the updating thread, so > they are merely inputs. I've also removed *_should_update from |update_results| > as they are also read-only in the updating thread. I added them there there > because I wanted to reduce the number of params passed by a few methods. > However, this seems to be confusing. > > The data being put into |update_results| now include *_icons and *_icons_assoc. > They are all updated by the updating thread, which means they are indeed "update > results". So I will keep the struct's name as UpdateResults. So they're in-out params, yes? I can see how they are results in the sense that they are the things taken away from the update and applied back to the JumpList instance, but they're not strictly results. Reducing the number of values that are bound in the callback is worthwhile. I think it's still worth considering calling it an UpdateTransaction and using doc comments and layout to make it clear which fields are in params, which are in-out params, and which (if any) are out params. > > also, does it make sense for JumpList to create an instance of this struct in > > its ctor and have that one instance get bounced between the JL and the > updating > > thread rather than creating it anew for each update? > > I feel it reads better with the current impl though. I'm not sure that there's a big difference. There may be less code and fewer copies of data being made if ownership of one instance of some struct bounces back and forth. Maybe it wouldn't make sense for all fields to be within this struct though. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:566: auto* update_results_raw = update_results.get(); wdyt of moving the first Bind out here now so that you don't have a naked pointer? https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:711: icons_created += SafeCreateIconFiles( if i understand correctly, here and on line 719 you want to create the new icon files that, if all goes well, will become *the* icon files for the jumplist. any failure between here and the end of this function should result in the new files being deleted. rather than having repeated "delete the new files" code blocks at the early exit points (lines 751, 760, and 789 in case of error), one way to slice this is to introduce the idea of a "transaction" that is in a pending state until it is committed. i'm imagining that an IconCache is its own object with a few methods, one of which is BeginTransaction, and returns some object that is held on the stack. anything stack unwind will lead to its destruction, which will roll-back any icon creation that took place. this transaction object has an additional Commit method that, when called, causes the new icons to persist and deletes the old icons that are no longer relevant. this will make the code here considerably cleaner. i suggest that you spend some time thinking about what some simple IconCache and IconCache::Transaction classes might look like. i think this would lend itself to some nice encapsulation and would be easily unit-tested. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:847: for (auto iter = icon_assoc->begin(); iter != icon_assoc->end(); ++iter) std::for_each(std::begin(*icon_assoc), std::end(*icon_assoc), [] (auto &value) { value.second = JumpListVersion::kCurrent; }); or for (auto& value : *icon_assoc) value.second = JumpListVersion::kCurrent; https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:71: enum class JumpListVersion { kCurrent, kNext, kShared }; // Indicates whether a JumpList icon file is used by the current jumplist, the next jumplist (following an in-progress update), or both. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:74: using URLIconCache = base::flat_map<std::string, base::FilePath>; rather than introducing another container to hold the JumpListVersion, can you not make this cache map an URL to a FilePath+Version?
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #3 (id:310001) 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: This issue passed the CQ dry run.
Description was changed from ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce a "IconAssociation" map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce an "IconAssociation" map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ==========
Patchset #4 (id:350001) has been deleted
Patchset #4 (id:370001) has been deleted
Patchset #4 (id:390001) has been deleted
Patchset #3 (id:330001) has been deleted
Patchset #4 (id:430001) has been deleted
Patchset #3 (id:410001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #3 (id:450001) 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 ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. Implementation-wise, we introduce an "IconAssociation" map which records the association between the icons and the JumpList versions. An icon file can associate with only the current JumpList, or only the updated JumpList, or both versions if it's one of the overlapping icons of two JumpLists. If the shell notification succeeds, we delete the icons associated with only the current JumpList; otherwise we delete the icons associated only with the updated JumpList. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ==========
Description was changed from ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ==========
Description was changed from ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. As a result, if the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ==========
Description was changed from ========== Delete the right JumpList icons based on the results of shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ==========
Hi Greg, PTAL the new patch set, thanks! Inspired by your comments, I realized there is a much easier and cleaner way to delete the right icons. I introduced a wrapper method to do "new JumpList creation + shell notification" only. After this, there is only place to delete icons. IMHO, I don't think creating another transaction-related class is that necessary for the new patch set, which is much cleaner than the previous ones. https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/180001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:556: auto update_results = base::MakeUnique<UpdateResults>(); On 2017/06/21 10:57:29, grt (UTC plus 2) wrote: > On 2017/06/20 19:33:59, chengx wrote: > > On 2017/06/20 10:29:18, grt (UTC plus 2) wrote: > > > meta comment: this object does more than just carry results back to the > > JumpList > > > instance. what differentiates the two lists above from the data being put > into > > > |update_results|? perhaps this should become UpdateTransaction if it has > > inputs > > > and outputs. > > > > The two lists above, i.e., *_pages_, are not updated by the updating thread, > so > > they are merely inputs. I've also removed *_should_update from > |update_results| > > as they are also read-only in the updating thread. I added them there there > > because I wanted to reduce the number of params passed by a few methods. > > However, this seems to be confusing. > > > > The data being put into |update_results| now include *_icons and > *_icons_assoc. > > They are all updated by the updating thread, which means they are indeed > "update > > results". So I will keep the struct's name as UpdateResults. > > So they're in-out params, yes? I can see how they are results in the sense that > they are the things taken away from the update and applied back to the JumpList > instance, but they're not strictly results. Reducing the number of values that > are bound in the callback is worthwhile. I think it's still worth considering > calling it an UpdateTransaction and using doc comments and layout to make it > clear which fields are in params, which are in-out params, and which (if any) > are out params. Yes, they're in-out params. I've renamed the enum class to UpdateTransaction and added the comments. > > > also, does it make sense for JumpList to create an instance of this struct > in > > > its ctor and have that one instance get bounced between the JL and the > > updating > > > thread rather than creating it anew for each update? > > > > I feel it reads better with the current impl though. > > I'm not sure that there's a big difference. There may be less code and fewer > copies of data being made if ownership of one instance of some struct bounces > back and forth. Maybe it wouldn't make sense for all fields to be within this > struct though. IMHO, I don't think it makes a big difference given that there are only 4 fields (including 2 bools) in this struct now. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:566: auto* update_results_raw = update_results.get(); On 2017/06/21 10:57:29, grt (UTC plus 2) wrote: > wdyt of moving the first Bind out here now so that you don't have a naked > pointer? Done. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:711: icons_created += SafeCreateIconFiles( On 2017/06/21 10:57:29, grt (UTC plus 2) wrote: > if i understand correctly, here and on line 719 you want to create the new icon > files that, if all goes well, will become *the* icon files for the jumplist. any > failure between here and the end of this function should result in the new files > being deleted. rather than having repeated "delete the new files" code blocks at > the early exit points (lines 751, 760, and 789 in case of error), one way to > slice this is to introduce the idea of a "transaction" that is in a pending > state until it is committed. i'm imagining that an IconCache is its own object > with a few methods, one of which is BeginTransaction, and returns some object > that is held on the stack. anything stack unwind will lead to its destruction, > which will roll-back any icon creation that took place. this transaction object > has an additional Commit method that, when called, causes the new icons to > persist and deletes the old icons that are no longer relevant. this will make > the code here considerably cleaner. > > i suggest that you spend some time thinking about what some simple IconCache and > IconCache::Transaction classes might look like. i think this would lend itself > to some nice encapsulation and would be easily unit-tested. Thanks a lot for the suggestion and your understanding to my code is absolutely correct. I agree the transaction concept fits well here. But to be honest, I don't think it requires so much modification as what you suggested. I have created a wrapper function called CreateNewJumpListAndNotifyOS to create a new JumpList and notify OS. The icon deletion is separated from this new API, so that there is only place for icon deletion now. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:847: for (auto iter = icon_assoc->begin(); iter != icon_assoc->end(); ++iter) On 2017/06/21 10:57:29, grt (UTC plus 2) wrote: > std::for_each(std::begin(*icon_assoc), std::end(*icon_assoc), > [] (auto &value) { value.second = JumpListVersion::kCurrent; }); > > or > > for (auto& value : *icon_assoc) > value.second = JumpListVersion::kCurrent; Done. I'll go with the second method. https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:71: enum class JumpListVersion { kCurrent, kNext, kShared }; On 2017/06/21 10:57:29, grt (UTC plus 2) wrote: > // Indicates whether a JumpList icon file is used by the current jumplist, the > next jumplist (following an in-progress update), or both. I've removed this enum class as it's no longer needed. Thanks for the suggestion though! https://codereview.chromium.org/2941323002/diff/290001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:74: using URLIconCache = base::flat_map<std::string, base::FilePath>; On 2017/06/21 10:57:29, grt (UTC plus 2) wrote: > rather than introducing another container to hold the JumpListVersion, can you > not make this cache map an URL to a FilePath+Version? Done. After a second thought, I think using the original URLIconCache alone is enough for the purpose of this CL. So I removed IconAssociation and keep URLIconCache unchanged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this is much easier to understand. thanks. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:568: most_visited_should_update, recently_closed_should_update, is it possible for either this->*_should_update_ to change state while the update is running? i thought that with the latest design, update_in_progress_ causes the handling of inbound notifications to be deferred until the running update completes. if this is correct, there's no need to bind these two bools into the callback -- they can be read from |this| in OnRunUpdateCompletion. perhaps add a DCHECK(!update_in_progress_) to OnDelayTimer, ProcessTabRestoreServiceNotification, and OnMostVisitedURLsAvailable to make this even more clear. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:596: most_visited_icons_.swap(update_results->most_visited_icons); nit: swap() used to be the efficient way to move stuff from |here| to |there| when you didn't need to access |here| any longer. now that we have move assignment operators, we have the same efficiency with: most_visited_icons_ = std::move(update_results->most_visited_icons); the only difference really being the point at which the old contents of |there| are destroyed. what do you think of replacing all occurrences of swap() in here with move assignment? i think it more clearly expresses the intent than swap(). https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:753: // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates. this doesn't abort the current update, as the current update is complete at this point -- it just skips the next N updates. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:768: void JumpList::RunUpdateJumpList( nit: move this above CreateNewJumpListAndNotifyOS? https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:777: if (!JumpListUpdater::IsEnabled()) is this needed? can IsEnabled() change from true to false within a given process? in other words, is it possible for RunUpdateJumpList to be called if IsEnabled() was false? this seems awfully late in the game to check for enablement if the state can't flip during execution. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:810: int icons_created = 0; nit: how about cleaning this up a tiny bit: // Clear the JumpList icon folder at |icon_dir| and the cache when // 1) |icon_cur| is empty. This happens when "Most visited" or "Recently // closed" category updates for the 1st time after Chrome is launched. // 2) The number of icons in |icon_dir| has exceeded the limit. if (icon_cur->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) { DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit); icon_cur->clear(); icon_next->clear(); // Create new icons only when the directory exists and is empty. if (!base::CreateDirectory(icon_dir) || !base::IsDirectoryEmpty(icon_dir)) return 0; } else if (!base::CreateDirectory(icon_dir)) { return 0; } return CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next); https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:838: URLIconCache* icon_cur, nit: pass |icon_cur| by constref https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:170: // update based on |update_results|, the flags |most_visited_should_update| nit: update_results -> update_transaction throughout https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:199: // and notifies OS. nit: "the OS."
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 #4 (id:490001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:568: most_visited_should_update, recently_closed_should_update, On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > is it possible for either this->*_should_update_ to change state while the > update is running? i thought that with the latest design, update_in_progress_ > causes the handling of inbound notifications to be deferred until the running > update completes. if this is correct, there's no need to bind these two bools > into the callback -- they can be read from |this| in OnRunUpdateCompletion. > > perhaps add a DCHECK(!update_in_progress_) to OnDelayTimer, > ProcessTabRestoreServiceNotification, and OnMostVisitedURLsAvailable to make > this even more clear. You are right. There is no need to bind these two bools into the callback for OnRunUpdateCompletion. I've also added the DCHECK in various places. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:596: most_visited_icons_.swap(update_results->most_visited_icons); On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > nit: swap() used to be the efficient way to move stuff from |here| to |there| > when you didn't need to access |here| any longer. now that we have move > assignment operators, we have the same efficiency with: > most_visited_icons_ = std::move(update_results->most_visited_icons); > the only difference really being the point at which the old contents of |there| > are destroyed. what do you think of replacing all occurrences of swap() in here > with move assignment? i think it more clearly expresses the intent than swap(). Done. I think std::move better conveys the intent here as what we need here is merely an efficient assignment, rather than exchanging content. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:753: // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates. On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > this doesn't abort the current update, as the current update is complete at this > point -- it just skips the next N updates. Done. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:768: void JumpList::RunUpdateJumpList( On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > nit: move this above CreateNewJumpListAndNotifyOS? Done. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:777: if (!JumpListUpdater::IsEnabled()) On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > is this needed? can IsEnabled() change from true to false within a given > process? in other words, is it possible for RunUpdateJumpList to be called if > IsEnabled() was false? this seems awfully late in the game to check for > enablement if the state can't flip during execution. I think it should be moved to the very beginning of the jumplist code. I prefer to use another small CL for this though. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:810: int icons_created = 0; On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > nit: how about cleaning this up a tiny bit: > > // Clear the JumpList icon folder at |icon_dir| and the cache when > // 1) |icon_cur| is empty. This happens when "Most visited" or "Recently > // closed" category updates for the 1st time after Chrome is launched. > // 2) The number of icons in |icon_dir| has exceeded the limit. > if (icon_cur->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) { > DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit); > icon_cur->clear(); > icon_next->clear(); > // Create new icons only when the directory exists and is empty. > if (!base::CreateDirectory(icon_dir) || !base::IsDirectoryEmpty(icon_dir)) > return 0; > } else if (!base::CreateDirectory(icon_dir)) { > return 0; > } > > return CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next); Done. Thanks for the suggestion! https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:838: URLIconCache* icon_cur, On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > nit: pass |icon_cur| by constref Done. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:170: // update based on |update_results|, the flags |most_visited_should_update| On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > nit: update_results -> update_transaction throughout Done. https://codereview.chromium.org/2941323002/diff/470001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:199: // and notifies OS. On 2017/06/23 09:36:14, grt (UTC plus 2) wrote: > nit: "the OS." Done.
Description was changed from ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115 ==========
Patchset #5 (id:530001) has been deleted
Patchset #4 (id:510001) has been deleted
Description was changed from ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115, 736530 ==========
Description was changed from ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115, 736530 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. Moreover, for any shell notification where there's a step times out, we now not only abort this jumplist update, but also cancel the next 10 updates as a penalty. This avoids consecutive jumplist shell notifications which not only fail but also time out. BUG=40407, 716115, 736530 ==========
Description was changed from ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. Moreover, for any shell notification where there's a step times out, we now not only abort this jumplist update, but also cancel the next 10 updates as a penalty. This avoids consecutive jumplist shell notifications which not only fail but also time out. BUG=40407, 716115, 736530 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. Moreover, for any shell notification where there's a step times out, we now not only abort this jumplist update as before, but also cancel the next 10 updates as a penalty. This avoids consecutive jumplist shell notifications which not only fail but also time out. BUG=40407, 716115, 736530 ==========
this looks good, but i wonder about the cl description. doesn't the current code already do the "skip the next 10" thing? https://codereview.chromium.org/2941323002/diff/550001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/550001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:82: base::TimeDelta::FromMilliseconds(300); is this change worth mentioning in the CL description? why drop the timeout here?
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 ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. Moreover, for any shell notification where there's a step times out, we now not only abort this jumplist update as before, but also cancel the next 10 updates as a penalty. This avoids consecutive jumplist shell notifications which not only fail but also time out. BUG=40407, 716115, 736530 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115, 736530 ==========
PTAL, thanks! On 2017/06/24 20:40:28, grt (UTC plus 2) wrote: > this looks good, but i wonder about the cl description. doesn't the current code > already do the "skip the next 10" thing? Sorry about the confusing CL description. After a second thought, I decide to revert the timeout related changes as it's unrelated to this CL title at all. I will use another small CL for the timeout related change and have a more accurate CL description there. https://codereview.chromium.org/2941323002/diff/550001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/550001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:82: base::TimeDelta::FromMilliseconds(300); On 2017/06/24 20:40:28, grt (UTC plus 2) wrote: > is this change worth mentioning in the CL description? why drop the timeout > here? Originally we were monitoring the total runtime of two AddCustomCategory calls. In the previous patch set, we only monitored the runtime of one call. So I dropped it from 500 ms to 300 ms. Since I have reverted the timeout related code change for now, the timeout is not changed back to 500 ms
Patchset #5 (id:570001) has been deleted
lgtm with nits https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:79: // The maximum allowed time for updating the "most visited" category via i think the old version of this comment was more correct https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:557: // Make local copies of JumpList member variables and use them for an update. are any of these locals actually needed? https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:564: update_transaction->most_visited_icons = most_visited_icons_; to consider for another CL: does it make sense to only set the caches in the transaction when the respective *_should_update_ member is true? how about std::move to move the cache into the transaction to avoid a copy? https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:169: // Updates certain JumpList member variables and/or triggers a new JumpList this comment is a little misleading since triggering a new update isn't based on |update_transaction|. how about: Handles the completion of an update by incorporating its results in |update_transaction| back into this instance. Additionally, a new update is triggered as needed to process notifications that arrived while the now-completed update was running.
I've addressed all the comments in the new patch set. Thanks! https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:79: // The maximum allowed time for updating the "most visited" category via On 2017/06/27 07:45:29, grt (UTC plus 2) wrote: > i think the old version of this comment was more correct Done. https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:557: // Make local copies of JumpList member variables and use them for an update. On 2017/06/27 07:45:29, grt (UTC plus 2) wrote: > are any of these locals actually needed? No, they are not needed any more. I've removed them. https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:564: update_transaction->most_visited_icons = most_visited_icons_; On 2017/06/27 07:45:29, grt (UTC plus 2) wrote: > to consider for another CL: does it make sense to only set the caches in the > transaction when the respective *_should_update_ member is true? how about > std::move to move the cache into the transaction to avoid a copy? Agreed that caches need to be set only when the corresponding flags are true. Also agreed that move constructor should be used. I've made the changes in this CL. If you have any concerns with the changes I've made, simply leave a comment and I'll fix it using another CL. Thanks! https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2941323002/diff/590001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:169: // Updates certain JumpList member variables and/or triggers a new JumpList On 2017/06/27 07:45:29, grt (UTC plus 2) wrote: > this comment is a little misleading since triggering a new update isn't based on > |update_transaction|. how about: > > Handles the completion of an update by incorporating its results in > |update_transaction| back into this instance. Additionally, a new update is > triggered as needed to process notifications that arrived while the > now-completed update was running. I'll happily accept this suggestion! Thanks!
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.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2941323002/#ps610001 (title: "Address comments.")
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": 610001, "attempt_start_ts": 1498761562273810,
"parent_rev": "dfa8893486d98a6a3d9d4693481f18443da7ce3d", "commit_rev":
"531f38bd194f9d8a0dd98d78a15f82947d742cec"}
Message was sent while issue was closed.
Description was changed from ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115, 736530 ========== to ========== Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG=40407, 716115, 736530 Review-Url: https://codereview.chromium.org/2941323002 Cr-Commit-Position: refs/heads/master@{#483439} Committed: https://chromium.googlesource.com/chromium/src/+/531f38bd194f9d8a0dd98d78a15f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:610001) as https://chromium.googlesource.com/chromium/src/+/531f38bd194f9d8a0dd98d78a15f... |
