Draw the mouse cursor in Chromoting Android client

Draw the mouse cursor in Chromoting Android client

This CL draws the mouse cursor shape on the client, at the most recent
position of an injected mouse move event (or the center of the screen if
no events have been injected yet).

BUG=270347
TEST=manual, verify mouse cursor appears and moves as expected.

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

[solb, 2013-09-14 00:29:40]

LGTM, other than these minor synchronization concerns:

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:230:
canvas.drawBitmap(cursorBitmap, mCursorPosition.x - hotspot.x,
It might be good to synchronize this on mCursorPosition so that a poorly-timed
mouse click can't result in the cursor's being drawn in an odd position.

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:259:
mCursorPosition.x = width / 2;
Synchronizing these on mTransform does no good, since they're never used within
a the critical period of its rendering.  Might be better to lock on
mCursorPosition itself?

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:318:
mCursorPosition.x = (int)coordinates[0];
I think these two lines should also be synchronized on mCursorPosition.

[Lambros, 2013-09-14 01:16:47]

Added synchronization - ptal and make sure it's OK?

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:230:
canvas.drawBitmap(cursorBitmap, mCursorPosition.x - hotspot.x,
On 2013/09/14 00:29:41, solb wrote:
> It might be good to synchronize this on mCursorPosition so that a poorly-timed
> mouse click can't result in the cursor's being drawn in an odd position.

Good catch! Done.

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:259:
mCursorPosition.x = width / 2;
On 2013/09/14 00:29:41, solb wrote:
> Synchronizing these on mTransform does no good, since they're never used
within
> a the critical period of its rendering.  Might be better to lock on
> mCursorPosition itself?

Done.

https://codereview.chromium.org/23532072/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:318:
mCursorPosition.x = (int)coordinates[0];
On 2013/09/14 00:29:41, solb wrote:
> I think these two lines should also be synchronized on mCursorPosition.

Done. I've also added a synchronize around mTransform, hope that's OK.

[solb, 2013-09-14 01:48:21]

LGTM; I also tested it out again, and everything looks good while connecting to
both Ubuntu and Win7 hosts.

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:323:
mTransform.invert(canvasToImage);
You were good to notice this. Your change ensures that clicks made while the
image resizes register at a consistent set of coordinates.

[Sergey Ulanov, 2013-09-14 02:21:29]

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:327:
sCursorHotspot.x = hotspotX;
it's better to create a new Point instance here instead of mutating it in-place.
Problem is that caller or getCursorHotspot() may store result and wouldn't
expect it to be change.

Also you can use Point.set() to set x and y at the same time.

[solb, 2013-09-14 03:44:41]

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:327:
sCursorHotspot.x = hotspotX;
On 2013/09/14 02:21:29, Sergey Ulanov wrote:
> it's better to create a new Point instance here instead of mutating it
in-place.
> Problem is that caller or getCursorHotspot() may store result and wouldn't
> expect it to be change.
> 
> Also you can use Point.set() to set x and y at the same time.

I think Sergey brings up a good point here.

[Lambros, 2013-09-16 21:52:58]

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/23532072/diff/6001/remoting/android/java/src/...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:327:
sCursorHotspot.x = hotspotX;
On 2013/09/14 02:21:29, Sergey Ulanov wrote:
> it's better to create a new Point instance here instead of mutating it
in-place.
> Problem is that caller or getCursorHotspot() may store result and wouldn't
> expect it to be change.
> 
> Also you can use Point.set() to set x and y at the same time.

Done.

[commit-bot: I haz the power, 2013-09-16 22:00:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/23532072/1...

[commit-bot: I haz the power, 2013-09-16 22:13:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-16 22:56:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/23532072/1...

[commit-bot: I haz the power, 2013-09-17 02:09:22]

Change committed as 223515