Chromoting Android app mouse/keyboard bugfixes and enhancements

Chromoting Android app mouse/keyboard bugfixes and enhancements

- Enable multitouch panning on devices whose gesture detectors filture out such events by default
- Fix problem where the transformation matrix was applied twice when tapping to single click
- Hardcode the expansions for the @#*+ symbols for now
- Fix issue where hitting the keypad enter key could close the software keyboard without warning

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

[solb, 2013-07-30 11:32:34]

This is to apply on top of https://codereview.chromium.org/21120005, which also
needs review.

[garykac, 2013-07-30 17:50:05]

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

https://codereview.chromium.org/21236002/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:78:
JniInterface.keyboardAction(KeyEvent.KEYCODE_2, depressed);
Don't you need to release the SHIFT key here as well?

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

https://codereview.chromium.org/21236002/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:318:
BUTTON_LEFT, true);
indent

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

https://codereview.chromium.org/21236002/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:219:
Log.e("jniiface", "Attempted to press a negative mouse button");
Why are there negative buttons?  Isn't this constrained to be 0, 1 and 3 (or
something like that)?

[solb, 2013-07-30 18:28:54]

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

https://codereview.chromium.org/21236002/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:78:
JniInterface.keyboardAction(KeyEvent.KEYCODE_2, depressed);
On 2013/07/30 17:50:05, garykac wrote:
> Don't you need to release the SHIFT key here as well?

I do. This method gets invoked once for the key down event and once for the
corresponding up event. I've updated its comment to make this more clear.

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

https://codereview.chromium.org/21236002/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:318:
BUTTON_LEFT, true);
On 2013/07/30 17:50:05, garykac wrote:
> indent

http://source.android.com/source/code-style.html#use-spaces-for-indentation

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

https://codereview.chromium.org/21236002/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:219:
Log.e("jniiface", "Attempted to press a negative mouse button");
On 2013/07/30 17:50:05, garykac wrote:
> Why are there negative buttons?  Isn't this constrained to be 0, 1 and 3 (or
> something like that)?

Yes, it is constrained thusly. However, I added these error-checks to enforce
these methods' contracts and prevent them from ever sending invalid button/key
presses to the host.

[garykac, 2013-07-30 18:48:59]

lgtm

[Sergey Ulanov, 2013-07-30 21:45:59]

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
File remoting/client/jni/android_keymap.h (right):

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:13: // TODO(solb): crbug.com/265945
Please keep the reference to the bug at the end. That's the way comments like
this are normally formatted, e.g.:

// TODO(solb): Blah blah blah.
// crbug.com/123123

[Sergey Ulanov, 2013-07-30 21:55:17]

more style nits.

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
File remoting/client/jni/android_keymap.cc (right):

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.cc:207: DCHECK(android < sizeof usb_keycodes
/ sizeof (uint32));
DCHECK_LT.
Please add parenthesis around usb_keycoudes.

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:15: const char* const CHAT_SERVER
= "talk.google.com";
This should be kChatServer (though it might be better to refer to it as XMPP
server). Same for the constants below.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

It's more common to use char arrays for const strings, e.g.:
const char kChatServer[] = "talk.google.com";

Also please put these constants in an anonymous namespace:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:109: void
ChromotingJniInstance::PerformKeyboardAction(int keyCode, bool keyDown) {
Here and in other functions: we don't use camel casing for variables in C++. So
these should be key_code, key_down. Maybe worth having a separate CL to fix it.

[solb, 2013-07-30 23:03:37]

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
File remoting/client/jni/android_keymap.cc (right):

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.cc:207: DCHECK(android < sizeof usb_keycodes
/ sizeof (uint32));
On 2013/07/30 21:55:18, Sergey Ulanov wrote:
> DCHECK_LT.
> Please add parenthesis around usb_keycoudes.

Done.

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
File remoting/client/jni/android_keymap.h (right):

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:13: // TODO(solb): crbug.com/265945
On 2013/07/30 21:46:00, Sergey Ulanov wrote:
> Please keep the reference to the bug at the end. That's the way comments like
> this are normally formatted, e.g.:
> 
> // TODO(solb): Blah blah blah.
> // crbug.com/123123

Done.

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:15: const char* const CHAT_SERVER
= "talk.google.com";
On 2013/07/30 21:55:18, Sergey Ulanov wrote:
> This should be kChatServer (though it might be better to refer to it as XMPP
> server). Same for the constants below.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...
> 
> It's more common to use char arrays for const strings, e.g.:
> const char kChatServer[] = "talk.google.com";
> 
> Also please put these constants in an anonymous namespace:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Wez and I have discussed this very same issue (see the first comment on
https://codereview.chromium.org/19297003/diff2/6001:11001/remoting/client/jni...),
but decided these shouldn't be namespaced because C++ treats const as static
const in this context. I am renaming them though.

https://codereview.chromium.org/21236002/diff/12001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:109: void
ChromotingJniInstance::PerformKeyboardAction(int keyCode, bool keyDown) {
On 2013/07/30 21:55:18, Sergey Ulanov wrote:
> Here and in other functions: we don't use camel casing for variables in C++.
So
> these should be key_code, key_down. Maybe worth having a separate CL to fix
it.

I just fixed it in this CL since it's already backed up behind another one.

[Sergey Ulanov, 2013-07-31 01:54:16]

LGTM. thanks!

https://codereview.chromium.org/21236002/diff/32001/remoting/client/jni/andro...
File remoting/client/jni/android_keymap.h (right):

https://codereview.chromium.org/21236002/diff/32001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:14: // crbug.com/265945
I meant that it should be formatted as follows:
 // TODO(solb): This is a temporary hack to convert Android's virtual keycodes
into USB
 // scancodes. Unfortunately, the current solution uses key mappings whose
 // layout is determined according to the client device's input locale instead
 // of the host's internationalization preferences. The whole process needs to
 // be rethought to accomplish this in an localizable and future-proof way.
 // crbug.com/265945

[solb, 2013-07-31 03:22:52]

https://codereview.chromium.org/21236002/diff/32001/remoting/client/jni/andro...
File remoting/client/jni/android_keymap.h (right):

https://codereview.chromium.org/21236002/diff/32001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:14: // crbug.com/265945
On 2013/07/31 01:54:16, Sergey Ulanov wrote:
> I meant that it should be formatted as follows:
>  // TODO(solb): This is a temporary hack to convert Android's virtual keycodes
> into USB
>  // scancodes. Unfortunately, the current solution uses key mappings whose
>  // layout is determined according to the client device's input locale instead
>  // of the host's internationalization preferences. The whole process needs to
>  // be rethought to accomplish this in an localizable and future-proof way.
>  // crbug.com/265945

Done.

[commit-bot: I haz the power, 2013-07-31 15:17:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/21236002/40001

[commit-bot: I haz the power, 2013-07-31 17:39:17]

Change committed as 214769