Patch 1/3 to get WebScheduler via WebThread

Patch 1/3 to get WebScheduler via WebThread

Now that all blink threads have schedulers it makes sense to get
hold the corresponding WebScheduler via WebThread.  This patch
is the chromium side of doing that.

Patch 2: https://codereview.chromium.org/1087203002
Patch 3: https://codereview.chromium.org/1084173002

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

[alex clarke (OOO till 29th), 2015-04-15 10:06:07]

Patchset #2 (id:20001) has been deleted

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

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

[alex clarke (OOO till 29th), 2015-04-15 11:23:05]

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

[commit-bot: I haz the power, 2015-04-15 11:23:26]

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

[commit-bot: I haz the power, 2015-04-15 11:31:56]

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

[commit-bot: I haz the power, 2015-04-15 11:31:57]

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...)

[rmcilroy, 2015-04-15 12:27:28]

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:14:
WorkerWebSchedulerImpl::WorkerWebSchedulerImpl(
Do we need a separate one of these for Worker and RendererScheduler? Could we
create a BaseScheduler interface which both RendererScheduler and
WorkerScheduler implement and use that here?

[Sami, 2015-04-15 12:37:13]

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/webthread_impl_for_worker_scheduler.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/webthread_impl_for_worker_scheduler.cc:55:
web_scheduler_.reset(nullptr);
WorkerWebSchedulerImpl should be destructed before worker_scheduler since the
former has a reference to the latter.

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/webthread_impl_for_worker_scheduler.h (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/webthread_impl_for_worker_scheduler.h:31: //
TODO(alexclarke): mark this as override when blink patch lands
We should still keep this as non-override even after that patch lands  since the
definition stays in the other repo. (I'm not sure how or why threadId got marked
as override.)

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:47:
tracked_objects::Location location(web_location.functionName(),
Is it worth writing a (possibly templated) helper to cut down on the repetition
here?

https://codereview.chromium.org/1088053003/diff/40001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_renderer_scheduler.h (right):

https://codereview.chromium.org/1088053003/diff/40001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_renderer_scheduler.h:26: //
TODO(alexclarke): mark this as override when blink patch lands
Ditto about override.

[alex clarke (OOO till 29th), 2015-04-15 14:25:46]

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/webthread_impl_for_worker_scheduler.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/webthread_impl_for_worker_scheduler.cc:55:
web_scheduler_.reset(nullptr);
On 2015/04/15 12:37:13, Sami wrote:
> WorkerWebSchedulerImpl should be destructed before worker_scheduler since the
> former has a reference to the latter.

Done.

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/webthread_impl_for_worker_scheduler.h (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/webthread_impl_for_worker_scheduler.h:31: //
TODO(alexclarke): mark this as override when blink patch lands
On 2015/04/15 12:37:13, Sami wrote:
> We should still keep this as non-override even after that patch lands  since
the
> definition stays in the other repo. (I'm not sure how or why threadId got
marked
> as override.)

Done.

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:14:
WorkerWebSchedulerImpl::WorkerWebSchedulerImpl(
On 2015/04/15 12:27:28, rmcilroy wrote:
> Do we need a separate one of these for Worker and RendererScheduler? Could we
> create a BaseScheduler interface which both RendererScheduler and
> WorkerScheduler implement and use that here?

There are enough common bits to do that, and I think it improves readability.

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:47:
tracked_objects::Location location(web_location.functionName(),
On 2015/04/15 12:37:13, Sami wrote:
> Is it worth writing a (possibly templated) helper to cut down on the
repetition
> here?

I was unable to get one template that can do it all because it needs to choose
between runIdleTask and runTask.  I suppose partial template specialization or
passing in a templatized function pointer would do it but really I think it was
getting way too complicated for little benefit.

A macro based approach would be easier but that's icky.

https://codereview.chromium.org/1088053003/diff/40001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_renderer_scheduler.h (right):

https://codereview.chromium.org/1088053003/diff/40001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_renderer_scheduler.h:26: //
TODO(alexclarke): mark this as override when blink patch lands
On 2015/04/15 12:37:13, Sami wrote:
> Ditto about override.

Done.

[rmcilroy, 2015-04-15 17:58:15]

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:14:
WorkerWebSchedulerImpl::WorkerWebSchedulerImpl(
On 2015/04/15 14:25:46, alexclarke1 wrote:
> On 2015/04/15 12:27:28, rmcilroy wrote:
> > Do we need a separate one of these for Worker and RendererScheduler? Could
we
> > create a BaseScheduler interface which both RendererScheduler and
> > WorkerScheduler implement and use that here?
> 
> There are enough common bits to do that, and I think it improves readability.

This wasn't actually what I meant. I meant that we could have a single
WebSchedulerImpl class which is used by the renderer_web_thread_impl and the
worker_web_thread_impl, with the only difference being the scheduler passed in
its constructer (RendererScheduler for the former and WorkerScheduler for the
latter). This would mean we would need to define a common BaseScheduler
interface which both RenderScheduler and WorkerScheduler inherit from (I realise
we dropped this from your original refractor CL but it seems like it would be
useful here). 

Would this be possible?

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

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:14:
WorkerWebSchedulerImpl::WorkerWebSchedulerImpl(
On 2015/04/15 17:58:15, rmcilroy wrote:
> On 2015/04/15 14:25:46, alexclarke1 wrote:
> > On 2015/04/15 12:27:28, rmcilroy wrote:
> > > Do we need a separate one of these for Worker and RendererScheduler? Could
> we
> > > create a BaseScheduler interface which both RendererScheduler and
> > > WorkerScheduler implement and use that here?
> > 
> > There are enough common bits to do that, and I think it improves
readability.
> 
> This wasn't actually what I meant. I meant that we could have a single
> WebSchedulerImpl class which is used by the renderer_web_thread_impl and the
> worker_web_thread_impl, with the only difference being the scheduler passed in
> its constructer (RendererScheduler for the former and WorkerScheduler for the
> latter). This would mean we would need to define a common BaseScheduler
> interface which both RenderScheduler and WorkerScheduler inherit from (I
realise
> we dropped this from your original refractor CL but it seems like it would be
> useful here). 
> 
> Would this be possible?

I suspect we'd end up with diamond inheritance problems if we introduced a
common BaseScheduler. Avoiding that will almost certainly snowball into quite a
big refactor and as such it's not appropriate to do that in this patch.

[Sami, 2015-04-16 10:44:01]

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:14:
WorkerWebSchedulerImpl::WorkerWebSchedulerImpl(
On 2015/04/16 09:11:55, alexclarke1 wrote:
> On 2015/04/15 17:58:15, rmcilroy wrote:
> > On 2015/04/15 14:25:46, alexclarke1 wrote:
> > > On 2015/04/15 12:27:28, rmcilroy wrote:
> > > > Do we need a separate one of these for Worker and RendererScheduler?
Could
> > we
> > > > create a BaseScheduler interface which both RendererScheduler and
> > > > WorkerScheduler implement and use that here?
> > > 
> > > There are enough common bits to do that, and I think it improves
> readability.
> > 
> > This wasn't actually what I meant. I meant that we could have a single
> > WebSchedulerImpl class which is used by the renderer_web_thread_impl and the
> > worker_web_thread_impl, with the only difference being the scheduler passed
in
> > its constructer (RendererScheduler for the former and WorkerScheduler for
the
> > latter). This would mean we would need to define a common BaseScheduler
> > interface which both RenderScheduler and WorkerScheduler inherit from (I
> realise
> > we dropped this from your original refractor CL but it seems like it would
be
> > useful here). 
> > 
> > Would this be possible?
> 
> I suspect we'd end up with diamond inheritance problems if we introduced a
> common BaseScheduler. Avoiding that will almost certainly snowball into quite
a
> big refactor and as such it's not appropriate to do that in this patch.

My view is that the WebScheduler adapters should be modeled after the family
tree of {Render,Worker}Scheduler. Right now there's no inheritance in the
former, so following that pattern here seems fine to me.

EDIT: based on offline discussion Ross will try to add a pure virtual
BaseScheduler that both RendererScheduler and WorkerScheduler implement.

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:47:
tracked_objects::Location location(web_location.functionName(),
On 2015/04/15 14:25:46, alexclarke1 wrote:
> On 2015/04/15 12:37:13, Sami wrote:
> > Is it worth writing a (possibly templated) helper to cut down on the
> repetition
> > here?
> 
> I was unable to get one template that can do it all because it needs to choose
> between runIdleTask and runTask.  I suppose partial template specialization or
> passing in a templatized function pointer would do it but really I think it
was
> getting way too complicated for little benefit.
> 
> A macro based approach would be easier but that's icky.

Okay, thanks for giving it a try anyway!

https://codereview.chromium.org/1088053003/diff/60001/content/child/scheduler...
File content/child/scheduler/webthread_impl_for_worker_scheduler.cc (right):

https://codereview.chromium.org/1088053003/diff/60001/content/child/scheduler...
content/child/scheduler/webthread_impl_for_worker_scheduler.cc:54:
web_scheduler_.reset(nullptr);
nit: I think you can just call reset() without any argument.

[rmcilroy, 2015-04-16 11:21:05]

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
File content/child/scheduler/worker_web_scheduler_impl.cc (right):

https://codereview.chromium.org/1088053003/diff/40001/content/child/scheduler...
content/child/scheduler/worker_web_scheduler_impl.cc:14:
WorkerWebSchedulerImpl::WorkerWebSchedulerImpl(
On 2015/04/16 10:44:01, Sami wrote:
> On 2015/04/16 09:11:55, alexclarke1 wrote:
> > On 2015/04/15 17:58:15, rmcilroy wrote:
> > > On 2015/04/15 14:25:46, alexclarke1 wrote:
> > > > On 2015/04/15 12:27:28, rmcilroy wrote:
> > > > > Do we need a separate one of these for Worker and RendererScheduler?
> Could
> > > we
> > > > > create a BaseScheduler interface which both RendererScheduler and
> > > > > WorkerScheduler implement and use that here?
> > > > 
> > > > There are enough common bits to do that, and I think it improves
> > readability.
> > > 
> > > This wasn't actually what I meant. I meant that we could have a single
> > > WebSchedulerImpl class which is used by the renderer_web_thread_impl and
the
> > > worker_web_thread_impl, with the only difference being the scheduler
passed
> in
> > > its constructer (RendererScheduler for the former and WorkerScheduler for
> the
> > > latter). This would mean we would need to define a common BaseScheduler
> > > interface which both RenderScheduler and WorkerScheduler inherit from (I
> > realise
> > > we dropped this from your original refractor CL but it seems like it would
> be
> > > useful here). 
> > > 
> > > Would this be possible?
> > 
> > I suspect we'd end up with diamond inheritance problems if we introduced a
> > common BaseScheduler. Avoiding that will almost certainly snowball into
quite
> a
> > big refactor and as such it's not appropriate to do that in this patch.
> 
> My view is that the WebScheduler adapters should be modeled after the family
> tree of {Render,Worker}Scheduler. Right now there's no inheritance in the
> former, so following that pattern here seems fine to me.
> 
> EDIT: based on offline discussion Ross will try to add a pure virtual
> BaseScheduler that both RendererScheduler and WorkerScheduler implement.

CL adds the common base scheduler (which I've named ChildScheduler) up at
https://codereview.chromium.org/1089833002

[alex clarke (OOO till 29th), 2015-04-16 13:34:31]

Patchset #5 (id:100001) has been deleted

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

PTAL

https://codereview.chromium.org/1088053003/diff/60001/content/child/scheduler...
File content/child/scheduler/webthread_impl_for_worker_scheduler.cc (right):

https://codereview.chromium.org/1088053003/diff/60001/content/child/scheduler...
content/child/scheduler/webthread_impl_for_worker_scheduler.cc:54:
web_scheduler_.reset(nullptr);
On 2015/04/16 10:44:01, Sami wrote:
> nit: I think you can just call reset() without any argument.

Done.

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

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

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

jochen@chromium.org: Can I please get an owners review for content/renderer and
content/test

[Sami, 2015-04-16 13:59:51]

Great, looks pretty simple now. lgtm.

https://codereview.chromium.org/1088053003/diff/130001/content/child/schedule...
File content/child/scheduler/web_scheduler_impl.cc (left):

https://codereview.chromium.org/1088053003/diff/130001/content/child/schedule...
content/child/scheduler/web_scheduler_impl.cc:100: 
Intentional?

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

https://codereview.chromium.org/1088053003/diff/130001/content/child/schedule...
File content/child/scheduler/web_scheduler_impl.cc (left):

https://codereview.chromium.org/1088053003/diff/130001/content/child/schedule...
content/child/scheduler/web_scheduler_impl.cc:100: 
On 2015/04/16 13:59:50, Sami wrote:
> Intentional?

Done.

[rmcilroy, 2015-04-16 14:21:26]

On 2015/04/16 14:03:05, alexclarke1 wrote:
>
https://codereview.chromium.org/1088053003/diff/130001/content/child/schedule...
> File content/child/scheduler/web_scheduler_impl.cc (left):
> 
>
https://codereview.chromium.org/1088053003/diff/130001/content/child/schedule...
> content/child/scheduler/web_scheduler_impl.cc:100: 
> On 2015/04/16 13:59:50, Sami wrote:
> > Intentional?
> 
> Done.

Looks great, thanks for making the changes! lgtm.

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

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

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

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/1088053003/#ps150001
(title: "Whitespace change")

[commit-bot: I haz the power, 2015-04-16 14:26:31]

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

[commit-bot: I haz the power, 2015-04-16 14:34:24]

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

[commit-bot: I haz the power, 2015-04-16 14:34:25]

Dry run: 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-16 14:41:51]

lgtm

[alex clarke (OOO till 29th), 2015-04-16 14:48:55]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-04-16 14:49:06]

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

[commit-bot: I haz the power, 2015-04-16 16:05:02]

Committed patchset #7 (id:150001)

[commit-bot: I haz the power, 2015-04-16 16:06:01]

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