Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issue 2807883002: Separate JumpListIcons delete task and update task (Closed)

Created:
3 years, 8 months ago by chengx
Modified:
3 years, 8 months ago
Reviewers:
gab
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Separate JumpListIcons delete task and update task The JumpListIcons delete task deletes the existing icons in JumpListIcons, while the update task creates the new icons, writes them into JumpListIcons and notifies the shell. It is better to separate them so we can track the individual cost. Since a base::SingleThreadTaskRunner is used for posting these tasks, the tasks are always running in the correct order. This CL also adds a base::SequencedTaskRunner for JumpListIconsOld delete tasks as they are not required to run on a single thread. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2807883002 Cr-Commit-Position: refs/heads/master@{#463396} Committed: https://chromium.googlesource.com/chromium/src/+/daa4c6f41ab1da23326f2ca39c863a0384587ed6

Patch Set 1 #

Total comments: 4

Patch Set 2 : Update comments and better variables. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -31 lines) Patch
M chrome/browser/win/jumplist.h View 1 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/win/jumplist.cc View 1 4 chunks +20 lines, -28 lines 0 comments Download
M chrome/browser/win/jumplist_file_util.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/win/jumplist_file_util.cc View 1 chunk +22 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (19 generated)
chengx
I created this CL to separate the two subtasks in task A as mentioned in ...
3 years, 8 months ago (2017-04-10 18:24:14 UTC) #12
gab
lgtm w/ nit https://codereview.chromium.org/2807883002/diff/1/chrome/browser/win/jumplist.h File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2807883002/diff/1/chrome/browser/win/jumplist.h#newcode190 chrome/browser/win/jumplist.h:190: // delete JumpListIconsOld sequentially. Update comment. ...
3 years, 8 months ago (2017-04-10 19:33:12 UTC) #13
chengx
https://codereview.chromium.org/2807883002/diff/1/chrome/browser/win/jumplist.h File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2807883002/diff/1/chrome/browser/win/jumplist.h#newcode190 chrome/browser/win/jumplist.h:190: // delete JumpListIconsOld sequentially. On 2017/04/10 19:33:12, gab wrote: ...
3 years, 8 months ago (2017-04-10 21:09:14 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2807883002/20001
3 years, 8 months ago (2017-04-10 21:10:27 UTC) #21
commit-bot: I haz the power
3 years, 8 months ago (2017-04-10 21:20:10 UTC) #24
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/daa4c6f41ab1da23326f2ca39c86...

Powered by Google App Engine
This is Rietveld 408576698