Introduce a SchedulerHelper in content/child/scheduler

Introduce a SchedulerHelper in content/child/scheduler

We are planning on adding a WorkerScheduler for blink worker threads,
which will reuse much of the RendererScheduler logic for posting
Default and IdleTask tasks.

This patch pulls out the common logic into a SchedulerHelper
(We're going to think of a better name for this) and moves a bunch of
dependencies into a new content/child/scheduler directory where the WorkerScheduler
will be added in https://codereview.chromium.org/1033643004/

A WorkerSceduler is a pre-requisite for refactoring away the Blink
Timer Heap as described in:
https://docs.google.com/document/d/163ow-1wjd6L0rAN3V_U6t12eqVkq4mXDDjVaA4OuvCA/edit?usp=sharing

BUG=463143
Committed: https://crrev.com/ef36eb7a40195ccfbc5668da2093848ac2877119
Cr-Commit-Position: refs/heads/master@{#323214}

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

Patchset #2 (id:20001) has been deleted

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

Patchset #2 (id:40001) has been deleted

[alex clarke (OOO till 29th), 2015-03-24 16:17:28]

Patchset #2 (id:60001) has been deleted

[alex clarke (OOO till 29th), 2015-03-24 16:32:22]

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

[alex clarke (OOO till 29th), 2015-03-24 16:32:22]

While the number of files is big, this patch is almost exclusively a refactor,
moving and renaming stuff.

Although I did fix a test bug with TestLongIdlePeriodRepeating.

[alex clarke (OOO till 29th), 2015-03-26 18:05:57]

This patch used the delegate as discussed off line.  PTAL

[Sami, 2015-03-27 01:15:56]

Thanks Alex, I think it looks great this way. I've got one suggestion for how
the task queues are registered.

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:22: class LongIdlePeriodDelegate {
Should we just call this SchedulerHelperDelegate? I'm imagining there might be
other stuff we need to add here over time. If we later split idle tasks into
their own helper then this interface makes more sense.

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:39: size_t task_queue_count,
Since everyone needs these four task queues, how about changing
|task_queue_count| to |extra_task_queue_count| (which can be 0) (or keep it as
total_task_queue_count if that makes it easier) and then have an enum in
SchedulerHelper:

enum QueueId {
  DEFAULT_TASK_QUEUE,
  CONTROL_TASK_QUEUE,
  IDLE_TASK_QUEUE,
  ...
  TASK_QUEUE_COUNT,
}

Then RendererSchedulerImpl can have:

enum QueueId {
  COMPOSITOR_TASK_QUEUE = SchedulerHelper::TASK_QUEUE_COUNT,
  LOADING_TASK_QUEUE,
  ...
  TASK_QUEUE_COUNT,
};

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
File content/child/scheduler/task_queue_selector_impl.h (right):

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
content/child/scheduler/task_queue_selector_impl.h:19: class CONTENT_EXPORT
TaskQueueSelectorImpl
nit: Instead of "Impl" let's call this something like
PrioritizingTaskQueueSelector to make its function clearer and to make it more
obvious why we need a different one for workers.

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.cc:129:
scheduler_helper_.ControlTaskRunner()->PostDelayedTask(
No need to fix in this patch but just a random observation: getting a task
runner has significant cost because of the thread safe atomic refcounting, so we
should always cache a reference beforehand.

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.h:160: SchedulerHelper
scheduler_helper_;
nit: Just calling this helper_ could cut down on some of the noise.

[alex clarke (OOO till 29th), 2015-03-27 10:55:29]

PTAL

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:22: class LongIdlePeriodDelegate {
On 2015/03/27 01:15:55, Sami (slow -- traveling) wrote:
> Should we just call this SchedulerHelperDelegate? I'm imagining there might be
> other stuff we need to add here over time. If we later split idle tasks into
> their own helper then this interface makes more sense.

Done.

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:39: size_t task_queue_count,
On 2015/03/27 01:15:55, Sami (slow -- traveling) wrote:
> Since everyone needs these four task queues, how about changing
> |task_queue_count| to |extra_task_queue_count| (which can be 0) (or keep it as
> total_task_queue_count if that makes it easier) and then have an enum in
> SchedulerHelper:
> 
> enum QueueId {
>   DEFAULT_TASK_QUEUE,
>   CONTROL_TASK_QUEUE,
>   IDLE_TASK_QUEUE,
>   ...
>   TASK_QUEUE_COUNT,
> }
> 
> Then RendererSchedulerImpl can have:
> 
> enum QueueId {
>   COMPOSITOR_TASK_QUEUE = SchedulerHelper::TASK_QUEUE_COUNT,
>   LOADING_TASK_QUEUE,
>   ...
>   TASK_QUEUE_COUNT,
> };

Done.

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
File content/child/scheduler/task_queue_selector_impl.h (right):

https://codereview.chromium.org/1025323003/diff/120001/content/child/schedule...
content/child/scheduler/task_queue_selector_impl.h:19: class CONTENT_EXPORT
TaskQueueSelectorImpl
On 2015/03/27 01:15:55, Sami (slow -- traveling) wrote:
> nit: Instead of "Impl" let's call this something like
> PrioritizingTaskQueueSelector to make its function clearer and to make it more
> obvious why we need a different one for workers.

So PrioritizingTaskQueueSelector does sound like a better name. However I intend
to use PrioritizingTaskQueueSelector for workers too.

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.cc:129:
scheduler_helper_.ControlTaskRunner()->PostDelayedTask(
On 2015/03/27 01:15:55, Sami (slow -- traveling) wrote:
> No need to fix in this patch but just a random observation: getting a task
> runner has significant cost because of the thread safe atomic refcounting, so
we
> should always cache a reference beforehand.

Done.

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1025323003/diff/120001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.h:160: SchedulerHelper
scheduler_helper_;
On 2015/03/27 01:15:55, Sami (slow -- traveling) wrote:
> nit: Just calling this helper_ could cut down on some of the noise.

Done.

[rmcilroy, 2015-03-27 14:34:40]

Overall looks really good. A couple of comments - most of which are nits.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:286: const {
nit - I would put the return type on it's own line and keep 'const {' on the
same line with method signature.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:291: const {
ditto

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:21: // Used to customize behaviour.
nit - would be nice to be a bit more informative in the comment

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:33: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:38: const char*
disabled_by_default_tracing_category,
Add the same comment as trace_event.h about tracing category strings - 

// Category strings must have application lifetime (statics or
// literals). They may not include " chars.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:40: SchedulerHelperDelegate*
scheduler_helper_delegate);
optional ocd nit - I would prefer this be the first or second argument.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:44: void
SetWorkBatchSizeForTesting(size_t work_batch_size);
nit - ForTesting methods at the bottom of the public section

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:75:
scoped_refptr<base::SingleThreadTaskRunner> ControlTaskRunner();
Add a comment for this please, and move it near the other TaskRunner getters

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:78: enum QueueId {
maybe you could move this to an enum class while you are here?

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:128: }
Use this in scheduler_helper.cc too?

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:159: base::TimeTicks
estimated_next_frame_begin_;
Let's rename this (and the associated getters / setters) to something like
estimated_end_of_idle_ now that this scheduler helper isn't associated with
frame timings any longer.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/single_thread_idle_task_runner.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/single_thread_idle_task_runner.h:29: const char*
tracing_category);
same comment about trace_category

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/task_queue_manager.h:77: const char*
disabled_by_default_tracing_category);
and here

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.cc:103: if
(!helper_.SchedulerTaskQueueManager())
Could we make this a more obvious method name - i.e, if (helper_.IsShutdown()).

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.h:51: void
CurrentIdleTaskDeadlineCallbackForTesting(
nit - GetCurrentIdleTaskDeadlineCallbackForTesting. This could probably just
return base::TimeTicks now too.  Ideally it would be great to avoid having to
add this function just for tests but I can't think of a good solution right now.

[rmcilroy, 2015-03-27 14:34:41]

Overall looks really good. A couple of comments - most of which are nits.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:33: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:40: SchedulerHelperDelegate*
scheduler_helper_delegate);
optional ocd nit - I would prefer this be the first or second argument.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:44: void
SetWorkBatchSizeForTesting(size_t work_batch_size);
nit - ForTesting methods at the bottom of the public section

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:75:
scoped_refptr<base::SingleThreadTaskRunner> ControlTaskRunner();
Add a comment for this please, and move it near the other TaskRunner getters

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:78: enum QueueId {
maybe you could move this to an enum class while you are here?

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:128: }
Use this in scheduler_helper.cc too?

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/single_thread_idle_task_runner.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/single_thread_idle_task_runner.h:29: const char*
tracing_category);
same comment about trace_category

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/task_queue_manager.h:77: const char*
disabled_by_default_tracing_category);
and here

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.cc:103: if
(!helper_.SchedulerTaskQueueManager())
Could we make this a more obvious method name - i.e, if (helper_.IsShutdown()).

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.h:51: void
CurrentIdleTaskDeadlineCallbackForTesting(
nit - GetCurrentIdleTaskDeadlineCallbackForTesting. This could probably just
return base::TimeTicks now too.  Ideally it would be great to avoid having to
add this function just for tests but I can't think of a good solution right now.

[alex clarke (OOO till 29th), 2015-03-27 16:09:17]

PTAL

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:286: const {
On 2015/03/27 14:34:38, rmcilroy wrote:
> nit - I would put the return type on it's own line and keep 'const {' on the
> same line with method signature.

This is what got cl format does :(

I hate it too!

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:291: const {
On 2015/03/27 14:34:38, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:21: // Used to customize behaviour.
On 2015/03/27 14:34:38, rmcilroy wrote:
> nit - would be nice to be a bit more informative in the comment

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:33: };
On 2015/03/27 14:34:40, rmcilroy wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:38: const char*
disabled_by_default_tracing_category,
On 2015/03/27 14:34:38, rmcilroy wrote:
> Add the same comment as trace_event.h about tracing category strings - 
> 
> // Category strings must have application lifetime (statics or
> // literals). They may not include " chars.

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:40: SchedulerHelperDelegate*
scheduler_helper_delegate);
On 2015/03/27 14:34:40, rmcilroy wrote:
> optional ocd nit - I would prefer this be the first or second argument.

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:44: void
SetWorkBatchSizeForTesting(size_t work_batch_size);
On 2015/03/27 14:34:40, rmcilroy wrote:
> nit - ForTesting methods at the bottom of the public section

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:75:
scoped_refptr<base::SingleThreadTaskRunner> ControlTaskRunner();
On 2015/03/27 14:34:40, rmcilroy wrote:
> Add a comment for this please, and move it near the other TaskRunner getters

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:78: enum QueueId {
On 2015/03/27 14:34:40, rmcilroy wrote:
> maybe you could move this to an enum class while you are here?

Unfortunately you can't extend an enum class, so I think this doesn't help.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:128: }
On 2015/03/27 14:34:40, rmcilroy wrote:
> Use this in scheduler_helper.cc too?

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:159: base::TimeTicks
estimated_next_frame_begin_;
On 2015/03/27 14:34:38, rmcilroy wrote:
> Let's rename this (and the associated getters / setters) to something like
> estimated_end_of_idle_ now that this scheduler helper isn't associated with
> frame timings any longer.

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/single_thread_idle_task_runner.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/single_thread_idle_task_runner.h:29: const char*
tracing_category);
On 2015/03/27 14:34:40, rmcilroy wrote:
> same comment about trace_category

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/task_queue_manager.h:77: const char*
disabled_by_default_tracing_category);
On 2015/03/27 14:34:40, rmcilroy wrote:
> and here

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.cc:103: if
(!helper_.SchedulerTaskQueueManager())
On 2015/03/27 14:34:41, rmcilroy wrote:
> Could we make this a more obvious method name - i.e, if
(helper_.IsShutdown()).

Done.

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1025323003/diff/140001/content/renderer/sched...
content/renderer/scheduler/renderer_scheduler_impl.h:51: void
CurrentIdleTaskDeadlineCallbackForTesting(
On 2015/03/27 14:34:41, rmcilroy wrote:
> nit - GetCurrentIdleTaskDeadlineCallbackForTesting. This could probably just
> return base::TimeTicks now too.  Ideally it would be great to avoid having to
> add this function just for tests but I can't think of a good solution right
now.

Done.

[Sami, 2015-03-27 17:22:28]

Looks great! Just small nits.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/null_idle_task_runner.h (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/null_idle_task_runner.h:20: };
DISALLOW_COPY_AND_ASSIGN(NullIdleTaskRunner);

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:311: NOTREACHED();
Is this going to trip now that we have queues that the helper doesn't know
about? Maybe we need to make this part of the delegate interface?

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:21: // Used to by scheduler
implementatiosn to customize idle behaviour.
s/sn/ns/

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:39: // literals). They may not
include " chars.
Please also mention that total_task_queue_count must be at least
TASK_QUEUE_COUNT.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:68: // Must be called from the main
thread.
Since we're no longer limited to _the_ main thread, should we replace all
mentions of it with something else (I can't think of a good name) or just add a
defininition next to the constructor that says the main thread means the thread
the scheduler is created on?

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:134: void CheckOnValidThread() const
{
nit: CheckOnMainThread() to make this match the method documentation.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:138: // Acessor methods.
s/Acessor/Accessor/

[alex clarke (OOO till 29th), 2015-03-27 17:38:52]

PTAL

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/null_idle_task_runner.h (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/null_idle_task_runner.h:20: };
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> DISALLOW_COPY_AND_ASSIGN(NullIdleTaskRunner);

Done.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:311: NOTREACHED();
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> Is this going to trip now that we have queues that the helper doesn't know
> about? Maybe we need to make this part of the delegate interface?

No because SchedulerHelper::QueueId is a distinct type from 
RenderSchedulerImpl::QueueId, and the tracing code calls
RenderSchedulerImpl::TaskQueueIdToString() which calls this if *it* doesn't
recognize the enum.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:21: // Used to by scheduler
implementatiosn to customize idle behaviour.
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> s/sn/ns/

Done.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:39: // literals). They may not
include " chars.
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> Please also mention that total_task_queue_count must be at least
> TASK_QUEUE_COUNT.

Done.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:68: // Must be called from the main
thread.
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> Since we're no longer limited to _the_ main thread, should we replace all
> mentions of it with something else (I can't think of a good name) or just add
a
> defininition next to the constructor that says the main thread means the
thread
> the scheduler is created on?

Done.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:134: void CheckOnValidThread() const
{
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> nit: CheckOnMainThread() to make this match the method documentation.

Not sure I agree in light of your comment above about removing references to
main thread.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:138: // Acessor methods.
On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> s/Acessor/Accessor/

Done.

[Sami, 2015-03-27 17:53:11]

Thanks, lgtm! Please update the description and let Ross have a peek before
landing.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:311: NOTREACHED();
On 2015/03/27 17:38:51, alexclarke1 wrote:
> On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> > Is this going to trip now that we have queues that the helper doesn't know
> > about? Maybe we need to make this part of the delegate interface?
> 
> No because SchedulerHelper::QueueId is a distinct type from 
> RenderSchedulerImpl::QueueId, and the tracing code calls
> RenderSchedulerImpl::TaskQueueIdToString() which calls this if *it* doesn't
> recognize the enum.

Oh I see, nice.

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/160001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:134: void CheckOnValidThread() const
{
On 2015/03/27 17:38:51, alexclarke1 wrote:
> On 2015/03/27 17:22:27, Sami (slow -- traveling) wrote:
> > nit: CheckOnMainThread() to make this match the method documentation.
> 
> Not sure I agree in light of your comment above about removing references to
> main thread.

Right, now that we're no longer calling it the main thread this can stay as is.

[rmcilroy, 2015-03-27 19:02:00]

Last set of comments.

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/140001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:286: const {
On 2015/03/27 16:09:17, alexclarke1 wrote:
> On 2015/03/27 14:34:38, rmcilroy wrote:
> > nit - I would put the return type on it's own line and keep 'const {' on the
> > same line with method signature.
> 
> This is what got cl format does :(
> 
> I hate it too!

That's annoying, thanks for fixing though!

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:277: base::TimeTicks
estimated_next_frame_begin) {
Update arg name here too.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:291: const {
You did the one above but not this one (fine to ignore given git cl format
though).

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:142: void
SetEstimatedEndOfIdle(base::TimeTicks estimated_next_frame_begin);
Update arg name too.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:144: const CancelableClosureHolder&
EndIdlePeriodClosure() const;
This doesn't seem to be used, let's remove it.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper_unittest.cc (left):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:345:
TEST_F(RendererSchedulerImplTest, TestIdleTaskExceedsDeadline) {
Please move this test here (SchedulerHelperUnittest).

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:431: int run_count = 0;
Could you add a test similar to this one (using start/end idle period instead of
the frame stuff) - it's important that we test this delayed endidleperiod tasks
get canceled when we end an idle period early.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:503:
clock_->AdvanceNow(maximum_idle_period_duration());
Please make this do the same as the previous test did (end idle period and check
that the Task isn't run).

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:549:
TEST_F(SchedulerHelperTest, TestLongIdlePeriodCantEnterLongIdlePeriod) {
nit - TestLongIdlePeriodWhenNotCanEnterLongIdlePeriod

[alex clarke (OOO till 29th), 2015-03-30 12:23:30]

PTAL

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:277: base::TimeTicks
estimated_next_frame_begin) {
On 2015/03/27 19:01:59, rmcilroy wrote:
> Update arg name here too.

Done.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:291: const {
On 2015/03/27 19:01:59, rmcilroy wrote:
> You did the one above but not this one (fine to ignore given git cl format
> though).

Done.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:142: void
SetEstimatedEndOfIdle(base::TimeTicks estimated_next_frame_begin);
On 2015/03/27 19:01:59, rmcilroy wrote:
> Update arg name too.

Done.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:144: const CancelableClosureHolder&
EndIdlePeriodClosure() const;
On 2015/03/27 19:01:59, rmcilroy wrote:
> This doesn't seem to be used, let's remove it.

It's used in RendererSchedulerImpl::DidCommitFrameToCompositor

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper_unittest.cc (left):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:345:
TEST_F(RendererSchedulerImplTest, TestIdleTaskExceedsDeadline) {
On 2015/03/27 19:01:59, rmcilroy wrote:
> Please move this test here (SchedulerHelperUnittest).

Done.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:431: int run_count = 0;
On 2015/03/27 19:01:59, rmcilroy wrote:
> Could you add a test similar to this one (using start/end idle period instead
of
> the frame stuff) - it's important that we test this delayed endidleperiod
tasks
> get canceled when we end an idle period early.

Done.  This was a bit of a pain, sadly there are only indirect methods of
testing that the closures got cancelled.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:503:
clock_->AdvanceNow(maximum_idle_period_duration());
On 2015/03/27 19:01:59, rmcilroy wrote:
> Please make this do the same as the previous test did (end idle period and
check
> that the Task isn't run).

Done.

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper_unittest.cc:549:
TEST_F(SchedulerHelperTest, TestLongIdlePeriodCantEnterLongIdlePeriod) {
On 2015/03/27 19:01:59, rmcilroy wrote:
> nit - TestLongIdlePeriodWhenNotCanEnterLongIdlePeriod

Done.

[rmcilroy, 2015-03-30 12:59:06]

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:144: const CancelableClosureHolder&
EndIdlePeriodClosure() const;
On 2015/03/30 12:23:29, alexclarke1 wrote:
> On 2015/03/27 19:01:59, rmcilroy wrote:
> > This doesn't seem to be used, let's remove it.
> 
> It's used in RendererSchedulerImpl::DidCommitFrameToCompositor

Right, I missed this sorry. Let's move the posting of the EndIdle task into the
Scheduler helper's StartIdlePeriod function (this is a better public API anyway
to avoid the caller having to worry about ending the idle period at the right
time). The current StartIdlePeriod can be an internal private method since I
don't think we call it from anywhere else. WDTY?

One other thought - the split between set_estimated_idle_deadline and starting
the idle period was due to the way the compositor sends the deadline. Could we
now pass the deadline as an argumen t to StartIdlePeriod (I.e.,
render_scheduler_impl keeps track of estimated_next_frame_begin as before and
passes it as a deadline to the StartIdlePeriod function)? This might make things
a bit simpler and more obvious to users of SchedulerHelper, Wdyt?

[alex clarke (OOO till 29th), 2015-03-30 14:02:55]

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/180001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:144: const CancelableClosureHolder&
EndIdlePeriodClosure() const;
On 2015/03/30 12:59:06, rmcilroy wrote:
> On 2015/03/30 12:23:29, alexclarke1 wrote:
> > On 2015/03/27 19:01:59, rmcilroy wrote:
> > > This doesn't seem to be used, let's remove it.
> > 
> > It's used in RendererSchedulerImpl::DidCommitFrameToCompositor
> 
> Right, I missed this sorry. Let's move the posting of the EndIdle task into
the
> Scheduler helper's StartIdlePeriod function (this is a better public API
anyway
> to avoid the caller having to worry about ending the idle period at the right
> time). The current StartIdlePeriod can be an internal private method since I
> don't think we call it from anywhere else. WDTY?
> 
> One other thought - the split between set_estimated_idle_deadline and starting
> the idle period was due to the way the compositor sends the deadline. Could we
> now pass the deadline as an argumen t to StartIdlePeriod (I.e.,
> render_scheduler_impl keeps track of estimated_next_frame_begin as before and
> passes it as a deadline to the StartIdlePeriod function)? This might make
things
> a bit simpler and more obvious to users of SchedulerHelper, Wdyt?

Done.

[alex clarke (OOO till 29th), 2015-03-30 14:09:18]

I was able to remove EstimatedEndOfIdle() too. PTAL.

[alex clarke (OOO till 29th), 2015-03-30 14:42:19]

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

[alex clarke (OOO till 29th), 2015-03-30 14:42:20]

+jochen@chromium.org Can I get an OWNERS review for this patch? I think Ross is
pretty happy with it, although I'll wait for his LGTM before submitting.

[rmcilroy, 2015-03-30 17:53:58]

Looks great, thanks Alex. lgtm once final comments are addressed.

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:218: if (new_state ==
IdlePeriodState::IN_SHORT_IDLE_PERIOD) {
I don't like making this dependent on the state - let's make a specific argument
for whether it posts the end_idle_period task.

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:132: base::TimeTicks
estimated_end_of_idle);
nit - let's just make this end_of_idle (drop the 'estimated_'). Also update the
comment.

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:172: base::TimeTicks
estimated_end_of_idle_;
Now that this is no longer estimated let's just call it idle_period_deadline_
and set it to NULL in EndIdlePeriod runs.

[rmcilroy, 2015-03-30 17:53:59]

Looks great, thanks Alex. lgtm once final comments are addressed.

[alex clarke (OOO till 29th), 2015-03-31 10:18:35]

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
File content/child/scheduler/scheduler_helper.cc (right):

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
content/child/scheduler/scheduler_helper.cc:218: if (new_state ==
IdlePeriodState::IN_SHORT_IDLE_PERIOD) {
On 2015/03/30 17:53:57, rmcilroy wrote:
> I don't like making this dependent on the state - let's make a specific
argument
> for whether it posts the end_idle_period task.

Done.

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
File content/child/scheduler/scheduler_helper.h (right):

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:132: base::TimeTicks
estimated_end_of_idle);
On 2015/03/30 17:53:57, rmcilroy wrote:
> nit - let's just make this end_of_idle (drop the 'estimated_'). Also update
the
> comment.

Done.

https://codereview.chromium.org/1025323003/diff/240001/content/child/schedule...
content/child/scheduler/scheduler_helper.h:172: base::TimeTicks
estimated_end_of_idle_;
On 2015/03/30 17:53:58, rmcilroy wrote:
> Now that this is no longer estimated let's just call it idle_period_deadline_
> and set it to NULL in EndIdlePeriod runs.

Done.

[jochen (gone - plz use gerrit), 2015-03-31 10:36:44]

lgtm

[alex clarke (OOO till 29th), 2015-03-31 10:38:12]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-03-31 10:38:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1025323003/#ps260001
(title: "Ross's comments")

[commit-bot: I haz the power, 2015-03-31 10:38:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/260001

[commit-bot: I haz the power, 2015-03-31 11:33:43]

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

[commit-bot: I haz the power, 2015-03-31 11:33:44]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[alex clarke (OOO till 29th), 2015-03-31 15:00:05]

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

[alex clarke (OOO till 29th), 2015-03-31 15:00:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1025323003/#ps280001
(title: "Fixes")

[commit-bot: I haz the power, 2015-03-31 15:00:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/280001

[Sami, 2015-03-31 15:20:52]

Still lgtm.

[alex clarke (OOO till 29th), 2015-03-31 16:39:53]

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

[alex clarke (OOO till 29th), 2015-03-31 16:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1025323003/#ps320001
(title: "Make TaskQueue::WillDeleteTaskQueueManager delete other queues")

[commit-bot: I haz the power, 2015-03-31 16:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/320001

[alex clarke (OOO till 29th), 2015-03-31 17:12:59]

The CQ bit was unchecked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-03-31 17:13:13]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-03-31 17:13:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/340001

[commit-bot: I haz the power, 2015-03-31 18:26:28]

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

[commit-bot: I haz the power, 2015-03-31 18:26:31]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[alex clarke (OOO till 29th), 2015-04-01 09:21:35]

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

[alex clarke (OOO till 29th), 2015-04-01 09:21:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1025323003/#ps360001
(title: "fix perftests")

[commit-bot: I haz the power, 2015-04-01 09:21:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/360001

[alex clarke (OOO till 29th), 2015-04-01 09:56:59]

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

[alex clarke (OOO till 29th), 2015-04-01 09:57:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1025323003/#ps400001
(title: "SchedulerHelperDelegate needs CONTENT_EXPORT for windows.")

[commit-bot: I haz the power, 2015-04-01 09:57:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/400001

[commit-bot: I haz the power, 2015-04-01 10:02:23]

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

[commit-bot: I haz the power, 2015-04-01 10:02:25]

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...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[alex clarke (OOO till 29th), 2015-04-01 10:39:59]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-04-01 10:40:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/400001

[commit-bot: I haz the power, 2015-04-01 10:45:25]

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

[commit-bot: I haz the power, 2015-04-01 10:45:27]

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...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[alex clarke (OOO till 29th), 2015-04-01 10:51:12]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-04-01 10:51:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1025323003/#ps420001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-01 10:51:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025323003/420001

[commit-bot: I haz the power, 2015-04-01 11:53:56]

Committed patchset #19 (id:420001)

[commit-bot: I haz the power, 2015-04-01 11:54:48]

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