[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.

BUG=134694
TEST=remoting_unittests

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

[alexeypa (please no reviews), 2012-08-30 20:54:13]

Please take a look. Thanks!

[Wez, 2012-08-31 18:18:02]

I'd prefer that DesktopEnvironmentFactory be a pure interface with a "standard"
user-desktop remoting implementation, and the Windows-specific "session"
implementation.

DesktopEnvironment should ideally be an interface as well, so that
implementations can easily associate other data (e.g. the IPC channel to the
desktop integration process) with the environment, choose to create stubs
on-demand, etc.

That arrangement could be taken a step further, by having
SessionDesktopEnvironment wrap a "standard" DesktopEnvironment, passing through
requests for the various stubs, except for returning the SessionEventExecutor
wrapping the underlying DE's EventExecutor, with the associated change in
ownership.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.cc:80: // check.
nit: If AudioCapturer::Create were to DCHECK if called on an unsupported
platform then I think we could do away with this comment. Alternatively, simply
state that AudioCapturer::Create() is currently only implemented for Windows
here.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.cc:81: #if !defined(OS_WIN)
nit: Move the #if to enclose the comment describing this code block.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.cc:299: // Create all event executors and
capturers.
nit: "Create the desktop integration implementation for the client to use."

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:128: // Then we create a ScreenRecorder passing
the message loops that
nit: Drop "Then we".

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:151: // Immediately add the connection and start
the session.
nit: Suggest just "Start the session."

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.h:81: scoped_refptr<base::SingleThreadTaskRunner>
network_task_runner,
Pass the ChromotingHostContext in here, so that ClientSession can grab the
TaskRunners it needs without them being passed individually here?

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/desktop_...
File remoting/host/desktop_environment.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/desktop_...
remoting/host/desktop_environment.h:35: virtual void StopAndDelete();
We don't actually need StopAndDelete() here, since we could StopAndDelete() the
EventExecutor in the DesktopEnvironment destructor; is this pre-emptive
preparation for the multi-process implementation?

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:123: desktop_environment_factory_(new
SessionDesktopEnvironmentFactory()),
Isn't SessionDesktopEnvironmentFactory Windows-only now that it lives in
remoting/host/win/?

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/win/sess...
File remoting/host/win/session_desktop_environment_factory.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/win/sess...
remoting/host/win/session_desktop_environment_factory.cc:27: #if defined(OS_WIN)
This only gets build on Windows, so I don't think you need these checks any
more?

[alexeypa (please no reviews), 2012-08-31 23:13:30]

> I'd prefer that DesktopEnvironmentFactory be a pure interface with a
"standard"
> user-desktop remoting implementation, and the Windows-specific "session"
> implementation.

> DesktopEnvironment should ideally be an interface as well, so that
> implementations can easily associate other data (e.g. the IPC channel to the
> desktop integration process) with the environment, choose to create stubs
> on-demand, etc.

> That arrangement could be taken a step further, by having
> SessionDesktopEnvironment wrap a "standard" DesktopEnvironment, passing
through
> requests for the various stubs, except for returning the SessionEventExecutor
> wrapping the underlying DE's EventExecutor, with the associated change in
> ownership.

I'll do it as a separate CL. This one is large enough already.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.cc:80: // check.
On 2012/08/31 18:18:02, Wez wrote:
> nit: If AudioCapturer::Create were to DCHECK if called on an unsupported
> platform then I think we could do away with this comment. Alternatively,
simply
> state that AudioCapturer::Create() is currently only implemented for Windows
> here.

Done.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.cc:81: #if !defined(OS_WIN)
On 2012/08/31 18:18:02, Wez wrote:
> nit: Move the #if to enclose the comment describing this code block.

Done.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.cc:299: // Create all event executors and
capturers.
On 2012/08/31 18:18:02, Wez wrote:
> nit: "Create the desktop integration implementation for the client to use."

Done.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:128: // Then we create a ScreenRecorder passing
the message loops that
On 2012/08/31 18:18:02, Wez wrote:
> nit: Drop "Then we".

Done.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:151: // Immediately add the connection and start
the session.
On 2012/08/31 18:18:02, Wez wrote:
> nit: Suggest just "Start the session."

Done.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.h:81: scoped_refptr<base::SingleThreadTaskRunner>
network_task_runner,
On 2012/08/31 18:18:02, Wez wrote:
> Pass the ChromotingHostContext in here, so that ClientSession can grab the
> TaskRunners it needs without them being passed individually here?

I didn't want to pass ChromotingHostContext because we eventually want to get
rid of it, letting the actual objects who need the task runners to hold
references to them.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/desktop_...
File remoting/host/desktop_environment.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/desktop_...
remoting/host/desktop_environment.h:35: virtual void StopAndDelete();
On 2012/08/31 18:18:02, Wez wrote:
> We don't actually need StopAndDelete() here, since we could StopAndDelete()
the
> EventExecutor in the DesktopEnvironment destructor; is this pre-emptive
> preparation for the multi-process implementation?

There is no real reason to keep it here at the moment. I removed it.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:123: desktop_environment_factory_(new
SessionDesktopEnvironmentFactory()),
On 2012/08/31 18:18:02, Wez wrote:
> Isn't SessionDesktopEnvironmentFactory Windows-only now that it lives in
> remoting/host/win/?

Yes, indeed.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/win/sess...
File remoting/host/win/session_desktop_environment_factory.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/win/sess...
remoting/host/win/session_desktop_environment_factory.cc:27: #if defined(OS_WIN)
On 2012/08/31 18:18:02, Wez wrote:
> This only gets build on Windows, so I don't think you need these checks any
> more?

Done.

[alexeypa (please no reviews), 2012-09-04 17:44:17]

Ping. Wez, if you are going to review it - please let us know. Otherwise, I'd
like to see some input from Simon. :-)

[Wez, 2012-09-04 18:41:45]

On 2012/09/04 17:44:17, alexeypa wrote:
> Ping. Wez, if you are going to review it - please let us know. Otherwise, I'd
> like to see some input from Simon. :-)

I will re-review the CL before it lands, yes, but I wasn't intending to do that
until Simon had provided his input.

[simonmorris, 2012-09-04 18:47:21]

http://codereview.chromium.org/10920019/diff/1/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10920019/diff/1/remoting/host/client_session.h...
remoting/host/client_session.h:117: void StopAndDelete();
Add a comment explaining what this does?

http://codereview.chromium.org/10920019/diff/1/remoting/host/win/session_desk...
File remoting/host/win/session_desktop_environment_factory.cc (right):

http://codereview.chromium.org/10920019/diff/1/remoting/host/win/session_desk...
remoting/host/win/session_desktop_environment_factory.cc:39:
video_capturer.Pass()));
This duplicates code, and it's confusing to have cross-platform code in
remoting/host/win.

Could this class have a DesktopEnvironmentFactory member, and delegate Create()
to it, but then construct another DesktopEnvironment using the members of the
Create()'d one, but adding the SessionEventExecutorWin wrapper to the
event_executor member?

That would also simplify the call site. You'd construct an ordinary
DesktopEnvironmentFactory, and on Windows, replace it by the Session-wrapped
version of itself.

http://codereview.chromium.org/10920019/diff/10001/remoting/host/chromoting_h...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/10920019/diff/10001/remoting/host/chromoting_h...
remoting/host/chromoting_host.cc:77: #if !defined(OS_WIN)
Add a HasAudioCapturer() method to DesktopEnvironmentFactory?

http://codereview.chromium.org/10920019/diff/10001/remoting/host/chromoting_h...
File remoting/host/chromoting_host_unittest.cc (left):

http://codereview.chromium.org/10920019/diff/10001/remoting/host/chromoting_h...
remoting/host/chromoting_host_unittest.cc:307: .After(client_authenticated);
Is it possible to keep these expectations? Or move them to
client_session_unittest.cc?

http://codereview.chromium.org/10920019/diff/10001/remoting/host/client_sessi...
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10920019/diff/10001/remoting/host/client_sessi...
remoting/host/client_session.h:117: void StopAndDelete();
Add a comment explaining how this is used, and, in particular, whether a
ClientSession must/should be deleted using this method.

Should the ClientSession dtor be protected? It's easy to imagine a CL that leads
to a StopAndDelete() followed by a ~ClientSession().

http://codereview.chromium.org/10920019/diff/10001/remoting/host/desktop_envi...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/10920019/diff/10001/remoting/host/desktop_envi...
remoting/host/desktop_environment.h:26: scoped_ptr<VideoFrameCapturer>
video_capturer);
I think it's clearer if the capturers are together. They send data in one
direction; event_executor sends it in the other.

[Wez, 2012-09-04 20:29:30]

http://codereview.chromium.org/10920019/diff/10001/remoting/host/chromoting_h...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/10920019/diff/10001/remoting/host/chromoting_h...
remoting/host/chromoting_host.cc:77: #if !defined(OS_WIN)
On 2012/09/04 18:47:21, simonmorris wrote:
> Add a HasAudioCapturer() method to DesktopEnvironmentFactory?

Good idea, although I'd suggest SupportsAudioCapture().

[alexeypa (please no reviews), 2012-09-05 22:53:29]

PTAL.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/client_s...
remoting/host/client_session.h:117: void StopAndDelete();
On 2012/09/04 18:47:21, simonmorris wrote:
> Add a comment explaining what this does?

Done.

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/win/sess...
File remoting/host/win/session_desktop_environment_factory.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/1/remoting/host/win/sess...
remoting/host/win/session_desktop_environment_factory.cc:39:
video_capturer.Pass()));
On 2012/09/04 18:47:21, simonmorris wrote:
> This duplicates code, and it's confusing to have cross-platform code in
> remoting/host/win.

It is Windows-only code. #if defined(OS_WIN) should've not been here. I removed
it in response to Wez's comment.

> Could this class have a DesktopEnvironmentFactory member, and delegate
Create()
> to it, but then construct another DesktopEnvironment using the members of the
> Create()'d one, but adding the SessionEventExecutorWin wrapper to the
> event_executor member?

That would make sense if all kinds of |DesktopEnvironment|'s supported chaining.
It seems that it is not going to be the case for IPC stubs that will live in the
networking process. So I'd like to have a little duplicated code but not let
this feature leak to the interface.

> That would also simplify the call site. You'd construct an ordinary
> DesktopEnvironmentFactory, and on Windows, replace it by the Session-wrapped
> version of itself.

This is how it is done now.

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/chro...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/chro...
remoting/host/chromoting_host.cc:77: #if !defined(OS_WIN)
On 2012/09/04 20:29:30, Wez wrote:
> On 2012/09/04 18:47:21, simonmorris wrote:
> > Add a HasAudioCapturer() method to DesktopEnvironmentFactory?
> 
> Good idea, although I'd suggest SupportsAudioCapture().

Done.

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/chro...
remoting/host/chromoting_host.cc:77: #if !defined(OS_WIN)
On 2012/09/04 18:47:21, simonmorris wrote:
> Add a HasAudioCapturer() method to DesktopEnvironmentFactory?

Done.

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/chro...
File remoting/host/chromoting_host_unittest.cc (left):

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/chro...
remoting/host/chromoting_host_unittest.cc:307: .After(client_authenticated);
On 2012/09/04 18:47:21, simonmorris wrote:
> Is it possible to keep these expectations? Or move them to
> client_session_unittest.cc?

It is hard because the event executor is created much later now. I moved the
expectations to client_session_unittest.cc.

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/clie...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/clie...
remoting/host/client_session.h:117: void StopAndDelete();
On 2012/09/04 18:47:21, simonmorris wrote:
> Add a comment explaining how this is used, and, in particular, whether a
> ClientSession must/should be deleted using this method.
> 
> Should the ClientSession dtor be protected? It's easy to imagine a CL that
leads
> to a StopAndDelete() followed by a ~ClientSession().

Done.

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/desk...
File remoting/host/desktop_environment.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/10001/remoting/host/desk...
remoting/host/desktop_environment.h:26: scoped_ptr<VideoFrameCapturer>
video_capturer);
On 2012/09/04 18:47:21, simonmorris wrote:
> I think it's clearer if the capturers are together. They send data in one
> direction; event_executor sends it in the other.

I think it is better to keep them sorted alphabetically. They are just a bunch
of pointers to objects managed in the same way.

It also does not matter which direction they send data at this point. All stubs
are getting called by the owner is the same way.

https://chromiumcodereview.appspot.com/10920019/diff/9003/remoting/host/event...
File remoting/host/event_executor.h (right):

https://chromiumcodereview.appspot.com/10920019/diff/9003/remoting/host/event...
remoting/host/event_executor.h:35: virtual void StopAndDelete() = 0;
I  am going to make EventExecutor a ref-counted object and get rid of this
instance of StopAndDelete. I'd like to do it in a follow up CL. I tired to make
it a part of this CL but the change turned out to be too large.

[simonmorris, 2012-09-05 23:25:31]

lgtm

[Wez, 2012-09-06 00:13:22]

LGTM!

http://codereview.chromium.org/10920019/diff/1/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10920019/diff/1/remoting/host/client_session.h...
remoting/host/client_session.h:81: scoped_refptr<base::SingleThreadTaskRunner>
network_task_runner,
On 2012/08/31 23:13:30, alexeypa wrote:
> On 2012/08/31 18:18:02, Wez wrote:
> > Pass the ChromotingHostContext in here, so that ClientSession can grab the
> > TaskRunners it needs without them being passed individually here?
> 
> I didn't want to pass ChromotingHostContext because we eventually want to get
> rid of it, letting the actual objects who need the task runners to hold
> references to them.

OK. :)

http://codereview.chromium.org/10920019/diff/1/remoting/host/win/session_desk...
File remoting/host/win/session_desktop_environment_factory.cc (right):

http://codereview.chromium.org/10920019/diff/1/remoting/host/win/session_desk...
remoting/host/win/session_desktop_environment_factory.cc:39:
video_capturer.Pass()));
On 2012/09/05 22:53:29, alexeypa wrote:
> On 2012/09/04 18:47:21, simonmorris wrote:
> > This duplicates code, and it's confusing to have cross-platform code in
> > remoting/host/win.
> 
> It is Windows-only code. #if defined(OS_WIN) should've not been here. I
removed
> it in response to Wez's comment.
> 
> > Could this class have a DesktopEnvironmentFactory member, and delegate
> Create()
> > to it, but then construct another DesktopEnvironment using the members of
the
> > Create()'d one, but adding the SessionEventExecutorWin wrapper to the
> > event_executor member?
> 
> That would make sense if all kinds of |DesktopEnvironment|'s supported
chaining.
> It seems that it is not going to be the case for IPC stubs that will live in
the
> networking process. So I'd like to have a little duplicated code but not let
> this feature leak to the interface.

Chaining should work for the IPC stubs depending on the behaviour being added by
the chaining implementation; let's discuss off-CL.

> > That would also simplify the call site. You'd construct an ordinary
> > DesktopEnvironmentFactory, and on Windows, replace it by the Session-wrapped
> > version of itself.
> 
> This is how it is done now.

http://codereview.chromium.org/10920019/diff/9003/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/10920019/diff/9003/remoting/host/client_sessio...
remoting/host/client_session.cc:124: // Create a ScreenRecorder passing the
message loops that it should run on.
nit: ...ScreenRecorder passing... -> ScreenRecorder, passing

http://codereview.chromium.org/10920019/diff/9003/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/10920019/diff/9003/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:26:
event_executor_.release()->StopAndDelete();
nit: Follow the same StopAndDelete pattern that you've applied to ClientSession
for EventExecutor?

http://codereview.chromium.org/10920019/diff/9003/remoting/host/event_executor.h
File remoting/host/event_executor.h (right):

http://codereview.chromium.org/10920019/diff/9003/remoting/host/event_executo...
remoting/host/event_executor.h:35: virtual void StopAndDelete() = 0;
On 2012/09/05 22:53:30, alexeypa wrote:
> I  am going to make EventExecutor a ref-counted object and get rid of this
> instance of StopAndDelete. I'd like to do it in a follow up CL. I tired to
make
> it a part of this CL but the change turned out to be too large.

Let's discuss whether or not EventExecutor should be ref-counted off-CL!

[alexeypa (please no reviews), 2012-09-06 17:12:02]

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

https://chromiumcodereview.appspot.com/10920019/diff/9003/remoting/host/clien...
remoting/host/client_session.cc:124: // Create a ScreenRecorder passing the
message loops that it should run on.
On 2012/09/06 00:13:23, Wez wrote:
> nit: ...ScreenRecorder passing... -> ScreenRecorder, passing

Done.