Fix keycode problems for modified keys.

Fix keycode problems for modified keys.

There is a problem with keycode generation for non-trivial characters
(i.e. those that have modifiers) because the event array generated is
not simply a KeyDown/KeyUp combination; each modifier also gets a pair
of events.

Instead, scan through the events and send the first KeyDown that isn't
a modifier key.  Also, don't send key events if nothing is changing
(as happens with the final "commit").

SendSythenticKeyEvent needed to be extended to accept modifier flags.

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

[bcwhite, 2014-08-05 19:11:51]

Ping?

[aurimas (slooooooooow), 2014-08-06 22:55:44]

lgtm

https://chromiumcodereview.appspot.com/426063002/diff/20001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/426063002/diff/20001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:338:
if (events == null)  // No known key sequence will create that character.
Add curly braces around return null;

In java we have no curlies only the if statement and the actual statement fit in
one line.

https://chromiumcodereview.appspot.com/426063002/diff/20001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:426:
KeyEvent.ACTION_UP));
In java we don't indent to align with the previous line. We do 8 spaces.

[aurimas (slooooooooow), 2014-08-06 22:56:15]

Sorry for the delay. Reviews got stuck with the movegeddon.

[bcwhite, 2014-08-07 13:41:00]

All moved in, now?

https://chromiumcodereview.appspot.com/426063002/diff/20001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/426063002/diff/20001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:338:
if (events == null)  // No known key sequence will create that character.
On 2014/08/06 22:55:44, aurimas wrote:
> Add curly braces around return null;
> 
> In java we have no curlies only the if statement and the actual statement fit
in
> one line.

Done.

But what's the point of a consistent style when it's not consistent?
(rhetorical question)

https://chromiumcodereview.appspot.com/426063002/diff/20001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:426:
KeyEvent.ACTION_UP));
On 2014/08/06 22:55:44, aurimas wrote:
> In java we don't indent to align with the previous line. We do 8 spaces.

Done.

(See previous comment.  :-)

[bcwhite, 2014-08-07 13:42:35]

jdduke@chromium.org: Please review changes in 
IME test

aelias@chromium.org: Please review changes in 
content/browser/renderer_host

[jdduke (slow), 2014-08-07 13:56:36]

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:371:
return new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL);
A shame to create garbage here, not sure if it's worth caching this KeyEvent
instance though.

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:312:
@SmallTest
Is there already test coverage for the following change in ImeAdapter.java?

if (keyCode > 0 && isCommit && mLastComposeText == null) {
        mLastSyntheticKeyCode = keyCode;
        return translateAndSendNativeEvents(keyEvent) &&
               translateAndSendNativeEvents(KeyEvent.changeAction(
                       keyEvent, KeyEvent.ACTION_UP));
}

[bcwhite, 2014-08-07 15:16:40]

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:371:
return new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL);
On 2014/08/07 13:56:36, jdduke wrote:
> A shame to create garbage here, not sure if it's worth caching this KeyEvent
> instance though.

Given how slow typing is to a machine, I don't think garbage is an issue here.

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://chromiumcodereview.appspot.com/426063002/diff/40001/content/public/an...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:312:
@SmallTest
> Is there already test coverage for the following change in ImeAdapter.java?

Yes.  The "Hacker's Keyboard" test covers this use case and shows that it
continues to work.

[jdduke (slow), 2014-08-07 15:36:35]

Thanks, ImeTest.java lgtm.

[aelias_OOO_until_Jul13, 2014-08-07 18:40:19]

renderer_host lgtm

[bcwhite, 2014-08-07 18:40:35]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:46:39]

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

[commit-bot: I haz the power, 2014-08-08 00:50:07]

Change committed as 288179