Update JingleThreadWrapper to allow it to be created using task runner.

Update JingleThreadWrapper to allow it to be created using task runner.

Previously JingleThreadWrapper required a MessageLoop. In NaCl
we don't have MessageLoop for the main plugin thread, but have a task runner.
This CL allows to use JingleThreadWrapper on the main NaCl thread.

BUG=134216
TBR=tommi@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151458

[Sergey Ulanov, 2012-08-09 22:49:52]

ping

[Wez, 2012-08-09 23:30:25]

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc
File jingle/glue/thread_wrapper.cc (right):

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:34: MessageLoop* message_loop =
MessageLoop::current();
This will only work if called from a MessageLoop; if you used
ThreadTaskRunnerHandle::Get() then it'd work for any thread with a TaskRunner,
although you wouldn't be able to hook destruction.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:165: base::Unretained(this)));
Should this be a WeakPtr?

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:217: base::Unretained(this), task_id));
WeakPtr?

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:221: base::Unretained(this), task_id),
WeakPtr?

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h
File jingle/glue/thread_wrapper.h (right):

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h#n...
jingle/glue/thread_wrapper.h:30: // MessageLoop is destroyed.
I think you mean that the ThreadWrapper will be destroyed automatically when the
MessageLoop is destroyed?

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h#n...
jingle/glue/thread_wrapper.h:31: static void EnsureForCurrentThread();
This should probably become EnsureForCurrentMessageLoop, since it only works on
threads with MessageLoops, and your TaskRunner changes are intended to make this
class usable on non-MessageLoop threads.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h#n...
jingle/glue/thread_wrapper.h:38:
JingleThreadWrapper(scoped_refptr<base::SingleThreadTaskRunner> task_runner);
nit: explicit

[Sergey Ulanov, 2012-08-13 21:15:34]

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc
File jingle/glue/thread_wrapper.cc (right):

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:34: MessageLoop* message_loop =
MessageLoop::current();
On 2012/08/09 23:30:25, Wez wrote:
> This will only work if called from a MessageLoop; if you used
> ThreadTaskRunnerHandle::Get() then it'd work for any thread with a TaskRunner,
> although you wouldn't be able to hook destruction.

Yes. That's intentional. We need it to add destruction observer. So after this
change there are two ways to use this class:

- JingleThreadWrapper::EnsureForCurrentThread() - this autodestroys the wrapper,
  but only works on threads with MessageLoop.
- Explicitly create and destroy JingleThreadWrapper - requires to delete the
wrapper explicitly, but allows to use this object on any thread.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:165: base::Unretained(this)));
On 2012/08/09 23:30:25, Wez wrote:
> Should this be a WeakPtr?

Yes. Thanks for catching this. Fixed.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:217: base::Unretained(this), task_id));
On 2012/08/09 23:30:25, Wez wrote:
> WeakPtr?

Done.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:221: base::Unretained(this), task_id),
On 2012/08/09 23:30:25, Wez wrote:
> WeakPtr?

Done.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h
File jingle/glue/thread_wrapper.h (right):

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h#n...
jingle/glue/thread_wrapper.h:30: // MessageLoop is destroyed.
On 2012/08/09 23:30:25, Wez wrote:
> I think you mean that the ThreadWrapper will be destroyed automatically when
the
> MessageLoop is destroyed?

Done.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h#n...
jingle/glue/thread_wrapper.h:31: static void EnsureForCurrentThread();
On 2012/08/09 23:30:25, Wez wrote:
> This should probably become EnsureForCurrentMessageLoop, since it only works
on
> threads with MessageLoops, and your TaskRunner changes are intended to make
this
> class usable on non-MessageLoop threads.

Good point. Done.

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.h#n...
jingle/glue/thread_wrapper.h:38:
JingleThreadWrapper(scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2012/08/09 23:30:25, Wez wrote:
> nit: explicit

Done.

[Wez, 2012-08-14 00:06:04]

lgtm

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc
File jingle/glue/thread_wrapper.cc (right):

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:34: MessageLoop* message_loop =
MessageLoop::current();
On 2012/08/13 21:15:34, sergeyu wrote:
> On 2012/08/09 23:30:25, Wez wrote:
> > This will only work if called from a MessageLoop; if you used
> > ThreadTaskRunnerHandle::Get() then it'd work for any thread with a
TaskRunner,
> > although you wouldn't be able to hook destruction.
> 
> Yes. That's intentional. We need it to add destruction observer. So after this
> change there are two ways to use this class:
> 
> - JingleThreadWrapper::EnsureForCurrentThread() - this autodestroys the
wrapper,
>   but only works on threads with MessageLoop.
> - Explicitly create and destroy JingleThreadWrapper - requires to delete the
> wrapper explicitly, but allows to use this object on any thread.

Would it be useful to have a LeakyEnsureForCurrentTaskRunner()?

[Sergey Ulanov, 2012-08-14 01:02:14]

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc
File jingle/glue/thread_wrapper.cc (right):

http://codereview.chromium.org/10823224/diff/1/jingle/glue/thread_wrapper.cc#...
jingle/glue/thread_wrapper.cc:34: MessageLoop* message_loop =
MessageLoop::current();
On 2012/08/14 00:06:04, Wez wrote:
> On 2012/08/13 21:15:34, sergeyu wrote:
> > On 2012/08/09 23:30:25, Wez wrote:
> > > This will only work if called from a MessageLoop; if you used
> > > ThreadTaskRunnerHandle::Get() then it'd work for any thread with a
> TaskRunner,
> > > although you wouldn't be able to hook destruction.
> > 
> > Yes. That's intentional. We need it to add destruction observer. So after
this
> > change there are two ways to use this class:
> > 
> > - JingleThreadWrapper::EnsureForCurrentThread() - this autodestroys the
> wrapper,
> >   but only works on threads with MessageLoop.
> > - Explicitly create and destroy JingleThreadWrapper - requires to delete the
> > wrapper explicitly, but allows to use this object on any thread.
> 
> Would it be useful to have a LeakyEnsureForCurrentTaskRunner()?

Don't think it's necessary. It's always possible to just create
JingleThreadWrapper without destroying it.

[commit-bot: I haz the power, 2012-08-14 01:02:26]

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

[commit-bot: I haz the power, 2012-08-14 01:29:51]

Try job failure for 10823224-8001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-14 01:47:03]

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

[commit-bot: I haz the power, 2012-08-14 01:47:07]

Presubmit check for 10823224-11003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer/media

Presubmit checks took 1.2s to calculate.

[commit-bot: I haz the power, 2012-08-14 01:47:53]

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

[commit-bot: I haz the power, 2012-08-14 08:25:29]

Try job failure for 10823224-11003 (retry) (retry) on linux_chromeos for steps
"browser_tests, content_browsertests".
It's a second try, previously, steps "browser_tests, content_browsertests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-14 08:48:13]

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

[commit-bot: I haz the power, 2012-08-14 11:02:56]

Change committed as 151458