Located vs non-located ui::KeyboardCode functions.

Located ui::KeyboardCode functions.

Refactor code converting located (e.g. VKEY_LSHIFT) and non-located
(e.g. VKEY_SHIFT) versions of ui::KeyboardCode.

BUG=444048
Committed: https://crrev.com/463548abf144e351be82cc21eb1a2e30f5284a09
Cr-Commit-Position: refs/heads/master@{#311335}

[kpschoedel, 2015-01-09 20:25:39]

kpschoedel@chromium.org changed reviewers:
+ sadrul@chromium.org, spang@chromium.org, wez@chromium.org

[kpschoedel, 2015-01-09 20:25:40]

wez@ for ui/events/keycodes/
spang@ for ui/events/ozone/
sadrul@ for ui/events/event.cc

This is one of a series of CLs stemming from discussions and TODOs from
https://codereview.chromium.org/742103002/

The end goal is a ui::KeyEvent that can be immutable, sets DOM |key_| and
|code_| appropriately on all platforms, and has fewer redundancies (particularly
between server X11 and library xkbcommon).

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc
File ui/events/event.cc (left):

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc#oldcode904
ui/events/event.cc:904: // if the value is available (as it is for all X11
native-based events).
The comment here no longer applies, since the old bots have since been updated.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:270: // Determine the non-located
VKEY corresponding to a located VKEY.
This functionality, used by Ozone keyboard layout, also exists in
DevToolsEventForwarder::VirtualKeyCodeWithoutLocation() and
content::GetWindowsKeyCodeWithoutLocation(), which I will consolidate in a
follow-up CL.

[spang, 2015-01-09 21:22:39]

lgtm

[Wez, 2015-01-10 02:06:29]

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc#newcode892
ui/events/event.cc:892: KeyboardCode KeyEvent::GetLocatedWindowsKeyboardCode()
const {
AFAICT, this only seems to be used when converting Aura events to Blink
KeyboardEvents, in which case it's wrong, since windowsKeyCode in those events
should be set to the non-located value, which is what other platforms provide
for those keys.

Or is there some later stage that un-locates these codes again...?

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:128: bool
IsRightSideDomCode(DomCode code) {
Why are you treating the modifiers to an IsRightSideDomCode() function while the
numeric pad keys are done in-line below?

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
Since you handle numeric pad keys in the reverse mapping, handle them here as
well?

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
nit: Could/should this method output the Location as well as the non-located
KeyboardCode?

[kpschoedel, 2015-01-12 16:34:06]

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc#newcode892
ui/events/event.cc:892: KeyboardCode KeyEvent::GetLocatedWindowsKeyboardCode()
const {
On 2015/01/10 02:06:28, Wez wrote:
> AFAICT, this only seems to be used when converting Aura events to Blink

There's another call inside mojo.

> KeyboardEvents, in which case it's wrong, since windowsKeyCode in those events
> should be set to the non-located value, which is what other platforms provide
> for those keys.
> 
> Or is there some later stage that un-locates these codes again...?

I assume so, since javascript ends up seeing non-located codes.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:128: bool
IsRightSideDomCode(DomCode code) {
On 2015/01/10 02:06:28, Wez wrote:
> Why are you treating the modifiers to an IsRightSideDomCode() function while
the
> numeric pad keys are done in-line below?

IsRightSideDomCode() exists because |code| doesn't change with remapping or
layout, so we can have for instance DomCode::ALT_LEFT with VKEY_LCONTROL. We
don't have any layouts that move numpad digits (although I can imagine a
'telephone' option). I'll add a IsNumpadDomCode() or equivalent for that if you
like, but see the last comment below.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
On 2015/01/10 02:06:28, Wez wrote:
> Since you handle numeric pad keys in the reverse mapping, handle them here as
> well?

I'll do that in a follow-up CL, since it's a behaviour change from the existing
code, so I'd like to keep it separate from refactoring with no intentional
behaviour change.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
On 2015/01/10 02:06:28, Wez wrote:
> nit: Could/should this method output the Location as well as the non-located
> KeyboardCode?

There is not currently a ui::Event-level analogue of Blink's DOM_KEY_LOCATION_….
I wouldn't mind adding one.

[Wez, 2015-01-13 03:08:36]

lgtm

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/event.cc#newcode892
ui/events/event.cc:892: KeyboardCode KeyEvent::GetLocatedWindowsKeyboardCode()
const {
On 2015/01/12 16:34:06, kpschoedel wrote:
> On 2015/01/10 02:06:28, Wez wrote:
> > AFAICT, this only seems to be used when converting Aura events to Blink
> 
> There's another call inside mojo.

Ah, I wonder if they opted to use located values to avoid the need for a
separate location field...

> > KeyboardEvents, in which case it's wrong, since windowsKeyCode in those
events
> > should be set to the non-located value, which is what other platforms
provide
> > for those keys.
> > 
> > Or is there some later stage that un-locates these codes again...?
> 
> I assume so, since javascript ends up seeing non-located codes.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:128: bool
IsRightSideDomCode(DomCode code) {
On 2015/01/12 16:34:06, kpschoedel wrote:
> On 2015/01/10 02:06:28, Wez wrote:
> > Why are you treating the modifiers to an IsRightSideDomCode() function while
> the
> > numeric pad keys are done in-line below?
> 
> IsRightSideDomCode() exists because |code| doesn't change with remapping or
> layout, so we can have for instance DomCode::ALT_LEFT with VKEY_LCONTROL. We
> don't have any layouts that move numpad digits (although I can imagine a
> 'telephone' option). I'll add a IsNumpadDomCode() or equivalent for that if
you
> like, but see the last comment below.

Acknowledged.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:270: // Determine the non-located
VKEY corresponding to a located VKEY.
On 2015/01/09 20:25:40, kpschoedel wrote:
> This functionality, used by Ozone keyboard layout, also exists in
> DevToolsEventForwarder::VirtualKeyCodeWithoutLocation() and
> content::GetWindowsKeyCodeWithoutLocation(), which I will consolidate in a
> follow-up CL.

Acknowledged.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
On 2015/01/12 16:34:06, kpschoedel wrote:
> On 2015/01/10 02:06:28, Wez wrote:
> > nit: Could/should this method output the Location as well as the non-located
> > KeyboardCode?
> 
> There is not currently a ui::Event-level analogue of Blink's
DOM_KEY_LOCATION_….
> I wouldn't mind adding one.

Arguably not worth it unless we actually need it. :)

If ui::Event ends up containing |key| and |code| then we could have an accessor
that just calls on to IsRightSideDomCode() to synthesize location information.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
On 2015/01/12 16:34:06, kpschoedel wrote:
> On 2015/01/10 02:06:28, Wez wrote:
> > Since you handle numeric pad keys in the reverse mapping, handle them here
as
> > well?
> 
> I'll do that in a follow-up CL, since it's a behaviour change from the
existing
> code, so I'd like to keep it separate from refactoring with no intentional
> behaviour change.

Acknowledged.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.h (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:44: // Determine the non-located
VKEY corresponding to a located VKEY.
Consider clarifying what a "located VKEY" is here.

[kpschoedel, 2015-01-13 16:21:08]

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:271: KeyboardCode
LocatedToNonLocatedKeyboardCode(KeyboardCode key_code) {
On 2015/01/13 03:08:36, Wez wrote:
> If ui::Event ends up containing |key| and |code| [...]

This is mostly implemented and should trickle in through M42.

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion.h (right):

https://codereview.chromium.org/796003005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:44: // Determine the non-located
VKEY corresponding to a located VKEY.
On 2015/01/13 03:08:36, Wez wrote:
> Consider clarifying what a "located VKEY" is here.

Done.

[sadrul, 2015-01-13 18:24:40]

lgtm

[kpschoedel, 2015-01-13 20:01:25]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-01-13 20:02:43]

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

[commit-bot: I haz the power, 2015-01-13 21:53:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-13 21:55:38]

Patchset 2 (id:??) landed as
https://crrev.com/463548abf144e351be82cc21eb1a2e30f5284a09
Cr-Commit-Position: refs/heads/master@{#311335}