Added support of relative mouse motion in Chromoting.

Added support of relative mouse motion in Chromoting.

This CL makes the client plugin treat a completely transparent mouse cursor as an indication that the host switched to relative mouse mode.

If the browser rejects or cancels the mouse lock for any reason, the plugin sets the cursor to the standard arrow pointer until a new cursor is set by the host.

The webapp has to send the 'allowMouseLock' message to enable this behavior.

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

[alexeypa (please no reviews), 2013-09-05 19:32:14]

PTAL.

[Wez, 2013-09-05 19:37:41]

On 2013/09/05 19:32:14, alexeypa wrote:
> PTAL.

FYI, updated CL description a little.

[Wez, 2013-09-05 20:24:45]

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:135: // Returns true |pixel| is
not completely transparent.
nit: "... true if ..."

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:332: RequestMouseLock();
nit: It looks strange to call RequestMouseLock() in the case that we're becoming
un-focused.

Does mouse lock fail in that case, BTW?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:546: // See if the cursor image
completely consists of transparent pixels.
nit: consists completely

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:550: &IsVisiblePixel) ==
src_row_data_end);
nit: Move this find_if() into an IsVisibleRow() helper alongside IsVisiblePixel.

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:557: // the hotspot as close to
the center of the new cursor shape as possible.
Nit, can we move this crop logic into a CropCursor() helper function without
complicating things too much?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:589: // Cancel mouse lock if any
and apply the new cursor image.
nit: if any and -> if active, and

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1092: void
ChromotingInstance::EnableMousePointer() {
This name doesn't feel right; you're setting the cursor shape, so call it
SetMouseCursor() or UpdateMouseCursor()?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1103: // the standard arrow
pointer instead.
Will we also be in this state if the host never sent us a cursor shape at all,
in which case comment could be:

"If there is no cursor shape stored, either because the host never supplied one,
or we were previously in mouse-lock mode, then use a standard arrow pointer."

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1111: // Request mouse lock if the
plugin is focused, mouse lock requested by
nit: lock if -> lock only if

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1112: // the host and no callback
is pending.
nit: mouse lock requested by the host -> the host-supplied cursor is empty

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1113: if (focused_ &&
!cursor_image_ && mouse_lock_state_ == MouseLockOff) {
nit: I think this function would be more readable w/ a set of three ifs w/
early-exits.

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1119: // Hide the cursor to
workaround http://crbug.com/285809.
nit: Suggest: "Hide cursor to avoid it becoming a black square (see
crbug.com/285809."

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1121: mouse_lock_state_ =
MouseLockPending;
nit: Since the comment "Hide the cursor" doesn't apply to this line, have a
blank line between it and the SetCursor() line :)

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1138: // Let the callback know
that the operation should be cancelled.
nit: Clarify here that we can't call UnlockMouse() while a LockMouse() request
is pending, if that's the case. Otherwise, just call UnlockMouse here. :)

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1145: // Wait until
MouseLockLost() is called.
nit: Suggest "Note that mouse-lock has been cancelled. We will continue to
receive locked events until MouseLockLost() is called back."

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1159: bool cancel =
(mouse_lock_state_ == MouseLockCancelling);
nit: cancel -> should_cancel?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:1171: CancelMouseLock();
So we can't dispatch the mouse-lock cancel until a previous mouse-lock request
has completed? That seems an unnecessary restriction in the API. :(

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:246: // Request the browser and
restores the cursor once the lock is gone.
This wording seems b0rked!

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:249: // Called when the browser
locked the mouse.
nit: Does this take any action? Is this also called if mouse-lock fails, as the
presence of |error| implies?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:262: // True if the plugin is
focused.
nit: has focus.

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:263: bool focused_;
nit: has_focus_;

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:266: // |cursor_image_| is set to
NULL.
Why is this set to NULL when mouse-lock is requested?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:273: MouseLockDisabled,
nit: You've called this state "Disabled", but it's changed via a message called
"allow" - either rename this to "Disallowed", or the message to
"enableMouseLock", or "enableAutoMouseLock" or "enableUseMouseLock".

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:275: MouseLockPending,
nit: MouseLockRequested or MouseLockRequestPending?

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/pepper...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/pepper...
remoting/client/plugin/pepper_input_handler.cc:114: // Don't handle
scroll-by-page events, for now..\admin2
Ctrl+V fail ;)

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/pepper...
File remoting/client/plugin/pepper_input_handler.h (right):

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/pepper...
remoting/client/plugin/pepper_input_handler.h:26: bool HandleInputEvent(const
pp::InputEvent& event, bool mouse_locked);
Can you move the |mouse_locked| state to a setter, rather than pollute
HandleInputEvent()?

https://codereview.chromium.org/23484015/diff/1/remoting/host/input_injector_...
File remoting/host/input_injector_linux.cc (right):

https://codereview.chromium.org/23484015/diff/1/remoting/host/input_injector_...
remoting/host/input_injector_linux.cc:307: CurrentTime);
nit: Filter out 0x0 movement?

https://codereview.chromium.org/23484015/diff/1/remoting/host/input_injector_...
File remoting/host/input_injector_win.cc (right):

https://codereview.chromium.org/23484015/diff/1/remoting/host/input_injector_...
remoting/host/input_injector_win.cc:231: if (SendInput(1, &input, sizeof(INPUT))
== 0)
How does this interact with mouse acceleration?

[Wez, 2013-09-05 21:19:38]

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/23484015/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.h:74: public pp::MouseLock {
Have you considered having PepperInputHandler derive from pp::MouseLock and
encapsulate the mouse-lock logic, rather than having ChromotingInstance do it?
Or breaking it out into its own component entirely?

[alexeypa (please no reviews), 2013-09-06 20:00:17]

PTAL

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:135: // Returns true |pixel| is
not completely transparent.
On 2013/09/05 20:24:45, Wez wrote:
> nit: "... true if ..."

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:332: RequestMouseLock();
On 2013/09/05 20:24:45, Wez wrote:
> nit: It looks strange to call RequestMouseLock() in the case that we're
becoming
> un-focused.

RequestMouseLock() checks whether the plugin is focused.

> Does mouse lock fail in that case, BTW?

Yes, as far as I can tell.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:546: // See if the cursor image
completely consists of transparent pixels.
On 2013/09/05 20:24:45, Wez wrote:
> nit: consists completely

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:550: &IsVisiblePixel) ==
src_row_data_end);
On 2013/09/05 20:24:45, Wez wrote:
> nit: Move this find_if() into an IsVisibleRow() helper alongside
IsVisiblePixel.

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:557: // the hotspot as close to
the center of the new cursor shape as possible.
On 2013/09/05 20:24:45, Wez wrote:
> Nit, can we move this crop logic into a CropCursor() helper function without
> complicating things too much?

Not sure. This code updates a bunch of local variables meaning a lot of
parameters passed in and out the function. I don't think it belong to this CL.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:589: // Cancel mouse lock if any
and apply the new cursor image.
On 2013/09/05 20:24:45, Wez wrote:
> nit: if any and -> if active, and

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1092: void
ChromotingInstance::EnableMousePointer() {
On 2013/09/05 20:24:45, Wez wrote:
> This name doesn't feel right; you're setting the cursor shape, so call it
> SetMouseCursor() or UpdateMouseCursor()?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1103: // the standard arrow
pointer instead.
On 2013/09/05 20:24:45, Wez wrote:
> Will we also be in this state if the host never sent us a cursor shape at all,
> in which case comment could be:

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1111: // Request mouse lock if the
plugin is focused, mouse lock requested by
On 2013/09/05 20:24:45, Wez wrote:
> nit: lock if -> lock only if

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1112: // the host and no callback
is pending.
On 2013/09/05 20:24:45, Wez wrote:
> nit: mouse lock requested by the host -> the host-supplied cursor is empty

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1113: if (focused_ &&
!cursor_image_ && mouse_lock_state_ == MouseLockOff) {
On 2013/09/05 20:24:45, Wez wrote:
> nit: I think this function would be more readable w/ a set of three ifs w/
> early-exits.

Six lines instead of one? I don't think so.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1119: // Hide the cursor to
workaround http://crbug.com/285809.
On 2013/09/05 20:24:45, Wez wrote:
> nit: Suggest: "Hide cursor to avoid it becoming a black square (see
> crbug.com/285809."

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1121: mouse_lock_state_ =
MouseLockPending;
On 2013/09/05 20:24:45, Wez wrote:
> nit: Since the comment "Hide the cursor" doesn't apply to this line, have a
> blank line between it and the SetCursor() line :)

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1138: // Let the callback know
that the operation should be cancelled.
On 2013/09/05 20:24:45, Wez wrote:
> nit: Clarify here that we can't call UnlockMouse() while a LockMouse() request
> is pending, if that's the case. Otherwise, just call UnlockMouse here. :)

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1145: // Wait until
MouseLockLost() is called.
On 2013/09/05 20:24:45, Wez wrote:
> nit: Suggest "Note that mouse-lock has been cancelled. We will continue to
> receive locked events until MouseLockLost() is called back."

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1159: bool cancel =
(mouse_lock_state_ == MouseLockCancelling);
On 2013/09/05 20:24:45, Wez wrote:
> nit: cancel -> should_cancel?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1171: CancelMouseLock();
On 2013/09/05 20:24:45, Wez wrote:
> So we can't dispatch the mouse-lock cancel until a previous mouse-lock request
> has completed? That seems an unnecessary restriction in the API. :(

I don't know. I implemented it this way because the state logic is simpler this
way.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.h (right):

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:74: public pp::MouseLock {
On 2013/09/05 21:19:38, Wez wrote:
> Have you considered having PepperInputHandler derive from pp::MouseLock and
> encapsulate the mouse-lock logic, rather than having ChromotingInstance do it?
> Or breaking it out into its own component entirely?

Done. Moved it to PepperInputHandler.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:246: // Request the browser and
restores the cursor once the lock is gone.
On 2013/09/05 20:24:45, Wez wrote:
> This wording seems b0rked!

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:249: // Called when the browser
locked the mouse.
On 2013/09/05 20:24:45, Wez wrote:
> nit: Does this take any action? Is this also called if mouse-lock fails, as
the
> presence of |error| implies?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:262: // True if the plugin is
focused.
On 2013/09/05 20:24:45, Wez wrote:
> nit: has focus.

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:263: bool focused_;
On 2013/09/05 20:24:45, Wez wrote:
> nit: has_focus_;

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:266: // |cursor_image_| is set to
NULL.
On 2013/09/05 20:24:45, Wez wrote:
> Why is this set to NULL when mouse-lock is requested?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:273: MouseLockDisabled,
On 2013/09/05 20:24:45, Wez wrote:
> nit: You've called this state "Disabled", but it's changed via a message
called
> "allow" - either rename this to "Disallowed", or the message to
> "enableMouseLock", or "enableAutoMouseLock" or "enableUseMouseLock".

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.h:275: MouseLockPending,
On 2013/09/05 20:24:45, Wez wrote:
> nit: MouseLockRequested or MouseLockRequestPending?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_input_handler.cc:114: // Don't handle
scroll-by-page events, for now..\admin2
On 2013/09/05 20:24:45, Wez wrote:
> Ctrl+V fail ;)

At least it wasn't the password. :-)

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
File remoting/client/plugin/pepper_input_handler.h (right):

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_input_handler.h:26: bool HandleInputEvent(const
pp::InputEvent& event, bool mouse_locked);
On 2013/09/05 20:24:45, Wez wrote:
> Can you move the |mouse_locked| state to a setter, rather than pollute
> HandleInputEvent()?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/host/input_in...
File remoting/host/input_injector_linux.cc (right):

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/host/input_in...
remoting/host/input_injector_linux.cc:307: CurrentTime);
On 2013/09/05 20:24:45, Wez wrote:
> nit: Filter out 0x0 movement?

Done.

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/host/input_in...
File remoting/host/input_injector_win.cc (right):

https://chromiumcodereview.appspot.com/23484015/diff/1/remoting/host/input_in...
remoting/host/input_injector_win.cc:231: if (SendInput(1, &input, sizeof(INPUT))
== 0)
On 2013/09/05 20:24:45, Wez wrote:
> How does this interact with mouse acceleration?

MSDN:

Relative mouse motion is subject to the effects of the mouse speed and the
two-mouse threshold values. A user sets these three values with the Pointer
Speed slider of the Control Panel's Mouse Properties sheet. You can obtain and
set these values using the SystemParametersInfo function.

[Wez, 2013-09-07 01:11:24]

lgtm

https://chromiumcodereview.appspot.com/23484015/diff/15001/remoting/client/pl...
File remoting/client/plugin/pepper_input_handler.h (right):

https://chromiumcodereview.appspot.com/23484015/diff/15001/remoting/client/pl...
remoting/client/plugin/pepper_input_handler.h:81: enum MouseLockState {
nit: This belongs at the top of the private section

[alexeypa (please no reviews), 2013-09-09 18:29:46]

https://chromiumcodereview.appspot.com/23484015/diff/15001/remoting/client/pl...
File remoting/client/plugin/pepper_input_handler.h (right):

https://chromiumcodereview.appspot.com/23484015/diff/15001/remoting/client/pl...
remoting/client/plugin/pepper_input_handler.h:81: enum MouseLockState {
On 2013/09/07 01:11:25, Wez wrote:
> nit: This belongs at the top of the private section

Done.

[commit-bot: I haz the power, 2013-09-09 18:30:21]

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

[commit-bot: I haz the power, 2013-09-09 22:26:50]

Change committed as 222110