Converted LocalInputMonitor into a SessionController instance.

DesktopEnvironment is now responsible for creation of the local input monitor.

LocalInputMonitor instances use the ClientSessionControl interface to pass notification about local mouse movements and to disconnect the client session when the disconnect shortcut (Ctrl+Alt+Esc) is pressed.

This CL also completely removes the MouseMoveObserver interface (along with its implementation in ChromotingHost) since it is not used any more.

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

[alexeypa (please no reviews), 2013-03-19 22:58:02]

PTAL.

[Sergey Ulanov, 2013-03-20 06:31:05]

Looks mostly good, but I feel iffy about SessionController interface.

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
File remoting/host/local_input_monitor.h (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
remoting/host/local_input_monitor.h:21: //
SessionController::EventHandler::Disconnect() this key combination is
s/this/when this/

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
remoting/host/local_input_monitor.h:25: class LocalInputMonitor : public
SessionController {
This class doesn't add anything to SessionController. Create() is the only
useful part. Do we still need it at all?

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
File remoting/host/local_input_monitor_linux.cc (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
remoting/host/local_input_monitor_linux.cc:72: // Posts DisconnectSession() call
to |delegate_ on the |caller_task_runner_|
nit: missing |

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
File remoting/host/session_controller.h (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
remoting/host/session_controller.h:16: class SessionController {
This interface is used for two unrelated features (1) monitoring mouse moves and
(2) updating screen resolution. LocalInputMonitor doesn't care about resolution,
while ResizingHostObserver doesn't touch delegate. So I wonder why we need this
interface at all? Can Delegate be moved to LocalInputMonitor?

Or is the intent here to provide a generic interface for optional features that
may or may not be enabled? In that case it may be better to name it something
like SessionExtension.

Also pleas add comments for this interface.

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
File remoting/host/session_controller_list.h (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
remoting/host/session_controller_list.h:17: class SessionControllerList
Add comment to explain what this class is used for

[alexeypa (please no reviews), 2013-03-20 17:05:29]

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
File remoting/host/local_input_monitor.h (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
remoting/host/local_input_monitor.h:21: //
SessionController::EventHandler::Disconnect() this key combination is
On 2013/03/20 06:31:06, sergeyu wrote:
> s/this/when this/

Done.

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
remoting/host/local_input_monitor.h:25: class LocalInputMonitor : public
SessionController {
On 2013/03/20 06:31:06, sergeyu wrote:
> This class doesn't add anything to SessionController. Create() is the only
> useful part. Do we still need it at all?

It is needed so that ClientSession/DesktopSessionAgent could own a
LocalInputMonitor instance.

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
File remoting/host/local_input_monitor_linux.cc (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/local_input_mon...
remoting/host/local_input_monitor_linux.cc:72: // Posts DisconnectSession() call
to |delegate_ on the |caller_task_runner_|
On 2013/03/20 06:31:06, sergeyu wrote:
> nit: missing |

Done.

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
File remoting/host/session_controller.h (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
remoting/host/session_controller.h:16: class SessionController {
On 2013/03/20 06:31:06, sergeyu wrote:
> This interface is used for two unrelated features (1) monitoring mouse moves
and
> (2) updating screen resolution. LocalInputMonitor doesn't care about
resolution,
> while ResizingHostObserver doesn't touch delegate. So I wonder why we need
this
> interface at all? 

It is indeed used for two unrelated purposes yet I think splitting it into two
separate interfaces will introduce more noise, but little or not gain:

- DesktopEnvironment will get another CreateXxx() method meanisng that it will
be added to all 4 variations of DesktopEnvironment and 4 mocked
DesktopEnvironment's are well.
- ClientSession and DesktopSessoinAgent will get another pointer with life-time
identical to session_controller_.

So I think mixing the two here is less evil.

> Can Delegate be moved to LocalInputMonitor?

No. This delegate will be used by the disconnect and continue windows as well.

> Or is the intent here to provide a generic interface for optional features
that
> may or may not be enabled? In that case it may be better to name it something
> like SessionExtension.

I picked "SessionController" implying that objects implementing it control the
client session in one of two ways:

- They invoke SessionController::Delegate methods directly telling session what
to do.
- They modify the underlying desktop environment in some way affecting the
client session (resolution change).

> Also pleas add comments for this interface.

Done.

I also added PauseSession() to SessionController::Delegate that will be used by
the continue window code.

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
File remoting/host/session_controller_list.h (right):

https://codereview.chromium.org/12594009/diff/1/remoting/host/session_control...
remoting/host/session_controller_list.h:17: class SessionControllerList
On 2013/03/20 06:31:06, sergeyu wrote:
> Add comment to explain what this class is used for

Done.

[Sergey Ulanov, 2013-03-20 17:23:31]

lgtm

https://codereview.chromium.org/12594009/diff/7001/remoting/host/session_cont...
File remoting/host/session_controller.h (right):

https://codereview.chromium.org/12594009/diff/7001/remoting/host/session_cont...
remoting/host/session_controller.h:17: // (representing some aspects of the
desktop environment) that can control
nit: drop the parentheses

https://codereview.chromium.org/12594009/diff/7001/remoting/host/session_cont...
remoting/host/session_controller.h:27: // method to all implementations of
DesktopEnvironment (eight at the moment of
nit: no need to be so wordy about it - maybe remove the stuff in parentheses

[alexeypa (please no reviews), 2013-03-20 17:34:31]

https://codereview.chromium.org/12594009/diff/7001/remoting/host/session_cont...
File remoting/host/session_controller.h (right):

https://codereview.chromium.org/12594009/diff/7001/remoting/host/session_cont...
remoting/host/session_controller.h:17: // (representing some aspects of the
desktop environment) that can control
On 2013/03/20 17:23:31, sergeyu wrote:
> nit: drop the parentheses

Done.

https://codereview.chromium.org/12594009/diff/7001/remoting/host/session_cont...
remoting/host/session_controller.h:27: // method to all implementations of
DesktopEnvironment (eight at the moment of
On 2013/03/20 17:23:31, sergeyu wrote:
> nit: no need to be so wordy about it - maybe remove the stuff in parentheses

Done.

[alexeypa (please no reviews), 2013-03-22 18:59:44]

Patch set #4 is a version of the CL rebased on top of
https://chromiumcodereview.appspot.com/12879006/. It changed noticeably from the
original version. Please take another look.

[Sergey Ulanov, 2013-03-23 19:54:22]

LGTM with some nits

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
File remoting/host/local_input_monitor.h (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
remoting/host/local_input_monitor.h:31: static scoped_ptr<LocalInputMonitor>
Create(
nit add comment that |client_session_control| is called on |caller_task_runner|

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
File remoting/host/local_input_monitor_linux.cc (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
remoting/host/local_input_monitor_linux.cc:139: void
LocalInputMonitorLinux::Core::Start() {
Move Start() to constructor?

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
File remoting/host/local_input_monitor_win.cc (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
remoting/host/local_input_monitor_win.cc:98: core_->Start();
get rid of Core::Start() too?

https://codereview.chromium.org/12594009/diff/27002/remoting/host/me2me_deskt...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/me2me_deskt...
remoting/host/me2me_desktop_environment.cc:14: #include
"remoting/host/local_input_monitor.h"
Don't need this

[alexeypa (please no reviews), 2013-03-25 15:54:07]

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
File remoting/host/local_input_monitor.h (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
remoting/host/local_input_monitor.h:31: static scoped_ptr<LocalInputMonitor>
Create(
On 2013/03/23 19:54:22, sergeyu wrote:
> nit add comment that |client_session_control| is called on
|caller_task_runner|

Done.

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
File remoting/host/local_input_monitor_linux.cc (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
remoting/host/local_input_monitor_linux.cc:139: void
LocalInputMonitorLinux::Core::Start() {
On 2013/03/23 19:54:22, sergeyu wrote:
> Move Start() to constructor?

No, PostTask cannot be called from a constructor because it increments the
reference counter of the object. If the task completes before the constructor
returns, the reference counter will be dropped back to zero and the object will
be destroyed.

I could collapse Start() and StartOnUiThread() but then it will be harder to
check that Start() was initially called on |caller_task_runner|.

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
File remoting/host/local_input_monitor_win.cc (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/local_input...
remoting/host/local_input_monitor_win.cc:98: core_->Start();
On 2013/03/23 19:54:22, sergeyu wrote:
> get rid of Core::Start() too?

No, because of the same reason.

https://codereview.chromium.org/12594009/diff/27002/remoting/host/me2me_deskt...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/12594009/diff/27002/remoting/host/me2me_deskt...
remoting/host/me2me_desktop_environment.cc:14: #include
"remoting/host/local_input_monitor.h"
On 2013/03/23 19:54:22, sergeyu wrote:
> Don't need this

Done.

[commit-bot: I haz the power, 2013-03-25 16:14:45]

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

[commit-bot: I haz the power, 2013-03-25 18:34:41]

Change committed as 190444