Set ui::KeyEvent::key_ (DOM Level 3 key) under X11.

Set ui::KeyEvent::key_ (DOM Level 3 key) under X11.

Shares more code between uses of X11 xkb and (Ozone) libxkbcommon.

BUG=444048
Committed: https://crrev.com/99c75c490c5caaa0e5fa59f06d1b823104ee52dd
Cr-Commit-Position: refs/heads/master@{#328967}

[kpschoedel, 2015-04-27 21:50:38]

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

[commit-bot: I haz the power, 2015-04-27 21:52:23]

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

[commit-bot: I haz the power, 2015-04-27 22:44:40]

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

[commit-bot: I haz the power, 2015-04-27 22:44:41]

Dry run: This issue passed the CQ dry run.

[kpschoedel, 2015-05-06 20:34:23]

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

[Wez, 2015-05-06 23:51:53]

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion.cc:438: case 0x0D:
Do we really want to map both of these to Enter? That means we're losing
information on the exact input event? :-S

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion.h:76: EVENTS_BASE_EXPORT DomKey
CharacterToDomKey(base::char16 character);
nit: Using char16 here means we're limited to UCS2 (aka Basic Multilingual
Plane) code points - I doubt that there are any DomKey enum values that
correspond to GB13080 characters, but _technically_, this should be a 32-bit
parameter, to allow the full Unicode range to be expressed.

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion_x.cc:917: *key =
XkbKeySymToDomKey(keysym, *ch);
nit: Shouldn't this be called XKeySymToDomKey()? Xkb...() makes it sound like
it's specific to some sort of XKB extension events, where these values are a
core part of X11.

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion_xkb.h:28: DomKey
XkbKeySymToDomKey(xkb_keysym_t keysym, base::char16 character);
Consider clarifying that these functions must never be called, directly or
indirectly, from within any sandboxed process; they should only ever be used by
the Browser process, since they need to call out to X11.

You've called this XkbKeySymToDomKey; AFAIK there's no difference between
xkb_keysym_t and stock X11 KeySyms, so the fact that this API uses XKB APIs
internally is really just an implementation detail - in which case I'd suggest
renaming to just XKeySymToDomKey() - WDYT?

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
File ui/events/keycodes/xkb_keysym.h (right):

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
ui/events/keycodes/xkb_keysym.h:9: // and values (XKB_KEY_...) for both
xkbcommon and traditional X11.
This table just seems to re-map the X11 and Sun KeySyms into a different
namespace - do we really need it? Can't we just use X11 KeySym values
consistently throughout the conversion code? I don't even see anywhere in the
codebase where the Sun values get used, for example.

[kpschoedel, 2015-05-07 16:11:31]

Patchset #2 (id:20001) has been deleted

[kpschoedel, 2015-05-07 16:21:33]

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion.cc:438: case 0x0D:
On 2015/05/06 23:51:53, Wez wrote:
> Do we really want to map both of these to Enter? That means we're losing
> information on the exact input event? :-S

Done.

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion.h:76: EVENTS_BASE_EXPORT DomKey
CharacterToDomKey(base::char16 character);
On 2015/05/06 23:51:53, Wez wrote:
> nit: Using char16 here means we're limited to UCS2 (aka Basic Multilingual
> Plane) code points - I doubt that there are any DomKey enum values that
> correspond to GB13080 characters, but _technically_, this should be a 32-bit
> parameter, to allow the full Unicode range to be expressed.

Done.

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion_x.cc:917: *key =
XkbKeySymToDomKey(keysym, *ch);
On 2015/05/06 23:51:53, Wez wrote:
> nit: Shouldn't this be called XKeySymToDomKey()? Xkb...() makes it sound like
> it's specific to some sort of XKB extension events, where these values are a
> core part of X11.

Done.

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

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/keyboard...
ui/events/keycodes/keyboard_code_conversion_xkb.h:28: DomKey
XkbKeySymToDomKey(xkb_keysym_t keysym, base::char16 character);
On 2015/05/06 23:51:53, Wez wrote:
> Consider clarifying that these functions must never be called, directly or
> indirectly, from within any sandboxed process; they should only ever be used
by
> the Browser process, since they need to call out to X11.

These don't — they're just direct maps.

> You've called this XkbKeySymToDomKey; AFAIK there's no difference between
> xkb_keysym_t and stock X11 KeySyms, so the fact that this API uses XKB APIs
> internally is really just an implementation detail - in which case I'd suggest
> renaming to just XKeySymToDomKey() - WDYT?

Done.

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
File ui/events/keycodes/xkb_keysym.h (right):

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
ui/events/keycodes/xkb_keysym.h:9: // and values (XKB_KEY_...) for both
xkbcommon and traditional X11.
On 2015/05/06 23:51:53, Wez wrote:
> This table just seems to re-map the X11 and Sun KeySyms into a different
> namespace - do we really need it? Can't we just use X11 KeySym values
> consistently throughout the conversion code? I don't even see anywhere in the
> codebase where the Sun values get used, for example.

We don't have the X11 headers when using xkbcommon (Ozone/Freon) and vice versa.
Unfortunately xkbcommon chose to use different names for the keysym constants
(and although the numeric values for corresponding keysyms appear to be the
same, so technically we could build with the ‘wrong’ headers, I'd rather not
rely on that). This is ugly but I don't see a better solution for sharing code
between consumers of X11 XKB and library xkbcommon. (This is a scripted list
from the xkbcommon headers, and no doubt it contains symbols that aren't
actually used anywhere in our code.)

[Wez, 2015-05-07 23:13:31]

LGTM w/ nits

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
File ui/events/keycodes/xkb_keysym.h (right):

https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
ui/events/keycodes/xkb_keysym.h:9: // and values (XKB_KEY_...) for both
xkbcommon and traditional X11.
On 2015/05/07 16:21:33, kpschoedel wrote:
> On 2015/05/06 23:51:53, Wez wrote:
> > This table just seems to re-map the X11 and Sun KeySyms into a different
> > namespace - do we really need it? Can't we just use X11 KeySym values
> > consistently throughout the conversion code? I don't even see anywhere in
the
> > codebase where the Sun values get used, for example.
> 
> We don't have the X11 headers when using xkbcommon (Ozone/Freon) and vice
versa.
> Unfortunately xkbcommon chose to use different names for the keysym constants
> (and although the numeric values for corresponding keysyms appear to be the
> same, so technically we could build with the ‘wrong’ headers, I'd rather not
> rely on that). This is ugly but I don't see a better solution for sharing code
> between consumers of X11 XKB and library xkbcommon. (This is a scripted list
> from the xkbcommon headers, and no doubt it contains symbols that aren't
> actually used anywhere in our code.)

I'm not familiar with xkbcommon, so this may be a dumb suggestion, but...

We're already "mixing" these XKB_... constants with X11 since we get XEvents,
containing X KeySym values, and we then compare them to these macros. We're
intrinsically assuming that the two sets are equivalent.

So... instead of this X11->XKB table, could we instead provide an XKB->X11 table
to use w/ xkbcommon, so that all the dependent code can consistently use the X11
versions internally?

Why do we even use KeySyms at all in the Ozone/Freon case - we don't run X at
all with those, do we?

https://codereview.chromium.org/1103263004/diff/50001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion_xkb.h (right):

https://codereview.chromium.org/1103263004/diff/50001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_xkb.h:28: DomKey
XKeySymToDomKey(xkb_keysym_t keysym, base::char16 character);
nit: Should this character also be uint32, and similarly for the return value of
the DeadXKeySym...() method above?

[spang, 2015-05-08 00:56:33]

On 2015/05/07 23:13:31, Wez wrote:
> LGTM w/ nits
> 
>
https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
> File ui/events/keycodes/xkb_keysym.h (right):
> 
>
https://codereview.chromium.org/1103263004/diff/1/ui/events/keycodes/xkb_keys...
> ui/events/keycodes/xkb_keysym.h:9: // and values (XKB_KEY_...) for both
> xkbcommon and traditional X11.
> On 2015/05/07 16:21:33, kpschoedel wrote:
> > On 2015/05/06 23:51:53, Wez wrote:
> > > This table just seems to re-map the X11 and Sun KeySyms into a different
> > > namespace - do we really need it? Can't we just use X11 KeySym values
> > > consistently throughout the conversion code? I don't even see anywhere in
> the
> > > codebase where the Sun values get used, for example.
> > 
> > We don't have the X11 headers when using xkbcommon (Ozone/Freon) and vice
> versa.
> > Unfortunately xkbcommon chose to use different names for the keysym
constants
> > (and although the numeric values for corresponding keysyms appear to be the
> > same, so technically we could build with the ‘wrong’ headers, I'd rather not
> > rely on that). This is ugly but I don't see a better solution for sharing
code
> > between consumers of X11 XKB and library xkbcommon. (This is a scripted list
> > from the xkbcommon headers, and no doubt it contains symbols that aren't
> > actually used anywhere in our code.)
> 
> I'm not familiar with xkbcommon, so this may be a dumb suggestion, but...
> 
> We're already "mixing" these XKB_... constants with X11 since we get XEvents,
> containing X KeySym values, and we then compare them to these macros. We're
> intrinsically assuming that the two sets are equivalent.
> 

Actually, the freon build compares to xkbcommon constants and the X11 build
compares to regular XKB constants once you've run the preprocessor. There's no
assumption of equivalence.

However, I've double checked and they are definitely equivalent. We can use
either set, but that brings up a different issue.. 

> So... instead of this X11->XKB table, could we instead provide an XKB->X11
table
> to use w/ xkbcommon, so that all the dependent code can consistently use the
X11
> versions internally?

The desktop build requires XKB. We do not add xkbcommon to the include path, or
assume it is available.

The CrOS build requires xkbcommon. We do not add xkb to the include path, or
assume it is even available.

That's the main issue IMO. We could add a dependency to either of these builds
to fix it, and just choose one of the headers to use. Or use the preprocessor to
sort this out as we have been doing.

If we add a dependency, I'd be xkbcommon to the desktop build. We will not have
xkb at all in the CrOS build environment very soon.

> 
> Why do we even use KeySyms at all in the Ozone/Freon case - we don't run X at
> all with those, do we?

We use keymaps designed for X. As does basically everyone else, including other
X11-replacement projects (wayland, mir). So we get X's symbols.

> 
>
https://codereview.chromium.org/1103263004/diff/50001/ui/events/keycodes/keyb...
> File ui/events/keycodes/keyboard_code_conversion_xkb.h (right):
> 
>
https://codereview.chromium.org/1103263004/diff/50001/ui/events/keycodes/keyb...
> ui/events/keycodes/keyboard_code_conversion_xkb.h:28: DomKey
> XKeySymToDomKey(xkb_keysym_t keysym, base::char16 character);
> nit: Should this character also be uint32, and similarly for the return value
of
> the DeadXKeySym...() method above?

[kpschoedel, 2015-05-08 01:24:06]

On 2015/05/07 23:13:31, Wez wrote:
> I'm not familiar with xkbcommon, so this may be a dumb suggestion, but...
> 
> We're already "mixing" these XKB_... constants with X11 since we get XEvents,
> containing X KeySym values, and we then compare them to these macros. We're
> intrinsically assuming that the two sets are equivalent.
> 
> So... instead of this X11->XKB table, could we instead provide an XKB->X11
table
> to use w/ xkbcommon, so that all the dependent code can consistently use the
X11
> versions internally?
> 
> Why do we even use KeySyms at all in the Ozone/Freon case - we don't run X at
> all with those, do we?

Ah, OK, I think I see what context I need to fill in. xkbcommon is essentially
the XKB extension from X11, ripped out and made into a standalone library. (This
was done by Wayland folks, IIRC.) There are a few API differences, mostly around
holding xkb context because there's no display server to do it, but they are
substantially the same, and they read the same configuration/layout files. But
xkbcommon, being a self-contained library, isn't guaranteed to have the X11
headers available, and so it has its own derivative of the <X11/*keysym*>
headers, <xkbcommon/xkbcommon.h>. X11 KeySym and xkb_keysym_t are functionally
equivalent; XK_... symbols and XKB_KEY_... symbols are functionally equivalent.
So what I'm trying to do here, to start, is allow code to be written with one
set of names, but build for either target; the end goal is to share more code
between desktop X11 and ChromeOS. 

On Ozone, we have, in principle, a platform-agnostic layout function that maps
(DomCode, EventFlags) to (DomKey, character, VKEY). For ChromeOS Ozone, we use
xkbcommon to provide that function, for various pragmatic reasons, including
keeping all the existing (ChromeOS-patched) layout files. In practice, we're not
quite there yet; for instance, dead-key/compose combinations still use X keysyms
(issue 442757), which is the one thing that does rely on X11 and xkbcommon
keysyms actually being identical, and also the last thing that still requires
ui::KeyEvents to carry X11 details on desktop X11.

[kpschoedel, 2015-05-08 01:36:53]

On 2015/05/08 00:56:33, spang wrote:
> Actually, the freon build compares to xkbcommon constants and the X11 build
> compares to regular XKB constants once you've run the preprocessor. There's no
> assumption of equivalence.

Actually, there is *one* lurking, but it's going away right after I finish
handling Dom codes in EventRewriter.

> However, I've double checked and they are definitely equivalent.

As have I — otherwise 442757 would have been a freon release blocker. (I have
oodles of cross-references and tables and mapping scripts.)

> The desktop build requires XKB.

That is, X11 with the XKB extension.

> We do not add xkbcommon to the include path, or
> assume it is available.
> 
> The CrOS
[freon]
> build requires xkbcommon. We do not add xkb
[X11]
> to the include path, or assume it is even available.

[sadrul, 2015-05-08 05:50:25]

rs lgtm

[kpschoedel, 2015-05-08 14:14:21]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-05-08 14:14:40]

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

[commit-bot: I haz the power, 2015-05-08 16:38:47]

Committed patchset #3 (id:50001)

[commit-bot: I haz the power, 2015-05-08 16:40:40]

Patchset 3 (id:??) landed as
https://crrev.com/99c75c490c5caaa0e5fa59f06d1b823104ee52dd
Cr-Commit-Position: refs/heads/master@{#328967}