Fix thread usage in chromoting host

Fix thread usage in chromoting host

There are several things done in this patch:
1. Isloate thread start and stop to ChromotingHostContext
2. SessionManager now doesn't own capturer and encoder, ownership moved to ChromotingHost
3. Fix up the sequence of actions when ChromotingHost shuts down

TEST=remoting_unittests
BUG=none

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

[Alpha Left Google, 2010-06-24 01:32:22]

This change is getting too big so I'm submitting this first. Unit tests will
follow.

[awong, 2010-06-24 01:45:47]

http://codereview.chromium.org/2829018/diff/5001/6001
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/2829018/diff/5001/6001#newcode147
remoting/host/chromoting_host.cc:147: FROM_HERE, NewRunnableMethod(this,
&ChromotingHost::DoShutdown));
After this point, are we expecting that no tasks are going to be run on this
object anymore?

We weren't ever very clear on this previously.

I wonder if we need a state variable that will No-op all our Do* functions...

http://codereview.chromium.org/2829018/diff/5001/6002
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/2829018/diff/5001/6002#newcode39
remoting/host/chromoting_host.h:39: //    screen captures. A EventExecutor is
created and registered with the
We've got multiple names for this kind of thing. Executor, Callback, etc.  Can
we settle on one name and normalize it across all our code?  I vote for Handler,
but whatever works. Just as long as it is consistent.

http://codereview.chromium.org/2829018/diff/5001/6002#newcode95
remoting/host/chromoting_host.h:95: // This method talks to the cloud to
register the host process. If
Let's get terms clear here.  You mean, login to the talk network.

I'd like to only say "register" when we're actually literally registering with
CRAPI.

http://codereview.chromium.org/2829018/diff/5001/6002#newcode133
remoting/host/chromoting_host.h:133: // cannot started of Shutdown() is called.
this sentence doesn't parse...can you reword it?

http://codereview.chromium.org/2829018/diff/5001/6004
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/2829018/diff/5001/6004#newcode17
remoting/host/chromoting_host_context.h:17: class ChromotingHostContext {
Is the main purpose of this avoid needing to pass each messageloop/thread in the
constructor individually?

That's fine...jsut wanted to be sure.

http://codereview.chromium.org/2829018/diff/5001/6004#newcode20
remoting/host/chromoting_host_context.h:20: virtual ~ChromotingHostContext();
Why all the viruals?  Are we expecting to inherit from this class??

http://codereview.chromium.org/2829018/diff/5001/6005
File remoting/host/chromoting_host_context_unittest.cc (right):

http://codereview.chromium.org/2829018/diff/5001/6005#newcode20
remoting/host/chromoting_host_context_unittest.cc:20: context.Stop();
Should we add expectations that check if the thread is stopped?

[Alpha Left Google, 2010-06-24 19:35:35]

http://codereview.chromium.org/2829018/diff/5001/6001
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/2829018/diff/5001/6001#newcode147
remoting/host/chromoting_host.cc:147: FROM_HERE, NewRunnableMethod(this,
&ChromotingHost::DoShutdown));
On 2010/06/24 01:45:47, awong wrote:
> After this point, are we expecting that no tasks are going to be run on this
> object anymore?
> 
> We weren't ever very clear on this previously.
> 
> I wonder if we need a state variable that will No-op all our Do* functions...

For this object we are only suppose to Start() and Shutdown() once. That can be
stated clear in the comments, besides that DoShutdown() already noops for
certain conditions.

http://codereview.chromium.org/2829018/diff/5001/6002
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/2829018/diff/5001/6002#newcode39
remoting/host/chromoting_host.h:39: //    screen captures. A EventExecutor is
created and registered with the
On 2010/06/24 01:45:47, awong wrote:
> We've got multiple names for this kind of thing. Executor, Callback, etc.  Can
> we settle on one name and normalize it across all our code?  I vote for
Handler,
> but whatever works. Just as long as it is consistent.

EventExecutor is a class that executes mouse / keyboard events, not the event
handler for classes we are talking about.

I thought in chromoting code I uses EventHandler across the board.

http://codereview.chromium.org/2829018/diff/5001/6002#newcode95
remoting/host/chromoting_host.h:95: // This method talks to the cloud to
register the host process. If
On 2010/06/24 01:45:47, awong wrote:
> Let's get terms clear here.  You mean, login to the talk network.
> 
> I'd like to only say "register" when we're actually literally registering with
> CRAPI.

Done.

http://codereview.chromium.org/2829018/diff/5001/6002#newcode133
remoting/host/chromoting_host.h:133: // cannot started of Shutdown() is called.
On 2010/06/24 01:45:47, awong wrote:
> this sentence doesn't parse...can you reword it?

Done.

http://codereview.chromium.org/2829018/diff/5001/6004
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/2829018/diff/5001/6004#newcode17
remoting/host/chromoting_host_context.h:17: class ChromotingHostContext {
On 2010/06/24 01:45:47, awong wrote:
> Is the main purpose of this avoid needing to pass each messageloop/thread in
the
> constructor individually?
> 
> That's fine...jsut wanted to be sure.

That's right. Just need to manage one object instead of multiple threads and
message loops. Can also make sure thread start / stop order is correct since
there's only once that does that.

http://codereview.chromium.org/2829018/diff/5001/6004#newcode20
remoting/host/chromoting_host_context.h:20: virtual ~ChromotingHostContext();
On 2010/06/24 01:45:47, awong wrote:
> Why all the viruals?  Are we expecting to inherit from this class??

For testing, yes.

http://codereview.chromium.org/2829018/diff/5001/6005
File remoting/host/chromoting_host_context_unittest.cc (right):

http://codereview.chromium.org/2829018/diff/5001/6005#newcode20
remoting/host/chromoting_host_context_unittest.cc:20: context.Stop();
On 2010/06/24 01:45:47, awong wrote:
> Should we add expectations that check if the thread is stopped?

Sure.

[awong, 2010-06-25 20:52:46]

http://codereview.chromium.org/2829018/diff/5001/6001
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/2829018/diff/5001/6001#newcode147
remoting/host/chromoting_host.cc:147: FROM_HERE, NewRunnableMethod(this,
&ChromotingHost::DoShutdown));
On 2010/06/24 19:35:36, Alpha wrote:
> On 2010/06/24 01:45:47, awong wrote:
> > After this point, are we expecting that no tasks are going to be run on this
> > object anymore?
> > 
> > We weren't ever very clear on this previously.
> > 
> > I wonder if we need a state variable that will No-op all our Do*
functions...
> 
> For this object we are only suppose to Start() and Shutdown() once. That can
be
> stated clear in the comments, besides that DoShutdown() already noops for
> certain conditions.


Sorry, I meant the On* functions.  Or is that overkill?

[Alpha Left Google, 2010-06-25 21:26:45]

http://codereview.chromium.org/2829018/diff/5001/6001
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/2829018/diff/5001/6001#newcode147
remoting/host/chromoting_host.cc:147: FROM_HERE, NewRunnableMethod(this,
&ChromotingHost::DoShutdown));
On 2010/06/25 20:52:46, awong wrote:
> On 2010/06/24 19:35:36, Alpha wrote:
> > On 2010/06/24 01:45:47, awong wrote:
> > > After this point, are we expecting that no tasks are going to be run on
this
> > > object anymore?
> > > 
> > > We weren't ever very clear on this previously.
> > > 
> > > I wonder if we need a state variable that will No-op all our Do*
> functions...
> > 
> > For this object we are only suppose to Start() and Shutdown() once. That can
> be
> > stated clear in the comments, besides that DoShutdown() already noops for
> > certain conditions.
> 
> 
> Sorry, I meant the On* functions.  Or is that overkill?

Sure, will do.

[awong, 2010-06-28 20:37:33]

LGTM

Assuming the remoting.gyp dependencies are removed again.

http://codereview.chromium.org/2829018/diff/22001/23014
File remoting/remoting.gyp (right):

http://codereview.chromium.org/2829018/diff/22001/23014#newcode310
remoting/remoting.gyp:310: # TODO(hclam): Remove all these extra dependencies
notifier is moved
Rebase and whack these dependencies?