Connect to DesktopEnvironment's stubs only when audio/video schedulers are about to be created.

Connect to DesktopEnvironment's stubs only when audio/video schedulers are about to be created.

This CL localizes usage of DesktopEnvironment to a single method that creates audio/video schedulers and hook up the event executor. This prepares further refactoring of DesktopEnvironment that should become a factory for classes interacting with the specific desktop environment.

Related changes in this CL:
  - MouseClampingFilter receives screen size on the proper thread.
  - ClientSessionTest.* tests now run asynchronously to support MouseClampingFilter change.
  - ClientSession does not expose DesktopEnvironment.
  - InputEventTracker can be re-attached to a different InputStub.

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

[alexeypa (please no reviews), 2013-01-05 01:21:10]

PTAL.

[alexeypa (please no reviews), 2013-01-08 17:56:38]

Ping.

[Wez, 2013-01-08 18:33:59]

Apologies for the slow turn-around...

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/capturer/vide...
File remoting/capturer/video_frame_capturer_fake.cc (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/capturer/vide...
remoting/capturer/video_frame_capturer_fake.cc:15: static const int kWidth =
VideoFrameCapturerFake::kWidth;
nit: using VideoFrameCapturerFake::kWidth;

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

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:127: // Attach the host clipboard and input
stubs.
nit: Attach -> Connect

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

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:67: void StopClientSession();
nit: Update this comment to explain what it means to stop the client session;
does this call cause ClientSession to tear-down, for example?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:70: // Mocks
DesktopEnvironmentFactory::Create();
nit: Creates a DesktopEnvironment with a fake VideoFrameCapturer, to mock
DesktopEnvironmentFactory::Create().

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:73: // Connects the client session.
nit: To what? e.g. does it actually attach a fake ConnectionToClient, or
something else?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:76: // Quit the message loop.
typo: Quit -> Quits

nit: What affect does quitting the MessageLoop have? e.g. does it quit the test,
or just quit a mid-test run of the loop, or something else?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:79: // Invoked when the client session
is fully stopped.
nit: What does it do? It seems to just release the ClientSession reference - is
it intended to indicate that the test can exit as well (via the
AutoThreadTaskRunner)?

e.g. Should it be:
// Releases the ClientSession when it has been fully stopped, allowing the
MessageLoop to quit.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:111: ui_task_runner_ = new
AutoThreadTaskRunner(
nit: Add some basic comments, e.g. "Arrange to run |message_loop_| until no
components depend on it" for this one.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:116: client_jid_ =
"user@domain/rest-of-jid";
This doesn't ever seem to be set to any other value; can it be replaced with a
constant?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:152: ui_task_runner_ = NULL;
Looks like ui_task_runner_ doesn't need to be a member of ClientSessionTest?

Or, you might move ui_task_runner_ = NULL into TearDown(), and put the
|message_loop_.Run()| call there.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:218: EXPECT_CALL(video_stub_,
ProcessVideoPacketPtr(_, _))
nit: Consider adding a comment to explain that this expectation uses the arrival
of the first video packet as a proxy for the transport streams having been
connected such that injecting input should work.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:398: // Inject the 1st event after once
a video packet has been received.
typo: "... after once ..."

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.cc (left):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:23:
set_input_size(capturer_->size_most_recent());
How close are we to being able to remove size_most_recent() from
VideoFrameCapturer?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.cc (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:25: // Ensure that the MouseInputFilter
is clamping to the current dimensions.
nit: "Configure the MouseInputFilter to clamp to the video dimensions." or
possibly just remove this.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:27:
video_packet->format().has_screen_height()) {
VideoPacket always has valid width & height in our current implementation, and
that's unlikely to change, so I'd suggest DCHECK()ing their presence here.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.h (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.h:15: // the current dimensions of a
VideoFrameCapturer instance before passing
nit: (Assuming the composition change suggested below) "Filtering VideoStub
implementation which configures a MouseInputFilter to clamp mouse events to fall
within the dimensions of the most-recently received video frame."

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.h:18: : public protocol::MouseInputFilter,
It would be cleaner to compose with MouseInputFilter than derived from it,
either by having this class contain & expose it, or having the caller pass one
in instead of |input_stub|.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/protocol/inpu...
File remoting/protocol/input_event_tracker.h (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/protocol/inpu...
remoting/protocol/input_event_tracker.h:27: void set_input_stub(InputStub*
input_stub) { input_stub_ = input_stub; }
Why not derived from InputFilter to get this, and replace input_stub->Foo()
calls w/ InputFilter::Foo()?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/protocol/inpu...
remoting/protocol/input_event_tracker.h:27: void set_input_stub(InputStub*
input_stub) { input_stub_ = input_stub; }
The problem with this is that it creates the question of the InputEventTracker's
semantics when |input_stub| is null. If you instead just add an InputFilter in
ClientSession, to pass to the InputEventTracker, then that becomes
ClientSession's problem, and ClientSession is in a position to arrange the right
release / NULLify ordering.

[alexeypa (please no reviews), 2013-01-08 20:00:13]

PTAL.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/capturer/vide...
File remoting/capturer/video_frame_capturer_fake.cc (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/capturer/vide...
remoting/capturer/video_frame_capturer_fake.cc:15: static const int kWidth =
VideoFrameCapturerFake::kWidth;
On 2013/01/08 18:34:00, Wez wrote:
> nit: using VideoFrameCapturerFake::kWidth;

This does not compile. I guess because VideoFrameCapturerFake is not a
namespace.

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

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:127: // Attach the host clipboard and input
stubs.
On 2013/01/08 18:34:00, Wez wrote:
> nit: Attach -> Connect

Done.

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

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:67: void StopClientSession();
On 2013/01/08 18:34:00, Wez wrote:
> nit: Update this comment to explain what it means to stop the client session;
> does this call cause ClientSession to tear-down, for example?

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:70: // Mocks
DesktopEnvironmentFactory::Create();
On 2013/01/08 18:34:00, Wez wrote:
> nit: Creates a DesktopEnvironment with a fake VideoFrameCapturer, to mock
> DesktopEnvironmentFactory::Create().

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:73: // Connects the client session.
On 2013/01/08 18:34:00, Wez wrote:
> nit: To what? e.g. does it actually attach a fake ConnectionToClient, or
> something else?

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:76: // Quit the message loop.
On 2013/01/08 18:34:00, Wez wrote:
> typo: Quit -> Quits
> 
> nit: What affect does quitting the MessageLoop have? e.g. does it quit the
test,
> or just quit a mid-test run of the loop, or something else?

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:79: // Invoked when the client session
is fully stopped.
On 2013/01/08 18:34:00, Wez wrote:
> nit: What does it do? It seems to just release the ClientSession reference -
is
> it intended to indicate that the test can exit as well (via the
> AutoThreadTaskRunner)?
> 
> e.g. Should it be:
> // Releases the ClientSession when it has been fully stopped, allowing the
> MessageLoop to quit.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:111: ui_task_runner_ = new
AutoThreadTaskRunner(
On 2013/01/08 18:34:00, Wez wrote:
> nit: Add some basic comments, e.g. "Arrange to run |message_loop_| until no
> components depend on it" for this one.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:116: client_jid_ =
"user@domain/rest-of-jid";
On 2013/01/08 18:34:00, Wez wrote:
> This doesn't ever seem to be set to any other value; can it be replaced with a
> constant?

It has to be a member variable. It is not a POD type, so it cannot be global.
The mocked method returns a reference, so it has to be backed up by something.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:152: ui_task_runner_ = NULL;
On 2013/01/08 18:34:00, Wez wrote:
> Looks like ui_task_runner_ doesn't need to be a member of ClientSessionTest?
> 
> Or, you might move ui_task_runner_ = NULL into TearDown(), and put the
> |message_loop_.Run()| call there.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:218: EXPECT_CALL(video_stub_,
ProcessVideoPacketPtr(_, _))
On 2013/01/08 18:34:00, Wez wrote:
> nit: Consider adding a comment to explain that this expectation uses the
arrival
> of the first video packet as a proxy for the transport streams having been
> connected such that injecting input should work.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:398: // Inject the 1st event after once
a video packet has been received.
On 2013/01/08 18:34:00, Wez wrote:
> typo: "... after once ..."

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.cc (left):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:23:
set_input_size(capturer_->size_most_recent());
On 2013/01/08 18:34:00, Wez wrote:
> How close are we to being able to remove size_most_recent() from
> VideoFrameCapturer?

I should be able to remove it completely in a follow up CL.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.cc (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:25: // Ensure that the MouseInputFilter
is clamping to the current dimensions.
On 2013/01/08 18:34:00, Wez wrote:
> nit: "Configure the MouseInputFilter to clamp to the video dimensions." or
> possibly just remove this.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:27:
video_packet->format().has_screen_height()) {
On 2013/01/08 18:34:00, Wez wrote:
> VideoPacket always has valid width & height in our current implementation

Verbatim codec does not always set the width and height.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.h (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.h:15: // the current dimensions of a
VideoFrameCapturer instance before passing
On 2013/01/08 18:34:00, Wez wrote:
> nit: (Assuming the composition change suggested below) "Filtering VideoStub
> implementation which configures a MouseInputFilter to clamp mouse events to
fall
> within the dimensions of the most-recently received video frame."

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.h:18: : public protocol::MouseInputFilter,
On 2013/01/08 18:34:00, Wez wrote:
> It would be cleaner to compose with MouseInputFilter than derived from it,
> either by having this class contain & expose it, or having the caller pass one
> in instead of |input_stub|.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/protocol/inpu...
File remoting/protocol/input_event_tracker.h (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/protocol/inpu...
remoting/protocol/input_event_tracker.h:27: void set_input_stub(InputStub*
input_stub) { input_stub_ = input_stub; }
On 2013/01/08 18:34:00, Wez wrote:
> The problem with this is that it creates the question of the
InputEventTracker's
> semantics when |input_stub| is null. If you instead just add an InputFilter in
> ClientSession, to pass to the InputEventTracker, then that becomes
> ClientSession's problem, and ClientSession is in a position to arrange the
right
> release / NULLify ordering. 

Done.

[Wez, 2013-01-08 21:02:41]

lgtm

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

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:116: client_jid_ =
"user@domain/rest-of-jid";
On 2013/01/08 20:00:14, alexeypa wrote:
> On 2013/01/08 18:34:00, Wez wrote:
> > This doesn't ever seem to be set to any other value; can it be replaced with
a
> > constant?
> 
> It has to be a member variable. It is not a POD type, so it cannot be global.
> The mocked method returns a reference, so it has to be backed up by something.
> 

Ah, OK.  Could it be a const member, initialized in the constructor?

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.cc (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.cc:27:
video_packet->format().has_screen_height()) {
On 2013/01/08 20:00:14, alexeypa wrote:
> On 2013/01/08 18:34:00, Wez wrote:
> > VideoPacket always has valid width & height in our current implementation
> 
> Verbatim codec does not always set the width and height. 

Hmmm... you really only need width & height on each frame, not each packet, so
that's fair enough I suppose.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.h (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.h:18: : public protocol::MouseInputFilter,
On 2013/01/08 20:00:14, alexeypa wrote:
> On 2013/01/08 18:34:00, Wez wrote:
> > It would be cleaner to compose with MouseInputFilter than derived from it,
> > either by having this class contain & expose it, or having the caller pass
one
> > in instead of |input_stub|.
> 
> Done.

I'd actually thought that the composed implementation would not derived from
InputStub at all, it would either be passed the MouseInputFilter to configure,
or it'd have an input_stub() getter to get at the InputStub interface of the
MouseInputFilter member.

[alexeypa (please no reviews), 2013-01-08 21:30:24]

FYI

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

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/client_s...
remoting/host/client_session_unittest.cc:152: ui_task_runner_ = NULL;
On 2013/01/08 20:00:14, alexeypa wrote:
> On 2013/01/08 18:34:00, Wez wrote:
> > Looks like ui_task_runner_ doesn't need to be a member of ClientSessionTest?
> > 
> > Or, you might move ui_task_runner_ = NULL into TearDown(), and put the
> > |message_loop_.Run()| call there.
> 
> Done.

Done.

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
File remoting/host/mouse_clamping_filter.h (right):

https://chromiumcodereview.appspot.com/11781003/diff/1/remoting/host/mouse_cl...
remoting/host/mouse_clamping_filter.h:18: : public protocol::MouseInputFilter,
On 2013/01/08 21:02:42, Wez wrote:
> On 2013/01/08 20:00:14, alexeypa wrote:
> > On 2013/01/08 18:34:00, Wez wrote:
> > > It would be cleaner to compose with MouseInputFilter than derived from it,
> > > either by having this class contain & expose it, or having the caller pass
> one
> > > in instead of |input_stub|.
> > 
> > Done.
> 
> I'd actually thought that the composed implementation would not derived from
> InputStub at all, it would either be passed the MouseInputFilter to configure,
> or it'd have an input_stub() getter to get at the InputStub interface of the
> MouseInputFilter member.

Done.

[commit-bot: I haz the power, 2013-01-08 21:32:36]

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

[commit-bot: I haz the power, 2013-01-09 00:53:40]

Change committed as 175648