Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871323004:
Use TaskScheduler instead of SequencedWorkerPool in native_desktop_media_list.cc. (Closed)
DescriptionUse TaskScheduler instead of SequencedWorkerPool in native_desktop_media_list.cc.
SequencedWorkerPool is being deprecated in favor of TaskScheduler.
BUG=667892
R=tommi@chromium.org
Review-Url: https://codereview.chromium.org/2871323004
Cr-Commit-Position: refs/heads/master@{#472791}
Committed: https://chromium.googlesource.com/chromium/src/+/2e4ad5792c9ac3b3be8d53af5cc9ce7324a430b8
Patch Set 1 #
Total comments: 5
Patch Set 2 : USER_VISIBLE #Messages
Total messages: 22 (13 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Please take a look. This CL was generated automatically. The base::MayBlock() trait was specified for all call sites and the base::TaskPriority::BACKGROUND trait was specified for all non-test call sites. That may not be appropriate for your use case. Please verify that appropriate traits were used https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h If everything looks good, lgtm and CQ this CL. Otherwise, tell us what's wrong. Thanks. - FAQ - What if bots are red? Ignore this CL. A human will look at errors shortly. What if the shutdown behavior is not set explicitly (no base::TaskShutdownBehavior)? If shutdown behavior is important for a task, it should be set explicitly. It's not necessary to specify it if you're fine with either BLOCK_SHUTDOWN or SKIP_ON_SHUTDOWN. Note that the default shutdown behavior is BLOCK_SHUTDOWN in SequencedWorkerPool and SKIP_ON_SHUTDOWN in TaskScheduler. What if the task priority is not set explicitly (no base::TaskPriority)? When there is no explicit priority, the priority is inherited from the calling context (e.g. a task posted from a BACKGROUND task without an explicit priority will have a BACKGROUND priority).
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.
tommi@chromium.org changed reviewers: + sergeyu@chromium.org
I don't know this code so I'd rather let Sergey take a look https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/native_desktop_media_list.cc (right): https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/native_desktop_media_list.cc:241: {base::MayBlock(), base::TaskPriority::BACKGROUND}); Sergey - is BACKGROUND right or should it be USER_VISIBLE? (I guess it's not guaranteed to be visible at this point)
LGTM once BACKGROUND is changed to USER_VISIBLE https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/native_desktop_media_list.cc (right): https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/native_desktop_media_list.cc:241: {base::MayBlock(), base::TaskPriority::BACKGROUND}); On 2017/05/16 12:34:29, tommi (sloooow) - chröme wrote: > Sergey - is BACKGROUND right or should it be USER_VISIBLE? (I guess it's not > guaranteed to be visible at this point) Yes. This should be USER_VISIBLE. https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/native_desktop_media_list.cc:349: {base::MayBlock(), base::TaskPriority::BACKGROUND}), same here. It's user-visible
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/native_desktop_media_list.cc (right): https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/native_desktop_media_list.cc:241: {base::MayBlock(), base::TaskPriority::BACKGROUND}); On 2017/05/16 19:35:34, Sergey Ulanov wrote: > On 2017/05/16 12:34:29, tommi (sloooow) - chröme wrote: > > Sergey - is BACKGROUND right or should it be USER_VISIBLE? (I guess it's not > > guaranteed to be visible at this point) > > Yes. This should be USER_VISIBLE. Done. https://codereview.chromium.org/2871323004/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/native_desktop_media_list.cc:349: {base::MayBlock(), base::TaskPriority::BACKGROUND}), On 2017/05/16 19:35:33, Sergey Ulanov wrote: > same here. It's user-visible Done.
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2871323004/#ps20001 (title: "USER_VISIBLE")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org
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": 20001, "attempt_start_ts": 1495109811056910,
"parent_rev": "4cc64c6cdb93dcded1e43a51792a2bf7f89714d8", "commit_rev":
"2e4ad5792c9ac3b3be8d53af5cc9ce7324a430b8"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in native_desktop_media_list.cc. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=tommi@chromium.org ========== to ========== Use TaskScheduler instead of SequencedWorkerPool in native_desktop_media_list.cc. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=tommi@chromium.org Review-Url: https://codereview.chromium.org/2871323004 Cr-Commit-Position: refs/heads/master@{#472791} Committed: https://chromium.googlesource.com/chromium/src/+/2e4ad5792c9ac3b3be8d53af5cc9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2e4ad5792c9ac3b3be8d53af5cc9... |
