DOM Level 3 .code and .key value enumerations.

DOM Level 3 .code and .key value enumerations.

R=garykac@chromium.org,sadrul@chromium.org
BUG=430191
Committed: https://crrev.com/b0eefc502609ba8060df6c8e71123adeeb9fcf83
Cr-Commit-Position: refs/heads/master@{#305306}

[garykac, 2014-10-14 00:25:04]

A general comment before digging into anything else.

The 'dom4' directory name was poorly chosen since DOM4 means something else and
these are now defined in the DOM3 events spec.

For new files being added, do you think it makes sense to start putting them in
a 'dom3' dir (sibling to 'dom4')? That way there'll be less to move (less
history "lost") when we move the existing files in a later CL.

[garykac, 2014-10-14 00:25:21]

https://codereview.chromium.org/641753003/diff/1/ui/events/keycodes/dom4/keyc...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/641753003/diff/1/ui/events/keycodes/dom4/keyc...
ui/events/keycodes/dom4/keycode_converter.h:33: //
https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm
// The DOM3 Events |code| values as defined in:
// http://www.w3.org/TR/DOM-Level-3-Events-code/

[kpschoedel, 2014-10-20 17:54:05]

New files moved to dom3/

Looks like 'enum class' will be available soon, so I'm going to add another
patch set for that, to forestall code/key errors.

https://codereview.chromium.org/641753003/diff/1/ui/events/keycodes/dom4/keyc...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/641753003/diff/1/ui/events/keycodes/dom4/keyc...
ui/events/keycodes/dom4/keycode_converter.h:33: //
https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm
On 2014/10/14 00:25:20, garykac wrote:
> // The DOM3 Events |code| values as defined in:
> // http://www.w3.org/TR/DOM-Level-3-Events-code/

Done.

[kpschoedel, 2014-10-20 18:23:39]

C++11 enum class is now permitted in Chromium.
http://chromium-cpp.appspot.com/

[Wez, 2014-10-22 21:29:31]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2014-10-22 21:29:32]

https://codereview.chromium.org/641753003/diff/60001/ui/events/keycodes/dom3/...
File ui/events/keycodes/dom3/dom_values.txt (right):

https://codereview.chromium.org/641753003/diff/60001/ui/events/keycodes/dom3/...
ui/events/keycodes/dom3/dom_values.txt:63: 0x000 -None                   -None  
                "No value"
Putting code and key in the same table implies 1: mapping, which isnt in general
the case; what's the benefit of a single table?

[kpschoedel, 2014-10-22 21:58:17]

On 2014/10/22 21:29:32, Wez wrote:
> Putting code and key in the same table implies 1: mapping, which isnt in
general
> the case; what's the benefit of a single table?

They're not 1:1, but they often correspond for non-printable keys; the
unmodified Tab |code| almost never maps to anything but the Tab |key|. So part
of the reason for having a single table is to act as a quick reference for when
these correspondences do or do not exist (e.g. 'VolumeUp' vs 'BrightnessUp'), as
well as exceptions to the pattern (e.g. 'Lang1' ↦ 'HangulMode').

Another reason was to keep the numeric values for corresponding entries the
same, to make it easier for someone doing quick printf debugging to recognize
them. This is less of a consideration now that a FooToString(...) function call
is no harder to type than a static_cast<int>(...).

Against that I see the risk that putting them in a single table may encourage
people to conflate them like Windows VK codes. I'm not strongly attached to a
single table and can easily split it if you prefer.

[kpschoedel, 2014-10-23 21:07:22]

Version with independent .code and .key definitions.

[Wez, 2014-10-23 21:14:39]

On 2014/10/23 21:07:22, kpschoedel wrote:
> Version with independent .code and .key definitions.

We already have a table w/ native<->code mappings in keycode_converter_data.h,
and that also provides a USB-like code for each key, which is arguably more
helpful than arbitrary values in the new dom_code.txt; WDYT?

Also see crrev.com/663523002 in which a contributor from Samsung is adding
|code| to Blink.

[kpschoedel, 2014-10-23 21:29:29]

On 2014/10/23 21:14:39, Wez wrote:
> We already have a table w/ native<->code mappings in keycode_converter_data.h,
> and that also provides a USB-like code for each key, which is arguably more
> helpful than arbitrary values in the new dom_code.txt; WDYT?

That sounds reasonable. The one thing lost is dense values to allow using the
enum value as an array index. I would still like to use build-generated files
(from native_codes.txt in this CL) to keep the consistency checking and
generated tables; I'll prototype this quickly and then work on remaining cases
if it looks good.

[kpschoedel, 2014-10-23 22:20:22]

This version builds the ui::DomCode enum with USB values from the native_code
table.

I haven't converted the tests yet, and have a bit of other cleanup to do; I'll
do that in the morning if this scheme looks OK to you in principle.

[kpschoedel, 2014-10-24 17:31:49]

This should be up-to-date with review requests now.

For the DOM3 .code names that were not already in the native_codes file, I
added:

0x010082  Sleep
0x010083  WakeUp
0x070032  IntlHash
0x070074  Open
0x070077  Select
0x070079  Again
0x07007e  Find
0x07008a  Props
0x0700bb  NumpadBackspace
0x0700d0  NumpadMemoryStore
0x0700d1  NumpadMemoryRecall
0x0700d2  NumpadMemoryClear
0x0700d3  NumpadMemoryAdd
0x0700d4  NumpadMemorySubtract
0x0700d8  NumpadClear
0x0700d9  NumpadClearEntry
0x0c0183  MediaSelect
0x0c0192  LaunchApp1
0x0c0194  LaunchApp2
0x0C022A  BrowserFavorites

and dummy codes for the ones with no USB equivalent:

0x000010  Hyper
0x000011  Super
0x000012  Fn
0x000013  FLock-FnLock
0x000014  Suspend
0x000015  Resume
0x000016  Turbo

[kpschoedel, 2014-10-24 20:07:58]

Patchset #8 (id:140001) has been deleted

[Wez, 2014-10-24 23:41:00]

Are the .txt tables sufficiently more readable to justify the extra complexity
of the generator.py script? What was wrong with a simple pair of headers?

[kpschoedel, 2014-10-25 00:41:26]

On 2014/10/24 23:41:00, Wez wrote:
> Are the .txt tables sufficiently more readable to justify the extra complexity
> of the generator.py script? What was wrong with a simple pair of headers?

Mostly, automatic consistency, currently for the enum constants and string
tables, and later between tables. For Ozone there are some platform-dependent
maps (.key-KeySym and possibly an XKB keycodes file so that XKB could use DOM3
codes directly — although that may no longer work with the sparse USB values)
and some platform-independent (looking up a Windows-compatible VKEY), and I was
hoping to do build-time consistency checking.

There's also the ability to use binary search for string lookups without extra
effort put into the tables.

For readability, the benefit is being able to use exactly the CamelCase names
from the DOM3 documents rather than the Chromium SHOUTY_CASE constants.

[kpschoedel, 2014-10-28 19:44:20]

Dropped the generator script.

[kpschoedel, 2014-10-29 21:48:13]

Ping.

[Wez, 2014-10-31 21:12:52]

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom3...
File ui/events/keycodes/dom3/dom_key.h (right):

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom3...
ui/events/keycodes/dom3/dom_key.h:47: enum class DomKey {
As for the keycode conversion table, let's have a table that actually maps the
enums to the corresponding string names; I'd be fine going back to your previous
model with a text file and a preprocessor script, or a macro-based approach also
seems fine.

Note that we don't care what the numeric values are since they will only be used
within the browser and never exposed through any API, so we don't need to set
them explicitly. When we pass them in to Blink, Blink should call back to
Chromium via the embedder API to resolve codes to strings.

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom4...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom4...
ui/events/keycodes/dom4/keycode_converter_data.h:15: USB_KEYMAP(0x000000,
0x0000, 0x0000, 0xffff, DomCode::NONE),  // Invalid
The point of putting the Code strings in this table is that we then have the
full set of mappings in one place; if you replace the strings with enum values,
which in turn are based upon the USB-like codes, then you're really just
duplicating the USB codes, albeit with a little extra type-safety.

We should keep the enum, USB[1], platform-specific and string names in a single
table, whether that's processed via macros as here, or using a pre-processor
script.

[1] Needed by Chromoting, so we can't get rid of it[2].
[2] Since we have to keep it, I figured we may as well use it as the enum
values, and so save one mapping, plus make debugging a little easier,
potentially.

[kpschoedel, 2014-10-31 21:36:52]

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom3...
File ui/events/keycodes/dom3/dom_key.h (right):

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom3...
ui/events/keycodes/dom3/dom_key.h:47: enum class DomKey {
Sure; I had included explicit values in my original dual code/key table largely
for mnemonic value. The only remaining reason would to keep them fixed across
changes, in case it became useful to use the enums across separately compiled
modules.

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom4...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/641753003/diff/180001/ui/events/keycodes/dom4...
ui/events/keycodes/dom4/keycode_converter_data.h:15: USB_KEYMAP(0x000000,
0x0000, 0x0000, 0xffff, DomCode::NONE),  // Invalid
If using these macros to provide definitions, does a pattern like
  USB_KEYMAP(0x070004, 0x0026, 0x001e, 0x0000, KEY_A, "KeyA"),  // aA
look reasonable to you?

[kpschoedel, 2014-11-03 16:34:58]

Patchset #11 (id:220001) has been deleted

[kpschoedel, 2014-11-03 16:39:50]

Switched to macro-based tables: use keycode_converter_data.h for DomCode, and
added dom_key_data.h in the same style for DomKey.

[alexst (slow to review), 2014-11-20 15:36:38]

alexst@chromium.org changed reviewers:
+ alexst@chromium.org

[alexst (slow to review), 2014-11-20 15:36:39]

Reviewer ping, it was almost 3 weeks since the last update. Please don't neglect
this, it's very important for ChromeOS for M41.

[garykac, 2014-11-20 21:57:56]

Overall lgtm. Thanks for doing this.

https://codereview.chromium.org/641753003/diff/260001/ui/events/keycodes/dom4...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/641753003/diff/260001/ui/events/keycodes/dom4...
ui/events/keycodes/dom4/keycode_converter_data.h:12: //  [1] DOM Level 3
KeyboardEvent code Values,
Not for this cl, but...

The W3C WG just decided that the dom3events name was too confusing (with d3e,
d4e, and uievents floating around).

The decision was to rename the D3E spec to UIEvents and move the current UIEvent
stuff into a separate doc.

Again, nothing to change for this cl. But I wanted to give a heads-up.

[Wez, 2014-11-20 22:06:34]

LGTM ++

[sadrul, 2014-11-21 18:26:05]

rubberstamp lgtm

[kpschoedel, 2014-11-21 18:26:58]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-11-21 18:27:19]

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

[commit-bot: I haz the power, 2014-11-21 18:32:16]

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

[commit-bot: I haz the power, 2014-11-21 18:32:17]

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

[kpschoedel, 2014-11-21 20:30:00]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-11-21 20:30:49]

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

[commit-bot: I haz the power, 2014-11-21 21:25:47]

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

[commit-bot: I haz the power, 2014-11-21 21:25:49]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kpschoedel, 2014-11-21 21:34:01]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-11-21 21:35:11]

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

[commit-bot: I haz the power, 2014-11-21 22:32:14]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2014-11-21 22:32:57]

Patchset 14 (id:??) landed as
https://crrev.com/b0eefc502609ba8060df6c8e71123adeeb9fcf83
Cr-Commit-Position: refs/heads/master@{#305306}