Add Android Chromoting client keyboard input

Add Android Chromoting client keyboard input

Tapping the screen with three fingers now shows more options, namely one to pop up a keyboard!

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

[solb, 2013-07-30 02:02:00]

This should be almost all of the keyboard stuff, although it still doesn't
handle special keyboard events (e.g. swipe-based typing) and may not be
compatible with certain software keyboards (i.e. those that don't share key
press events).

[garykac, 2013-07-30 17:45:26]

lgtm

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

https://codereview.chromium.org/21120005/diff/25001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:70: /** Why
the screen dimensions are changing. */
I don't understand this comment. What does TRUE mean for this?

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

https://codereview.chromium.org/21120005/diff/25001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.cc:11: //
"ui/base/keycodes/keyboard_code_conversion_android.cc"
Note here that some of the mappings (eg: 'a' -> 0x07004) are assuming a US key
layout.

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

https://codereview.chromium.org/21120005/diff/25001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:17: // be rethought to accomplish this in
an localizable and future-proof way.
Please file a bug for this and add a TODO that refers to it.

[solb, 2013-07-30 18:17:39]

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

https://codereview.chromium.org/21120005/diff/25001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:70: /** Why
the screen dimensions are changing. */
On 2013/07/30 17:45:26, garykac wrote:
> I don't understand this comment. What does TRUE mean for this?

Oops! That was left over from a more complicated intermediate implementation.

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

https://codereview.chromium.org/21120005/diff/25001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.cc:11: //
"ui/base/keycodes/keyboard_code_conversion_android.cc"
On 2013/07/30 17:45:26, garykac wrote:
> Note here that some of the mappings (eg: 'a' -> 0x07004) are assuming a US key
> layout.

Done.

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

https://codereview.chromium.org/21120005/diff/25001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:17: // be rethought to accomplish this in
an localizable and future-proof way.
On 2013/07/30 17:45:26, garykac wrote:
> Please file a bug for this and add a TODO that refers to it. 

265945

[Sergey Ulanov, 2013-07-30 19:19:26]

some drive-by nits and questions

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

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:68: private
Constraint mConstraint;
Should this be called mZoomConstraint?

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:135:
mConstraint = (double)image.getWidth()/image.getHeight() >
mConstraint is calculated and used in the same method. Why do we need to store
it between redraws?

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:183: else {
nit: else goes to the previous line.

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:200: else {
same here.

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:225:
mRecheckConstraint = true;
Can we reset mContraint here?

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:293: if
(event.getPointerCount() == 1) {
Do we still need to handle the event here when handled = true?
I.e. if user is scrolling do we still send that event to the host?

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

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:18: // TODO(solb): crbug.com/265945
nit: TODO() should be on line 13 before the comment.

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

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:114: this,
two arguments below this one would fit on this line.

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:128: LOG(WARNING) << "Ignoring
unknown keycode: " << keyCode;
nit: add {}

[commit-bot: I haz the power, 2013-07-30 21:25:46]

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

[solb, 2013-07-30 21:30:54]

I've already pushed a build of this patchset, so rather than cause confusion I'm
letting it through and addressing your concerns as part of
https://codereview.chromium.org/21236002.

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

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:68: private
Constraint mConstraint;
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> Should this be called mZoomConstraint?

Not really, since it also affects panning behavior. I feel like it could use a
better name, but I haven't thought of an appropriate one yet.

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:135:
mConstraint = (double)image.getWidth()/image.getHeight() >
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> mConstraint is calculated and used in the same method. Why do we need to store
> it between redraws?

Sometimes we need to keep the old value between redraws (e.g. when opening a
software keyboard, we'd like to keep the view parameters as consistent as
possible.

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:183: else {
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> nit: else goes to the previous line.

Handling in https://codereview.chromium.org/21236002 to let this one through CQ.

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:200: else {
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> same here.

https://codereview.chromium.org/21236002

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:225:
mRecheckConstraint = true;
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> Can we reset mContraint here?

No, because we won't know the screen size until onSurfaceChanged() or the image
size until run().

https://codereview.chromium.org/21120005/diff/37001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:293: if
(event.getPointerCount() == 1) {
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> Do we still need to handle the event here when handled = true?
> I.e. if user is scrolling do we still send that event to the host?

Unfortunately, in order to ensure the gesture detectors don't overlook desired
events, I have to accept all events in onDown()and onScaleBegin(), which results
in their consuming some events that they don't actually use for panning or
zooming (e.g. non-multitouch gestures). I've experimented with a better way to
ignore these, but with little success. There is definitely room for future
improvement; it's a little more sensitive to spotting mouse events during
panning and zooming than I'd like.

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

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/andro...
remoting/client/jni/android_keymap.h:18: // TODO(solb): crbug.com/265945
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> nit: TODO() should be on line 13 before the comment.

Patched in https://codereview.chromium.org/21236002, which would never land if I
didn't let this through CQ.

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

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:114: this,
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> two arguments below this one would fit on this line.

It's formatted in accordance with
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
("If the function has many arguments, consider having one per line if this makes
the code more readable.")

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:128: LOG(WARNING) << "Ignoring
unknown keycode: " << keyCode;
On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> nit: add {}

Done in https://codereview.chromium.org/21236002

[Sergey Ulanov, 2013-07-30 21:42:10]

lgtm

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

https://codereview.chromium.org/21120005/diff/37001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:114: this,
On 2013/07/30 21:30:54, solb wrote:
> On 2013/07/30 19:19:27, Sergey Ulanov wrote:
> > two arguments below this one would fit on this line.
> 
> It's formatted in accordance with
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> ("If the function has many arguments, consider having one per line if this
makes
> the code more readable.")

"if this makes the code more readable". IMO it doesn't make this code more
readable, particularly in this case with base::Bind() when all actual parameters
for the function fit on one line. But feel free to disagree.

[commit-bot: I haz the power, 2013-07-30 22:34:36]

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=win&number...

[commit-bot: I haz the power, 2013-07-31 00:08:16]

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

[commit-bot: I haz the power, 2013-07-31 00:37:51]

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=win_rel&nu...

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

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

[commit-bot: I haz the power, 2013-07-31 09:20:24]

Change committed as 214646