Task Scheduler.

Task Scheduler.

[fdoray, 2016-02-11 17:30:34]

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:29: DCHECK_NE(TimeTicks(),
task.delayed_run_time);
DCHECK(sequence.get());
DCHECK(priority_queue);

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:57: // to avoid holding 2 locks at
the same time.
multiple locks

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:58: TimeTicks now = Now();
const TimeTicks now = Now();

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:20: namespace task_scheduler {
Could we have a subtle namespace for things that people shouldn't use?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:48: TimeTicks
GetNextDelayedTaskReadyTime() const;
GetNextDelayedRunTime()

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:76: // |delayed_run_time| is in front
of the priority queue.
task.delayed_run_time

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager_unittest.cc:52: // sequence and
priority queue by PostReadyTasks() when the current time is
current time *becomes

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager_unittest.cc:80: // sequence and
priority queue by PostReadyTasks() when the current time is
current time *becomes

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:95: // CreateTaskRunnerWithTraits are supported.
remove "are supported"

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:98: TaskTraits traits);
How will we know that |parent_task_runner| is SEQUENCED?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.cc:45: SequenceSortKey* sort_key) const {
DCHECK(sort_key);

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
File base/task_scheduler/priority_queue.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:27: // |sequence_inserted_callback| is
invoked whenever a sequence is added to the
is *pushed* to the priority queue

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:38: // calling this to avoid getting a
stale result. The returned result can be
to *mitigate the chances* of getting a stale result

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:43: class BASE_EXPORT Transaction {
: public NonThreadSafe

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:79: // modify the priority queue while the
returned Transaction is alive. The
The *returned Transaction

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:86: // Controls access to
|priority_queue_|.
to |container_|

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_lock.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:49: const auto& iter =
should it be const auto (no &)?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:52: DCHECK(iter == thread_locks.end());
Since predecessors are specified when locks are constructed, it is not possible
to have cycles in |allowed_predecessors_|. That means that AssertSafeAcquire
already guarantees that there is no reentrancy, right?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:60: const auto& iter =
should it be const auto (no &)?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:78: LockVector& thread_locks =
acquired_locks_[id];
const LockVector&

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:18: DCHECK(task.get());
DCHECK first?
+ DCHECK(prev_num_tasks);

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequence.h
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.h:18: #include "base/task_scheduler/task.h"
#include "task_traits.h" for kNumTaskPriorities.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_traits.h:110: enum class ExecutionMode {
Add a comment explaining that this is used to create task runners?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.cc:262: void
ThreadPool::WorkerThreadBecomesIdleCallback(WorkerThread* worker_thread) {
DCHECK that |worker_thread| belongs to this thread pool?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.h:72: // the threads won't exit. This method is
not thread-safe.
"This method can only be called once per ThreadPool."

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:76: // method is not thread-safe.
This can only be called once per WorkerThread.

[robliao, 2016-02-11 21:56:26]

robliao@chromium.org changed reviewers:
+ robliao@chromium.org

[robliao, 2016-02-11 21:56:27]

In progress comments so far.

https://codereview.chromium.org/1685423002/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1685423002/diff/1/base/base.gyp#newcode542
base/base.gyp:542: 'task_scheduler/delayed_task_manager_unittest.cc',
We'll need to do this for BUILD.gn as well.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:20: namespace task_scheduler {
On 2016/02/11 17:30:32, fdoray wrote:
> Could we have a subtle namespace for things that people shouldn't use?

Maybe it's the return of the internal namespace.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:17:
Bind(&TaskSchedulerImpl::ReinsertSequenceCallback, Unretained(this));
We'll want to comment that the TaskScheduler lives forever, so Unretained is
safe here.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:46: // TODO(fdoray): Support
WithSequenceToken().
Remove this comment.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:92: traits.WithFileIO();
traits = traits.WithFileIO() to avoid the return value ignore.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.h:38: void JoinAllThreadsForTesting();
Is it ever valid to call this without calling Shutdown()?

Maybe this can be ShutdownAndJoinAllThreadsForTesting().

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:247: for (;;) {
Maybe this should be while(!shutdown_manager->shutdown_completed())

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:72: bool HasSingleThreadedTasks() const;
Since we're claiming this is thread safe, maybe we should just put the memory
barrier in the implementation.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:76: // method is not thread-safe.
On 2016/02/11 17:30:34, fdoray wrote:
> This can only be called once per WorkerThread.

We should be able to assert this.

[robliao, 2016-02-11 22:30:08]

More comments.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.cc:305: if
(!worker_thread->HasSingleThreadedTasks()) {
Maybe a controller pattern might make reasoning over the global state of the
threadpool easier.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:260: sequence = GetWork();
Can we remove this somehow?

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:271: DCHECK(task);
Remove DCHECK.

[robliao, 2016-02-11 22:49:30]

Next batch of comments.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.cc:25: DCHECK(TaskScheduler::GetInstance());
Remove DCHECK and the one in CreateTaskRunnerWithTraits.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:98: TaskTraits traits);
On 2016/02/11 17:30:33, fdoray wrote:
> How will we know that |parent_task_runner| is SEQUENCED?

That's a good question. We currently don't have a good way to introspect into
this pointer and enforce that the TaskRunners are actually our task runners.
We could have either all task runners we create extend from a TaskRunnerType
class that provides one virtual method to return the TaskRunnerType.

Alternatively, we could create an unordered_map of all TaskRunners we've handed
out. We would have to use WeakPtrs for this and it would probably be pretty
ugly.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:18: DCHECK(task.get());
On 2016/02/11 17:30:33, fdoray wrote:
> DCHECK first?
> + DCHECK(prev_num_tasks);

Both DCHECKs are unnecessary as both will crash below.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/shutdow...
File base/task_scheduler/shutdown_manager.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/shutdow...
base/task_scheduler/shutdown_manager.cc:58: ++num_tasks_blocking_shutdown_;
Should this be here?

[robliao, 2016-02-11 22:59:04]

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_lock.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:49: const auto& iter =
On 2016/02/11 17:30:33, fdoray wrote:
> should it be const auto (no &)?

Looks like it. Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:52: DCHECK(iter == thread_locks.end());
On 2016/02/11 17:30:33, fdoray wrote:
> Since predecessors are specified when locks are constructed, it is not
possible
> to have cycles in |allowed_predecessors_|. That means that AssertSafeAcquire
> already guarantees that there is no reentrancy, right?

That's a good point. When ahead and added a hard CHECK(lock == predecessor) to
the top too.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:60: const auto& iter =
On 2016/02/11 17:30:33, fdoray wrote:
> should it be const auto (no &)?

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_lock.cc:78: LockVector& thread_locks =
acquired_locks_[id];
On 2016/02/11 17:30:33, fdoray wrote:
> const LockVector&

Done.

[fdoray, 2016-02-12 04:16:20]

https://codereview.chromium.org/1685423002/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1685423002/diff/1/base/base.gyp#newcode542
base/base.gyp:542: 'task_scheduler/delayed_task_manager_unittest.cc',
On 2016/02/11 21:56:27, robliao wrote:
> We'll need to do this for BUILD.gn as well.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:29: DCHECK_NE(TimeTicks(),
task.delayed_run_time);
On 2016/02/11 17:30:32, fdoray wrote:
> DCHECK(sequence.get());
> DCHECK(priority_queue);

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:57: // to avoid holding 2 locks at
the same time.
On 2016/02/11 17:30:32, fdoray wrote:
> multiple locks

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:58: TimeTicks now = Now();
On 2016/02/11 17:30:32, fdoray wrote:
> const TimeTicks now = Now();

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:20: namespace task_scheduler {
On 2016/02/11 21:56:27, robliao wrote:
> On 2016/02/11 17:30:32, fdoray wrote:
> > Could we have a subtle namespace for things that people shouldn't use?
> 
> Maybe it's the return of the internal namespace.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:48: TimeTicks
GetNextDelayedTaskReadyTime() const;
On 2016/02/11 17:30:32, fdoray wrote:
> GetNextDelayedRunTime()

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:76: // |delayed_run_time| is in front
of the priority queue.
On 2016/02/11 17:30:32, fdoray wrote:
> task.delayed_run_time

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager_unittest.cc:52: // sequence and
priority queue by PostReadyTasks() when the current time is
On 2016/02/11 17:30:33, fdoray wrote:
> current time *becomes

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager_unittest.cc:80: // sequence and
priority queue by PostReadyTasks() when the current time is
On 2016/02/11 17:30:32, fdoray wrote:
> current time *becomes

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.cc:25: DCHECK(TaskScheduler::GetInstance());
On 2016/02/11 22:49:30, robliao wrote:
> Remove DCHECK and the one in CreateTaskRunnerWithTraits.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:95: // CreateTaskRunnerWithTraits are supported.
On 2016/02/11 17:30:33, fdoray wrote:
> remove "are supported"

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:98: TaskTraits traits);
On 2016/02/11 22:49:30, robliao wrote:
> On 2016/02/11 17:30:33, fdoray wrote:
> > How will we know that |parent_task_runner| is SEQUENCED?
> 
> That's a good question. We currently don't have a good way to introspect into
> this pointer and enforce that the TaskRunners are actually our task runners.
> We could have either all task runners we create extend from a TaskRunnerType
> class that provides one virtual method to return the TaskRunnerType.
> 
> Alternatively, we could create an unordered_map of all TaskRunners we've
handed
> out. We would have to use WeakPtrs for this and it would probably be pretty
> ugly.

Solution 1 seems good but... ref-counted classes can't use multiple inheritance
and our TaskRunners already inherit from (Sequenced|SingleThread)TaskRunner.
Maybe we could have all our TaskRunners inherit from SchedulerTaskRunner which
would itself inherit from TaskRunner (no more
SequencedTaskRunner/SingleThreadTaskRunner inheritance). We would need special
TaskRunners for (Thread|Sequenced)TaskRunnerHandle.

Solution 2 is indeed ugly.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.cc:45: SequenceSortKey* sort_key) const {
On 2016/02/11 17:30:33, fdoray wrote:
> DCHECK(sort_key);

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
File base/task_scheduler/priority_queue.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:27: // |sequence_inserted_callback| is
invoked whenever a sequence is added to the
On 2016/02/11 17:30:33, fdoray wrote:
> is *pushed* to the priority queue

WontFix

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:38: // calling this to avoid getting a
stale result. The returned result can be
On 2016/02/11 17:30:33, fdoray wrote:
> to *mitigate the chances* of getting a stale result

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:43: class BASE_EXPORT Transaction {
On 2016/02/11 17:30:33, fdoray wrote:
> : public NonThreadSafe

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:79: // modify the priority queue while the
returned Transaction is alive. The
On 2016/02/11 17:30:33, fdoray wrote:
> The *returned Transaction

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:86: // Controls access to
|priority_queue_|.
On 2016/02/11 17:30:33, fdoray wrote:
> to |container_|

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:18: DCHECK(task.get());
On 2016/02/11 22:49:30, robliao wrote:
> On 2016/02/11 17:30:33, fdoray wrote:
> > DCHECK first?
> > + DCHECK(prev_num_tasks);
> 
> Both DCHECKs are unnecessary as both will crash below.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequence.h
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.h:18: #include "base/task_scheduler/task.h"
On 2016/02/11 17:30:33, fdoray wrote:
> #include "task_traits.h" for kNumTaskPriorities.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/shutdow...
File base/task_scheduler/shutdown_manager.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/shutdow...
base/task_scheduler/shutdown_manager.cc:58: ++num_tasks_blocking_shutdown_;
On 2016/02/11 22:49:30, robliao wrote:
> Should this be here?

I think the code is correct. A SKIP_ON_SHUTDOWN task scheduled before shutdown
has started blocks shutdown.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:17:
Bind(&TaskSchedulerImpl::ReinsertSequenceCallback, Unretained(this));
On 2016/02/11 21:56:27, robliao wrote:
> We'll want to comment that the TaskScheduler lives forever, so Unretained is
> safe here.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:46: // TODO(fdoray): Support
WithSequenceToken().
On 2016/02/11 21:56:27, robliao wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:92: traits.WithFileIO();
On 2016/02/11 21:56:27, robliao wrote:
> traits = traits.WithFileIO() to avoid the return value ignore.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.h:38: void JoinAllThreadsForTesting();
On 2016/02/11 21:56:27, robliao wrote:
> Is it ever valid to call this without calling Shutdown()?
> 
> Maybe this can be ShutdownAndJoinAllThreadsForTesting().

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_traits.h:110: enum class ExecutionMode {
On 2016/02/11 17:30:33, fdoray wrote:
> Add a comment explaining that this is used to create task runners?

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.cc:262: void
ThreadPool::WorkerThreadBecomesIdleCallback(WorkerThread* worker_thread) {
On 2016/02/11 17:30:33, fdoray wrote:
> DCHECK that |worker_thread| belongs to this thread pool?

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.cc:305: if
(!worker_thread->HasSingleThreadedTasks()) {
On 2016/02/11 22:30:07, robliao wrote:
> Maybe a controller pattern might make reasoning over the global state of the
> threadpool easier.

I agree. I'll try to write this (tomorrow in the plane?) and we'll see if it
makes the code simpler. Since threads already have to lock
|shared_priority_queue_| each time that they get work, I don't think that having
an extra lock to protect a manager which knows about the state of each thread in
a pool will harm parallelism.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.h:72: // the threads won't exit. This method is
not thread-safe.
On 2016/02/11 17:30:33, fdoray wrote:
> "This method can only be called once per ThreadPool."

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:247: for (;;) {
On 2016/02/11 21:56:27, robliao wrote:
> Maybe this should be while(!shutdown_manager->shutdown_completed())

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:260: sequence = GetWork();
On 2016/02/11 22:30:07, robliao wrote:
> Can we remove this somehow?

We could if we built something that atomically checks if there is work to do and
calls WaitUntilWorkIsAvailable if there isn't. I'll work on this tomorrow.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:271: DCHECK(task);
On 2016/02/11 22:30:07, robliao wrote:
> Remove DCHECK.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:72: bool HasSingleThreadedTasks() const;
On 2016/02/11 21:56:27, robliao wrote:
> Since we're claiming this is thread safe, maybe we should just put the memory
> barrier in the implementation.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:76: // method is not thread-safe.
On 2016/02/11 21:56:27, robliao wrote:
> On 2016/02/11 17:30:34, fdoray wrote:
> > This can only be called once per WorkerThread.
> 
> We should be able to assert this.

Done. I added a DCHECK, which is unfortunately not thread-safe. Do you think
that I should add a lock just to make sure that a method which is supposed to be
used in tests only isn't called multiple times?