WebThreadImplForMessageLoop to use blink scheduler

Hook the WebThreadImplForMessageLoop up to post taks through the blink
scheduler.  Also routes the blink timers though the scheduler.

BUG=391005
Committed: https://crrev.com/83a9d24050d94dc3f334ffc13fc43918055fefc9
Cr-Commit-Position: refs/heads/master@{#303463}

[alex clarke (OOO till 29th), 2014-11-04 01:33:55]

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

[Sami, 2014-11-04 01:59:02]

https://codereview.chromium.org/690703002/diff/40001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/40001/content/child/blink_plat...
content/child/blink_platform_impl.cc:420: BlinkPlatformImpl::BlinkPlatformImpl()
Could we make main_thread_task_runner a parameter here? This constructor might
end up passing it to other things before the derived class replaces it with the
correct one.

https://codereview.chromium.org/690703002/diff/40001/content/child/blink_plat...
content/child/blink_platform_impl.cc:495: if (main_thread_task_runner_.get() &&
|main_thread_task_runner_| should always be non-null. IMO this would read a
little easier like this:

  scoped_refptr<base::SingleThreadTaskRunner> task_runner;
  if (main_thread_task_runner_->BelongsToCurrentThread()) {
    task_runner = main_thread_task_runner_;
  } else {
    task_runner = base::MessageLoopProxy::current();
  }

[alex clarke (OOO till 29th), 2014-11-04 18:40:21]

https://codereview.chromium.org/690703002/diff/40001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/40001/content/child/blink_plat...
content/child/blink_platform_impl.cc:420: BlinkPlatformImpl::BlinkPlatformImpl()
On 2014/11/04 01:59:01, Sami wrote:
> Could we make main_thread_task_runner a parameter here? This constructor might
> end up passing it to other things before the derived class replaces it with
the
> correct one.

I've added a new constructor rather than altering all the existing ones since
that would make the patch significantly larger.

https://codereview.chromium.org/690703002/diff/40001/content/child/blink_plat...
content/child/blink_platform_impl.cc:495: if (main_thread_task_runner_.get() &&
On 2014/11/04 01:59:01, Sami wrote:
> |main_thread_task_runner_| should always be non-null. IMO this would read a
> little easier like this:

That's not always true and in fact the browser will crash on startup if we
assume this.

> 
>   scoped_refptr<base::SingleThreadTaskRunner> task_runner;
>   if (main_thread_task_runner_->BelongsToCurrentThread()) {
>     task_runner = main_thread_task_runner_;
>   } else {
>     task_runner = base::MessageLoopProxy::current();
>   }

[Sami, 2014-11-06 00:22:59]

https://codereview.chromium.org/690703002/diff/60001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/60001/content/child/blink_plat...
content/child/blink_platform_impl.cc:430: BlinkPlatformImpl::BlinkPlatformImpl(
Delegated constructors still blocked it looks like:
https://codereview.chromium.org/695683002/

If we're sufficiently bothered about this we could consider helping to get them
accepted.

https://codereview.chromium.org/690703002/diff/60001/content/child/blink_plat...
content/child/blink_platform_impl.cc:511:
main_thread_task_runner_->BelongsToCurrentThread()) {
Could you rewrite this in a way that we don't do the TLS lookup
MessageLoopProxy::current() if we've got a valid main_thread_task_runner?

https://codereview.chromium.org/690703002/diff/60001/content/renderer/rendere...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/60001/content/renderer/rendere...
content/renderer/renderer_blink_platform_impl.cc:30: #include
"content/child/webthread_impl.h"
Is this needed?

[alex clarke (OOO till 29th), 2014-11-06 00:40:49]

https://codereview.chromium.org/690703002/diff/60001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/60001/content/child/blink_plat...
content/child/blink_platform_impl.cc:430: BlinkPlatformImpl::BlinkPlatformImpl(
On 2014/11/06 00:22:59, Sami wrote:
> Delegated constructors still blocked it looks like:
> https://codereview.chromium.org/695683002/
> 
> If we're sufficiently bothered about this we could consider helping to get
them
> accepted.

Lets just get this patch in :)  I can add a fixme to use them when available.

https://codereview.chromium.org/690703002/diff/60001/content/child/blink_plat...
content/child/blink_platform_impl.cc:511:
main_thread_task_runner_->BelongsToCurrentThread()) {
On 2014/11/06 00:22:59, Sami wrote:
> Could you rewrite this in a way that we don't do the TLS lookup
> MessageLoopProxy::current() if we've got a valid main_thread_task_runner?

Done.

https://codereview.chromium.org/690703002/diff/60001/content/renderer/rendere...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/60001/content/renderer/rendere...
content/renderer/renderer_blink_platform_impl.cc:30: #include
"content/child/webthread_impl.h"
On 2014/11/06 00:22:59, Sami wrote:
> Is this needed?

Done.

[Sami, 2014-11-06 22:20:45]

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:261:
shared_timer_.SetTaskRunner(main_thread_task_runner_);
Couldn't we do this in the base class since we're already giving it the task
runner?

[alex clarke (OOO till 29th), 2014-11-06 22:28:34]

PTAL

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:261:
shared_timer_.SetTaskRunner(main_thread_task_runner_);
On 2014/11/06 22:20:44, Sami wrote:
> Couldn't we do this in the base class since we're already giving it the task
> runner?

Done.

[rmcilroy, 2014-11-06 22:31:19]

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

[rmcilroy, 2014-11-06 22:31:20]

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:233: :
BlinkPlatformImpl(main_thread_task_runner),
Could we only have a renderer_scheduler as an argument to the constructor and
call BlinkPlatformImpl(renderer_scheduler->DefaultTaskRunner()) here instead?

[Sami, 2014-11-06 22:39:35]

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:233: :
BlinkPlatformImpl(main_thread_task_runner),
On 2014/11/06 22:31:20, rmcilroy wrote:
> Could we only have a renderer_scheduler as an argument to the constructor and
> call BlinkPlatformImpl(renderer_scheduler->DefaultTaskRunner()) here instead?

Oh yeah, good idea.

[alex clarke (OOO till 29th), 2014-11-06 22:54:37]

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:233: :
BlinkPlatformImpl(main_thread_task_runner),
On 2014/11/06 22:31:20, rmcilroy wrote:
> Could we only have a renderer_scheduler as an argument to the constructor and
> call BlinkPlatformImpl(renderer_scheduler->DefaultTaskRunner()) here instead?

Done.

https://codereview.chromium.org/690703002/diff/120001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:233: :
BlinkPlatformImpl(main_thread_task_runner),
On 2014/11/06 22:39:35, Sami wrote:
> On 2014/11/06 22:31:20, rmcilroy wrote:
> > Could we only have a renderer_scheduler as an argument to the constructor
and
> > call BlinkPlatformImpl(renderer_scheduler->DefaultTaskRunner()) here
instead?
> 
> Oh yeah, good idea.

Acknowledged.

[Sami, 2014-11-07 05:26:25]

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.cc:452:
shared_timer_.SetTaskRunner(main_thread_task_runner_);
Do we need to check that the runner is non-null here? IIRC you mentioned in can
be null in some cases.

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.h (right):

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.h:168: base::OneShotTimer<BlinkPlatformImpl>
shared_timer_;
nit: no longer needs to be protected.

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

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.cc:452:
shared_timer_.SetTaskRunner(main_thread_task_runner_);
On 2014/11/07 05:26:25, Sami wrote:
> Do we need to check that the runner is non-null here? IIRC you mentioned in
can
> be null in some cases.

It probably doesn't hurt to check, however the problem I was getting before
where the browser tab would crash super early doesn't occur anymore.  I'll add
it if you like.

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.h (right):

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.h:168: base::OneShotTimer<BlinkPlatformImpl>
shared_timer_;
On 2014/11/07 05:26:25, Sami wrote:
> nit: no longer needs to be protected.

Done.

[Sami, 2014-11-07 18:48:34]

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.cc:452:
shared_timer_.SetTaskRunner(main_thread_task_runner_);
On 2014/11/07 15:19:16, alexclarke1 wrote:
> On 2014/11/07 05:26:25, Sami wrote:
> > Do we need to check that the runner is non-null here? IIRC you mentioned in
> can
> > be null in some cases.
> 
> It probably doesn't hurt to check, however the problem I was getting before
> where the browser tab would crash super early doesn't occur anymore.  I'll add
> it if you like.
> 

Ok, I'm still a bit confused how this thing would be null in practice. However
to be consistent we shouldn't then need the null check in currentThread either,
right?

[alex clarke (OOO till 29th), 2014-11-07 19:18:57]

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/690703002/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.cc:452:
shared_timer_.SetTaskRunner(main_thread_task_runner_);
On 2014/11/07 18:48:33, Sami wrote:
> On 2014/11/07 15:19:16, alexclarke1 wrote:
> > On 2014/11/07 05:26:25, Sami wrote:
> > > Do we need to check that the runner is non-null here? IIRC you mentioned
in
> > can
> > > be null in some cases.
> > 
> > It probably doesn't hurt to check, however the problem I was getting before
> > where the browser tab would crash super early doesn't occur anymore.  I'll
add
> > it if you like.
> > 
> 
> Ok, I'm still a bit confused how this thing would be null in practice. However
> to be consistent we shouldn't then need the null check in currentThread
either,
> right?

As discussed offline I added the null check here too since the other one is
needed.

[Sami, 2014-11-07 19:19:54]

Thank you, lgtm!

[alex clarke (OOO till 29th), 2014-11-07 20:21:14]

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

[alex clarke (OOO till 29th), 2014-11-07 20:21:15]

+ jochen@ Can you please review this patch?

Thanks!
Alex Clarke

[jochen (gone - plz use gerrit), 2014-11-09 22:39:44]

will look at it eventually, btw, did you see
https://codereview.chromium.org/507873003

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

On 2014/11/09 22:39:44, jochen (OOO) wrote:
> will look at it eventually, btw, did you see
> https://codereview.chromium.org/507873003

Interesting. I'll make sure Sami and Ross know about that patch.

[jochen (gone - plz use gerrit), 2014-11-10 13:49:04]

lgtm

[alex clarke (OOO till 29th), 2014-11-10 13:50:39]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2014-11-10 13:51:24]

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

[commit-bot: I haz the power, 2014-11-10 14:53:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-11-10 14:54:20]

Patchset 11 (id:??) landed as
https://crrev.com/83a9d24050d94dc3f334ffc13fc43918055fefc9
Cr-Commit-Position: refs/heads/master@{#303463}