Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717813002:
Not update jumplist icons when deletion of JumplistIcon folder fails (Closed)
DescriptionNot update jumplist icons when deletion of JumplistIcon folder fails
According to UMA, the deletion of JumplistIcons folder fails for a small
portion of Chrome users. In this case, we should skip updating the jumplist
icons which will bloat the folder. In addition, there is no need to run
base::CreateDirectory() trying to create the same folder.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2717813002
Cr-Commit-Position: refs/heads/master@{#454674}
Committed: https://chromium.googlesource.com/chromium/src/+/eae9a2c34f7315b1aaa17bf265c879681106c90a
Patch Set 1 #Patch Set 2 : Skip updating jumplist icons if deletion of src folder fails. #
Total comments: 2
Patch Set 3 : Exit early rather than using a flag to cancel rest of the work. #Messages
Total messages: 42 (31 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...
Description was changed from ========== Don't re-create the directory if its deletion fails. BUG=40407,179576 ========== to ========== Not re-create JumplistIcons folder if its deletion fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to run base::CreateDirectory() trying to create the same folder. Adding this logic can help save some computation effort in this case. BUG=40407,179576 ==========
Description was changed from ========== Not re-create JumplistIcons folder if its deletion fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to run base::CreateDirectory() trying to create the same folder. Adding this logic can help save some computation effort in this case. BUG=40407,179576 ========== to ========== Not re-create folder JumplistIcon if its deletion fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to run base::CreateDirectory() trying to create the same folder. Adding this logic can help save some computation effort in this case. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
chengx@chromium.org changed reviewers: + gab@chromium.org
Hey gab@, I made a small change to jumplist.CC. PTAL~
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Is it really worth it to save that tiny bit of computation in an edge case where things are already messed up?
On 2017/02/26 18:07:58, gab (UTC plus 11 Tokyo) wrote: > Is it really worth it to save that tiny bit of computation in an edge case where > things are already messed up? IMHO, it is a better folder operation logic although the improvement it brings seems tiny. I simply want to fix a nit I introduced in my previous CL. Thanks!
On 2017/02/27 04:41:53, chengx wrote: > On 2017/02/26 18:07:58, gab (UTC plus 11 Tokyo) wrote: > > Is it really worth it to save that tiny bit of computation in an edge case > where > > things are already messed up? > > IMHO, it is a better folder operation logic although the improvement it brings > seems tiny. I simply want to fix a nit I introduced in my previous CL. Thanks! Ok but isn't weird to say that on failure we keep going at all?
On 2017/02/27 21:43:08, gab (UTC plus 9 Tokyo) wrote: > On 2017/02/27 04:41:53, chengx wrote: > > On 2017/02/26 18:07:58, gab (UTC plus 11 Tokyo) wrote: > > > Is it really worth it to save that tiny bit of computation in an edge case > > where > > > things are already messed up? > > > > IMHO, it is a better folder operation logic although the improvement it brings > > seems tiny. I simply want to fix a nit I introduced in my previous CL. Thanks! > > Ok but isn't weird to say that on failure we keep going at all? It's just saying that on failure of deleting |icon_dir|, we don't bother trying to create the folder again because it is there already. I agree that we need to do something if |icon_dir| fails to delete itself, probably stop updating the jumplist icons to avoid bloating this folder. I am not sure yet, but if I decide to do so, I would probably use another CL.
On 2017/02/27 22:57:34, chengx wrote: > On 2017/02/27 21:43:08, gab (UTC plus 9 Tokyo) wrote: > > On 2017/02/27 04:41:53, chengx wrote: > > > On 2017/02/26 18:07:58, gab (UTC plus 11 Tokyo) wrote: > > > > Is it really worth it to save that tiny bit of computation in an edge case > > > where > > > > things are already messed up? > > > > > > IMHO, it is a better folder operation logic although the improvement it > brings > > > seems tiny. I simply want to fix a nit I introduced in my previous CL. > Thanks! > > > > Ok but isn't weird to say that on failure we keep going at all? > > It's just saying that on failure of deleting |icon_dir|, we don't bother trying > to create the folder again because it is there already. I agree that we need to > do something if |icon_dir| fails to delete itself, probably stop updating the > jumplist icons to avoid bloating this folder. I am not sure yet, but if I decide > to do so, I would probably use another CL. Right, I'm saying we don't we just do that instead?
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 ========== Not re-create folder JumplistIcon if its deletion fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to run base::CreateDirectory() trying to create the same folder. Adding this logic can help save some computation effort in this case. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to update the jumplist icons which will bloat the folder, in addition to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to update the jumplist icons which will bloat the folder, in addition to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to update the jumplist icons which will bloat the folder, in addition to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, there is no need to update the jumplist icons which will bloat the folder, in addition to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/28 02:02:33, gab (UTC plus 9 Tokyo) wrote: > On 2017/02/27 22:57:34, chengx wrote: > > On 2017/02/27 21:43:08, gab (UTC plus 9 Tokyo) wrote: > > > On 2017/02/27 04:41:53, chengx wrote: > > > > On 2017/02/26 18:07:58, gab (UTC plus 11 Tokyo) wrote: > > > > > Is it really worth it to save that tiny bit of computation in an edge > case > > > > where > > > > > things are already messed up? > > > > > > > > IMHO, it is a better folder operation logic although the improvement it > > brings > > > > seems tiny. I simply want to fix a nit I introduced in my previous CL. > > Thanks! > > > > > > Ok but isn't weird to say that on failure we keep going at all? > > > > It's just saying that on failure of deleting |icon_dir|, we don't bother > trying > > to create the folder again because it is there already. I agree that we need > to > > do something if |icon_dir| fails to delete itself, probably stop updating the > > jumplist icons to avoid bloating this folder. I am not sure yet, but if I > decide > > to do so, I would probably use another CL. > > Right, I'm saying we don't we just do that instead? Alright, done!
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable "folder_operation_status" will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable |folder_operation_status| will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable |folder_operation_status| will be removed eventually, a new variable is used to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable |folder_operation_status| will be removed eventually, a new variable is created to record the deletion status of folder JumplistIcons. BUG=40407,179576 ==========
Behavior lgtm w/ code done as in suggestion Thanks! Gab https://codereview.chromium.org/2717813002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2717813002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:289: Prefer an early exit (return;) above instead of capturing a bool and cancelling work below. To make sure you still invoke UMA, here's a neat trick: On line 266: uint32_t folder_operation_status = FolderOperationResult::SUCCESS; base::ScopedClosureRunner log_operation_status_when_done( base::Bind([](uint32_t* folder_operation_status_ptr) -> { UMA_HISTOGRAM_ENUMERATION("WinJumplist.DetailedFolderResults", *folder_operation_status_ptr, FolderOperationResult::END); }, base::Unretained(&folder_operation_status)));
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 ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. As variable |folder_operation_status| will be removed eventually, a new variable is created to record the deletion status of folder JumplistIcons. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. BUG=40407,179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2717813002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2717813002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:289: On 2017/03/03 07:35:01, gab (OOO until Mar 13) wrote: > Prefer an early exit (return;) above instead of capturing a bool and cancelling > work below. > > To make sure you still invoke UMA, here's a neat trick: > > On line 266: > > uint32_t folder_operation_status = FolderOperationResult::SUCCESS; > base::ScopedClosureRunner log_operation_status_when_done( > base::Bind([](uint32_t* folder_operation_status_ptr) -> { > UMA_HISTOGRAM_ENUMERATION("WinJumplist.DetailedFolderResults", > *folder_operation_status_ptr, > FolderOperationResult::END); > }, base::Unretained(&folder_operation_status))); Sounds good. I have updated accordingly. Btw, I think base::Bind([](uint32_t* folder_operation_status_ptr) -> { should be base::Bind([](uint32_t* folder_operation_status_ptr) {
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 Link to the patchset: https://codereview.chromium.org/2717813002/#ps40001 (title: "Exit early rather than using a flag to cancel rest of the work.")
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": 1488575268829760,
"parent_rev": "6265c08c4c295251e8530fcedd0885771d6698f6", "commit_rev":
"eae9a2c34f7315b1aaa17bf265c879681106c90a"}
Message was sent while issue was closed.
Description was changed from ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. BUG=40407,179576 ========== to ========== Not update jumplist icons when deletion of JumplistIcon folder fails According to UMA, the deletion of JumplistIcons folder fails for a small portion of Chrome users. In this case, we should skip updating the jumplist icons which will bloat the folder. In addition, there is no need to run base::CreateDirectory() trying to create the same folder. BUG=40407,179576 Review-Url: https://codereview.chromium.org/2717813002 Cr-Commit-Position: refs/heads/master@{#454674} Committed: https://chromium.googlesource.com/chromium/src/+/eae9a2c34f7315b1aaa17bf265c8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/eae9a2c34f7315b1aaa17bf265c8... |
