Add base::ThreadTaskRunner class.

Add base::ThreadTaskRunnerHandler class.

The class will be used to hold a reference to the thread's task runner.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=136110

[willchan no longer on Chromium, 2012-05-07 20:32:10]

I don't like the name being TaskRunner suffixed. I think Handle is necessary,
otherwise it gets confused with actual TaskRunners.

Maybe we could make Handle a public nested class and add a public static
CurrentHandle() function on SingleThreadTaskRunner. I dunno.

Sorry, not much time to think about this, I'm at a conference.

[Sergey Ulanov, 2012-05-07 23:51:10]

On 2012/05/07 20:32:10, willchan wrote:
> I don't like the name being TaskRunner suffixed. I think Handle is necessary,
> otherwise it gets confused with actual TaskRunners.

Renamed it to ThreadTaskRunnerHandle.

> 
> Maybe we could make Handle a public nested class and add a public static
> CurrentHandle() function on SingleThreadTaskRunner. I dunno.

I don't think that is good idea. If we make it a nested class then it can be
mistakenly interpreted as if the Handle is required for a
SingleThreadTaskRunner.

> 
> Sorry, not much time to think about this, I'm at a conference.

[willchan no longer on Chromium, 2012-05-08 00:26:09]

Other folks chime in by tomorrow afternoon please. Otherwise I will approve
as is. Thanks.
On May 7, 2012 4:51 PM, <sergeyu@chromium.org> wrote:

> On 2012/05/07 20:32:10, willchan wrote:
>
>> I don't like the name being TaskRunner suffixed. I think Handle is
>> necessary,
>> otherwise it gets confused with actual TaskRunners.
>>
>
> Renamed it to ThreadTaskRunnerHandle.
>
>
>  Maybe we could make Handle a public nested class and add a public static
>> CurrentHandle() function on SingleThreadTaskRunner. I dunno.
>>
>
> I don't think that is good idea. If we make it a nested class then it can
> be
> mistakenly interpreted as if the Handle is required for a
> SingleThreadTaskRunner.
>
>
>  Sorry, not much time to think about this, I'm at a conference.
>>
>
>
>
>
http://codereview.chromium.**org/10380016/<http://codereview.chromium.org/103...
>

[Ami GONE FROM CHROMIUM, 2012-05-08 01:30:14]

LGTM % English

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
File base/thread_task_runner_handle.h (right):

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:15: // ThreadTaskRunner stores reference to the
main task runner for each
s/stores/stores a/

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:16: // thread. Not more than one of these
objects created per
s/created/can be created/

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:18: // function will be returning the
|task_runner| specified in the
s/be returning/return/

[willchan no longer on Chromium, 2012-05-08 02:35:06]

Sorry, I didn't actually read the code last time :) I have some nits, and after
that, LGTM, since I think I was only waiting for Ami (Wez appears to be OOO).

Oh, and please fix the CL description and Subject to have the right name.

http://codereview.chromium.org/10380016/diff/2001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/10380016/diff/2001/base/message_loop.h#newcode521
base/message_loop.h:521: // The message loop proxy associated with this message
loop, if one exists.
Can we construct these guys in the initializer list and make them const? There's
no reason for ThreadTaskRunnerHandle to be scoped_ptr and non-const, right?

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
File base/thread_task_runner_handle.h (right):

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:15: // ThreadTaskRunner stores reference to the
main task runner for each
s/ThreadTaskRunner/ThreadTaskRunnerHandle/

[Sergey Ulanov, 2012-05-09 18:34:11]

http://codereview.chromium.org/10380016/diff/2001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/10380016/diff/2001/base/message_loop.h#newcode521
base/message_loop.h:521: // The message loop proxy associated with this message
loop, if one exists.
On 2012/05/08 02:35:06, willchan wrote:
> Can we construct these guys in the initializer list and make them const?
There's
> no reason for ThreadTaskRunnerHandle to be scoped_ptr and non-const, right?

I thought about it - problem is that MessageLoopProxyImpl depends on
MessageLoop::current() and so it must be created only after current() is set.

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
File base/thread_task_runner_handle.h (right):

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:15: // ThreadTaskRunner stores reference to the
main task runner for each
On 2012/05/08 01:30:14, Ami Fischman wrote:
> s/stores/stores a/

Done.

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:15: // ThreadTaskRunner stores reference to the
main task runner for each
On 2012/05/08 02:35:06, willchan wrote:
> s/ThreadTaskRunner/ThreadTaskRunnerHandle/

Done.

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:16: // thread. Not more than one of these
objects created per
On 2012/05/08 01:30:14, Ami Fischman wrote:
> s/created/can be created/

Done.

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:16: // thread. Not more than one of these
objects created per
On 2012/05/08 01:30:14, Ami Fischman wrote:
> s/created/can be created/

Done.

http://codereview.chromium.org/10380016/diff/2001/base/thread_task_runner_han...
base/thread_task_runner_handle.h:18: // function will be returning the
|task_runner| specified in the
On 2012/05/08 01:30:14, Ami Fischman wrote:
> s/be returning/return/

Done.

[commit-bot: I haz the power, 2012-05-09 18:35:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10380016/8001

[willchan no longer on Chromium, 2012-05-09 18:38:48]

http://codereview.chromium.org/10380016/diff/2001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/10380016/diff/2001/base/message_loop.h#newcode521
base/message_loop.h:521: // The message loop proxy associated with this message
loop, if one exists.
On 2012/05/09 18:34:11, sergeyu wrote:
> On 2012/05/08 02:35:06, willchan wrote:
> > Can we construct these guys in the initializer list and make them const?
> There's
> > no reason for ThreadTaskRunnerHandle to be scoped_ptr and non-const, right?
> 
> I thought about it - problem is that MessageLoopProxyImpl depends on
> MessageLoop::current() and so it must be created only after current() is set.

Good point. OK.

[commit-bot: I haz the power, 2012-05-09 19:59:24]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Ryan Sleevi, 2012-05-31 19:36:40]

https://chromiumcodereview.appspot.com/10380016/diff/8001/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10380016/diff/8001/base/message_loop.c...
base/message_loop.cc:204: thread_task_runner_handle_.reset();
Not to resurrect a dead review, but when looking at this code after chatting
with Will on IRC, shouldn't this code run /after/
WillDestroyCurrentMessageLoop()?

Currently, any code that executes in WillDestroyCurrentMessageLoop() can
"safely" call MessageLoopProxy::current() and still get the current proxy (even
if their tasks will never run).

With this change, any code updated to use ThreadTaskRunnerHandle::Get(), that
runs within the context of WillDestroyCurrentMessageLoop(), will get NULL
instead. Seems like a bug?

(Or not, but I was hoping someone could comment on this)

[Sergey Ulanov, 2012-05-31 20:14:04]

https://chromiumcodereview.appspot.com/10380016/diff/8001/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10380016/diff/8001/base/message_loop.c...
base/message_loop.cc:204: thread_task_runner_handle_.reset();
On 2012/05/31 19:36:41, Ryan Sleevi wrote:
> Not to resurrect a dead review, but when looking at this code after chatting
> with Will on IRC, shouldn't this code run /after/
> WillDestroyCurrentMessageLoop()?
> 
> Currently, any code that executes in WillDestroyCurrentMessageLoop() can
> "safely" call MessageLoopProxy::current() and still get the current proxy
(even
> if their tasks will never run).
> 
> With this change, any code updated to use ThreadTaskRunnerHandle::Get(), that
> runs within the context of WillDestroyCurrentMessageLoop(), will get NULL
> instead. Seems like a bug?
> 
> (Or not, but I was hoping someone could comment on this)

This code runs only after all message loop destruction observers are notified -
see FOR_EACH_OBSERVER() two lines above.
MessageLoopProxy::WillDestroyCurrentMessageLoop() is called later, but that
shouldn't matter - MessageLoopProxy shouldn't depend on ThreadTaskRunnerHandle.