Make JingleThreadMessageLoop usable without JingleThread.

Make JingleThreadMessageLoop usable without JingleThread.

BUG=None
TEST=Unittests

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

[Sergey Ulanov, 2011-07-01 18:17:45]

I will use JingleThreadMessageLoop to simplify JingleSessionTest in my next CL.

[Wez, 2011-07-01 20:33:00]

Comments ahoy!

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:77: MessageLoop* message_loop_;
Normally the pump is created by the relevant MessageLoop, which passes itself to
the pump's Run(), I think.  Would that layout not work here?

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:108: }
Sorry, I'm not clear on what TaskPump is actually for, here?

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.h (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:34: class JingleThreadMessageLoop : public
MessageLoop {
Isn't this really a Jingle-aware MessageLoop, so no need for Thread in the name?

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:43: // created here because we never call
Run() or RunAllPending() for
nit: It's not being created here, this is just the definition of the member that
holds it.

AutoRunState is actually for use when nesting MessageLoop::Run() calls, to
ensure that only the top-most Run() is exited by Quit() calls made within it. 
It also seems to be an un-documented detail within MessageLoop, so I'm dubious
about using it, especially in this context.

Would it make more sense to call talk_base::Thread::Run() *through*
JingleThreadMessageLoop::Run()?  The latter Run call would then create the
JingleMessagePump, create the AutoRunState on the stack, and call Run() on the
pump with itself as the delegate; the pump would then call
talk_base::Thread::Run().

[Sergey Ulanov, 2011-07-01 21:14:28]

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:77: MessageLoop* message_loop_;
On 2011/07/01 20:33:00, Wez wrote:
> Normally the pump is created by the relevant MessageLoop, which passes itself
to
> the pump's Run(), I think.  Would that layout not work here?
No, because nobody ever calls Run() on this Pump. MessageLoop calls
ScheduleWork() and ScheduleDelayedWork() when new tasks are posted, and then the
tasks are executed in OnMessage(). A more elegant solution would be possible if
Post() and PostDelayed() were virtual in MessageLoop, but they are not.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:108: }
On 2011/07/01 20:33:00, Wez wrote:
> Sorry, I'm not clear on what TaskPump is actually for, here?
It's an implementation of talk_base::TaskRunner that we need to run XMPP code.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.h (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:34: class JingleThreadMessageLoop : public
MessageLoop {
On 2011/07/01 20:33:00, Wez wrote:
> Isn't this really a Jingle-aware MessageLoop, so no need for Thread in the
name?

It's a MessageLoop that runs tasks on a libjingle thread. JingleMessageLoop - is
very confusing. For example it may be interpreted as a MessageLoop that can run
libjingle tasks/messages.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:43: // created here because we never call
Run() or RunAllPending() for
On 2011/07/01 20:33:00, Wez wrote:
> nit: It's not being created here, this is just the definition of the member
that
> holds it.
Moved the comment.

> 
> AutoRunState is actually for use when nesting MessageLoop::Run() calls, to
> ensure that only the top-most Run() is exited by Quit() calls made within it. 
> It also seems to be an un-documented detail within MessageLoop, so I'm dubious
> about using it, especially in this context.
Yes, it's a bit hacky, but MessageLoop was never intended to be used in a ways
we use it here. We will be able to get rid of this code when we switch to using
chromium-based IO in the host.

> Would it make more sense to call talk_base::Thread::Run() *through*
> JingleThreadMessageLoop::Run()?  The latter Run call would then create the
> JingleMessagePump, create the AutoRunState on the stack, and call Run() on the
> pump with itself as the delegate; the pump would then call
> talk_base::Thread::Run().
Yes, we have something like this in jingle/glue/thread_wrapper.[h|cc]. This
class accomplishes the opposite task: it runs MessageLoop on a jingle thread.
The reason we need it is because we use libjingle-based IO (on host and in
tests) which works only on a thread that runs talk_base::Thread::Run(). If try
to use libjingle IO code on a regular chromium code it will not work because
there is no way it can get async IO notifications.

[Wez, 2011-07-01 21:49:28]

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:108: }
On 2011/07/01 21:14:28, sergeyu wrote:
> On 2011/07/01 20:33:00, Wez wrote:
> > Sorry, I'm not clear on what TaskPump is actually for, here?
> It's an implementation of talk_base::TaskRunner that we need to run XMPP code.

Yes, what I was really asking is that, given it's the "obvious" implementation,
is it even required?  Does talk_base not already have a TaskRunner
implementation of this sort?

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:157: // Returns task pump if the thread is
running, otherwise NULL is returned.
nit: This comment is already present in the header.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:158: TaskPump* JingleThread::task_pump() {
nit: Presumably this is only callable from the JingleThread itself?

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.h (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:34: class JingleThreadMessageLoop : public
MessageLoop {
On 2011/07/01 21:14:28, sergeyu wrote:
> On 2011/07/01 20:33:00, Wez wrote:
> > Isn't this really a Jingle-aware MessageLoop, so no need for Thread in the
> name?
> 
> It's a MessageLoop that runs tasks on a libjingle thread. JingleMessageLoop -
is
> very confusing. For example it may be interpreted as a MessageLoop that can
run
> libjingle tasks/messages.

OK, understood.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:43: // created here because we never call
Run() or RunAllPending() for
On 2011/07/01 21:14:28, sergeyu wrote:
> On 2011/07/01 20:33:00, Wez wrote:
> > nit: It's not being created here, this is just the definition of the member
> that
> > holds it.
> Moved the comment.
> 
> > 
> > AutoRunState is actually for use when nesting MessageLoop::Run() calls, to
> > ensure that only the top-most Run() is exited by Quit() calls made within
it. 
> > It also seems to be an un-documented detail within MessageLoop, so I'm
dubious
> > about using it, especially in this context.
> Yes, it's a bit hacky, but MessageLoop was never intended to be used in a ways
> we use it here. We will be able to get rid of this code when we switch to
using
> chromium-based IO in the host.
> 
> > Would it make more sense to call talk_base::Thread::Run() *through*
> > JingleThreadMessageLoop::Run()?  The latter Run call would then create the
> > JingleMessagePump, create the AutoRunState on the stack, and call Run() on
the
> > pump with itself as the delegate; the pump would then call
> > talk_base::Thread::Run().
> Yes, we have something like this in jingle/glue/thread_wrapper.[h|cc]. This
> class accomplishes the opposite task: it runs MessageLoop on a jingle thread.
> The reason we need it is because we use libjingle-based IO (on host and in
> tests) which works only on a thread that runs talk_base::Thread::Run(). If try
> to use libjingle IO code on a regular chromium code it will not work because
> there is no way it can get async IO notifications.

Yes, but what I'm suggesting is that since we are in control of the
JingleThread::Run(), we can have it create the MessageLoop & Run() it, then have
that Run() the MessagePump, and finally have the MessagePump call
talk_base::Thread::Run()?  That way jingle has the right kind of thread to run
on.

[Sergey Ulanov, 2011-07-01 22:40:54]

Thanks for suggestions on how to improve this code!

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:108: }
On 2011/07/01 21:49:28, Wez wrote:
> On 2011/07/01 21:14:28, sergeyu wrote:
> > On 2011/07/01 20:33:00, Wez wrote:
> > > Sorry, I'm not clear on what TaskPump is actually for, here?
> > It's an implementation of talk_base::TaskRunner that we need to run XMPP
code.
> 
> Yes, what I was really asking is that, given it's the "obvious"
implementation,
> is it even required?  Does talk_base not already have a TaskRunner
> implementation of this sort?
There is no default implementation of TaskRunner in libjingle. I don't know the
reason for that.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:157: // Returns task pump if the thread is
running, otherwise NULL is returned.
On 2011/07/01 21:49:28, Wez wrote:
> nit: This comment is already present in the header.

Done.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:158: TaskPump* JingleThread::task_pump() {
On 2011/07/01 21:49:28, Wez wrote:
> nit: Presumably this is only callable from the JingleThread itself?
No, XmppSignalStrategy calls it.

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.h (right):

http://codereview.chromium.org/7302002/diff/2002/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.h:43: // created here because we never call
Run() or RunAllPending() for
On 2011/07/01 21:49:28, Wez wrote:
> On 2011/07/01 21:14:28, sergeyu wrote:
> > On 2011/07/01 20:33:00, Wez wrote:
> > > nit: It's not being created here, this is just the definition of the
member
> > that
> > > holds it.
> > Moved the comment.
> > 
> > > 
> > > AutoRunState is actually for use when nesting MessageLoop::Run() calls, to
> > > ensure that only the top-most Run() is exited by Quit() calls made within
> it. 
> > > It also seems to be an un-documented detail within MessageLoop, so I'm
> dubious
> > > about using it, especially in this context.
> > Yes, it's a bit hacky, but MessageLoop was never intended to be used in a
ways
> > we use it here. We will be able to get rid of this code when we switch to
> using
> > chromium-based IO in the host.
> > 
> > > Would it make more sense to call talk_base::Thread::Run() *through*
> > > JingleThreadMessageLoop::Run()?  The latter Run call would then create the
> > > JingleMessagePump, create the AutoRunState on the stack, and call Run() on
> the
> > > pump with itself as the delegate; the pump would then call
> > > talk_base::Thread::Run().
> > Yes, we have something like this in jingle/glue/thread_wrapper.[h|cc]. This
> > class accomplishes the opposite task: it runs MessageLoop on a jingle
thread.
> > The reason we need it is because we use libjingle-based IO (on host and in
> > tests) which works only on a thread that runs talk_base::Thread::Run(). If
try
> > to use libjingle IO code on a regular chromium code it will not work because
> > there is no way it can get async IO notifications.
> 
> Yes, but what I'm suggesting is that since we are in control of the
> JingleThread::Run(), we can have it create the MessageLoop & Run() it, then
have
> that Run() the MessagePump, and finally have the MessagePump call
> talk_base::Thread::Run()?  That way jingle has the right kind of thread to run
> on.

Done.

[Wez, 2011-07-01 22:49:08]

LGTM :)