TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
TBR=danakj@chromium.org

Committed: https://crrev.com/546624d0d8ab564a914e669a0437cc51f0222aea
Cr-Commit-Position: refs/heads/master@{#425474}

[fdoray, 2016-10-11 15:50:22]

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

[fdoray, 2016-10-11 15:50:23]

PTAL

[robliao, 2016-10-12 20:55:45]

This particular approach looks fine. I've suggested an alternative approach that
keeps the DelayedTaskManager around. We can discuss on that thread.

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager.cc (left):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager.cc:56: void
DelayedTaskManager::AddDelayedTask(std::unique_ptr<Task> task,
An alternative implementation would be to route the service thread here and keep
the Delayed Task Manager. That way the SchedulerWorkerPools only have to know
about their own threads.

This has the benefit of keeping all of the policy about delayed tasks here (and
makes it an easy place to calculate metrics over the delayed tasks.

Thoughts?

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:384: TimeDelta delay) {
I would be okay with storing the delay on the task now that we actually need it.
This would simplify the signature of PostTaskWithSequence. This has the benefit
of keeping all topical information like delay and delayed_run_time_ on the task,
removing the need for the DCHECK_EQ below.

Aside: Unsurprisingly, MessageLoop converts this delay into a TimeTicks later
on. Might be worthwhile to consider how we can pipe TimeTicks all the way
through.

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:411:
std::move(service_thread_task_runner));
Can this be done inline to remove the declaration above?

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.cc:67: service_thread_.Stop();
What's the motivation for this move?
I would expect the service thread to remain the last thread terminated.

Also, this will destroy any pending delayed tasks. I expect that's okay for
tests, right?

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.cc:95: const bool
service_thread_start_result = service_thread_.StartWithOptions(
Inline this call to the CHECK below.

[fdoray, 2016-10-13 13:51:30]

PTAnL

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager.cc (left):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager.cc:56: void
DelayedTaskManager::AddDelayedTask(std::unique_ptr<Task> task,
On 2016/10/12 20:55:45, robliao wrote:
> An alternative implementation would be to route the service thread here and
keep
> the Delayed Task Manager. That way the SchedulerWorkerPools only have to know
> about their own threads.
> 
> This has the benefit of keeping all of the policy about delayed tasks here
(and
> makes it an easy place to calculate metrics over the delayed tasks.
> 
> Thoughts?

Instead of passing a TaskRunner to SchedulerWorkerPools, we could pass an opaque
Callback that posts a delayed task to the service thread TaskRunner?

But SchedulerWorkerPools will have to know about the service thread TaskRunner
anyways to support FileDescriptorWatcher::WatchReadable/Writable [1].

I'm hesitant to keep DelayedTaskManager (153 + 88 LOC, extra lock, extra
priority queue) just to have all the policy about delayed tasks in one place. I
think we should delete it for now and restore it later if necessary (e.g. if we
introduce a new metric or a new policy about delayed tasks).

[1] FileDescriptorWatcher relies on a MessageLoopForIO pointer stored in TLS (I
intend to replace this with a TaskRunner pointer).

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:384: TimeDelta delay) {
On 2016/10/12 20:55:45, robliao wrote:
> I would be okay with storing the delay on the task now that we actually need
it.
> This would simplify the signature of PostTaskWithSequence. This has the
benefit
> of keeping all topical information like delay and delayed_run_time_ on the
task,
> removing the need for the DCHECK_EQ below.
> 
> Aside: Unsurprisingly, MessageLoop converts this delay into a TimeTicks later
> on. Might be worthwhile to consider how we can pipe TimeTicks all the way
> through.

Done. Stored the delay with Task and added TODO.

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:411:
std::move(service_thread_task_runner));
On 2016/10/12 20:55:45, robliao wrote:
> Can this be done inline to remove the declaration above?

Done.

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.cc:67: service_thread_.Stop();
On 2016/10/12 20:55:45, robliao wrote:
> What's the motivation for this move?
> I would expect the service thread to remain the last thread terminated.
> 
> Also, this will destroy any pending delayed tasks. I expect that's okay for
> tests, right?

I didn't like the fact that the service thread could recreate a SchedulerWorker
that had already been joined. This is now addressed in a separate CL
https://codereview.chromium.org/2414043002/

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.cc:95: const bool
service_thread_start_result = service_thread_.StartWithOptions(
On 2016/10/12 20:55:45, robliao wrote:
> Inline this call to the CHECK below.

Done.

[robliao, 2016-10-13 17:03:39]

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager.cc (left):

https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager.cc:56: void
DelayedTaskManager::AddDelayedTask(std::unique_ptr<Task> task,
On 2016/10/13 13:51:29, fdoray wrote:
> On 2016/10/12 20:55:45, robliao wrote:
> > An alternative implementation would be to route the service thread here and
> keep
> > the Delayed Task Manager. That way the SchedulerWorkerPools only have to
know
> > about their own threads.
> > 
> > This has the benefit of keeping all of the policy about delayed tasks here
> (and
> > makes it an easy place to calculate metrics over the delayed tasks.
> > 
> > Thoughts?
> 
> Instead of passing a TaskRunner to SchedulerWorkerPools, we could pass an
opaque
> Callback that posts a delayed task to the service thread TaskRunner?
> 
> But SchedulerWorkerPools will have to know about the service thread TaskRunner
> anyways to support FileDescriptorWatcher::WatchReadable/Writable [1].
> 
> I'm hesitant to keep DelayedTaskManager (153 + 88 LOC, extra lock, extra
> priority queue) just to have all the policy about delayed tasks in one place.
I
> think we should delete it for now and restore it later if necessary (e.g. if
we
> introduce a new metric or a new policy about delayed tasks).
> 
> [1] FileDescriptorWatcher relies on a MessageLoopForIO pointer stored in TLS
(I
> intend to replace this with a TaskRunner pointer).

We wouldn't keep around everything in the DelayedTaskManager. It would simply
forward tasks to the service thread task runner. The locks and queue goes away.
We would simply use the abstraction we have a little longer until we determine
it's no longer necessary.

I suspect that its probably we would like to gather data about the sorts of
delayed tasks going through the system, so we would need a central gathering
point somewhere.

[fdoray, 2016-10-13 18:30:10]

On 2016/10/13 17:03:39, robliao wrote:
>
https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
> File base/task_scheduler/delayed_task_manager.cc (left):
> 
>
https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
> base/task_scheduler/delayed_task_manager.cc:56: void
> DelayedTaskManager::AddDelayedTask(std::unique_ptr<Task> task,
> On 2016/10/13 13:51:29, fdoray wrote:
> > On 2016/10/12 20:55:45, robliao wrote:
> > > An alternative implementation would be to route the service thread here
and
> > keep
> > > the Delayed Task Manager. That way the SchedulerWorkerPools only have to
> know
> > > about their own threads.
> > > 
> > > This has the benefit of keeping all of the policy about delayed tasks here
> > (and
> > > makes it an easy place to calculate metrics over the delayed tasks.
> > > 
> > > Thoughts?
> > 
> > Instead of passing a TaskRunner to SchedulerWorkerPools, we could pass an
> opaque
> > Callback that posts a delayed task to the service thread TaskRunner?
> > 
> > But SchedulerWorkerPools will have to know about the service thread
TaskRunner
> > anyways to support FileDescriptorWatcher::WatchReadable/Writable [1].
> > 
> > I'm hesitant to keep DelayedTaskManager (153 + 88 LOC, extra lock, extra
> > priority queue) just to have all the policy about delayed tasks in one
place.
> I
> > think we should delete it for now and restore it later if necessary (e.g. if
> we
> > introduce a new metric or a new policy about delayed tasks).
> > 
> > [1] FileDescriptorWatcher relies on a MessageLoopForIO pointer stored in TLS
> (I
> > intend to replace this with a TaskRunner pointer).
> 
> We wouldn't keep around everything in the DelayedTaskManager. It would simply
> forward tasks to the service thread task runner. The locks and queue goes
away.
> We would simply use the abstraction we have a little longer until we determine
> it's no longer necessary.
> 
> I suspect that its probably we would like to gather data about the sorts of
> delayed tasks going through the system, so we would need a central gathering
> point somewhere.

Don't you think that TaskTracker would be a good place for this? Every delayed
task goes through TaskTracker when it is posted and when it runs. TaskTracker
has access to the task's |delay|, |delayed_run_time| and post location
(FROM_HERE).

Keeping DelayedTaskManager without lock/priority queue wouldn't be that bad. If
you really want to, I can keep it.

[robliao, 2016-10-13 18:33:43]

On 2016/10/13 18:30:10, fdoray wrote:
> On 2016/10/13 17:03:39, robliao wrote:
> >
>
https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
> > File base/task_scheduler/delayed_task_manager.cc (left):
> > 
> >
>
https://codereview.chromium.org/2405243003/diff/20001/base/task_scheduler/del...
> > base/task_scheduler/delayed_task_manager.cc:56: void
> > DelayedTaskManager::AddDelayedTask(std::unique_ptr<Task> task,
> > On 2016/10/13 13:51:29, fdoray wrote:
> > > On 2016/10/12 20:55:45, robliao wrote:
> > > > An alternative implementation would be to route the service thread here
> and
> > > keep
> > > > the Delayed Task Manager. That way the SchedulerWorkerPools only have to
> > know
> > > > about their own threads.
> > > > 
> > > > This has the benefit of keeping all of the policy about delayed tasks
here
> > > (and
> > > > makes it an easy place to calculate metrics over the delayed tasks.
> > > > 
> > > > Thoughts?
> > > 
> > > Instead of passing a TaskRunner to SchedulerWorkerPools, we could pass an
> > opaque
> > > Callback that posts a delayed task to the service thread TaskRunner?
> > > 
> > > But SchedulerWorkerPools will have to know about the service thread
> TaskRunner
> > > anyways to support FileDescriptorWatcher::WatchReadable/Writable [1].
> > > 
> > > I'm hesitant to keep DelayedTaskManager (153 + 88 LOC, extra lock, extra
> > > priority queue) just to have all the policy about delayed tasks in one
> place.
> > I
> > > think we should delete it for now and restore it later if necessary (e.g.
if
> > we
> > > introduce a new metric or a new policy about delayed tasks).
> > > 
> > > [1] FileDescriptorWatcher relies on a MessageLoopForIO pointer stored in
TLS
> > (I
> > > intend to replace this with a TaskRunner pointer).
> > 
> > We wouldn't keep around everything in the DelayedTaskManager. It would
simply
> > forward tasks to the service thread task runner. The locks and queue goes
> away.
> > We would simply use the abstraction we have a little longer until we
determine
> > it's no longer necessary.
> > 
> > I suspect that its probably we would like to gather data about the sorts of
> > delayed tasks going through the system, so we would need a central gathering
> > point somewhere.
> 
> Don't you think that TaskTracker would be a good place for this? Every delayed
> task goes through TaskTracker when it is posted and when it runs. TaskTracker
> has access to the task's |delay|, |delayed_run_time| and post location
> (FROM_HERE).
> 
> Keeping DelayedTaskManager without lock/priority queue wouldn't be that bad.
If
> you really want to, I can keep it.

TaskTracker would indeed probably be a good place for it. Let's take advantage
of the DelayedTaskManager abstraction for now and then we can think a little bit
more about
removing it.

I would like to avoid removing it and then putting it back in later.

[fdoray, 2016-10-13 20:13:00]

PTAnL

The service thread can wake up a SchedulerWorker that has been joined when a
delayed task becomes ripe for execution. This was an issue before this CL. It
will be addressed it a separate CL.

[robliao, 2016-10-14 18:15:28]

Thanks! lgtm + nit.

https://codereview.chromium.org/2405243003/diff/80001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/80001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager_unittest.cc:25: constexpr TimeDelta
kDelay = TimeDelta::FromHours(1);
Nit: Rename to kLongDelay.

[fdoray, 2016-10-14 20:05:33]

https://codereview.chromium.org/2405243003/diff/80001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/80001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager_unittest.cc:25: constexpr TimeDelta
kDelay = TimeDelta::FromHours(1);
On 2016/10/14 18:15:28, robliao wrote:
> Nit: Rename to kLongDelay.

Done.

[fdoray, 2016-10-14 20:05:47]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-10-14 20:05:47]

The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/2405243003/#ps100001
(title: "CR robliao #9")

[fdoray, 2016-10-14 20:05:51]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2016-10-14 20:06:22]

fdoray@chromium.org changed reviewers:
+ danakj@chromium.org

[fdoray, 2016-10-14 20:06:23]

+danakj@ TBR for base/BUILD.gn

[fdoray, 2016-10-14 20:06:38]

Description was changed from

==========
TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
==========

to

==========
TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
TBR=danakj@chromium.org
==========

[fdoray, 2016-10-14 20:06:42]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-14 20:07:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 21:40:58]

Description was changed from

==========
TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
TBR=danakj@chromium.org
==========

to

==========
TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
TBR=danakj@chromium.org
==========

[commit-bot: I haz the power, 2016-10-14 21:40:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-14 21:43:26]

Description was changed from

==========
TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
TBR=danakj@chromium.org
==========

to

==========
TaskScheduler: Replace the SchedulerServiceThread with a base::Thread.

In order to support the FileDescriptorWatcher API within
TaskScheduler, the service thread needs to run a MessageLoopForIO
on POSIX. The simplest way to run a MessageLoopForIO is to use a
base::Thread.

BUG=553459
TBR=danakj@chromium.org

Committed: https://crrev.com/546624d0d8ab564a914e669a0437cc51f0222aea
Cr-Commit-Position: refs/heads/master@{#425474}
==========

[commit-bot: I haz the power, 2016-10-14 21:43:27]

Patchset 6 (id:??) landed as
https://crrev.com/546624d0d8ab564a914e669a0437cc51f0222aea
Cr-Commit-Position: refs/heads/master@{#425474}

[gab, 2016-10-17 19:09:17]

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

[gab, 2016-10-17 19:09:19]

lgtm % comments

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager.cc:40: Unretained(worker_pool),
Passed(std::move(task)),
std::move not required inside Passed() IIRC

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager.h (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Weird that this file is marked as 'D'...

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager_unittest.cc:78: // Run tasks that are
ripe for execution on the service thread. Don't expect
"service thread" here is confusing because this test isn't creating a service
thread, it's using the test's main thread.

Can we drop "on the service thread"?

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:306:
TEST_P(TaskSchedulerWorkerPoolImplTest, PostDelayedTaskNeverRuns) {
I'm not convinced that this test is required.
TaskSchedulerWorkerPoolImplTest.PostDelayedTask already tests that the
functionality is hooked up and this thus feels like it's merely re-testing
DelayedTaskManager and base::Thread?

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:336:
TimeDelta::FromMilliseconds(250) + TestTimeouts::tiny_timeout());
Also EXPECT_GT(..., tiny_timeout)

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:90: MessageLoop::TYPE_IO;
Document why this is required

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:98: const
SchedulerWorkerPoolImpl::ReEnqueueSequenceCallback
Move this below |delayed_task_manager_| instantiation.

[fdoray, 2016-10-18 20:10:49]

Comments addressed in https://codereview.chromium.org/2426813004/

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager.cc:40: Unretained(worker_pool),
Passed(std::move(task)),
On 2016/10/17 19:09:18, gab wrote:
> std::move not required inside Passed() IIRC

It's required. See other call sites
https://cs.chromium.org/chromium/src/base/bind_helpers.h?sq=package:chromium&...

c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(40): error
C2672: 'Passed': no matching overloaded function found
c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(40): error
C2784: 'base::internal::PassedWrapper<T> base::Passed(T *)': could not deduce
template argument for 'T *' from
'std::unique_ptr<base::internal::Task,std::default_delete<_Ty>>'
        with
        [
            _Ty=base::internal::Task
        ]
c:\src\chromium\src\base\bind_helpers.h(444): note: see declaration of
'base::Passed'
c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(40): error
C2783: 'base::internal::PassedWrapper<T> base::Passed(T &&)': could not deduce
template argument for '__formal'
c:\src\chromium\src\base\bind_helpers.h(440): note: see declaration of
'base::Passed'
c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(42): error
C2664: 'bool base::TaskRunner::PostDelayedTask(const tracked_objects::Location
&,const base::Closure &,base::TimeDelta)': cannot convert argument 2 from
'base::Callback<R (base::internal::SchedulerWorker
*),base::internal::CopyMode::Copyable,base::internal::RepeatMode::Repeating>' to
'const base::Closure &'
        with
        [
            R=void
        ]
c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(42): note:
Reason: cannot convert from 'base::Callback<R (base::internal::SchedulerWorker
*),base::internal::CopyMode::Copyable,base::internal::RepeatMode::Repeating>' to
'const base::Closure'
        with
        [
            R=void
        ]
c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(42): note: No
user-defined-conversion operator available that can perform this conversion, or
the operator cannot be called

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager.h (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/10/17 19:09:18, gab wrote:
> Weird that this file is marked as 'D'...

In the first patch set I deleted it and the code review tool isn't able to
update this letter.

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager_unittest.cc:78: // Run tasks that are
ripe for execution on the service thread. Don't expect
On 2016/10/17 19:09:18, gab wrote:
> "service thread" here is confusing because this test isn't creating a service
> thread, it's using the test's main thread.
> 
> Can we drop "on the service thread"?

Done.

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:306:
TEST_P(TaskSchedulerWorkerPoolImplTest, PostDelayedTaskNeverRuns) {
On 2016/10/17 19:09:18, gab wrote:
> I'm not convinced that this test is required.
> TaskSchedulerWorkerPoolImplTest.PostDelayedTask already tests that the
> functionality is hooked up and this thus feels like it's merely re-testing
> DelayedTaskManager and base::Thread?

Removed the test.

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:324: WaitableEvent
task_ran(WaitableEvent::ResetPolicy::MANUAL,
I think this test is useful because it verifies that the delay isn't "lost" (you
did "loose" delays when you implemented the redirection of SWP to
TaskScheduler).

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:336:
TimeDelta::FromMilliseconds(250) + TestTimeouts::tiny_timeout());
On 2016/10/17 19:09:18, gab wrote:
> Also EXPECT_GT(..., tiny_timeout)

Done.

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:90: MessageLoop::TYPE_IO;
On 2016/10/17 19:09:19, gab wrote:
> Document why this is required

Done.

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:98: const
SchedulerWorkerPoolImpl::ReEnqueueSequenceCallback
On 2016/10/17 19:09:19, gab wrote:
> Move this below |delayed_task_manager_| instantiation.

Done.

[gab, 2016-10-18 20:59:03]

Thanks, reply below FTR

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
File base/task_scheduler/delayed_task_manager.cc (right):

https://codereview.chromium.org/2405243003/diff/100001/base/task_scheduler/de...
base/task_scheduler/delayed_task_manager.cc:40: Unretained(worker_pool),
Passed(std::move(task)),
On 2016/10/18 20:10:49, fdoray wrote:
> On 2016/10/17 19:09:18, gab wrote:
> > std::move not required inside Passed() IIRC
> 
> It's required. See other call sites
>
https://cs.chromium.org/chromium/src/base/bind_helpers.h?sq=package:chromium&...
> 
> c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(40): error
> C2672: 'Passed': no matching overloaded function found
> c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(40): error
> C2784: 'base::internal::PassedWrapper<T> base::Passed(T *)': could not deduce
> template argument for 'T *' from
> 'std::unique_ptr<base::internal::Task,std::default_delete<_Ty>>'
>         with
>         [
>             _Ty=base::internal::Task
>         ]
> c:\src\chromium\src\base\bind_helpers.h(444): note: see declaration of
> 'base::Passed'
> c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(40): error
> C2783: 'base::internal::PassedWrapper<T> base::Passed(T &&)': could not deduce
> template argument for '__formal'
> c:\src\chromium\src\base\bind_helpers.h(440): note: see declaration of
> 'base::Passed'
> c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(42): error
> C2664: 'bool base::TaskRunner::PostDelayedTask(const tracked_objects::Location
> &,const base::Closure &,base::TimeDelta)': cannot convert argument 2 from
> 'base::Callback<R (base::internal::SchedulerWorker
> *),base::internal::CopyMode::Copyable,base::internal::RepeatMode::Repeating>'
to
> 'const base::Closure &'
>         with
>         [
>             R=void
>         ]
> c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(42): note:
> Reason: cannot convert from 'base::Callback<R (base::internal::SchedulerWorker
> *),base::internal::CopyMode::Copyable,base::internal::RepeatMode::Repeating>'
to
> 'const base::Closure'
>         with
>         [
>             R=void
>         ]
> c:\src\chromium\src\base\task_scheduler\delayed_task_manager.cc(42): note: No
> user-defined-conversion operator available that can perform this conversion,
or
> the operator cannot be called

Ah interesting, in retrospect it makes sense. base::Passed was previously
adapted to work with scoped_ptr (which was a hack with MOVE_ONLY_CPP03 macro or
something) but I guess it can't work around it for std::unique_ptr (i.e. can't
fake an rvalue). This mostly makes base::Passed outdated I think but wtv that's
beyond this CL and it's still required in today's Bind world.