Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(376)

Issue 2875393002: Use TaskScheduler instead of SequencedWorkerPool in session_manager_operation.cc.

Created:
1 year, 10 months ago by fdoray
Modified:
1 year, 7 months ago
CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -7 lines) Patch
M chrome/browser/chromeos/settings/session_manager_operation.cc View 2 chunks +3 lines, -7 lines 2 comments Download

Messages

Total messages: 17 (7 generated)
fdoray
1 year, 10 months ago (2017-05-12 17:05:30 UTC) #1
fdoray
Please take a look. This CL was generated automatically. The base::MayBlock() trait was specified for ...
1 year, 10 months ago (2017-05-12 17:05:32 UTC) #3
fdoray
On 2017/05/12 17:05:32, fdoray wrote: > Please take a look. > > This CL was ...
1 year, 10 months ago (2017-05-19 19:26:30 UTC) #9
fdoray
On 2017/05/19 19:26:30, fdoray wrote: > On 2017/05/12 17:05:32, fdoray wrote: > > Please take ...
1 year, 9 months ago (2017-05-30 16:56:26 UTC) #10
fdoray
+mnissler@chromium.org +pastarmovj@chromium.org, (no reply from other reviewers in 2 weeks)
1 year, 9 months ago (2017-05-30 16:57:16 UTC) #12
bartfab (slow)
lgtm https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/settings/session_manager_operation.cc File chrome/browser/chromeos/settings/session_manager_operation.cc (right): https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/settings/session_manager_operation.cc#newcode165 chrome/browser/chromeos/settings/session_manager_operation.cc:165: {base::MayBlock(), base::TaskPriority::BACKGROUND, Nit: #include "base/task_scheduler/task_traits.h"
1 year, 9 months ago (2017-06-12 15:05:48 UTC) #13
bartfab (slow)
https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/settings/session_manager_operation.cc File chrome/browser/chromeos/settings/session_manager_operation.cc (right): https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/settings/session_manager_operation.cc#newcode165 chrome/browser/chromeos/settings/session_manager_operation.cc:165: {base::MayBlock(), base::TaskPriority::BACKGROUND, I think this should be base::TaskPriority::USER_BLOCKING. We ...
1 year, 9 months ago (2017-06-12 15:08:07 UTC) #14
pastarmovj
On 2017/06/12 15:08:07, bartfab (slow) wrote: > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/settings/session_manager_operation.cc > File chrome/browser/chromeos/settings/session_manager_operation.cc (right): > > https://codereview.chromium.org/2875393002/diff/1/chrome/browser/chromeos/settings/session_manager_operation.cc#newcode165 ...
1 year, 8 months ago (2017-07-13 14:16:00 UTC) #15
bartfab (slow)
On 2017/07/13 14:16:00, pastarmovj wrote: > On 2017/06/12 15:08:07, bartfab (slow) wrote: > > > ...
1 year, 8 months ago (2017-07-17 14:55:19 UTC) #16
pastarmovj
1 year, 7 months ago (2017-07-28 10:43:00 UTC) #17
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.

Powered by Google App Engine
This is Rietveld 408576698