Use DOM- rather than Windows-based key code for non-layout cases.

Use DOM- rather than Windows-based key code for non-layout cases.

BUG=444048
Committed: https://crrev.com/4675e7cbedf99fcc18ae9bfcf19225740fed9387
Cr-Commit-Position: refs/heads/master@{#326111}

[kpschoedel, 2015-01-14 21:00:32]

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

[kpschoedel, 2015-01-14 21:00:32]

wez@ for ui/events/keycodes/
spang@ for ui/events/ozone/
sadrul@ for ui/events/*

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

This CL fulfills to TODOs to use |code| rather than the Windows-based VKEY for
fallback and control character interpretation, mostly by borrowing from the
Ozone ‘stub’ layout.

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:36: // implemented in event_x.cc).
A subsequent CL will remove the Get…FromKeyCode() functions.

(Aside: historically there's a mix of FooToBar() and GetBarFromFoo() names for
conversions; in these changes I'm trying to use FooToBar() consistently for
mappings and reserve GetX() for object getters.)

DomCodeToMeaning() replaces Get…FromKeyCode() and the other functions below are
used both by it and by Ozone layout.

[spang, 2015-01-16 18:37:03]

lgtm

[Wez, 2015-01-17 02:06:48]

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:19: #define DK 0x8000
Use a const char16 for this instead of a macro?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:23: } kPrintableCodeMap[] = {
Suggest moving this into a separate file e.g. dom_us_layout_mappings.h or
similar, to unclutter this file and make it really explicit that these are
US-English only and should not in general be used.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:368: // where the VKEYs are
interpreted positionally (located).
This also assumes a US English layout.

What is the actual use-case for all these functions that assume US English
layouts? Aside from tests we shouldn't be relying on these even as fall-backs.
:-/

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:982: // determined from a
physical scan code.
This comment is in the header; no need for it here - same for the other
functions in this .cc

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:983: DomCode
KeyboardCodeToDomCode(KeyboardCode key_code) {
nit: Move this to appear in the same order relative to
DomCodeToNonLocatedKeyboardCode as in the header.

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:31: // information to help
determine the correct character.
nit: Skimming this comment block, it's not clear to me exactly how it relates to
the functions below. e.g. this mention of "a special native event object"
doesn't seem to tie up with anything below?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:36: // implemented in event_x.cc).
On 2015/01/14 21:00:32, kpschoedel wrote:
> A subsequent CL will remove the Get…FromKeyCode() functions.
> 
> (Aside: historically there's a mix of FooToBar() and GetBarFromFoo() names for
> conversions; in these changes I'm trying to use FooToBar() consistently for
> mappings and reserve GetX() for object getters.)
> 
> DomCodeToMeaning() replaces Get…FromKeyCode() and the other functions below
are
> used both by it and by Ozone layout.

+1 to the FooToBar() function names. :)

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:39: EVENTS_BASE_EXPORT bool
GetMeaningFromKeyCode(KeyboardCode key_code,
Suggest adding a comment to this w/ TODO(...) and referring to the bug # for the
remaining cleanup work that will remove this.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:47: KeyboardCode* key_code);
Provide a brief comment documenting the in & out parameters?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:49: // Obtains the control
character corresponding to a physical key.
Need to clarify what you mean by "control character"; are you saying that this
function returns the ASCII control code that the physical key would generate if
Control is pressed, or something else?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:50: // In some contexts this is
used instead of the key layout.
Why? What is the layout that this function applies? e.g. if my keyboard is set
for Hebrew then does Control+<character key> get mapped to a control code as
though the layout were US English? Or does the mapping fail, or something else?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:52: // is interpreted as a control
character.
So this parameter _only_ succeeds if the supplied dom_code and flags would
result in an ASCII control code?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:67: EVENTS_BASE_EXPORT
KeyboardCode DomCodeToKeyboardCode(DomCode dom_code);
How does this behave for Numeric Keypad keys, which are non-printable if NumLock
is off, and printable otherwise?

How is this function intended to work if e.g. the user has configured their
Linux or ChromeOS device to swap Control and CapsLock?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:72:
DomCodeToNonLocatedKeyboardCode(DomCode dom_code);
nit: Do you need this or could you just let the caller pass the VKEY on to the
LocatedToNonLocated helper function?

If you're returning a non-located code, is it helpful to also (optionally?)
return the location separately?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:88: // determined from a physical
scan code.
Do you mean "when a key event has no |code| value associated with it, e.g. if it
was generated synthetically by JavaScript, with only a VKEY value supplied"?

[kpschoedel, 2015-01-21 02:07:56]

Patchset #2 (id:20001) has been deleted

[kpschoedel, 2015-02-18 17:14:01]

I'm temporarily not working on this while busy with another project, but am
publishing the current state.

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:23: } kPrintableCodeMap[] = {
On 2015/01/17 02:06:47, Wez wrote:
> Suggest moving this into a separate file e.g. dom_us_layout_mappings.h or
> similar, to unclutter this file and make it really explicit that these are
> US-English only and should not in general be used.

Done.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:368: // where the VKEYs are
interpreted positionally (located).
On 2015/01/17 02:06:47, Wez wrote:
> This also assumes a US English layout.
> 
> What is the actual use-case for all these functions that assume US English
> layouts? Aside from tests we shouldn't be relying on these even as fall-backs.
> :-/

This appears to have been done originally, quite a while ago
<https://codereview.chromium.org/6713027/>, to provide (as part of IME work) to
provide the constructor KeyEvent(EventType, KeyboardCode, flags). I don't know
the reason for avoiding layout. Given time I am hoping to remove the several
‘incomplete’ KeyEvent constructors as part of <https//crbug.com/444045> and have
every KeyEvent be fully and explicitly defined at construction. 

GetCharacterFromKeyCode() is also used in connection with menu
shortcuts/accelerators, which I assume is an explicit attempt to avoid layout.

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:31: // information to help
determine the correct character.
On 2015/01/17 02:06:47, Wez wrote:
> nit: Skimming this comment block, it's not clear to me exactly how it relates
to
> the functions below. e.g. this mention of "a special native event object"
> doesn't seem to tie up with anything below?

That dates to the original implementation of GetCharacterFromKeyCode(), and my
interpretation of the comment is that is was intended to create fake
platform-native events if necessary to support events where the character did
not correspond to the VKEY in US layout.

I've revised the comment for now, though it will go away along with the
functions.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:39: EVENTS_BASE_EXPORT bool
GetMeaningFromKeyCode(KeyboardCode key_code,
On 2015/01/17 02:06:48, Wez wrote:
> Suggest adding a comment to this w/ TODO(...) and referring to the bug # for
the
> remaining cleanup work that will remove this.

Done.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:47: KeyboardCode* key_code);
On 2015/01/17 02:06:47, Wez wrote:
> Provide a brief comment documenting the in & out parameters?

Done.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:49: // Obtains the control
character corresponding to a physical key.
On 2015/01/17 02:06:48, Wez wrote:
> Need to clarify what you mean by "control character"; are you saying that this
> function returns the ASCII control code that the physical key would generate
if
> Control is pressed, or something else?

Yes, see below.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:50: // In some contexts this is
used instead of the key layout.
On 2015/01/17 02:06:48, Wez wrote:
> Why?

As far as I can tell from searching commits, this was added several years ago to
support the field blink::WebKeyboardEvent::text, whose comment sheds no light on
the matter. 

> What is the layout that this function applies? e.g. if my keyboard is set
> for Hebrew then does Control+<character key> get mapped to a control code as
> though the layout were US English?

I not entirely sure of the original intent, but I assume it's related to
Windows-style shortcuts (i.e. what's the paste shortcut on a Hebrew keyboard?)
with a dash of Windows-style confusion between shortcuts and ASCII control
codes. To me this makes some sense for non-Latin layouts but none at all for
AZERTY/QWERTZ/Dvorak/etc., but I'm aiming not to change any existing behaviour
in this CL — issues 444045 and 444048 are meant to make the relatively sane DOM
3 |code| and |key| the source of truth in ui::KeyEvent so that reasonable
behaviour can be built on top.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:52: // is interpreted as a control
character.
On 2015/01/17 02:06:48, Wez wrote:
> So this parameter _only_ succeeds if the supplied dom_code and flags would
> result in an ASCII control code?

Yes, commented.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:67: EVENTS_BASE_EXPORT
KeyboardCode DomCodeToKeyboardCode(DomCode dom_code);
On 2015/01/17 02:06:48, Wez wrote:
> How does this behave for Numeric Keypad keys, which are non-printable if
NumLock
> is off, and printable otherwise?
> 
> How is this function intended to work if e.g. the user has configured their
> Linux or ChromeOS device to swap Control and CapsLock?

Does the modified comment help?

This is currently used as a last resort by XKB layout if other methods of
determining a suitable VKEY fail.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:72:
DomCodeToNonLocatedKeyboardCode(DomCode dom_code);
On 2015/01/17 02:06:47, Wez wrote:
> nit: Do you need this or could you just let the caller pass the VKEY on to the
> LocatedToNonLocated helper function?

Right, removed this.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:88: // determined from a physical
scan code.
On 2015/01/17 02:06:48, Wez wrote:
> Do you mean "when a key event has no |code| value associated with it, e.g. if
it
> was generated synthetically by JavaScript, with only a VKEY value supplied"?

At the moment this is used for the old constructor KeyEvent(EventType,
KeyboardCode, flags) to remove some dependence on the VKEY later in the code.
Most uses of that are in tests (where it will be possible to supply the DomCode
directly) and I haven't yet isolated the other uses.

[Wez, 2015-02-19 23:16:09]

Comments on the latest parked state. :)

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:368: // where the VKEYs are
interpreted positionally (located).
On 2015/02/18 17:14:01, kpschoedel wrote:
> On 2015/01/17 02:06:47, Wez wrote:
> > This also assumes a US English layout.
> > 
> > What is the actual use-case for all these functions that assume US English
> > layouts? Aside from tests we shouldn't be relying on these even as
fall-backs.
> > :-/
> 
> This appears to have been done originally, quite a while ago
> <https://codereview.chromium.org/6713027/>, to provide (as part of IME work)
to
> provide the constructor KeyEvent(EventType, KeyboardCode, flags). I don't know
> the reason for avoiding layout. Given time I am hoping to remove the several
> ‘incomplete’ KeyEvent constructors as part of <https//crbug.com/444045> and
have
> every KeyEvent be fully and explicitly defined at construction. 
> 
> GetCharacterFromKeyCode() is also used in connection with menu
> shortcuts/accelerators, which I assume is an explicit attempt to avoid layout.

Yeah, I'm not sure what the rationale in the IME approach was, unfortunately,
although IIRC most non-European language layouts use US English VKEY values for
most keys, so perhaps that's true of all the languages that use IMEs?

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:982: // determined from a
physical scan code.
On 2015/01/17 02:06:47, Wez wrote:
> This comment is in the header; no need for it here - same for the other
> functions in this .cc

Ping.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:983: DomCode
KeyboardCodeToDomCode(KeyboardCode key_code) {
On 2015/01/17 02:06:47, Wez wrote:
> nit: Move this to appear in the same order relative to
> DomCodeToNonLocatedKeyboardCode as in the header.

Ping.

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:31: // information to help
determine the correct character.
On 2015/02/18 17:14:01, kpschoedel wrote:
> On 2015/01/17 02:06:47, Wez wrote:
> > nit: Skimming this comment block, it's not clear to me exactly how it
relates
> to
> > the functions below. e.g. this mention of "a special native event object"
> > doesn't seem to tie up with anything below?
> 
> That dates to the original implementation of GetCharacterFromKeyCode(), and my
> interpretation of the comment is that is was intended to create fake
> platform-native events if necessary to support events where the character did
> not correspond to the VKEY in US layout.
> 
> I've revised the comment for now, though it will go away along with the
> functions.

Acknowledged.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:50: // In some contexts this is
used instead of the key layout.
On 2015/02/18 17:14:01, kpschoedel wrote:
> On 2015/01/17 02:06:48, Wez wrote:
> > Why?
> 
> As far as I can tell from searching commits, this was added several years ago
to
> support the field blink::WebKeyboardEvent::text, whose comment sheds no light
on
> the matter.

:(  That sounds like something Gary may know more about.

> > What is the layout that this function applies? e.g. if my keyboard is set
> > for Hebrew then does Control+<character key> get mapped to a control code as
> > though the layout were US English?
> 
> I not entirely sure of the original intent, but I assume it's related to
> Windows-style shortcuts (i.e. what's the paste shortcut on a Hebrew keyboard?)
> with a dash of Windows-style confusion between shortcuts and ASCII control
> codes. To me this makes some sense for non-Latin layouts but none at all for
> AZERTY/QWERTZ/Dvorak/etc., but I'm aiming not to change any existing behaviour
> in this CL — issues 444045 and 444048 are meant to make the relatively sane
DOM
> 3 |code| and |key| the source of truth in ui::KeyEvent so that reasonable
> behaviour can be built on top.

OK, SGTM

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:88: // determined from a physical
scan code.
On 2015/02/18 17:14:01, kpschoedel wrote:
> On 2015/01/17 02:06:48, Wez wrote:
> > Do you mean "when a key event has no |code| value associated with it, e.g.
if
> it
> > was generated synthetically by JavaScript, with only a VKEY value supplied"?
> 
> At the moment this is used for the old constructor KeyEvent(EventType,
> KeyboardCode, flags) to remove some dependence on the VKEY later in the code.
> Most uses of that are in tests (where it will be possible to supply the
DomCode
> directly) and I haven't yet isolated the other uses.

OK, cool. Would it cause a lot of churn to rename to include "UsLayout" in the
name?

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
File ui/events/keycodes/dom_us_layout_data.h (right):

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
ui/events/keycodes/dom_us_layout_data.h:3: // found in the LICENSE file.
nit: Generally I think this file could benefit from the comments on entries all
being aligned consistently.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
ui/events/keycodes/dom_us_layout_data.h:12: const base::char16 kDeadKey =
0x8000;
nit: Suggest kDeadKeyFlag

nit: Presence of dead-keys means that this is a US English / International
layout, doesn't it? I thought normal US English didn't have dead keys?

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
ui/events/keycodes/dom_us_layout_data.h:376: {DomCode::KEY_A, VKEY_A},          
   // 0x070004 KeyA
Inconsistent comment indentation between here and IntlHash onwards?

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion.h (right):

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion.h:48: // is other better way to
obtain the meaning (e.g. actual keyboard layout).
Do you mean where there *isn't* a better way?

Suggest including "UsLayout" in the name so that it's clear this shouldn't
generally be used.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion.h:57: // containing EF_CONTROL_DOWN)
under a specific US ASCII layout.
You mean under _the_ US English layout?

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion.h:75: // layout-independent physical
key (e.g. DomCode::KEY_Q -> VKEY_Q). This
Rather than layout independent, you're really saying it's hard-wire for US
English again, aren't you?

https://codereview.chromium.org/841263005/diff/40001/ui/events/ozone/layout/x...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right):

https://codereview.chromium.org/841263005/diff/40001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:783: // that we do not
care about.
This comment's a little confusing; you mean that it sets |character|, or clears
it to 0x0 if it's not a key we care about...?

[kpschoedel, 2015-04-10 18:31:53]

The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run

[kpschoedel, 2015-04-10 18:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/841263005/#ps60001 (title:
"address review comments (wez@)")

[kpschoedel, 2015-04-10 18:32:34]

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:19: #define DK 0x8000
On 2015/01/17 02:06:47, Wez wrote:
> Use a const char16 for this instead of a macro?

Removed entirely.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:982: // determined from a
physical scan code.
On 2015/02/19 23:16:08, Wez wrote:
> On 2015/01/17 02:06:47, Wez wrote:
> > This comment is in the header; no need for it here - same for the other
> > functions in this .cc
> 
> Ping.

Done.

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.cc:983: DomCode
KeyboardCodeToDomCode(KeyboardCode key_code) {
On 2015/02/19 23:16:08, Wez wrote:
> On 2015/01/17 02:06:47, Wez wrote:
> > nit: Move this to appear in the same order relative to
> > DomCodeToNonLocatedKeyboardCode as in the header.
> 
> Ping.

Done.

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

https://codereview.chromium.org/841263005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion.h:88: // determined from a physical
scan code.
On 2015/02/19 23:16:08, Wez wrote:
> On 2015/02/18 17:14:01, kpschoedel wrote:
> > On 2015/01/17 02:06:48, Wez wrote:
> > > Do you mean "when a key event has no |code| value associated with it, e.g.
> if
> > it
> > > was generated synthetically by JavaScript, with only a VKEY value
supplied"?
> > 
> > At the moment this is used for the old constructor KeyEvent(EventType,
> > KeyboardCode, flags) to remove some dependence on the VKEY later in the
code.
> > Most uses of that are in tests (where it will be possible to supply the
> DomCode
> > directly) and I haven't yet isolated the other uses.
> 
> OK, cool. Would it cause a lot of churn to rename to include "UsLayout" in the
> name?

Done.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
File ui/events/keycodes/dom_us_layout_data.h (right):

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
ui/events/keycodes/dom_us_layout_data.h:3: // found in the LICENSE file.
On 2015/02/19 23:16:08, Wez wrote:
> nit: Generally I think this file could benefit from the comments on entries
all
> being aligned consistently.

Agreed & done. Sadly clang_format undoes existing comment alignment and produces
these 'locally aligned' comments.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
ui/events/keycodes/dom_us_layout_data.h:12: const base::char16 kDeadKey =
0x8000;
On 2015/02/19 23:16:08, Wez wrote:
> nit: Suggest kDeadKeyFlag
> 
> nit: Presence of dead-keys means that this is a US English / International
> layout, doesn't it? I thought normal US English didn't have dead keys?

Done. This originated from the Ozone stub (before we got xkb working) where it
was useful to have dead keys, but that is not the case in its new role.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/dom_u...
ui/events/keycodes/dom_us_layout_data.h:376: {DomCode::KEY_A, VKEY_A},          
   // 0x070004 KeyA
On 2015/02/19 23:16:08, Wez wrote:
> Inconsistent comment indentation between here and IntlHash onwards?

Done.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion.h (right):

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion.h:48: // is other better way to
obtain the meaning (e.g. actual keyboard layout).
On 2015/02/19 23:16:08, Wez wrote:
> Do you mean where there *isn't* a better way?
> 
> Suggest including "UsLayout" in the name so that it's clear this shouldn't
> generally be used.

Done.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion.h:57: // containing EF_CONTROL_DOWN)
under a specific US ASCII layout.
On 2015/02/19 23:16:08, Wez wrote:
> You mean under _the_ US English layout?

Done.

https://codereview.chromium.org/841263005/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion.h:75: // layout-independent physical
key (e.g. DomCode::KEY_Q -> VKEY_Q). This
On 2015/02/19 23:16:08, Wez wrote:
> Rather than layout independent, you're really saying it's hard-wire for US
> English again, aren't you?

The intent was that the VKEY here is used purely to label a physical key, so
it's US layout in the same sense that DOM3 code 'KeyA' or USB 07:0004 being
called 'Keyboard a and A' is US layout.

However, after looking at the remaining uses in this refactoring, I think it's
better to eliminate this function entirely.

https://codereview.chromium.org/841263005/diff/40001/ui/events/ozone/layout/x...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right):

https://codereview.chromium.org/841263005/diff/40001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:783: // that we do not
care about.
On 2015/02/19 23:16:09, Wez wrote:
> This comment's a little confusing; you mean that it sets |character|, or
clears
> it to 0x0 if it's not a key we care about...?

Yes, it made sense while I reviewed it.... Regardless of what the xkb layout
defines, ChromeOS only allows certain Control+Key combinations to generate
characters (from C0). The next CL in this cleanup chain removes
GetCharacterFromKeyCode(), so I'll have another look then.

[commit-bot: I haz the power, 2015-04-10 20:37:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/841263005/60001

[commit-bot: I haz the power, 2015-04-10 22:08:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-10 22:08:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[kpschoedel, 2015-04-14 16:48:32]

The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run

[kpschoedel, 2015-04-14 16:48:33]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/841263005/#ps80001 (title:
"fix Windows build")

[commit-bot: I haz the power, 2015-04-14 16:48:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/841263005/80001

[commit-bot: I haz the power, 2015-04-14 17:01:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-14 17:01:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kpschoedel, 2015-04-14 17:28:41]

Patchset #4 (id:80001) has been deleted

[Wez, 2015-04-21 02:33:28]

lgtm

[sadrul, 2015-04-21 03:26:54]

Are the test failures related to the CL?

[kpschoedel, 2015-04-21 14:23:08]

On 2015/04/21 03:26:54, sadrul wrote:
> Are the test failures related to the CL?

They are. I've been tracking down the crash and will have a new patch set soon.

[kpschoedel, 2015-04-21 17:25:04]

The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run

[kpschoedel, 2015-04-21 17:25:04]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/841263005/#ps120001
(title: "Fix virtual keyboard, added caution and unit test")

[kpschoedel, 2015-04-21 17:25:48]

Now reviewable.

[commit-bot: I haz the power, 2015-04-21 17:26:10]

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

[commit-bot: I haz the power, 2015-04-21 18:37:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-21 18:37:02]

Dry run: This issue passed the CQ dry run.

[sadrul, 2015-04-21 18:48:16]

lgtm

https://codereview.chromium.org/841263005/diff/120001/ui/keyboard/keyboard_ut...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/841263005/diff/120001/ui/keyboard/keyboard_ut...
ui/keyboard/keyboard_util.cc:223: }
This seems a bit unfortunate. Is it possible to automatically map VKEY_* to the
corresponding DomCode::ARROW_* and DomKey::ARROW_* keys?

[kpschoedel, 2015-04-21 19:32:07]

https://codereview.chromium.org/841263005/diff/120001/ui/keyboard/keyboard_ut...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/841263005/diff/120001/ui/keyboard/keyboard_ut...
ui/keyboard/keyboard_util.cc:223: }
On 2015/04/21 18:48:16, sadrul wrote:
> This seems a bit unfortunate. Is it possible to automatically map VKEY_* to
the
> corresponding DomCode::ARROW_* and DomKey::ARROW_* keys?

Done, using DomCode as the source of truth. In the future, after most of the
KeyEvent cleanup and Ozone/X11 consolidation, I hope to add something a little
more streamlined, like a constructor or utility function building a KeyEvent
with construction-time layout from a (DomCode, flags) pair on systems that can
do it (ChromeOS at least).

https://codereview.chromium.org/841263005/diff/120001/ui/keyboard/keyboard_ut...
ui/keyboard/keyboard_util.cc:312: modifiers);
Remark: if the virtual keyboard knows the intended output, it should probably
shift to using the full constructor rather than having the event go through
layout.

[sadrul, 2015-04-21 19:42:17]

still lgtm

https://codereview.chromium.org/841263005/diff/140001/ui/keyboard/keyboard_ut...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/841263005/diff/140001/ui/keyboard/keyboard_ut...
ui/keyboard/keyboard_util.cc:250: }
Can lines 214:229 and 234:249 be moved to a separate function? (feel free to do
that in a separate CL)

[kpschoedel, 2015-04-21 19:53:15]

https://codereview.chromium.org/841263005/diff/140001/ui/keyboard/keyboard_ut...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/841263005/diff/140001/ui/keyboard/keyboard_ut...
ui/keyboard/keyboard_util.cc:250: }
On 2015/04/21 19:42:17, sadrul wrote:
> Can lines 214:229 and 234:249 be moved to a separate function? (feel free to
do
> that in a separate CL)

Yes. I'm still thinking about how best to express this as I work through this
set of CLs toward immutable KeyEvent (crbug.com/444045). There are both
layout-independent (like this) and layout-dependent cases of building a KeyEvent
starting from incomplete information, and I want to be sure the distinction is
clear at the call sites.

[kpschoedel, 2015-04-21 19:53:22]

The CQ bit was checked by kpschoedel@chromium.org

[kpschoedel, 2015-04-21 19:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org,
spang@chromium.org
Link to the patchset: https://codereview.chromium.org/841263005/#ps140001
(title: "address review comments (sadrul)")

[commit-bot: I haz the power, 2015-04-21 19:53:41]

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

[commit-bot: I haz the power, 2015-04-21 20:58:22]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-04-21 20:59:56]

Patchset 6 (id:??) landed as
https://crrev.com/4675e7cbedf99fcc18ae9bfcf19225740fed9387
Cr-Commit-Position: refs/heads/master@{#326111}