Fix Ctrl+Alt+Esc keyboard shortcut in Me2Me host.

Fix Ctrl+Alt+Esc keyboard shortcut in Me2Me host.

Refactored LocalInputMonitor interface so that it doesn't interact with 
the ChromotingHost object directly.

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

[Jamie, 2012-05-25 21:19:24]

lgtm with nits.

http://codereview.chromium.org/10447041/diff/1/remoting/host/chromoting_host.cc
File remoting/host/chromoting_host.cc (left):

http://codereview.chromium.org/10447041/diff/1/remoting/host/chromoting_host....
remoting/host/chromoting_host.cc:348: }
Did these functions need to be re-ordered? AFACT, set_protocol_config hasn't
changed, but that would be a lot more obvious if it was still in the same place.

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

http://codereview.chromium.org/10447041/diff/1/remoting/host/chromoting_host....
remoting/host/chromoting_host.h:130: virtual void OnMouseMoved(const SkIPoint&
new_pos) OVERRIDE;
I think it would be clearer if the word "Local" appeared somewhere, either in
the interface name or the method name, to indicate that this doesn't fire for
remote mouse movements.

http://codereview.chromium.org/10447041/diff/1/remoting/host/local_input_moni...
File remoting/host/local_input_monitor_mac.mm (right):

http://codereview.chromium.org/10447041/diff/1/remoting/host/local_input_moni...
remoting/host/local_input_monitor_mac.mm:29: class LocalInputMonitorMac : public
LocalInputMonitor {
I realize that you inherited this from the older code, but I got confused
initially with the variable name |monitors_|, thinking it was referring to VDUs.
I wonder if Observer might be a better terminology throughout? Right now it's a
bit inconsistent, particuarly wrt comments vs. code.

http://codereview.chromium.org/10447041/diff/1/remoting/host/local_input_moni...
remoting/host/local_input_monitor_mac.mm:74: // Called to remove a mouse move
observers. Returns true if it was the
s/observers/observer/

[Sergey Ulanov, 2012-05-25 21:59:30]

http://codereview.chromium.org/10447041/diff/1/remoting/host/chromoting_host.cc
File remoting/host/chromoting_host.cc (left):

http://codereview.chromium.org/10447041/diff/1/remoting/host/chromoting_host....
remoting/host/chromoting_host.cc:348: }
On 2012/05/25 21:19:24, Jamie wrote:
> Did these functions need to be re-ordered? AFACT, set_protocol_config hasn't
> changed, but that would be a lot more obvious if it was still in the same
place.

I reordered them so that they are in the same order as in the header. Returned
it back, will reorder in a separate CL.

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

http://codereview.chromium.org/10447041/diff/1/remoting/host/chromoting_host....
remoting/host/chromoting_host.h:130: virtual void OnMouseMoved(const SkIPoint&
new_pos) OVERRIDE;
On 2012/05/25 21:19:24, Jamie wrote:
> I think it would be clearer if the word "Local" appeared somewhere, either in
> the interface name or the method name, to indicate that this doesn't fire for
> remote mouse movements.

Done.

http://codereview.chromium.org/10447041/diff/1/remoting/host/local_input_moni...
File remoting/host/local_input_monitor_mac.mm (right):

http://codereview.chromium.org/10447041/diff/1/remoting/host/local_input_moni...
remoting/host/local_input_monitor_mac.mm:29: class LocalInputMonitorMac : public
LocalInputMonitor {
On 2012/05/25 21:19:24, Jamie wrote:
> I realize that you inherited this from the older code, but I got confused
> initially with the variable name |monitors_|, thinking it was referring to
VDUs.
> I wonder if Observer might be a better terminology throughout? Right now it's
a
> bit inconsistent, particuarly wrt comments vs. code.

Yes, I agree that the current terminology is confusing, but I don't think that
renaming monitors_ to observers_ would make it better, as observers_ can be
confused with MouseMoveObservers. I renamed LocalInputMonitorImpl to
LocalInputMonitorManager - this makes relationship between
LocalInputMonitorManager and LocalInputMonitorMac clearer, though it's not
completely correct to call it a manager.

http://codereview.chromium.org/10447041/diff/1/remoting/host/local_input_moni...
remoting/host/local_input_monitor_mac.mm:74: // Called to remove a mouse move
observers. Returns true if it was the
On 2012/05/25 21:19:24, Jamie wrote:
> s/observers/observer/
It actually removes monitor.

[commit-bot: I haz the power, 2012-05-25 22:20:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10447041/8001

[Wez, 2012-05-25 22:58:33]

On 2012/05/25 22:20:38, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/sergeyu%40chromium.org/10447041/8001

Hey! I haven't reviewed this yet! :P

[Sergey Ulanov, 2012-05-25 23:08:07]

On 2012/05/25 22:58:33, Wez wrote:
> On 2012/05/25 22:20:38, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/sergeyu%2540chromium.org/10447041/8001
> 
> Hey! I haven't reviewed this yet! :P

You still have couple hours until win_rel bot completes :)
I've unchecked commit bit.

[Wez, 2012-05-25 23:32:17]

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor.h (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor.h:19: virtual void Start(MouseMoveObserver*
mouse_move_observer,
MouseMoveObserver only has a single method, so why not make it a base::Callback,
akin to disconnect_callback, rather than an interface.

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor.h:20: const base::Closure&
disconnect_callback) = 0;
This looks a little odd; can you add a comment to explain what "disconnect
callback" is for, since in the context of LocalInputMonitor it's not obvious
(there is nothing to disconnect here!).

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor_mac.mm (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor_mac.mm:29: class LocalInputMonitorMac : public
LocalInputMonitor {
Why have you moved this definition into an anonymous namespace in remoting, but
then defined the implementations in the remoting namespace?

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor_thread_linux.h (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor_thread_linux.h:24: explicit
LocalInputMonitorThread(
No need to be so explicit here.

[Sergey Ulanov, 2012-05-26 01:06:14]

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor.h (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor.h:19: virtual void Start(MouseMoveObserver*
mouse_move_observer,
On 2012/05/25 23:32:18, Wez wrote:
> MouseMoveObserver only has a single method, so why not make it a
base::Callback,
> akin to disconnect_callback, rather than an interface.

You can't store callbacks in stl containers and the windows implementation of
LocalInputMonitor needs it.
Also interface looks cleaner than callback in this case IMHO.

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor.h:20: const base::Closure&
disconnect_callback) = 0;
On 2012/05/25 23:32:18, Wez wrote:
> This looks a little odd; can you add a comment to explain what "disconnect
> callback" is for, since in the context of LocalInputMonitor it's not obvious
> (there is nothing to disconnect here!).

Done.

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor_mac.mm (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor_mac.mm:29: class LocalInputMonitorMac : public
LocalInputMonitor {
On 2012/05/25 23:32:18, Wez wrote:
> Why have you moved this definition into an anonymous namespace in remoting,
but
> then defined the implementations in the remoting namespace?

It was that way before my change. I just moved declaration of the class above
LocalInputMonitorManager.
Moved everything to the anonymous namespace

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor_thread_linux.h (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor_thread_linux.h:24: explicit
LocalInputMonitorThread(
On 2012/05/25 23:32:18, Wez wrote:
> No need to be so explicit here.

Done.

[Wez, 2012-05-29 16:57:33]

lgtm

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
File remoting/host/local_input_monitor.h (right):

http://codereview.chromium.org/10447041/diff/8001/remoting/host/local_input_m...
remoting/host/local_input_monitor.h:19: virtual void Start(MouseMoveObserver*
mouse_move_observer,
On 2012/05/26 01:06:15, sergeyu wrote:
> On 2012/05/25 23:32:18, Wez wrote:
> > MouseMoveObserver only has a single method, so why not make it a
> base::Callback,
> > akin to disconnect_callback, rather than an interface.
> 
> You can't store callbacks in stl containers and the windows implementation of
> LocalInputMonitor needs it.
> Also interface looks cleaner than callback in this case IMHO.

An alternative would be to allow LocalInputMonitor to accept multiple
LocalInputObservers, or to allow multiple instances to be created, so that both
methods then go on the LocalInputObserver interface.

[commit-bot: I haz the power, 2012-05-29 18:03:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10447041/3003

[commit-bot: I haz the power, 2012-05-29 20:35:27]

Change committed as 139362