Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875393002:
Use TaskScheduler instead of SequencedWorkerPool in session_manager_operation.cc.
DescriptionUse TaskScheduler instead of SequencedWorkerPool in session_manager_operation.cc.
SequencedWorkerPool is being deprecated in favor of TaskScheduler.
BUG=667892
R=bartfab@chromium.org
Patch Set 1 #
Total comments: 2
Messages
Total messages: 17 (7 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.
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in session_manager_operation.cc. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=bartfab@chromium.org ========== to ========== Use TaskScheduler instead of SequencedWorkerPool in session_manager_operation.cc. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=bartfab@chromium.org ==========
fdoray@chromium.org changed reviewers: + tnagel@chromium.org
On 2017/05/12 17:05:32, fdoray wrote: > 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). +tnagel@chromium.org (no reply from bartfab in one week)
On 2017/05/19 19:26:30, fdoray wrote: > On 2017/05/12 17:05:32, fdoray wrote: > > 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). > > mailto:+tnagel@chromium.org (no reply from bartfab in one week) +mnissler@chromium.org +pastarmovj@chromium.org PTAL (no reply from other reviewers in 2 weeks)
fdoray@chromium.org changed reviewers: + mnissler@chromium.org, pastarmovj@chromium.org
+mnissler@chromium.org +pastarmovj@chromium.org, (no reply from other reviewers in 2 weeks)
lgtm https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... File chrome/browser/chromeos/settings/session_manager_operation.cc (right): https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... chrome/browser/chromeos/settings/session_manager_operation.cc:165: {base::MayBlock(), base::TaskPriority::BACKGROUND, Nit: #include "base/task_scheduler/task_traits.h"
https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... File chrome/browser/chromeos/settings/session_manager_operation.cc (right): https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... chrome/browser/chromeos/settings/session_manager_operation.cc:165: {base::MayBlock(), base::TaskPriority::BACKGROUND, I think this should be base::TaskPriority::USER_BLOCKING. We use this background task runner to validate policy. This validation blocks the login flow.
On 2017/06/12 15:08:07, bartfab (slow) wrote: > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... > File chrome/browser/chromeos/settings/session_manager_operation.cc (right): > > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... > chrome/browser/chromeos/settings/session_manager_operation.cc:165: > {base::MayBlock(), base::TaskPriority::BACKGROUND, > I think this should be base::TaskPriority::USER_BLOCKING. We use this background > task runner to validate policy. This validation blocks the login flow. Isn't it better to fix this in depth rather than pipe though the sequenced task runner all the way through? I was trying to go this rabbit hole and must admit that it is a much bigger undertaking but presumably better in the long run see https://chromium-review.googlesource.com/c/567001
On 2017/07/13 14:16:00, pastarmovj wrote: > On 2017/06/12 15:08:07, bartfab (slow) wrote: > > > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... > > File chrome/browser/chromeos/settings/session_manager_operation.cc (right): > > > > > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... > > chrome/browser/chromeos/settings/session_manager_operation.cc:165: > > {base::MayBlock(), base::TaskPriority::BACKGROUND, > > I think this should be base::TaskPriority::USER_BLOCKING. We use this > background > > task runner to validate policy. This validation blocks the login flow. > > Isn't it better to fix this in depth rather than pipe though the sequenced task > runner all the way through? > > I was trying to go this rabbit hole and must admit that it is a much bigger > undertaking but presumably better in the long run see > https://chromium-review.googlesource.com/c/567001 Sure, if you want to clean this up - go ahead :).
On 2017/07/17 14:55:19, bartfab (OOO til 7th Aug) wrote: > On 2017/07/13 14:16:00, pastarmovj wrote: > > On 2017/06/12 15:08:07, bartfab (slow) wrote: > > > > > > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... > > > File chrome/browser/chromeos/settings/session_manager_operation.cc (right): > > > > > > > > > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/set... > > > chrome/browser/chromeos/settings/session_manager_operation.cc:165: > > > {base::MayBlock(), base::TaskPriority::BACKGROUND, > > > I think this should be base::TaskPriority::USER_BLOCKING. We use this > > background > > > task runner to validate policy. This validation blocks the login flow. > > > > Isn't it better to fix this in depth rather than pipe though the sequenced > task > > runner all the way through? > > > > I was trying to go this rabbit hole and must admit that it is a much bigger > > undertaking but presumably better in the long run see > > https://chromium-review.googlesource.com/c/567001 > > Sure, if you want to clean this up - go ahead :). Hi fdoray, please address the comment from bartfab and land this CL the bigger cleanup I have in mind will not be done soon enough so this clean up is a welcome intermediate step. LGTM from me too. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
