(reland) Adds TimeDomains to the TaskQueueManager

Adds TimeDomains to the TaskQueueManager

This refactor isolates the logic dealing with delayed tasks,
making it easier to support multiple independent virtual
time sources.

BUG=546953
Committed: https://crrev.com/fc164806bdf051e28d9160f6854b1782fddc5776
Cr-Commit-Position: refs/heads/master@{#360641}

Committed: https://crrev.com/03d7888e494029c76e5a4151e24b29a8d4e3bb84
Cr-Commit-Position: refs/heads/master@{#360802}

[alex clarke (OOO till 29th), 2015-11-11 16:20:09]

Description was changed from

==========
Adds a DelayedTaskDelegate

This refactor makes it easy to support per queue virtual time.

BUG=546953
==========

to

==========
Adds a DelayedTaskDelegate

This refactor isolates the logic dealing with delayed tasks,
making it easier to support multiple independent virtual
time sources.

BUG=546953
==========

[alex clarke (OOO till 29th), 2015-11-11 16:20:16]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-11 16:20:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/20001

[commit-bot: I haz the power, 2015-11-11 16:30:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-11 16:30:11]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[alex clarke (OOO till 29th), 2015-11-11 16:58:19]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2015-11-11 16:58:20]

PTAL :)

[alex clarke (OOO till 29th), 2015-11-12 10:57:21]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-12 10:57:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/20001

[commit-bot: I haz the power, 2015-11-12 12:05:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-12 12:05:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alex clarke (OOO till 29th), 2015-11-12 13:36:36]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-12 13:36:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/40001

[commit-bot: I haz the power, 2015-11-12 14:02:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-12 14:02:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alex clarke (OOO till 29th), 2015-11-12 14:37:56]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-12 14:38:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/60001

[alex clarke (OOO till 29th), 2015-11-16 10:36:28]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-16 10:37:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/100001

[commit-bot: I haz the power, 2015-11-16 11:51:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-16 11:51:54]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-11-17 10:19:18]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 10:19:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/120001

[Sami, 2015-11-17 10:31:06]

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/delayed_task_delegate.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/delayed_task_delegate.h:20: class SCHEDULER_EXPORT
DelayedTaskDelegate
This class exposes public functions that deal with classes from the internal
namespace, which seems weird. Should we split this further into the
implementation part and a pure virtual delegate that the virtual time code can
override?

I guess one option would be to add the new methods on the TQM delegate, but
perhaps that becomes too confusing.

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/delayed_task_delegate.h:31: virtual void
InvalidateNowCache() = 0;
I kinda prefer the earlier way of passing around a LazyNow explicitly because
it's much clearer what the lifetime of the cached value is. With this API it's
much easier to accidentally forget to invalidate the cache. WDYT?

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/delayed_task_delegate_impl.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/delayed_task_delegate_impl.h:27: // TimeSource
protected implementation:
TimeSource?

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue.h:138: DelayedTaskDelegate*
delayed_task_delegate;
Should this be a scoped_refptr?

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:81:
any_thread().delayed_task_delegate = scoped_refptr<DelayedTaskDelegate>();
nit: I think there's a release() or reset() for this.

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue_impl.h:135: size_t
IncommingQueueSizeForTest() const;
typo: incoming

[commit-bot: I haz the power, 2015-11-17 11:32:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 11:32:31]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-11-18 15:30:08]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[alex clarke (OOO till 29th), 2015-11-18 15:30:15]

PTAL

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/delayed_task_delegate.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/delayed_task_delegate.h:20: class SCHEDULER_EXPORT
DelayedTaskDelegate
On 2015/11/17 10:31:06, Sami wrote:
> This class exposes public functions that deal with classes from the internal
> namespace, which seems weird. Should we split this further into the
> implementation part and a pure virtual delegate that the virtual time code can
> override?
> 
> I guess one option would be to add the new methods on the TQM delegate, but
> perhaps that becomes too confusing.

We made them private as discussed offline.

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/delayed_task_delegate.h:31: virtual void
InvalidateNowCache() = 0;
On 2015/11/17 10:31:06, Sami wrote:
> I kinda prefer the earlier way of passing around a LazyNow explicitly because
> it's much clearer what the lifetime of the cached value is. With this API it's
> much easier to accidentally forget to invalidate the cache. WDYT?

Done.

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/delayed_task_delegate_impl.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/delayed_task_delegate_impl.h:27: // TimeSource
protected implementation:
On 2015/11/17 10:31:06, Sami wrote:
> TimeSource?

Done.

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue.h:138: DelayedTaskDelegate*
delayed_task_delegate;
On 2015/11/17 10:31:06, Sami wrote:
> Should this be a scoped_refptr?

Its a bit of a pain to do that since suddenly the clang style extension forces
us to define out of line constructor, destructor and copy constructors.

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:81:
any_thread().delayed_task_delegate = scoped_refptr<DelayedTaskDelegate>();
On 2015/11/17 10:31:06, Sami wrote:
> nit: I think there's a release() or reset() for this.

I can't see anything like that on scoped_refptr

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.h (right):

https://codereview.chromium.org/1432263002/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue_impl.h:135: size_t
IncommingQueueSizeForTest() const;
On 2015/11/17 10:31:06, Sami wrote:
> typo: incoming

Done.

[commit-bot: I haz the power, 2015-11-18 15:30:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/160001

[commit-bot: I haz the power, 2015-11-18 16:02:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-18 16:02:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2015-11-18 18:36:24]

I really like how this moves all the delayed task and time handling into a
single place. I had two suggestions for untangling the dependencies with the TQM
and TQMDelegate a bit.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:82: any_thread().time_domain =
scoped_refptr<TimeDomain>();
IIRC you can do = nullptr.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:255: //
TaskQueueManager::UpdateQueues no longer needs to consider
nit: function name is out of date.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:344: // |incoming_queue| is now
empty so TaskQueueManager::UpdateQueues no longer
nit: function name is out of date.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
File components/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.cc:60: // Make sure there's one (and only
one) task posted to
nit: this comment is making assumptions about the implementation of
ScheduleDoWork (it does not need to have anything to do with the delegate).

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:30: virtual LazyNow GetLazyNow() = 0;
nit: "Create" since this is creating a new one from scratch.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:32: // Some TimeDomains support vitual
time, this method tells us to advance time
typo: virtual

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:60: // Schedules a call to
MoveReadyDelayedTasksToIncomingQueue when this
nit: might want to fully qualify this to make it clear where the function lives:

TaskQueueImpl::MoveReadyDelayedTasksToIncomingQueue()

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:83: virtual void
ScheduleDoWork(base::TimeDelta delay) = 0;
I wonder if we can simplify this part a bit: the contract says that the
implementation should arrange for TQM::DoWork() to be called at a later time,
but TQM::DoWork() is private so you can't really implement this without TQM's
help.

What if we instead just exposed a (public) virtual ScheduleWakeup (or something)
that takes the function to run as a parameter? That way the time domain doesn't
really need to know anything about TQM.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:104: TaskQueueManagerDelegate*
task_queue_manager_delegate_;  // NOT OWNED
I wonder if we remove the dependency on TaskQueueManagerDelegate here? This
class barely needs it and the only subclass that needs it is the real time one.

The only need for the delegate here is for hopping onto the right thread if
we're posting a delayed task from a different one. I wonder if we could do that
in the TaskQueueImpl instead? That would also fix a bug that we have that
posting the thread-hop task onto the delegate (instead of a task queue) may
delay it indefinitely depending on the work batch size.

[alex clarke (OOO till 29th), 2015-11-19 12:20:12]

PTAL

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:255: //
TaskQueueManager::UpdateQueues no longer needs to consider
On 2015/11/18 18:36:24, Sami wrote:
> nit: function name is out of date.

Done.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:344: // |incoming_queue| is now
empty so TaskQueueManager::UpdateQueues no longer
On 2015/11/18 18:36:24, Sami wrote:
> nit: function name is out of date.

Done.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
File components/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.cc:60: // Make sure there's one (and only
one) task posted to
On 2015/11/18 18:36:24, Sami wrote:
> nit: this comment is making assumptions about the implementation of
> ScheduleDoWork (it does not need to have anything to do with the delegate).

Done.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:32: // Some TimeDomains support vitual
time, this method tells us to advance time
On 2015/11/18 18:36:24, Sami wrote:
> typo: virtual

Done.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:60: // Schedules a call to
MoveReadyDelayedTasksToIncomingQueue when this
On 2015/11/18 18:36:24, Sami wrote:
> nit: might want to fully qualify this to make it clear where the function
lives:
> 
> TaskQueueImpl::MoveReadyDelayedTasksToIncomingQueue()

Done.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:83: virtual void
ScheduleDoWork(base::TimeDelta delay) = 0;
On 2015/11/18 18:36:24, Sami wrote:
> I wonder if we can simplify this part a bit: the contract says that the
> implementation should arrange for TQM::DoWork() to be called at a later time,
> but TQM::DoWork() is private so you can't really implement this without TQM's
> help.
> 
> What if we instead just exposed a (public) virtual ScheduleWakeup (or
something)
> that takes the function to run as a parameter? That way the time domain
doesn't
> really need to know anything about TQM.

It's looking like all the various forms of virtual time will have this function
be a NOP.  Given that I think the "DoWork" part of the name and the comment
possibly isn't helpful.

Instead of passing in a function I propose to rename this to RequestWakeup and
leave what it does up to the implementer. E.g. RealTimeDomain takes a do work
closure and posts it on the delegate.

https://codereview.chromium.org/1432263002/diff/160001/components/scheduler/b...
components/scheduler/base/time_domain.h:104: TaskQueueManagerDelegate*
task_queue_manager_delegate_;  // NOT OWNED
On 2015/11/18 18:36:24, Sami wrote:
> I wonder if we remove the dependency on TaskQueueManagerDelegate here? This
> class barely needs it and the only subclass that needs it is the real time
one.
> 
> The only need for the delegate here is for hopping onto the right thread if
> we're posting a delayed task from a different one. I wonder if we could do
that
> in the TaskQueueImpl instead? That would also fix a bug that we have that
> posting the thread-hop task onto the delegate (instead of a task queue) may
> delay it indefinitely depending on the work batch size.

Done.

[alex clarke (OOO till 29th), 2015-11-19 12:20:28]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 12:20:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/180001

[alex clarke (OOO till 29th), 2015-11-19 12:22:12]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 12:22:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/200001

[commit-bot: I haz the power, 2015-11-19 12:50:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 12:50:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2015-11-19 13:01:03]

Thanks, just small nits. The main question is whether we can limit TimeDomain to
be a main thread only thing or do we need to add a lock on LazyNow creation.

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:161: // non-delayed task (which is
thread safe) to schedule it.
Could you retain the comment about this being a little inefficient but we don't
expect this to be common?

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:165:
any_thread().task_queue_manager->GetNextSequenceNumber(), true);
I assume we can't reuse the sequence number generated above for some reason?
Maybe add a comment?

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:508: base::AutoLock
lock(any_thread_lock_);
Looks like this should be main thread only since TimeDomain::ScheduleDelayedWork
is?

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.h (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.h:168: scoped_refptr<TimeDomain>
time_domain;
Can this go onto the MainThreadOnly struct instead? It doesn't look like we
actually need to access this from other threads. The only exception is the
LazyNow creation which we do on arbitrary threads but I *think* we only use the
resulting object on the main thread.

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue_manager.h (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_manager.h:179: bool
ATimeDomainCanAdvance();
nit: the name makes this sound like an idempotent getter but this actually
causes time to advance. Maybe something like TryAdvanceTime or
AdvanceTimeIfNeeded?

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/time_domain.h:29: virtual LazyNow GetLazyNow() = 0;
nit: Create instead of Get?

Also, add a note that this needs to be thread safe.

[Sami, 2015-11-19 13:01:53]

(Btw could you also update the patch description?)

[alex clarke (OOO till 29th), 2015-11-19 13:53:45]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 13:54:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/170020
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/170020

[alex clarke (OOO till 29th), 2015-11-19 13:54:35]

Description was changed from

==========
Adds a DelayedTaskDelegate

This refactor isolates the logic dealing with delayed tasks,
making it easier to support multiple independent virtual
time sources.

BUG=546953
==========

to

==========
Adds TimeDomains to the TaskQueueManager

This refactor isolates the logic dealing with delayed tasks,
making it easier to support multiple independent virtual
time sources.

BUG=546953
==========

[alex clarke (OOO till 29th), 2015-11-19 13:54:48]

PTAL

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:161: // non-delayed task (which is
thread safe) to schedule it.
On 2015/11/19 13:01:03, Sami wrote:
> Could you retain the comment about this being a little inefficient but we
don't
> expect this to be common?

Done.

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:165:
any_thread().task_queue_manager->GetNextSequenceNumber(), true);
On 2015/11/19 13:01:03, Sami wrote:
> I assume we can't reuse the sequence number generated above for some reason?
> Maybe add a comment?

Feels a bit trixy to do that :(

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:508: base::AutoLock
lock(any_thread_lock_);
On 2015/11/19 13:01:03, Sami wrote:
> Looks like this should be main thread only since
TimeDomain::ScheduleDelayedWork
> is?

Done, MigrateQueue is main thread only.

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.h (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_impl.h:168: scoped_refptr<TimeDomain>
time_domain;
On 2015/11/19 13:01:03, Sami wrote:
> Can this go onto the MainThreadOnly struct instead? It doesn't look like we
> actually need to access this from other threads. The only exception is the
> LazyNow creation which we do on arbitrary threads but I *think* we only use
the
> resulting object on the main thread.

No. It's used by TaskQueueImpl::EnqueueTaskLocked which can be run on an
arbitrary thread.

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue_manager.h (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue_manager.h:179: bool
ATimeDomainCanAdvance();
On 2015/11/19 13:01:03, Sami wrote:
> nit: the name makes this sound like an idempotent getter but this actually
> causes time to advance. Maybe something like TryAdvanceTime or
> AdvanceTimeIfNeeded?

Done.

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1432263002/diff/200001/components/scheduler/b...
components/scheduler/base/time_domain.h:29: virtual LazyNow GetLazyNow() = 0;
On 2015/11/19 13:01:03, Sami wrote:
> nit: Create instead of Get?
> 
> Also, add a note that this needs to be thread safe.

Done.

[commit-bot: I haz the power, 2015-11-19 14:20:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 14:20:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2015-11-19 14:52:23]

Thanks, couple more small observations.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:179: void
TaskQueueImpl::ScheduleDelayedWorkTask(base::TimeTicks desired_run_time) {
Should we be holding the any thread lock here? Also DCHECK main thread?

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:319: void
TaskQueueImpl::EnqueueDelayedTaskLocked(const Task& pending_task) {
Not something for this patch but we should consider merging EnqueueTaskLocked
and EnqueueDelayedTaskLocked since they're almost identical.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.cc:82: this, spec.time_domain ?
spec.time_domain : real_time_domain_.get(),
Should we DCHECK that the time domain has been registered to this TQM? Since
registering/unregistering is manual it might be easy to forget.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.cc:115: if
(!real_time_domain_->NextScheduledRunTime(&next_pending_delayed_task))
Seems like this should do a min() across all time domains?

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager.h (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.h:112: const
scoped_refptr<RealTimeDomain>& real_time_domain() const {
It's slightly weird that you can't create a TQM that only uses virtual time but
I guess we can live with that limitation.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager_unittest.cc:1417: }
Maybe also unregister the domains so that code is being exercised?

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/time_domain.cc:168:
state->BeginDictionary("time_domain");
Does this work? Looks like |state| will be appending into an array when this is
called so we can't give a name to this dictionary.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/time_domain.h:29: // any thread.
Could you add a TODO for making this main thread only? I don't think there's any
actual reason we'd want to call this from another thread, and with the current
implementation we'll be grabbing two locks every time a task is posted to a
virtual time domain.

[alex clarke (OOO till 29th), 2015-11-19 15:20:02]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 15:20:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/230001

[alex clarke (OOO till 29th), 2015-11-19 15:57:14]

PTAL

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:179: void
TaskQueueImpl::ScheduleDelayedWorkTask(base::TimeTicks desired_run_time) {
On 2015/11/19 14:52:23, Sami wrote:
> Should we be holding the any thread lock here? Also DCHECK main thread?

Locking isn't needed here.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:319: void
TaskQueueImpl::EnqueueDelayedTaskLocked(const Task& pending_task) {
On 2015/11/19 14:52:23, Sami wrote:
> Not something for this patch but we should consider merging EnqueueTaskLocked
> and EnqueueDelayedTaskLocked since they're almost identical.

Acknowledged.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.cc:82: this, spec.time_domain ?
spec.time_domain : real_time_domain_.get(),
On 2015/11/19 14:52:23, Sami wrote:
> Should we DCHECK that the time domain has been registered to this TQM? Since
> registering/unregistering is manual it might be easy to forget.

Done.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.cc:115: if
(!real_time_domain_->NextScheduledRunTime(&next_pending_delayed_task))
On 2015/11/19 14:52:23, Sami wrote:
> Seems like this should do a min() across all time domains?

No this was deliberate.  The long idle logic goes berserk if it deals with
virtual time tasks, and posts millions of pointless tasks :(

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager.h (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.h:112: const
scoped_refptr<RealTimeDomain>& real_time_domain() const {
On 2015/11/19 14:52:23, Sami wrote:
> It's slightly weird that you can't create a TQM that only uses virtual time
but
> I guess we can live with that limitation.

We could change that I suppose but it needs access to the do work closure which
can't be created until after the TQM has been created.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager_unittest.cc:1417: }
On 2015/11/19 14:52:23, Sami wrote:
> Maybe also unregister the domains so that code is being exercised?

Done.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/time_domain.cc:168:
state->BeginDictionary("time_domain");
On 2015/11/19 14:52:23, Sami wrote:
> Does this work? Looks like |state| will be appending into an array when this
is
> called so we can't give a name to this dictionary.

Yes it does, without it the results were badly formatted.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/time_domain.h:29: // any thread.
On 2015/11/19 14:52:23, Sami wrote:
> Could you add a TODO for making this main thread only? I don't think there's
any
> actual reason we'd want to call this from another thread, and with the current
> implementation we'll be grabbing two locks every time a task is posted to a
> virtual time domain.

Done.

[Sami, 2015-11-19 16:11:20]

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:179: void
TaskQueueImpl::ScheduleDelayedWorkTask(base::TimeTicks desired_run_time) {
On 2015/11/19 15:57:14, alexclarke1 wrote:
> On 2015/11/19 14:52:23, Sami wrote:
> > Should we be holding the any thread lock here? Also DCHECK main thread?
> 
> Locking isn't needed here.

Hmm? any_thread() asserts that we're holding the lock.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager.h (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.h:112: const
scoped_refptr<RealTimeDomain>& real_time_domain() const {
On 2015/11/19 15:57:14, alexclarke1 wrote:
> On 2015/11/19 14:52:23, Sami wrote:
> > It's slightly weird that you can't create a TQM that only uses virtual time
> but
> > I guess we can live with that limitation.
> 
> We could change that I suppose but it needs access to the do work closure
which
> can't be created until after the TQM has been created.

One way to fix that would be to always explicitly pass that closure around to
completely uncouple TQM and the time domain, but we can consider doing that
later.

https://codereview.chromium.org/1432263002/diff/270001/components/scheduler/c...
File components/scheduler/child/scheduler_helper.h (right):

https://codereview.chromium.org/1432263002/diff/270001/components/scheduler/c...
components/scheduler/child/scheduler_helper.h:85: const
scoped_refptr<RealTimeDomain>& real_time_domain() const;
nit: move into accessor methods section below.

[alex clarke (OOO till 29th), 2015-11-19 16:50:54]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 16:51:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/290001

[alex clarke (OOO till 29th), 2015-11-19 16:52:57]

PTAL

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:179: void
TaskQueueImpl::ScheduleDelayedWorkTask(base::TimeTicks desired_run_time) {
On 2015/11/19 16:11:20, Sami wrote:
> On 2015/11/19 15:57:14, alexclarke1 wrote:
> > On 2015/11/19 14:52:23, Sami wrote:
> > > Should we be holding the any thread lock here? Also DCHECK main thread?
> > 
> > Locking isn't needed here.
> 
> Hmm? any_thread() asserts that we're holding the lock.

Seems daft to take a lock here.  Instead I put it on main_thread_only() too.

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
File components/scheduler/base/task_queue_manager.h (right):

https://codereview.chromium.org/1432263002/diff/170020/components/scheduler/b...
components/scheduler/base/task_queue_manager.h:112: const
scoped_refptr<RealTimeDomain>& real_time_domain() const {
On 2015/11/19 16:11:20, Sami wrote:
> On 2015/11/19 15:57:14, alexclarke1 wrote:
> > On 2015/11/19 14:52:23, Sami wrote:
> > > It's slightly weird that you can't create a TQM that only uses virtual
time
> > but
> > > I guess we can live with that limitation.
> > 
> > We could change that I suppose but it needs access to the do work closure
> which
> > can't be created until after the TQM has been created.
> 
> One way to fix that would be to always explicitly pass that closure around to
> completely uncouple TQM and the time domain, but we can consider doing that
> later.

Acknowledged.

https://codereview.chromium.org/1432263002/diff/270001/components/scheduler/c...
File components/scheduler/child/scheduler_helper.h (right):

https://codereview.chromium.org/1432263002/diff/270001/components/scheduler/c...
components/scheduler/child/scheduler_helper.h:85: const
scoped_refptr<RealTimeDomain>& real_time_domain() const;
On 2015/11/19 16:11:20, Sami wrote:
> nit: move into accessor methods section below.

Done.

[alex clarke (OOO till 29th), 2015-11-19 16:53:07]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 16:53:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/310001

[commit-bot: I haz the power, 2015-11-19 17:11:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 17:11:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[alex clarke (OOO till 29th), 2015-11-19 18:08:00]

PTAL

[Sami, 2015-11-19 18:09:40]

lgtm! Thanks for your patience.

[alex clarke (OOO till 29th), 2015-11-19 18:18:30]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-11-19 18:18:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/330001

[commit-bot: I haz the power, 2015-11-19 19:29:22]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2015-11-19 19:30:31]

Patchset 18 (id:??) landed as
https://crrev.com/fc164806bdf051e28d9160f6854b1782fddc5776
Cr-Commit-Position: refs/heads/master@{#360641}

[tsergeant, 2015-11-19 23:15:44]

A revert of this CL (patchset #18 id:330001) has been created in
https://codereview.chromium.org/1461143003/ by tsergeant@chromium.org.

The reason for reverting is: This CL is suspected of causing test failures on
Windows and Android:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

WebThreadImplForWorkerSchedulerTest.TestIdleTask is failing with

FATAL:task_queue_impl.h(41)] Check failed: enqueue_order_set_..

[alex clarke (OOO till 29th), 2015-11-20 10:34:56]

Description was changed from

==========
Adds TimeDomains to the TaskQueueManager

This refactor isolates the logic dealing with delayed tasks,
making it easier to support multiple independent virtual
time sources.

BUG=546953

Committed: https://crrev.com/fc164806bdf051e28d9160f6854b1782fddc5776
Cr-Commit-Position: refs/heads/master@{#360641}
==========

to

==========
Adds TimeDomains to the TaskQueueManager

This refactor isolates the logic dealing with delayed tasks,
making it easier to support multiple independent virtual
time sources.

BUG=546953

Committed: https://crrev.com/fc164806bdf051e28d9160f6854b1782fddc5776
Cr-Commit-Position: refs/heads/master@{#360641}
==========

[alex clarke (OOO till 29th), 2015-11-20 10:35:08]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-11-20 10:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1432263002/#ps350001
(title: "Fix thread_hop_task DCHECK")

[commit-bot: I haz the power, 2015-11-20 10:35:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432263002/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432263002/350001

[commit-bot: I haz the power, 2015-11-20 11:40:41]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2015-11-20 11:41:22]

Patchset 19 (id:??) landed as
https://crrev.com/03d7888e494029c76e5a4151e24b29a8d4e3bb84
Cr-Commit-Position: refs/heads/master@{#360802}