Add a WorkerScheduler and a WebThreadImplForWorker

Add a WorkerScheduler and a WebThreadImplForWorker

Adds a simple WorkerScheduler which supports Default and Idle Tasks (via
Long Idle Periods). A follow up Blink patch will refactor
WorkerThread.cpp to use these Idle Tasks.

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/510ed9c42029e1b2924ac05082c958591f029624
Cr-Commit-Position: refs/heads/master@{#324847}

Committed: https://crrev.com/b37d4c285dc2f505b006187d2651f6db96d9a42c
Cr-Commit-Position: refs/heads/master@{#325026}

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

Patchset #3 (id:40001) has been deleted

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

Patchset #2 (id:20001) has been deleted

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

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

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

Patchset #2 (id:60001) has been deleted

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

Fixed the diffbase

[rmcilroy, 2015-03-30 18:00:54]

Only had a very quick look, but overall this looks great. I'll have a more
in-depth look once the SchedulerHelper stuff has landed.

https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
File content/child/scheduler/worker_scheduler_impl.cc (right):

https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
content/child/scheduler/worker_scheduler_impl.cc:28:
helper_.InitiateLongIdlePeriodAfterWakeup();
Is it useful to do this afterwakeup rather than just doing it from the start?
I'm guessing this is to avoid doing GC before any other tasks are scheduled, but
we probably shouldn't have posted any idletask until some other work has been
done anyway right?

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

https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
File content/child/scheduler/worker_scheduler_impl.cc (right):

https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
content/child/scheduler/worker_scheduler_impl.cc:28:
helper_.InitiateLongIdlePeriodAfterWakeup();
On 2015/03/30 18:00:54, rmcilroy wrote:
> Is it useful to do this afterwakeup rather than just doing it from the start?
> I'm guessing this is to avoid doing GC before any other tasks are scheduled,
but
> we probably shouldn't have posted any idletask until some other work has been
> done anyway right?

The main reason for this is some of the unit tests don't work if I do an
InitiateLongIdlePeriod right off the bat.

Hopefully it doesn't really matter in practice.  WDYT?

[rmcilroy, 2015-04-01 10:24:35]

On 2015/03/31 17:03:50, alexclarke1 wrote:
>
https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
> File content/child/scheduler/worker_scheduler_impl.cc (right):
> 
>
https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
> content/child/scheduler/worker_scheduler_impl.cc:28:
> helper_.InitiateLongIdlePeriodAfterWakeup();
> On 2015/03/30 18:00:54, rmcilroy wrote:
> > Is it useful to do this afterwakeup rather than just doing it from the
start?
> > I'm guessing this is to avoid doing GC before any other tasks are scheduled,
> but
> > we probably shouldn't have posted any idletask until some other work has
been
> > done anyway right?
> 
> The main reason for this is some of the unit tests don't work if I do an
> InitiateLongIdlePeriod right off the bat.
> 
> Hopefully it doesn't really matter in practice.  WDYT?

I'd like to understand why this causes failures because I don't think it should.
Even if we start an idle period there shouldn't be any idle tasks in the idle
task queue until some point later (i.e., at least 0.3 second after a real task
got run on the worker). In this case, it shouldn't matter whether we start the
idle period immediately or after-wakeup should it?

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

On 2015/04/01 10:24:35, rmcilroy wrote:
> On 2015/03/31 17:03:50, alexclarke1 wrote:
> >
>
https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
> > File content/child/scheduler/worker_scheduler_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1033643004/diff/80001/content/child/scheduler...
> > content/child/scheduler/worker_scheduler_impl.cc:28:
> > helper_.InitiateLongIdlePeriodAfterWakeup();
> > On 2015/03/30 18:00:54, rmcilroy wrote:
> > > Is it useful to do this afterwakeup rather than just doing it from the
> start?
> > > I'm guessing this is to avoid doing GC before any other tasks are
scheduled,
> > but
> > > we probably shouldn't have posted any idletask until some other work has
> been
> > > done anyway right?
> > 
> > The main reason for this is some of the unit tests don't work if I do an
> > InitiateLongIdlePeriod right off the bat.
> > 
> > Hopefully it doesn't really matter in practice.  WDYT?
> 
> I'd like to understand why this causes failures because I don't think it
should.
> Even if we start an idle period there shouldn't be any idle tasks in the idle
> task queue until some point later (i.e., at least 0.3 second after a real task
> got run on the worker). In this case, it shouldn't matter whether we start the
> idle period immediately or after-wakeup should it?

If I s/helper_.InitiateLongIdlePeriodAfterWakeup/helper_.InitiateLongIdlePeriod
the following tests fail because the idle tasks don't run.

[  FAILED  ] WorkerSchedulerImplTest.TestPostIdleTask
[  FAILED  ] WorkerSchedulerImplTest.TestPostDefaultAndIdleTasks
[  FAILED  ] WorkerSchedulerImplTest.TestPostIdleTaskWithWakeupNeeded_Wakeup
[  FAILED  ] WorkerSchedulerImplTest.TestPostDefaultDelayedAndIdleTasks
[  FAILED  ] WorkerSchedulerImplTest.TestIdleDeadlineWithPendingDelayedTask
[  FAILED  ]
WorkerSchedulerImplTest.TestPostIdleTaskAfterRunningUntilIdle_WithWakeUp

I think this happens because when InitiateLongIdlePeriod() is called in the
constructor, the idle queue is empty hence
initiate_next_long_idle_period_after_wakeup_closure_ gets posted on the
control_task_after_wakeup_runner_ but there's nothing to wake it up.

These tests pass with InitiateLongIdlePeriodAfterWakeup() because that posts a
task to call InitiateLongIdlePeriod() and by the time that
runs, there have been idle tasks posted.

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

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

[commit-bot: I haz the power, 2015-04-01 12:31:11]

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

[commit-bot: I haz the power, 2015-04-01 12:40:41]

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

[commit-bot: I haz the power, 2015-04-01 12:40:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alex clarke (OOO till 29th), 2015-04-01 17:31:19]

I think I've fixed Ross's concern re initiating long idle.

[Sami, 2015-04-02 10:19:46]

Looks pretty good I think.

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
File content/child/scheduler/task_queue_manager.cc (left):

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/task_queue_manager.cc:309:
MoveReadyDelayedTasksToIncomingQueueLocked(lazy_now);
Did you mean to make this change? If so, probably better to do it in another
patch.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker.cc:45: task_runner_ = NULL;
nullptr (here and below)

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Should we rename this to webthread_impl_for_worker_scheduler.h or rename the
other one to webthread_impl_for_renderer.h (or _renderer_main.h)?

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler.h:49: // Does any post construction
initilaziation needed such as initiaiting long
Could you add a note that needs to be called before the scheduler can be used
(and maybe move this next to the top of this file)?

typo: initialization, initializing

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.cc:15:
WorkerSchedulerImpl::WorkerSchedulerImpl(
I like how how this class doesn't really need to do anything special -- just
plumbing.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:23: public
SchedulerHelper::SchedulerHelperDelegate {
I guess the delegate could be inherited privately?

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:29: // Implement
WorkerScheduler:
nit: "WorkerScheduler implementation:" is the usual phrasing.

https://codereview.chromium.org/1033643004/diff/160001/content/child/threaded...
File content/child/threaded_data_provider.h (right):

https://codereview.chromium.org/1033643004/diff/160001/content/child/threaded...
content/child/threaded_data_provider.h:81: WebThreadImplForWorker&
background_thread_;
Not your fault, but I think this should be a pointer according to the style
guide. Feel free to leave as is if it's too difficult to change.

However, should the type be just blink::WebThread if this class isn't calling
any of the subclass methods?

[rmcilroy, 2015-04-02 11:01:26]

looks pretty good - a couple of more comments.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.cc:15:
WorkerSchedulerImpl::WorkerSchedulerImpl(
On 2015/04/02 10:19:46, Sami wrote:
> I like how how this class doesn't really need to do anything special -- just
> plumbing.

+1 :)

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.cc:32:
helper_.InitiateLongIdlePeriod();
Could you add an initialized_ flag to the class which is set to True here, and
DCHECK(initialized_) in the other functions.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:23: public
SchedulerHelper::SchedulerHelperDelegate {
On 2015/04/02 10:19:46, Sami wrote:
> I guess the delegate could be inherited privately?

+1 (and for RendererSchedulerImpl too if possible)

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl_unittest.cc:137:
EXPECT_TRUE(run_order.empty());
Hmm, I see why this is happening but I'm wondering if we actually want it to
happen. Generally I don't think this situation would ever happen because the
idle task being posted would be being posted by another task which had been
posted (and thus would wake-up the scheduler). Maybe we should be testing that
situation instead, or alternatively, maybe we could ensure that if an idle-task
is posted it wakes-up the initiate_long_idle_period closure assuming it is
asleep (I'm not quite sure how we would do this cleanly though) - either way
this is possibly something for a future CL - lets, discuss in-person.

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl_unittest.cc:267: 
could you also add a test similar to 
SchedulerHelperTest::TestLongIdlePeriodDoesNotWakeScheduler to ensure that
posting after-wakeup idle tasks doesn't wake up the scheduler and cause the
initiate-long-idle tasks to keep ticking.

[kinuko, 2015-04-02 13:03:09]

A few drive-by comments (mostly minor)

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker.cc:19: cc::CompletionEvent
completion;
Drive-by question: I'm a bit surprised to see cc code here. It looks like we can
easily replace this with base::WaitableEvent, and to me it feels more natural.
Do you have a particular reason to use this class here?

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:23: public
SchedulerHelper::SchedulerHelperDelegate {
style-nit: private inheritance is not encouraged by our style guide (composition
is preferred if we want to hide the inheritance relationship)

https://codereview.chromium.org/1033643004/diff/160001/content/child/webthrea...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/1033643004/diff/160001/content/child/webthrea...
content/child/webthread_impl.h:37: IdleTask* idle_task);
nit: indent

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

PTAL

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
File content/child/scheduler/task_queue_manager.cc (left):

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/task_queue_manager.cc:309:
MoveReadyDelayedTasksToIncomingQueueLocked(lazy_now);
On 2015/04/02 10:19:45, Sami wrote:
> Did you mean to make this change? If so, probably better to do it in another
> patch.

Done.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker.cc:19: cc::CompletionEvent
completion;
On 2015/04/02 13:03:08, kinuko wrote:
> Drive-by question: I'm a bit surprised to see cc code here. It looks like we
can
> easily replace this with base::WaitableEvent, and to me it feels more natural.
> Do you have a particular reason to use this class here?

No particular reason. Changed.

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker.cc:45: task_runner_ = NULL;
On 2015/04/02 10:19:46, Sami wrote:
> nullptr (here and below)

Done.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/04/02 10:19:46, Sami wrote:
> Should we rename this to webthread_impl_for_worker_scheduler.h or rename the
> other one to webthread_impl_for_renderer.h (or _renderer_main.h)?

Done.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler.h:49: // Does any post construction
initilaziation needed such as initiaiting long
On 2015/04/02 10:19:46, Sami wrote:
> Could you add a note that needs to be called before the scheduler can be used
> (and maybe move this next to the top of this file)?
> 
> typo: initialization, initializing

Done.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.cc:15:
WorkerSchedulerImpl::WorkerSchedulerImpl(
On 2015/04/02 10:19:46, Sami wrote:
> I like how how this class doesn't really need to do anything special -- just
> plumbing.

Acknowledged.

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.cc:32:
helper_.InitiateLongIdlePeriod();
On 2015/04/02 11:01:26, rmcilroy wrote:
> Could you add an initialized_ flag to the class which is set to True here, and
> DCHECK(initialized_) in the other functions.

Done.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:23: public
SchedulerHelper::SchedulerHelperDelegate {
On 2015/04/02 10:19:46, Sami wrote:
> I guess the delegate could be inherited privately?

In light of the style guide comments I think making this protected isn't worth
doing.

People would have to downcast the WorkerScheduler to get hold of this Impl
class, and I doubt that would be allowed via code-review.

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:29: // Implement
WorkerScheduler:
On 2015/04/02 10:19:46, Sami wrote:
> nit: "WorkerScheduler implementation:" is the usual phrasing.

Done.

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

https://codereview.chromium.org/1033643004/diff/160001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl_unittest.cc:267: 
On 2015/04/02 11:01:26, rmcilroy wrote:
> could you also add a test similar to 
> SchedulerHelperTest::TestLongIdlePeriodDoesNotWakeScheduler to ensure that
> posting after-wakeup idle tasks doesn't wake up the scheduler and cause the
> initiate-long-idle tasks to keep ticking. 

Done.

https://codereview.chromium.org/1033643004/diff/160001/content/child/threaded...
File content/child/threaded_data_provider.h (right):

https://codereview.chromium.org/1033643004/diff/160001/content/child/threaded...
content/child/threaded_data_provider.h:81: WebThreadImplForWorker&
background_thread_;
On 2015/04/02 10:19:46, Sami wrote:
> Not your fault, but I think this should be a pointer according to the style
> guide. Feel free to leave as is if it's too difficult to change.
> 
> However, should the type be just blink::WebThread if this class isn't calling
> any of the subclass methods?

Yes strictly speaking that is true.  I'd prefer to leave it alone though.

https://codereview.chromium.org/1033643004/diff/160001/content/child/webthrea...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/1033643004/diff/160001/content/child/webthrea...
content/child/webthread_impl.h:37: IdleTask* idle_task);
On 2015/04/02 13:03:08, kinuko wrote:
> nit: indent

Done.

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

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

[commit-bot: I haz the power, 2015-04-02 15:20:22]

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

[commit-bot: I haz the power, 2015-04-02 15:30:35]

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

[commit-bot: I haz the power, 2015-04-02 15:39:31]

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

[commit-bot: I haz the power, 2015-04-02 15:39:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alex clarke (OOO till 29th), 2015-04-02 17:00:44]

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

[commit-bot: I haz the power, 2015-04-02 17:02:08]

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

[commit-bot: I haz the power, 2015-04-02 17:37:18]

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

[commit-bot: I haz the power, 2015-04-02 17:37:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alex clarke (OOO till 29th), 2015-04-08 13:21:44]

Post Easter PING :)

[rmcilroy, 2015-04-08 18:34:24]

A couple of last comments but lgtm once these are addressed (but please wait for
Sami to take a look too).

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
File content/child/scheduler/webthread_impl_for_worker_scheduler_unittest.cc
(right):

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker_scheduler_unittest.cc:120: 
How about some idle task tests too?

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
File content/child/scheduler/worker_scheduler.h (right):

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
content/child/scheduler/worker_scheduler.h:24: // initialization needed such as
initiating long idle periods.
nit - /s/initiating long idle periods/initializing idle period detection/ (long
idle periods are an implementation detail)

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
content/child/scheduler/worker_scheduler.h:42: // Must be called from the main
thread.
'main thread' should be 'the worker's thread' right? (thoughout)

https://codereview.chromium.org/1033643004/diff/220001/content/child/webthrea...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/1033643004/diff/220001/content/child/webthrea...
content/child/webthread_impl.h:5: #ifndef CONTENT_CHILD_WEBTHREAD_IMPL_H_
This file should be webthread_base.h now shouldn't it?

[alex clarke (OOO till 29th), 2015-04-09 10:02:37]

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
File content/child/scheduler/webthread_impl_for_worker_scheduler_unittest.cc
(right):

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker_scheduler_unittest.cc:120: 
On 2015/04/08 18:34:24, rmcilroy wrote:
> How about some idle task tests too?

So I agree we should have a test to confirm that idle tasks are wired up, but we
probably don't need to have loads of tests here.

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
File content/child/scheduler/worker_scheduler.h (right):

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
content/child/scheduler/worker_scheduler.h:24: // initialization needed such as
initiating long idle periods.
On 2015/04/08 18:34:24, rmcilroy wrote:
> nit - /s/initiating long idle periods/initializing idle period detection/
(long
> idle periods are an implementation detail)

Done.

https://codereview.chromium.org/1033643004/diff/220001/content/child/schedule...
content/child/scheduler/worker_scheduler.h:42: // Must be called from the main
thread.
On 2015/04/08 18:34:24, rmcilroy wrote:
> 'main thread' should be 'the worker's thread' right? (thoughout)

Done.

https://codereview.chromium.org/1033643004/diff/220001/content/child/webthrea...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/1033643004/diff/220001/content/child/webthrea...
content/child/webthread_impl.h:5: #ifndef CONTENT_CHILD_WEBTHREAD_IMPL_H_
On 2015/04/08 18:34:24, rmcilroy wrote:
> This file should be webthread_base.h now shouldn't it?

Done.

[Sami, 2015-04-09 12:02:44]

Thanks, lgtm.

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

https://codereview.chromium.org/1033643004/diff/240001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker_scheduler_unittest.cc:150:
EXPECT_THAT(calls, testing::HasSubstr("willProcessTask run didProcessTask"));
Neat!

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

https://codereview.chromium.org/1033643004/diff/240001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:52: bool initalized_;
typo: initialized_

Also looks like we could wrap this in an #ifndef NDEBUG?

[alex clarke (OOO till 29th), 2015-04-10 11:41:31]

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

https://codereview.chromium.org/1033643004/diff/240001/content/child/schedule...
content/child/scheduler/worker_scheduler_impl.h:52: bool initalized_;
On 2015/04/09 12:02:43, Sami wrote:
> typo: initialized_
> 
> Also looks like we could wrap this in an #ifndef NDEBUG?

Done.

[alex clarke (OOO till 29th), 2015-04-10 11:42:31]

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

[alex clarke (OOO till 29th), 2015-04-10 11:42:32]

jochen@chromium.org Can I please get an owners review for the non-scheduler
directory changes?

Thanks!

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

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

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

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

[commit-bot: I haz the power, 2015-04-10 16:36:35]

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

[commit-bot: I haz the power, 2015-04-10 16:43:24]

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

[commit-bot: I haz the power, 2015-04-10 16:43:26]

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

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

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

[alex clarke (OOO till 29th), 2015-04-10 16:46:37]

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/1033643004/#ps300001
(title: "Yet another rebase")

[alex clarke (OOO till 29th), 2015-04-10 16:47:24]

Patchset #13 (id:300001) has been deleted

[commit-bot: I haz the power, 2015-04-10 16:47:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1033643004/300001

[alex clarke (OOO till 29th), 2015-04-10 16:50:09]

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

[alex clarke (OOO till 29th), 2015-04-10 16:50:09]

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/1033643004/#ps320001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-10 16:50:27]

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

[commit-bot: I haz the power, 2015-04-10 16:59:39]

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

[commit-bot: I haz the power, 2015-04-10 16:59:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

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

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

[alex clarke (OOO till 29th), 2015-04-10 17:17:56]

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/1033643004/#ps340001
(title: "couldn't #ifndef NDEBUG because there are non-debug builds where the
dcheck was being compiled")

[commit-bot: I haz the power, 2015-04-10 17:18:19]

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

[commit-bot: I haz the power, 2015-04-10 17:28:35]

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

[commit-bot: I haz the power, 2015-04-10 17:28:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alex clarke (OOO till 29th), 2015-04-13 09:07:47]

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

[alex clarke (OOO till 29th), 2015-04-13 09:07:47]

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/1033643004/#ps360001
(title: "try and fix 32bit compile")

[commit-bot: I haz the power, 2015-04-13 09:07:56]

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

[commit-bot: I haz the power, 2015-04-13 09:15:52]

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

[commit-bot: I haz the power, 2015-04-13 09:15:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jochen (gone - plz use gerrit), 2015-04-13 13:08:52]

lgtm

[alex clarke (OOO till 29th), 2015-04-13 13:39:02]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-04-13 13:40:23]

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

[commit-bot: I haz the power, 2015-04-13 14:28:25]

Committed patchset #15 (id:360001)

[commit-bot: I haz the power, 2015-04-13 14:29:21]

Patchset 15 (id:??) landed as
https://crrev.com/510ed9c42029e1b2924ac05082c958591f029624
Cr-Commit-Position: refs/heads/master@{#324847}

[Nate Chapin, 2015-04-13 16:47:59]

A revert of this CL (patchset #15 id:360001) has been created in
https://codereview.chromium.org/1081933002/ by japhet@chromium.org.

The reason for reverting is: Possible cause of webkit_unit_tests failures
ImageFrameGeneratorTest.incompleteDecodeBecomesCompleteMultiThreaded and
DeferredImageDecoderTest.decodeOnOtherThread:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/46585.

[alex clarke (OOO till 29th), 2015-04-13 17:18:35]

Sami can you please take a look at my fix for the webkit_unit_tests flakes.

[Sami, 2015-04-13 17:38:49]

https://codereview.chromium.org/1033643004/diff/380001/content/child/schedule...
File content/child/scheduler/webthread_impl_for_worker_scheduler.cc (right):

https://codereview.chromium.org/1033643004/diff/380001/content/child/schedule...
content/child/scheduler/webthread_impl_for_worker_scheduler.cc:31:
TaskRunner()->PostTask(
In other words this shutdown task mustn't be allowed to bypass the things that
have already been queued through the scheduler? If the message loop was routed
through the scheduler then we wouldn't need this, but this lgtm for now.

[alex clarke (OOO till 29th), 2015-04-14 09:07:53]

On 2015/04/13 17:38:49, Sami wrote:
>
https://codereview.chromium.org/1033643004/diff/380001/content/child/schedule...
> File content/child/scheduler/webthread_impl_for_worker_scheduler.cc (right):
> 
>
https://codereview.chromium.org/1033643004/diff/380001/content/child/schedule...
> content/child/scheduler/webthread_impl_for_worker_scheduler.cc:31:
> TaskRunner()->PostTask(
> In other words this shutdown task mustn't be allowed to bypass the things that
> have already been queued through the scheduler? 

Yes.

> If the message loop was routed
> through the scheduler then we wouldn't need this, but this lgtm for now.

I'll keep my fingers crossed you get that stuff landed :)

[alex clarke (OOO till 29th), 2015-04-14 09:58:04]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-04-14 09:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1033643004/#ps380001
(title: "Fix bug where tasks posted to a worker thread did not always run when
the thread was shutdown")

[commit-bot: I haz the power, 2015-04-14 09:58:20]

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

[commit-bot: I haz the power, 2015-04-14 11:35:06]

Committed patchset #16 (id:380001)

[commit-bot: I haz the power, 2015-04-14 11:35:58]

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

[alex clarke (OOO till 29th), 2015-04-14 13:52:21]

A revert of this CL (patchset #16 id:380001) has been created in
https://codereview.chromium.org/1088933002/ by alexclarke@chromium.org.

The reason for reverting is: Looks like this broke
ImageFrameGeneratorTest.incompleteDecodeBecomesCompleteMultiThreaded.