[Chromoting] Refactoring DesktopEnvironment and moving screen/audio recorders to ClientSession.

[Chromoting] Refactoring DesktopEnvironment and moving screen/audio recorders to ClientSession.

This CL changes the way screen/audio recorders and event executors are managed. New DesktopEnvironmentFactory class is now used by ChromotingHost's owner to specify the kind of desktop environment (or virtual terminal) to be used by the host. Screen/audio recorders and event executors now owned by the ClientSession instance, so there is a separate set of recorders and stubs exists for each authenticated client session. Clients sessions can now be torn dowsn in parallel with the host shuttting down.

This is the 3rd attempt to land this change. This version includes:
- |ClientSession| objects are torn down asynchronously now.
- |ClientSession| objects are ref-counted to facilitate the asynchronous shutdown. They still have to be used and destroyed on the network thread.
- |ChromotingHost| now waits until all connections are torn down before deleting the session manager.
- The unit tests were fixed to run message loops until all asynchronous object have been destroyed.

BUG=134694
TEST=remoting_unittests

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

[alexeypa (please no reviews), 2012-09-11 20:13:12]

Another attempt to land the DesktopEnvironment refactoring CL. This version
fixes the unit tests to keep the message loops to run long enough. It also
addresses comments from the previos review.

The patch set #1 is the previous version of the CL. The 2nd patch set is the
fixed version.

PTAL. Thanks!

[Sergey Ulanov, 2012-09-11 22:30:16]

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
remoting/host/chromoting_host.cc:19: #include "remoting/host/audio_capturer.h"
Please remove this icnlude.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
remoting/host/chromoting_host.cc:240: if (client->is_authenticated()) {
It's better to move this if and the Stop() call outside of the for loop -
otherwise this code reads as if you stop all clients.
Move erase() call outside of the loop may also improve readability

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
remoting/host/chromoting_host.cc:251: CHECK(!"Could not find the client
session.");
NOTREACHED()? 
Or if you need a message
  NOTREACHED() << "Could not find the client session.".
if you need it to be compiled in release builds then use LOG(FATAL)

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:214: if (!active_recorders_) {
nit: remove {}

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:235: DCHECK(!active_recorders_);
DCHECK for thread please.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:261: if (!active_recorders_) {
remove {}

[Sergey Ulanov, 2012-09-11 22:32:56]

BTW to workaround the problem with trybots failing to apply the patch you can
specify revision using --revision=123342 flag.

[alexeypa (please no reviews), 2012-09-11 22:56:51]

PTAL.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
remoting/host/chromoting_host.cc:19: #include "remoting/host/audio_capturer.h"
On 2012/09/11 22:30:16, sergeyu wrote:
> Please remove this icnlude.

Done.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
remoting/host/chromoting_host.cc:240: if (client->is_authenticated()) {
On 2012/09/11 22:30:16, sergeyu wrote:
> It's better to move this if and the Stop() call outside of the for loop -
> otherwise this code reads as if you stop all clients.
> Move erase() call outside of the loop may also improve readability

Done.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/chrom...
remoting/host/chromoting_host.cc:251: CHECK(!"Could not find the client
session.");
On 2012/09/11 22:30:16, sergeyu wrote:
> NOTREACHED()? 
> Or if you need a message
>   NOTREACHED() << "Could not find the client session.".
> if you need it to be compiled in release builds then use LOG(FATAL) 

I restored the old CHECK after moving the logic out of the loop.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:214: if (!active_recorders_) {
On 2012/09/11 22:30:16, sergeyu wrote:
> nit: remove {}

Done.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:235: DCHECK(!active_recorders_);
On 2012/09/11 22:30:16, sergeyu wrote:
> DCHECK for thread please.

Done. These is a DCHECK in base::NonThreadSafe also.

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:261: if (!active_recorders_) {
On 2012/09/11 22:30:16, sergeyu wrote:
> remove {}

Done.

[Sergey Ulanov, 2012-09-11 23:02:41]

lgtm

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/10915206/diff/4002/remoting/host/clien...
remoting/host/client_session.cc:235: DCHECK(!active_recorders_);
On 2012/09/11 22:56:51, alexeypa wrote:
> On 2012/09/11 22:30:16, sergeyu wrote:
> > DCHECK for thread please.
> 
> Done. These is a DCHECK in base::NonThreadSafe also.

Ah, right, sorry. Then we don't need to duplicate it here.