Handle accent keys from physical keyboards.

Handle accent keys from physical keyboards.

BUG=230921
Committed: https://crrev.com/13ff5ad10cb62596ebd35e27ab9dbb002dcada17
Cr-Commit-Position: refs/heads/master@{#307474}

[bcwhite, 2014-12-02 22:22:21]

bcwhite@chromium.org changed reviewers:
+ aurimas@chromium.org, tedchoc@chromium.org

[bcwhite, 2014-12-02 22:22:26]

I started off doing things in different places but ended up putting it in the
same function as Ted's original patch.  :-)

[Ted C, 2014-12-03 02:30:14]

overall this looks good to me...let's wait for aurimas to take a peek tomorrow

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:221:
mPendingAccent = 0;
do we need to do this as well on restartInput (or whatever it is called today)?

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:388:
int nextAccent = pendingAccent;
In what cases is this used?  Just wondering what combination of key strokes
would not result in it either being consumed or discarded on the following
keystroke?

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:525:
int unichar = event.getUnicodeChar();
The other file called them unicodeChar, so I would do that here for consistency.

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:527:
int combined = KeyCharacterMap.getDeadChar(accentChar, unichar);
Any reason to use KeyCharacterMap instead of KeyEvent like in the other file?

And just for sanity, this really is a map lookup right?  We call this a few
times per key stroke, but I suspect it's pretty much "free".

[bcwhite, 2014-12-03 15:49:10]

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:221:
mPendingAccent = 0;
On 2014/12/03 02:30:14, Ted C wrote:
> do we need to do this as well on restartInput (or whatever it is called
today)?

Seems the smart thing to do.  Done.

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:388:
int nextAccent = pendingAccent;
On 2014/12/03 02:30:14, Ted C wrote:
> In what cases is this used?  Just wondering what combination of key strokes
> would not result in it either being consumed or discarded on the following
> keystroke?

It's used to keep all the accent-processing code at the top and the
non-accent-processing code at the bottom.  But thinking about it some more...  I
should be able to just switch the two blocks and simplify this.

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:525:
int unichar = event.getUnicodeChar();
On 2014/12/03 02:30:14, Ted C wrote:
> The other file called them unicodeChar, so I would do that here for
consistency.

Done.

https://codereview.chromium.org/759033002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:527:
int combined = KeyCharacterMap.getDeadChar(accentChar, unichar);
On 2014/12/03 02:30:14, Ted C wrote:
> Any reason to use KeyCharacterMap instead of KeyEvent like in the other file?

Not particularly.  I was typing both names in different places and probably just
got the wrong one here (which happens to also work).


> And just for sanity, this really is a map lookup right?  We call this a few
> times per key stroke, but I suspect it's pretty much "free".

The Android code I looked at doesn't seem to cache the information, at least not
at the KeyEvent/KeyCharacterMap level.  The lower levels may.  Assuming a human
typing, it shouldn't be an issue.

[aurimas (slooooooooow), 2014-12-04 00:33:33]

LGTM % nits

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:221:
mPendingAccent = 0;
Can we create a constant NO_ACCENT or something like that so it is easier to
understand what all the zeros mean.

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:376:
// Short-cut modifier keys so they're not affected my accents.
s/my/by

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:396:
combinedChar = KeyEvent.getDeadChar(mPendingAccent, unicodeChar);
There are two places: AdapterInputConnection#sendKeyEvent and
ImeAdapter#translateAndSendNativeEvents that call exact same thing, can we make
a static method that both of them use?

[bcwhite, 2014-12-04 01:52:23]

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:221:
mPendingAccent = 0;
On 2014/12/04 00:33:33, aurimas wrote:
> Can we create a constant NO_ACCENT or something like that so it is easier to
> understand what all the zeros mean.

Done.

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:376:
// Short-cut modifier keys so they're not affected my accents.
On 2014/12/04 00:33:33, aurimas wrote:
> s/my/by

Done.

https://codereview.chromium.org/759033002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:396:
combinedChar = KeyEvent.getDeadChar(mPendingAccent, unicodeChar);
On 2014/12/04 00:33:33, aurimas wrote:
> There are two places: AdapterInputConnection#sendKeyEvent and
> ImeAdapter#translateAndSendNativeEvents that call exact same thing, can we
make
> a static method that both of them use?

Done.

[bcwhite, 2014-12-09 14:49:16]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2014-12-09 14:50:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759033002/120001

[commit-bot: I haz the power, 2014-12-09 15:22:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-09 15:23:03]

Patchset 7 (id:??) landed as
https://crrev.com/13ff5ad10cb62596ebd35e27ab9dbb002dcada17
Cr-Commit-Position: refs/heads/master@{#307474}