Revise ui::DomKey to unify character and non-character codes.

Revise ui::DomKey to unify character and non-character codes.

DomKey becomes a single integer value corresponding to the DOM
UI Events KeyboardEvent.key string; it can represent either a
Unicode code point or one of the defined non-printable values from
<https://w3c.github.io/DOM-Level-3-Events-key/>;.

In the previous representation, ui::DomKey enumerated only the non-
printable values and had a sentinel to indicated that a character value
was held separately. Much of this CL therefore merely replaces
|key, character| pairs with a single |key| value. The most substantial
changes are to
  ui/events/keycodes/dom/dom_key.h
  ui/events/keycodes/dom/keycode_converter.h
  ui/events/event.h
and associated implementations.

BUG=227231
Committed: https://crrev.com/51d2e327f7a4ff8347867713e098ea81fd601df7
Cr-Commit-Position: refs/heads/master@{#346152}

[kpschoedel, 2015-08-07 20:42:11]

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

[kpschoedel, 2015-08-07 20:42:12]

+Wez for preliminary review of ui::DomKey.

Since many of the files in this CL have only mechanical changes of |key,
character| to |key| alone, I've added CL comments to mark the files with other
changes.

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/input_method_engine.cc:315:
ui::KeycodeConverter::KeyStringToDomKey(event.key.c_str()),
KeyStringToDomKey() now handles the case where |key| contains a Unicode
character rather than a standard name.

https://codereview.chromium.org/1284433002/diff/1/chrome/test/chromedriver/ke...
File chrome/test/chromedriver/keycode_text_conversion_ozone.cc (right):

https://codereview.chromium.org/1284433002/diff/1/chrome/test/chromedriver/ke...
chrome/test/chromedriver/keycode_text_conversion_ozone.cc:49: if
(!dom_key.IsUnicode()) {
The original code here succeeded for DomKey::CHARACTER and also the dual-coded
keys (BackSpace, Tab, Enter Escape, Delete), which matches DomKey::IsUnicode().
It also succeeded on dead keys, which I believe was unintentional and is not
preserved.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode815
ui/events/event.cc:815: base::char16 KeyEvent::GetCharacter() const {
We still need a separate GetCharacter() for cases like Control+A where the |key|
is 'A' and the character is ^A (SOH, 0x01). That will be done in a follow-up CL;
this one is intended to have no user-visible changes, so we still have Control+A
incorrectly generating a |key| of 0x01.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.h#newcode782
ui/events/event.h:782: mutable DomKey key_;
|key_| now subsumes |character_|.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:11: 
Main change here.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key_data.inc (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key_data.inc:23: 
In the previous code, there were some special cases and the function
|CharacterToDomKey()| to make sure that for the dual-coded values both the key
and character would match (e.g. key_=DomKey::BACK_SPACE character_='\b'). It was
possible to explictly construct a KeyEvent for which this was not true, which
would have behaved inconsistently.

The new representation ensures that KeyStringToDomKey("\b") ==
KeyStringToDomKey("Backspace") == DomKey::BACKSPACE == '\b' with no special
effort.

Since these values are in the name table, |DomKeyToKeyString()| returns the
standard names, and at the same time |DomKey::IsUnicode()| is true, which
matches the way the values are used. The cost is segregating them is this file,
so as not to interfere with the auto-enumeration of the other key values.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
File ui/events/keycodes/dom/keycode_converter.cc (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter.cc:201: // static
This now returns a std::string; there are no current users of this function that
are interested in *only* the static case.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter.h:67: 
KeyStringToDomKey() and DomKeyToKeyString() now handle printable characters as
well as standard names.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
File ui/events/keycodes/dom/keycode_converter_unittest.cc (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter_unittest.cc:133: TEST(KeycodeConverter,
DomKey) {
Added cases for Unicode characters.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion.cc:160: return DomKey(key_code -
VKEY_A + 1);
Now DomKey::BACKSPACE == '\b' and so on.

https://codereview.chromium.org/1284433002/diff/1/ui/events/ozone/layout/xkb/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (left):

https://codereview.chromium.org/1284433002/diff/1/ui/events/ozone/layout/xkb/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:755: } else if
(*dom_key == DomKey::DEAD) {
Dead keys now have distinct codes, handled by NonPrintableXKeySymToDomKey().

[kpschoedel, 2015-08-13 14:06:50]

kpschoedel@chromium.org changed reviewers:
+ dtapuska@chromium.org

[kpschoedel, 2015-08-13 14:06:50]

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

[kpschoedel, 2015-08-13 14:06:51]

+dtapuska@ FYI (see previous comments before looking at the code)

[dtapuska, 2015-08-13 14:47:26]

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode820
ui/events/event.cc:820: return key_;
I find this tricky.. In that the code assumes that TYPE_UNICODE is 0 and that it
can safely call operator Base() on it.

Doesn't this generate a narrowing warning?

I'd prefer explicit inline calls.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:36: using Base = int32_t;
Any specific reason we'd use int32_t here? As opposed to uint32_t?

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:86: DomKey(Base value) : value_(value) {}
I do find this sneaky; in that character -> DomKey assignment assumes
TYPE_UNICODE == 0; and that there really is no static assertion anywhere that
indicates that is a requirement.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
File ui/events/keycodes/dom/keycode_converter.cc (left):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter.cc:180: for (size_t i = 0; i <
kDomKeyMapEntries; ++i) {
Can this not use bsearch?

The DomKeyMap is an ordered array is it not?

It really depends on which one is used more frequently; the DomKeyToKeyString or
KeyStringToDomKey... but if the structure was made in an ordered fashion one
could be binary searched.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
File ui/events/keycodes/dom/keycode_converter.cc (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter.cc:212: return "";
I believe its preferred to return std::string(); same for below too..

[kpschoedel, 2015-08-13 15:46:06]

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode820
ui/events/event.cc:820: return key_;
On 2015/08/13 14:47:25, dtapuska wrote:
> I find this tricky.. In that the code assumes that TYPE_UNICODE is 0 and that
it
> can safely call operator Base() on it.

This proposal is for a DomKey that is explicitly defined to be UCS-32 with
non-standard extra code points for the non-printable keys that weren't already
inherited from ASCII; hence the implicit conversions. I'm not opposed to making
DomKey an explicitly discriminated combination of UCS-32 and the other values
instead. Wez, WDYT? 

> Doesn't this generate a narrowing warning?

It doesn't, but should have! (The UCS2 legacy is a whole other barrel of rotting
fish that I'm not dealing with here.)

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:36: using Base = int32_t;
On 2015/08/13 14:47:26, dtapuska wrote:
> Any specific reason we'd use int32_t here? As opposed to uint32_t?

In isolation I'd use uint32_t, but Blink currently uses 'int', and
signed/unsigned interactions can bite; also the style guide frowns somewhat on
unsigned ints. If we had a base::char32 I'd use that type.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:86: DomKey(Base value) : value_(value) {}
On 2015/08/13 14:47:26, dtapuska wrote:
> I do find this sneaky; in that character -> DomKey assignment assumes
> TYPE_UNICODE == 0; and that there really is no static assertion anywhere that
> indicates that is a requirement.

Will add static assertions as necessary for the final representation.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
File ui/events/keycodes/dom/keycode_converter.cc (left):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter.cc:180: for (size_t i = 0; i <
kDomKeyMapEntries; ++i) {
On 2015/08/13 14:47:26, dtapuska wrote:
> Can this not use bsearch?

I've generally been leaving these things alone, as with ui::DomCode (which
originates from USB-scancode tables) and would put such changes in a separate
CL. 

> The DomKeyMap is an ordered array is it not?

Not currently, though it could be made so without much difficulty.

> It really depends on which one is used more frequently; the DomKeyToKeyString
or
> KeyStringToDomKey... but if the structure was made in an ordered fashion one
> could be binary searched.

Both are rare. Outside of tests and errors, I think DomKeyToKeyString would only
be used to generate web events (I don't know, but I'd hope that only happens
when there's something defined to consume them), and KeysStringToDomKey only by
IME extensions. Likewise for ui::DomCode.

dom_key_map[] is *almost* directly indexable, and could be split up to make it
so for the non-Unicode values. The corresponding DomCode map could similarly be
split up by USB page into fairly dense subtables. (My original preference was
for scripts to generate appropriate tables, including the various mappings
elsewhere for other codes like X keysyms and Windows VKEYs.)

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
File ui/events/keycodes/dom/keycode_converter.cc (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/keyc...
ui/events/keycodes/dom/keycode_converter.cc:212: return "";
On 2015/08/13 14:47:26, dtapuska wrote:
> I believe its preferred to return std::string(); same for below too..

Will do.

[kpschoedel, 2015-08-13 19:39:16]

On 2015/08/13 15:46:06, kpschoedel wrote:
> This proposal is for a DomKey that is explicitly defined to be UCS-32 with
> non-standard extra code points for the non-printable keys that weren't already
> inherited from ASCII; hence the implicit conversions.

To elaborate, the argument for this representation: We need symbolic constants
for the non-printable |key| values, and the style guide discourages #defines
(which we can't do with the current dom_key_data.inc definitions anyway). So the
constants are either in an |enum class| or they're integers. If they're in an
|enum class| they have to be a different type than |DomKey| itself, which makes
the code ugly. If they're integers, we can't distinguish them from characters by
type, so they need to be distinguished by value anyway (we could have separate
explicit conversions, e.g. |static DomKey::FromUnicode(c)|, but that could only
be enforced dynamically), and if we don't have implicit conversion for the
constants, the code gets even uglier.

[Wez, 2015-08-13 22:31:49]

Overall this looks like a much cleaner approach than the separated DomKey enum &
charactor code point.

I'm inclined to agree w/ Dave that implicit conversion from DomKey<->Unicode
isn't a good idea, though; I think explicit ToChar and FromChar helpers should
be sufficiently readable and concise.

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/input_method_engine.cc:315:
ui::KeycodeConverter::KeyStringToDomKey(event.key.c_str()),
On 2015/08/07 20:42:11, kpschoedel wrote:
> KeyStringToDomKey() now handles the case where |key| contains a Unicode
> character rather than a standard name.

What does it return if the string passed is neither a single Unicode code-point,
nor a non-printable key name?

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode674
ui/events/event.cc:674: key_() {
No need to initialize |key_| if it takes no ctor params, here and below.

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode695
ui/events/event.cc:695: platform_keycode_(0),
Could any of these fields have their initial values inlined into the definition
in the header now?

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode815
ui/events/event.cc:815: base::char16 KeyEvent::GetCharacter() const {
On 2015/08/07 20:42:12, kpschoedel wrote:
> We still need a separate GetCharacter() for cases like Control+A where the
|key|
> is 'A' and the character is ^A (SOH, 0x01). That will be done in a follow-up
CL;
> this one is intended to have no user-visible changes, so we still have
Control+A
> incorrectly generating a |key| of 0x01.

Acknowledged.

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode820
ui/events/event.cc:820: return key_;
On 2015/08/13 15:46:06, kpschoedel wrote:
> On 2015/08/13 14:47:25, dtapuska wrote:
> > I find this tricky.. In that the code assumes that TYPE_UNICODE is 0 and
that
> it
> > can safely call operator Base() on it.
> 
> This proposal is for a DomKey that is explicitly defined to be UCS-32 with
> non-standard extra code points for the non-printable keys that weren't already
> inherited from ASCII; hence the implicit conversions. I'm not opposed to
making
> DomKey an explicitly discriminated combination of UCS-32 and the other values
> instead. Wez, WDYT?

I'd be fine with something like DomKey::ToChar(key), for the purposes of
readability. That would have the benefit of allowing us to DCHECK() that the
DomKey actually _is_ a Unicode value before we return it.

> > Doesn't this generate a narrowing warning?
> 
> It doesn't, but should have! (The UCS2 legacy is a whole other barrel of
rotting
> fish that I'm not dealing with here.)

Yeah, totally... Does it not complain even on Clang bots...? :(

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:24: // of any keystroke to be carried as a
single integer value.
Why aren't dead-keys encoded using the corresponding Unicode combining
characters?

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:36: using Base = int32_t;
On 2015/08/13 15:46:06, kpschoedel wrote:
> On 2015/08/13 14:47:26, dtapuska wrote:
> > Any specific reason we'd use int32_t here? As opposed to uint32_t?
> 
> In isolation I'd use uint32_t, but Blink currently uses 'int', and
> signed/unsigned interactions can bite; also the style guide frowns somewhat on
> unsigned ints. If we had a base::char32 I'd use that type.

Agreed; the Unicode range only extends to 0x10ffff, so we can fit comfortably in
the 31-bit range and avoid nasty signed/unsigned interactions. We could even
make non-printable DomKey values live in the -ve space if we felt crafty (and
confident that nothing anywhere assumes they're unsigned... ;)

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:55: //    - for a dead key, the code point of
the associated combining character;
See above re using the Unicode combining character code-points.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:69: #define DOM_KEY_MAP_DECLARATION enum Key :
Base
nit: Introduce this macro block with a comment to explain we're pulling in the
non-printable DomKey constants so they'll have names under DomKey?

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:81: // Create a DomKey, with the
undefined-value sentinal DomKey::NONE.
nit: sentinel

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/keyboard...
File ui/events/keycodes/keyboard_code_conversion_xkb.cc (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion_xkb.cc:432: return character;
This means that if we see a Unicode combining-character we won't treat it as a
dead key - not sure how well Xorg currently supports that case anyway, of
course...

[kpschoedel, 2015-08-14 01:20:07]

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:24: // of any keystroke to be carried as a
single integer value.
On 2015/08/13 22:31:48, Wez wrote:
> Why aren't dead-keys encoded using the corresponding Unicode combining
> characters?

The main reason is that they're actually two distinct things. In current
practice a dead key is a non-printable operator key, represented by UI Events
key 'Dead', that combines with a following key to produce a different character;
the combinations are specific and predetermined (on X11 and descendants by
lookup tables, but on the Mac by keyboard layout state transitions) and
arbitrary, not necessarily preserving the identity of the components (e.g. on
ChromeOS, per some long-ago issue, looking up <dead-acute> + <c> yields <ç>
rather than <ć>).

A combining character, on the other hand, is a Unicode character in its own
right, represented in UI Events key by itself (per _key Values_ §1 “support for
the full range of Unicode codepoints”), that decorates a *preceding* character
(in any combination, as shown by ‘Zalgo’ text), with both retaining their
identity — U+0063 LATIN SMALL LETTER C followed by U+0301 COMBINING ACUTE ACCENT
is inherently <ć>. There *are* keyboard layouts that produce actual combining
characters rather than using dead keys (e.g. Bulgarian).

The secondary reason (just to share the misery) is that there exist dead keys
that don't in any sense correspond to a Unicode combining character. The French
Bépo layout has a <dead_currency> key such that (for example) <dead_currency> +
<c> ↦ <¢>, <dead_currency> + <l> ↦ <£>, etc, and a <dead_greek> such that
<dead_greek> + <a> ↦ <α> etc. ChromeOS doesn't currently have these in the dead
key table, but really only because our table derives from an outdated GTK and no
one's ever complained.

[Wez, 2015-08-14 01:23:08]

On 13 August 2015 at 18:20, <kpschoedel@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
> File ui/events/keycodes/dom/dom_key.h (right):
>
>
>
https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
> ui/events/keycodes/dom/dom_key.h:24: // of any keystroke to be carried
> as a single integer value.
> On 2015/08/13 22:31:48, Wez wrote:
>
>> Why aren't dead-keys encoded using the corresponding Unicode combining
>> characters?
>>
>
> The main reason is that they're actually two distinct things. In current
> practice a dead key is a non-printable operator key, represented by UI
> Events key 'Dead', that combines with a following key to produce a
> different character; the combinations are specific and predetermined (on
> X11 and descendants by lookup tables, but on the Mac by keyboard layout
> state transitions) and arbitrary, not necessarily preserving the
> identity of the components (e.g. on ChromeOS, per some long-ago issue,
> looking up <dead-acute> + <c> yields <ç> rather than <ć>).
>
> A combining character, on the other hand, is a Unicode character in its
> own right, represented in UI Events key by itself (per _key Values_ §1
> “support for the full range of Unicode codepoints”), that decorates a
> *preceding* character (in any combination, as shown by ‘Zalgo’ text),
> with both retaining their identity — U+0063 LATIN SMALL LETTER C
> followed by U+0301 COMBINING ACUTE ACCENT is inherently <ć>. There
> *are* keyboard layouts that produce actual combining characters rather
> than using dead keys (e.g. Bulgarian).
>
> The secondary reason (just to share the misery) is that there exist dead
> keys that don't in any sense correspond to a Unicode combining
> character. The French Bépo layout has a <dead_currency> key such that
> (for example) <dead_currency> + <c> ↦ <¢>, <dead_currency> + <l> ↦ <£>,
> etc, and a <dead_greek> such that <dead_greek> + <a> ↦ <α> etc. ChromeOS
> doesn't currently have these in the dead key table, but really only
> because our table derives from an outdated GTK and no one's ever
> complained.
>
> https://codereview.chromium.org/1284433002/


Well now I'm kinda sorry I asked. :P  Apologies for making you write an
essay to explain that, but thanks!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kpschoedel, 2015-08-14 13:52:08]

On 2015/08/14 01:23:08, Wez wrote:
> Well now I'm kinda sorry I asked. :P  Apologies for making you write an
> essay to explain that, but thanks!

Not a problem; it's good to have reasons written down somewhere. I'll add a
two-line version as a comment.

Now I'm curious as to why Unicode chose post-combining rather than
pre-combining, which would have made the common case of dead key handling
identical to Unicode normalization, and also let string processing know when a
composition sequence is complete without reading past it.

[kpschoedel, 2015-08-14 14:36:54]

On 2015/08/14 13:52:08, kpschoedel wrote:
> Now I'm curious as to why Unicode chose post-combining rather than
> pre-combining, which would have made the common case of dead key handling
> identical to Unicode normalization, and also let string processing know when a
> composition sequence is complete without reading past it.

Unicode's rationale (from Unicode 1.0):

“In the Unicode standard, all non-spacing marks are encoded following the base
characters. [....] This convention is different from the convention in ISO 6937,
and the bibliographic standard ISO 5926. The reasons for the old convention were
conformity with ‘dead keys’ on mechanical typewriters (no longer a consideration
for computers), and considerations of efficiency in serial parsing of character
streams which included diacritics.

“The convention used by the Unicode standard is consistent with the logical
order of other non-spacing marks in Semitic and Indic scripts, the great
majority of which follow the base characters with respect to which they are
positioned. To avoid the complication of defining and implementing non-spacing
marks on both sides of base characters, the Unicode standard specifies that all
non-spacing marks must follow their base characters.”

[kpschoedel, 2015-08-14 19:56:29]

On 2015/08/13 22:31:49, Wez wrote:
> I'm inclined to agree w/ Dave that implicit conversion from DomKey<->Unicode
> isn't a good idea, though; I think explicit ToChar and FromChar helpers should
> be sufficiently readable and concise.

This turns out to be a bit of a problem for the various conversion tables, since
constexpr isn't allowed yet. Preprocessor macros would work....

[kpschoedel, 2015-08-14 21:43:54]

… an example is kKeyboardCodeToDomKey[] in
ui/events/keycodes/keyboard_code_conversion.cc, which (currently) has entries
like
    {VKEY_CLEAR, DomKey::CLEAR, 0},
    {VKEY_SPACE, DomKey::CHARACTER, ' '},
which is fine because both parts of the (key, character) tuple are constants.
Changing to    
    {VKEY_CLEAR, DomKey::CLEAR},
    {VKEY_SPACE, DomKey::FromChar(' ')},
makes it a non-constant initializer unless |FromChar()| is constexpr, and we
can't use constexpr until VC2013 is retired.

[kpschoedel, 2015-08-17 16:03:12]

Summary of conversation with dtapuska@ this morning: I'll see whether compilers
are smart enough to do the constant folding without |constexpr|, and investigate
workarounds if not. This does preclude validity checking on |FromChar|, but it
can still be done on extraction.

[kpschoedel, 2015-08-17 18:01:53]

Without |constexpr|, gcc generates dynamic initializers, even when there is
nothing but a simple |return| in the function. A C preprocessor macro
|DOM_KEY_FROM_CHAR(c)| works, but requires declaring the internal representation
|public|. (The Unicode-superset representation doesn't have this problem because
the constants are used as-is.)

[kpschoedel, 2015-08-17 20:16:14]

Patch Set 2 for the non-representational review comments, to get those out of
the way.

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/input_method_engine.cc:315:
ui::KeycodeConverter::KeyStringToDomKey(event.key.c_str()),
On 2015/08/13 22:31:48, Wez wrote:
> What does it return if the string passed is neither a single Unicode
code-point,
> nor a non-printable key name?

DomKey::NONE; this is noted in the declaration comment. In this case the effect
is the same as the previous code. I'll find a specific reviewer for this file
once the general plan is settled.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode674
ui/events/event.cc:674: key_() {
On 2015/08/13 22:31:48, Wez wrote:
> No need to initialize |key_| if it takes no ctor params, here and below.

Done.

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode695
ui/events/event.cc:695: platform_keycode_(0),
On 2015/08/13 22:31:48, Wez wrote:
> Could any of these fields have their initial values inlined into the
definition
> in the header now?

Yes, this hasn't been touched since inline initialization became encouraged.
Besides key_ = DomKey::NONE, we have is_char_ = false and platform_keycode_ = 0
for several constructors.

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode820
ui/events/event.cc:820: return key_;
On 2015/08/13 22:31:48, Wez wrote:
> Yeah, totally... Does it not complain even on Clang bots...? :(

I haven't looked into it, but I assume builds use only ‘-Wall’, which doesn't
include ‘-Wconversion’ (because gcc added it relatively late).

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:69: #define DOM_KEY_MAP_DECLARATION enum Key :
Base
On 2015/08/13 22:31:48, Wez wrote:
> nit: Introduce this macro block with a comment to explain we're pulling in the
> non-printable DomKey constants so they'll have names under DomKey?

Done.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:81: // Create a DomKey, with the
undefined-value sentinal DomKey::NONE.
On 2015/08/13 22:31:48, Wez wrote:
> nit: sentinel

Ouch. I can spell, but my fingers can't.

[Wez, 2015-08-18 22:34:32]

Definitely liking the way this cleans up calling code!

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1284433002/diff/1/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/input_method_engine.cc:315:
ui::KeycodeConverter::KeyStringToDomKey(event.key.c_str()),
On 2015/08/17 20:16:14, kpschoedel wrote:
> On 2015/08/13 22:31:48, Wez wrote:
> > What does it return if the string passed is neither a single Unicode
> code-point,
> > nor a non-printable key name?
> 
> DomKey::NONE; this is noted in the declaration comment. In this case the
effect
> is the same as the previous code. I'll find a specific reviewer for this file
> once the general plan is settled.

Acknowledged.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode695
ui/events/event.cc:695: platform_keycode_(0),
On 2015/08/17 20:16:14, kpschoedel wrote:
> On 2015/08/13 22:31:48, Wez wrote:
> > Could any of these fields have their initial values inlined into the
> definition
> > in the header now?
> 
> Yes, this hasn't been touched since inline initialization became encouraged.
> Besides key_ = DomKey::NONE, we have is_char_ = false and platform_keycode_ =
0
> for several constructors.

Acknowledged.

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/1/ui/events/keycodes/dom/dom_...
ui/events/keycodes/dom/dom_key.h:24: // of any keystroke to be carried as a
single integer value.
On 2015/08/14 01:20:07, kpschoedel wrote:
> On 2015/08/13 22:31:48, Wez wrote:
> > Why aren't dead-keys encoded using the corresponding Unicode combining
> > characters?
> 
> The main reason is that they're actually two distinct things. In current
> practice a dead key is a non-printable operator key, represented by UI Events
> key 'Dead', that combines with a following key to produce a different
character;
> the combinations are specific and predetermined (on X11 and descendants by
> lookup tables, but on the Mac by keyboard layout state transitions) and
> arbitrary, not necessarily preserving the identity of the components (e.g. on
> ChromeOS, per some long-ago issue, looking up <dead-acute> + <c> yields <ç>
> rather than <ć>).
> 
> A combining character, on the other hand, is a Unicode character in its own
> right, represented in UI Events key by itself (per _key Values_ §1 “support
for
> the full range of Unicode codepoints”), that decorates a *preceding* character
> (in any combination, as shown by ‘Zalgo’ text), with both retaining their
> identity — U+0063 LATIN SMALL LETTER C followed by U+0301 COMBINING ACUTE
ACCENT
> is inherently <ć>. There *are* keyboard layouts that produce actual combining
> characters rather than using dead keys (e.g. Bulgarian).

Oh, interesting; I wonder what Chrome does with those keys right now..?

> The secondary reason (just to share the misery) is that there exist dead keys
> that don't in any sense correspond to a Unicode combining character. The
French
> Bépo layout has a <dead_currency> key such that (for example) <dead_currency>
+
> <c> ↦ <¢>, <dead_currency> + <l> ↦ <£>, etc, and a <dead_greek> such that
> <dead_greek> + <a> ↦ <α> etc. ChromeOS doesn't currently have these in the
dead
> key table, but really only because our table derives from an outdated GTK and
no
> one's ever complained.

I was thinking of suggesting we ignore the Unicode post-combination and (ab)use
the combining code points as pre-combining dead-keys, but it sounds like there's
enough differences to warrant the separate scheme. Sad. :(

[kpschoedel, 2015-08-19 19:01:44]

On 2015/08/18 22:34:32, Wez wrote:
> Oh, interesting; I wonder what Chrome does with those keys right now..?

They work correctly, as far as I can tell. I think that's an inevitable
consequence of handling general Unicode text (which may have been pasted or
pre-filled rather than typed) in input fields.

[Wez, 2015-08-19 20:24:59]

On 2015/08/19 19:01:44, kpschoedel wrote:
> On 2015/08/18 22:34:32, Wez wrote:
> > Oh, interesting; I wonder what Chrome does with those keys right now..?
> 
> They work correctly, as far as I can tell. I think that's an inevitable
> consequence of handling general Unicode text (which may have been pasted or
> pre-filled rather than typed) in input fields.

Thanks, that makes sense.

Are you close to being review-ready with this CL?

[kpschoedel, 2015-08-19 21:04:33]

On 2015/08/19 20:24:59, Wez wrote:
> Are you close to being review-ready with this CL?

Changed representation of DomKey. Still need another pass for the initializers.

[kpschoedel, 2015-08-20 15:10:23]

This is substantially reviewable now.

(I'll change the CHECKs after a CQ dry run.)

[kpschoedel, 2015-08-20 15:10:33]

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

[commit-bot: I haz the power, 2015-08-20 15:10:58]

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

[commit-bot: I haz the power, 2015-08-20 16:58:56]

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

[commit-bot: I haz the power, 2015-08-20 16:58:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kpschoedel, 2015-08-21 20:11:12]

After a day of head-scratching it turns out the win64 failures were an unrelated
coincidence (r344240 - r344778).

[kpschoedel, 2015-08-24 16:07:54]

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

[commit-bot: I haz the power, 2015-08-24 16:08:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1284433002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1284433002/100001

[commit-bot: I haz the power, 2015-08-24 16:52:59]

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

[commit-bot: I haz the power, 2015-08-24 16:53:00]

Dry run: This issue passed the CQ dry run.

[kpschoedel, 2015-08-24 21:05:12]

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

[kpschoedel, 2015-08-24 21:05:12]

kpschoedel@chromium.org changed required reviewers:
+ sadrul@chromium.org, yukishiino@chromium.org

[kpschoedel, 2015-08-24 21:05:13]

Widening the audience…
+sadrul@ for ui/events/event.* etc.
+yukishiino@ for character_composer / IME
+spang@ for ozone

[Yuki, 2015-08-25 05:32:34]

+cc: shuchen@ FYI

lgtm for ui/base/ime/chromeos/

[spang, 2015-08-25 15:53:31]

lgtm

[Wez, 2015-08-26 23:38:34]

LGTM :D

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.cc#newcode820
ui/events/event.cc:820: return key_;
On 2015/08/17 20:16:14, kpschoedel wrote:
> On 2015/08/13 22:31:48, Wez wrote:
> > Yeah, totally... Does it not complain even on Clang bots...? :(
> 
> I haven't looked into it, but I assume builds use only ‘-Wall’, which doesn't
> include ‘-Wconversion’ (because gcc added it relatively late).

Acknowledged.

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

https://codereview.chromium.org/1284433002/diff/1/ui/events/event.h#newcode782
ui/events/event.h:782: mutable DomKey key_;
On 2015/08/07 20:42:12, kpschoedel wrote:
> |key_| now subsumes |character_|.

*celebratory dance*

https://codereview.chromium.org/1284433002/diff/100001/ui/base/ime/chromeos/c...
File ui/base/ime/chromeos/character_composer.cc (right):

https://codereview.chromium.org/1284433002/diff/100001/ui/base/ime/chromeos/c...
ui/base/ime/chromeos/character_composer.cc:231: Find(tree_index, entries,
static_cast<uint16_t>(character), &result)) {
It's worrying to be pulling a 32-bit Unicode code-point and coercing it here;
does the ComposeChecker need fixing to handle 32-bit, or is there some reason it
can assume 16-bit? e.g. because ECMAScript dictates UTF-16? (raising the
question of how things are supposed to behave if a user manages to type a
GB13080 character... :P

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
ui/events/keycodes/dom/dom_key.h:29: // key 'Unidentified'.
nit: It's confusing to mention "undefined value" above, versus "Unidentified"
here - is Unidentified specific to keys we can't identify, where otherwise DOM
key works for other keys..?

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
ui/events/keycodes/dom/dom_key.h:75: "suspicious representation change");
:D

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
ui/events/keycodes/dom/dom_key.h:114: bool IsDead() const { return (value_ &
TYPE_MASK) == TYPE_DEAD; }
nit: Name this and ToDeadKeyCharacter, below, consistently.

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
ui/events/keycodes/dom/dom_key.h:137: static DomKey Dead(int32_t
combining_character) {
nit: Name this consistently w/ FromCharacter and IsDead, ToDeadKeyCharacter,
e.g. FromDeadKeyCharacter?

[kpschoedel, 2015-08-27 15:47:54]

https://codereview.chromium.org/1284433002/diff/100001/ui/base/ime/chromeos/c...
File ui/base/ime/chromeos/character_composer.cc (right):

https://codereview.chromium.org/1284433002/diff/100001/ui/base/ime/chromeos/c...
ui/base/ime/chromeos/character_composer.cc:231: Find(tree_index, entries,
static_cast<uint16_t>(character), &result)) {
The short answer is that there are no existing compositions involving non-BMP
characters (and the table generator checks that), so they're rejected above
(line 224). I considered adding non-BMP support here when I converted this from
X11 to DomKey, but unused code is frowned upon. Maybe I can find a use case —
the X11 tables that were used in ChromeOS were pretty old — and add it.

The long answer is probably very very long. |ui::KeyEvent| has historically held
a |base::char16|, and similarly |KeyEvent::GetCharacter()| returns a
|base::char16|. X11 ships one key layout with non-BMP characters (Avestan) and
it works on Linux Chrome, but I don't currently understand *how*.

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
File ui/events/keycodes/dom/dom_key.h (right):

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
ui/events/keycodes/dom/dom_key.h:29: // key 'Unidentified'.
I'm not sure how to write this. DomKey::UNIDENTIFIED is 'Unidentified'
https://w3c.github.io/DOM-Level-3-Events-key/#keys-special and as far as code
using DomKey is concerned it's just another standard-defined |key|.


DomKey::NONE should not ever be exposed to the web. Currently, in ui::KeyEvent,
|key_ == DomKey::NONE| indicates that keyboard layout hasn't been done yet. If
layout fails (e.g. if |code| is not mapped), then key_ is set to
DomKey::UNDEFINED.

https://codereview.chromium.org/1284433002/diff/100001/ui/events/keycodes/dom...
ui/events/keycodes/dom/dom_key.h:137: static DomKey Dead(int32_t
combining_character) {
For dead keys, the combining character is an added attribute (not present in the
web KeyboardEvent) rather than its identity (as for a Unicode key). How about
IsDeadKey(), ToDeadKeyCombiningCharacter(), DeadKeyFromCombiningCharacter()?

[sadrul, 2015-08-27 19:20:43]

lgtm

[kpschoedel, 2015-08-27 19:28:45]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-08-27 19:29:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1284433002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1284433002/100001

[commit-bot: I haz the power, 2015-08-27 19:41:14]

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

[commit-bot: I haz the power, 2015-08-27 19:41:16]

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-08-27 19:46:05]

kpschoedel@chromium.org changed reviewers:
+ samuong@chromium.org

[kpschoedel, 2015-08-27 19:46:05]

kpschoedel@chromium.org changed required reviewers:
+ samuong@chromium.org

[kpschoedel, 2015-08-27 19:46:06]

+samuong@ for //chrome/test/chromedriver/*

[kpschoedel, 2015-08-27 22:13:10]

https://codereview.chromium.org/1284433002/diff/100001/ui/base/ime/chromeos/c...
File ui/base/ime/chromeos/character_composer.cc (right):

https://codereview.chromium.org/1284433002/diff/100001/ui/base/ime/chromeos/c...
ui/base/ime/chromeos/character_composer.cc:231: Find(tree_index, entries,
static_cast<uint16_t>(character), &result)) {
Followups:

There are no current non-BMP candidates for character composition. The closest
is that current X11 has some Compose key sequences involving music notation, but
(a) there's no layout with the components of those sequences, and (b) the
Compose key is not commonly used on ChromeOS.

The Avestan keyboard layout, with non-BMP characters, currently works on Linux
because IME takes over the keystrokes; the result is a textInput event rather
than a keypress event. (This is not a supported ChromeOS layout, and unlikely to
become so, as according to Wikipedia it's not a living language.)

[samuong, 2015-08-27 22:17:34]

lgtm

[kpschoedel, 2015-08-28 13:57:10]

The CQ bit was checked by kpschoedel@chromium.org

[kpschoedel, 2015-08-28 13:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
wez@chromium.org, yukishiino@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1284433002/#ps120001
(title: "IsDead")

[commit-bot: I haz the power, 2015-08-28 13:57:25]

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

[commit-bot: I haz the power, 2015-08-28 15:29:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-28 15:30:29]

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