Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848763002:
Re-enable UMA metric WinJumplist.CreateIconFilesCount (Closed)
DescriptionRe-enable UMA metric WinJumplist.CreateIconFilesCount
This UMA metric tracks the number of icons requested to create per
JumpList update. The actual number of icons created may be less if it
fails to create certain icons due to random reasons.
Since I've landed some CLs reducing this number and will try to
further reduce it, I think this metric should be re-enabled to track
the progress.
BUG=40407, 179576, 715902
Review-Url: https://codereview.chromium.org/2848763002
Cr-Commit-Position: refs/heads/master@{#468367}
Committed: https://chromium.googlesource.com/chromium/src/+/4f731454df3bdd7076668f4c906dc1eaab38e8ad
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Fix comment. #
Messages
Total messages: 30 (22 generated)
Description was changed from ========== Re-enable WinJumplist.CreateIconFilesCount metric BUG=40407, 179576 ========== to ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the amount of icons created per JumpList update. Since I've landed some CLs reducing this amount and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576 ==========
Description was changed from ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the amount of icons created per JumpList update. Since I've landed some CLs reducing this amount and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576 ========== to ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the amount of icons created per JumpList update. Since I've landed some CLs reducing this amount and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org, isherman@chromium.org
PTAL this small change, 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...
https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:118: return false; this will leak the temp file. should you not delete it in this failure case? https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:243: // Record the amout of icons created per JumpList update. nit: amount -> number https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:266: icons_created += std::min(most_visited_pages.size(), most_visited_items); is the intention to report the number of files successfully created? if so, there are error paths in CreateIconFiles that may result in fewer files being created than requested. how about making CreateIconFiles return the number of files it actually created so that the real value can be used directly?
Description was changed from ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the amount of icons created per JumpList update. Since I've landed some CLs reducing this amount and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ========== to ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons created per JumpList update. Since I've landed some CLs reducing this amount and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ==========
Description was changed from ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons created per JumpList update. Since I've landed some CLs reducing this amount and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ========== to ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons created per JumpList update. Since I've landed some CLs reducing this number and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ==========
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 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 ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons created per JumpList update. Since I've landed some CLs reducing this number and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ========== to ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons requested to create per JumpList update. The actual number of icons created may be less if it fails to create certain icons due to random reasons. Since I've landed some CLs reducing this number and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ==========
PATL at the new patch set. Thanks! https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:118: return false; On 2017/04/28 07:17:00, grt (UTC plus 2) wrote: > this will leak the temp file. should you not delete it in this failure case? Yes, it should be deleted here right away rather than in the next JumpList update. I'll use another small CL for this. Thanks for catching this bug! https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:243: // Record the amout of icons created per JumpList update. On 2017/04/28 07:17:00, grt (UTC plus 2) wrote: > nit: amount -> number Done. https://codereview.chromium.org/2848763002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:266: icons_created += std::min(most_visited_pages.size(), most_visited_items); On 2017/04/28 07:17:00, grt (UTC plus 2) wrote: > is the intention to report the number of files successfully created? if so, > there are error paths in CreateIconFiles that may result in fewer files being > created than requested. how about making CreateIconFiles return the number of > files it actually created so that the real value can be used directly? Actually, I only care about the number of icons that Chrome requests to create. The number of icons created may be fewer than that of being requested, which can be due to various reasons. Some of them are even unrelated to Chrome. What I focus on is to reduce the request amount. I have renamed "icons_created" to "icons_to_create", and updated the CL description to better convey my intention. Sorry about the confusion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml lgtm
lgtm with a nit https://codereview.chromium.org/2848763002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2848763002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:243: // Record the number of icons created per JumpList update. nit: // Record the desired number of icons to create in this JumpList update.
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 fixed the nit in the new patch set. Thanks! https://codereview.chromium.org/2848763002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2848763002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:243: // Record the number of icons created per JumpList update. On 2017/05/01 08:44:38, grt (UTC plus 2) wrote: > nit: > // Record the desired number of icons to create in this JumpList update. Done.
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 isherman@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2848763002/#ps40001 (title: "Fix comment.")
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": 40001, "attempt_start_ts": 1493663631771730,
"parent_rev": "d2bad93f4d4f3417ce867dcd5631983cb2e36534", "commit_rev":
"4f731454df3bdd7076668f4c906dc1eaab38e8ad"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons requested to create per JumpList update. The actual number of icons created may be less if it fails to create certain icons due to random reasons. Since I've landed some CLs reducing this number and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 ========== to ========== Re-enable UMA metric WinJumplist.CreateIconFilesCount This UMA metric tracks the number of icons requested to create per JumpList update. The actual number of icons created may be less if it fails to create certain icons due to random reasons. Since I've landed some CLs reducing this number and will try to further reduce it, I think this metric should be re-enabled to track the progress. BUG=40407, 179576, 715902 Review-Url: https://codereview.chromium.org/2848763002 Cr-Commit-Position: refs/heads/master@{#468367} Committed: https://chromium.googlesource.com/chromium/src/+/4f731454df3bdd7076668f4c906d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4f731454df3bdd7076668f4c906d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
