base: Make it possible to replace the MessageLoop's task runner

base: Make it possible to replace the MessageLoop's task runner

This patch makes it possible to customize the task posting behavior
of a MessageLoop. More specifically, a client can change the value
returned by MessageLoop::task_runner() as well as
ThreadTaskRunnerHandle::Get() on the target thread. The original task
runner can still be used to post tasks to be run on the message loop.

The Blink/renderer scheduler will use this functionality to manage task
posting on the renderer main thread. This is needed to ensure consistent
ordering of tasks posted through the MessageLoop w.r.t. tasks posted to
the scheduler.

Design doc: https://docs.google.com/a/chromium.org/document/d/1qxdh2I61_aB_Uzh1QgNqvdWFBCL_E65G2smoSySw7KU/edit#

Origional patch by Sami Kyostila: https://codereview.chromium.org/998063002/

BUG=465354

[alex clarke (OOO till 29th), 2015-06-24 15:42:43]

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

[commit-bot: I haz the power, 2015-06-24 15:43:42]

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

[commit-bot: I haz the power, 2015-06-24 18:29:05]

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

[commit-bot: I haz the power, 2015-06-24 18:29:05]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-06-25 10:58:59]

alexclarke@chromium.org changed reviewers:
+ danakj@chromium.org, jam@chromium.org, kinuko@chromium.org

[alex clarke (OOO till 29th), 2015-06-25 10:58:59]

Sami is on vacation.  And I'm hoping to get this patch landed on his behalf.  My
understanding is that MessageLoopProxy is now gone and it's now possible to
proceed with this refactor.

Please take a look.

[alex clarke (OOO till 29th), 2015-06-25 11:02:06]

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

[commit-bot: I haz the power, 2015-06-25 11:04:15]

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

[commit-bot: I haz the power, 2015-06-25 11:43:56]

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

[commit-bot: I haz the power, 2015-06-25 11:43:57]

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

[alex clarke (OOO till 29th), 2015-06-25 13:03:44]

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

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

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

[commit-bot: I haz the power, 2015-06-25 14:41:29]

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

[commit-bot: I haz the power, 2015-06-25 14:41:30]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-06-29 17:21:29]

Ping :)

[danakj, 2015-06-29 19:07:23]

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/binda...
File base/message_loop/bindable_single_thread_task_runner.h (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/binda...
base/message_loop/bindable_single_thread_task_runner.h:12: // A
SingleThreadTaskRunner which can be bound to a specific thread after it
what does "can be bound to a specific thread" here mean. Does it mean "must be
bound to a thread before being used, and only used on that thread"? Does it mean
"optionally may be bound to a thread" (then what if it isn't)? Please add some
mode details here.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:407: SwapTaskRunner(task_runner_);
can we DCHECK that the return from this is null?

can you .Pass() the task_runner_?

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:407: SwapTaskRunner(task_runner_);
Why do we want to support both:

1. SwapTaskRunner
2. BindToCurrentThread

and

1. BindToCurrentThread
2. SwapTaskRunner

?

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:418: // This reset() is needed or
ThreadTaskRunnerHandle::Get() will crash.
We're not calling Get() here.. can you expand the explanation in this comment?

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:301:
scoped_refptr<BindableSingleThreadTaskRunner> task_runner() {
What happened to the TODO?

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:429: static scoped_ptr<MessageLoop>
CreateUnbound(
Can you make this static in a separate patch?

[danakj, 2015-06-29 19:09:06]

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:301:
scoped_refptr<BindableSingleThreadTaskRunner> task_runner() {
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> What happened to the TODO?

Should the fact that this is Bindable really be public? Should this instead
return a STTR?

[kinuko, 2015-06-30 02:26:56]

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:311:
scoped_refptr<BindableSingleThreadTaskRunner> task_runner);
Would we want to call this before the message loop's bound to a thread (other
than in test)?  Creating an unbound message loop is a private construction
method, if there're not such cases just asserting that the loop's already bound
might be simpler/clearer?

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:429: static scoped_ptr<MessageLoop>
CreateUnbound(
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> Can you make this static in a separate patch?

This's already in ToT, you can drop it from this patch (sorry for the
confusion).

[alex clarke (OOO till 29th), 2015-06-30 10:51:44]

PTAL

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/binda...
File base/message_loop/bindable_single_thread_task_runner.h (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/binda...
base/message_loop/bindable_single_thread_task_runner.h:12: // A
SingleThreadTaskRunner which can be bound to a specific thread after it
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> what does "can be bound to a specific thread" here mean. Does it mean "must be
> bound to a thread before being used, and only used on that thread"? Does it
mean
> "optionally may be bound to a thread" (then what if it isn't)? Please add some
> mode details here.

Done.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:407: SwapTaskRunner(task_runner_);
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> can we DCHECK that the return from this is null?
> 
> can you .Pass() the task_runner_?

Done.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:407: SwapTaskRunner(task_runner_);
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> Why do we want to support both:
> 
> 1. SwapTaskRunner
> 2. BindToCurrentThread
> 
> and
> 
> 1. BindToCurrentThread
> 2. SwapTaskRunner
> 
> ?

I don't know, and Sami is currently OOO so I can't ask him.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:418: // This reset() is needed or
ThreadTaskRunnerHandle::Get() will crash.
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> We're not calling Get() here.. can you expand the explanation in this comment?

It's there because in the original review kinuko asked why there are two resets.
 This does look weird, and it's because reset() doesn't behave the way we would
expect.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:301:
scoped_refptr<BindableSingleThreadTaskRunner> task_runner() {
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> What happened to the TODO?

That was a mistake.  I note it compiles if we change it to:

const scoped_refptr<BindableSingleThreadTaskRunner>& task_runner() {
  return task_runner_;
}

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:301:
scoped_refptr<BindableSingleThreadTaskRunner> task_runner() {
On 2015/06/29 19:09:05, danakj OOO back june 28 wrote:
> On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> > What happened to the TODO?
> 
> Should the fact that this is Bindable really be public? Should this instead
> return a STTR?

I agree the fact it's Bindable feels like an implementation detail that ideally
shouldn't be public.

I think we have the following options:

1. Return const scoped_refptr<BindableSingleThreadTaskRunner>&
2. Add a base_task_runner_ as well and return const
scoped_refptr<SingleThreadTaskRunner>& here
3. Regard returning a const reference as micro-optimization and leave it as is,
and remove the todo.

I think I prefer #1 or #3, WDYT?

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:311:
scoped_refptr<BindableSingleThreadTaskRunner> task_runner);
On 2015/06/30 02:26:55, kinuko wrote:
> Would we want to call this before the message loop's bound to a thread (other
> than in test)?  

Apparently yes, see SchedulerMessageLoopDelegate::SetDefaultTaskRunner in
https://codereview.chromium.org/987193002/


> Creating an unbound message loop is a private construction
> method, if there're not such cases just asserting that the loop's already
bound
> might be simpler/clearer?

I think it would have been cleaner, but it seems the method is needed.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:429: static scoped_ptr<MessageLoop>
CreateUnbound(
On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> Can you make this static in a separate patch?

Acknowledged.

https://codereview.chromium.org/1206893003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.h:429: static scoped_ptr<MessageLoop>
CreateUnbound(
On 2015/06/30 02:26:55, kinuko wrote:
> On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
> > Can you make this static in a separate patch?
> 
> This's already in ToT, you can drop it from this patch (sorry for the
> confusion).

Acknowledged.

[alex clarke (OOO till 29th), 2015-07-02 13:25:49]

PING :)

[Sami, 2015-07-07 13:11:10]

Thanks for shepherding this while I was away Alex. Since Rietveld doesn't let
you transfer code reviews between accounts, I'll just resume the original review
and respond to the comments there: https://codereview.chromium.org/998063002/