Ozone keyboard layout

Ozone keyboard layout

This replaces core X11 XKB on Ozone, providing a platform-independent
interface mapping (DomCode, EventFlags) to (DomKey, character)
and KeyboardCode, and a layout set/property interface equivalent
to ImeKeyboard.

BUG=430194
Committed: https://crrev.com/763ff21d56e31d5b18a43c635f233de89a2ad9f0
Cr-Commit-Position: refs/heads/master@{#308390}

[kpschoedel, 2014-11-20 16:12:00]

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

[kpschoedel, 2014-11-20 16:18:38]

This is not landable yet (dependencies notwithstanding), as more gyp/gn files
need to change, plus some unit tests. See description and inline comments.

shuchen@ fengyuan@ FYI — I am assuming issue 362698 will provide code for the
libxkbcommon version of KeyboardLayoutsOzone::SetCurrentLayoutByName().

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

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode34
ui/events/event.cc:34: #include "ui/ozone/public/ozone_platform.h"
Will need +ui/ozone dependency.

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode680
ui/events/event.cc:680: #endif
Should the Ozone event source instead be responsible for providing a correct
key_code_ in all cases?

We would probably then want another constructor (which would also be used for
hypothetical platforms that don't do layout, e.g. caca and possibly future
embedded Brillage that uses key events for control buttons but not actual
keyboards), like:

 KeyEvent(EventType type,
           KeyboardCode key_code,
           DomCode dom_code,
           EventFlags flags,
           DomKey dom_key,
           base::char16 character)

In the general case determining the Windows VKEY requires calling the layout
function, possibly several times — see keyboard_code_conversion_x.cc for gory
details.

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode729
ui/events/event.cc:729: // positional coding.
I have a refactoring of GetCharacter() in the upcoming DomKey CL. Since
character and DomKey depend on layout, I plan a common layout function called
from GetDomKey(), GetCharacter() and variants. Today GetCharacter() is a lazy
evaluation of KeyEvent::character_, and I plan the same for KeyEvent::dom_key_,
as some uses of key events only use physical keys and never actually require
layout.

https://codereview.chromium.org/742103002/diff/1/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/742103002/diff/1/ui/events/events.gyp#newcode347
ui/events/events.gyp:347: '../ozone/ozone.gyp:ozone',
A consequence of having the keyboard layout manager owned by OzonePlatform is
that |events| depends on |ozone| (and a bunch more unit tests will need to
initialize it).

https://codereview.chromium.org/742103002/diff/1/ui/events/ozone/events_ozone...
File ui/events/ozone/events_ozone.gyp (right):

https://codereview.chromium.org/742103002/diff/1/ui/events/ozone/events_ozone...
ui/events/ozone/events_ozone.gyp:115: 'layout/stub/stub_keyboard_layouts.h',
I've tentatively put keyboard layouts under
  ui/events/ozone/layout/<type>/
e.g.
  ui/events/ozone/layout/stub/  -- hardcoded US layout
  ui/events/ozone/layout/xkb/   -- libxkbcommon or host X11
  ui/events/ozone/layout/none/  -- for layoutless platforms

https://codereview.chromium.org/742103002/diff/1/ui/events/ozone/layout/stub/...
File ui/events/ozone/layout/stub/stub_keyboard_layouts.cc (right):

https://codereview.chromium.org/742103002/diff/1/ui/events/ozone/layout/stub/...
ui/events/ozone/layout/stub/stub_keyboard_layouts.cc:229: // TODO(kpschoedel):
move this to a shared location.
The next maps are here temporarily to due to CL interdepencence; they would be
used by any Ozone or future DOM-based layouts.

https://codereview.chromium.org/742103002/diff/1/ui/ozone/public/keyboard_lay...
File ui/ozone/public/keyboard_layouts.h (right):

https://codereview.chromium.org/742103002/diff/1/ui/ozone/public/keyboard_lay...
ui/ozone/public/keyboard_layouts.h:17: class OZONE_EXPORT KeyboardLayoutsOzone {
https://docs.google.com/document/d/1AjtS4zAA4KW7yEEShgjYRppck8ZFUpb7PMcz0fh7I...

I hate this name.

[sadrul, 2014-11-21 21:25:25]

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

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode34
ui/events/event.cc:34: #include "ui/ozone/public/ozone_platform.h"
On 2014/11/20 16:18:38, kpschoedel wrote:
> Will need +ui/ozone dependency.

//ui/ozone has deps on //ui/events/, so we can't currently do this.

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode680
ui/events/event.cc:680: #endif
On 2014/11/20 16:18:38, kpschoedel wrote:
> Should the Ozone event source instead be responsible for providing a correct
> key_code_ in all cases?

I think so, yeah.

> 
> We would probably then want another constructor (which would also be used for
> hypothetical platforms that don't do layout, e.g. caca and possibly future
> embedded Brillage that uses key events for control buttons but not actual
> keyboards), like:
> 
>  KeyEvent(EventType type,
>            KeyboardCode key_code,
>            DomCode dom_code,
>            EventFlags flags,
>            DomKey dom_key,
>            base::char16 character)
> 
> In the general case determining the Windows VKEY requires calling the layout
> function, possibly several times — see keyboard_code_conversion_x.cc for gory
> details.

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

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

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode34
ui/events/event.cc:34: #include "ui/ozone/public/ozone_platform.h"
On 2014/11/21 21:25:25, sadrul wrote:
> //ui/ozone has deps on //ui/events/, so we can't currently do this.

Argh. OK, this needs a rethink. Layout is fortunately orthogonal to the rest of
the Ozone platform, so I susppose it doesn't have to be there.

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode680
ui/events/event.cc:680: #endif
On 2014/11/21 21:25:25, sadrul wrote:
> On 2014/11/20 16:18:38, kpschoedel wrote:
> > Should the Ozone event source instead be responsible for providing a correct
> > key_code_ in all cases?
> 
> I think so, yeah.

In that case, I think we'll want the extended constructor since correctly
determining the Windows key_code in the general case requires calling into the
key layout, possibly multiple times (see keyboard_code_conversion_x.cc), so the
caller will typically have the key/character as well:

> >  KeyEvent(EventType type,
> >            KeyboardCode key_code,
> >            DomCode dom_code,
> >            EventFlags flags,
> >            DomKey dom_key,
> >            base::char16 character)

[spang, 2014-11-21 21:58:52]

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

https://codereview.chromium.org/742103002/diff/1/ui/events/event.cc#newcode34
ui/events/event.cc:34: #include "ui/ozone/public/ozone_platform.h"
On 2014/11/21 21:43:33, kpschoedel wrote:
> On 2014/11/21 21:25:25, sadrul wrote:
> > //ui/ozone has deps on //ui/events/, so we can't currently do this.
> 
> Argh. OK, this needs a rethink. Layout is fortunately orthogonal to the rest
of
> the Ozone platform, so I susppose it doesn't have to be there.

Can we really say it is orthogonal?

I think we should follow a model where ui::KeyEvent is just data. Thta way,
there wouldn't be anything in the key event class that uses the layout.

If we put the smarts into KeyEvent, a dependency cycle is basically unavoidable.

[kpschoedel, 2014-11-21 22:47:23]

On 2014/11/21 21:58:52, spang wrote:
> Can we really say it is orthogonal?

In the sense that the same layout system may be used for different event sources
(and obviously different graphics systems).

KeyEvent has historically got the character interpretation lazily
(KeyEvent::GetCharacter()) and although I don't know the history of this there
is some justification for it as some uses only care about the Windows code.

To make it 'just data' we'd use the extended constructor as above. I'm not
opposed to that.

[spang, 2014-11-21 22:53:49]

On 2014/11/21 22:47:23, kpschoedel wrote:
> On 2014/11/21 21:58:52, spang wrote:
> > Can we really say it is orthogonal?
> 
> In the sense that the same layout system may be used for different event
sources
> (and obviously different graphics systems).
> 
> KeyEvent has historically got the character interpretation lazily
> (KeyEvent::GetCharacter()) and although I don't know the history of this there
> is some justification for it as some uses only care about the Windows code.
> 
> To make it 'just data' we'd use the extended constructor as above. I'm not
> opposed to that.

I think places we want layout to happen can be very obvious. As a oversimplified
example something like

  scoped_ptr<ui::KeyEvent> key_event =
layout_engine->BuildMyInterpretedKey(DomCode::KEY_K);

would be very readable. Or

  ui::KeyEvent rewrite_me;
  layout_engine->RelayoutKey(DomCode::KEY_K, &rewrite_me);

layout engine impl would not come from ui/events, so we can avoid the cycle.

[kpschoedel, 2014-11-21 23:03:00]

On 2014/11/21 22:53:49, spang wrote:
> I think places we want layout to happen can be very obvious. As a
oversimplified
> example something like
> 
>   scoped_ptr<ui::KeyEvent> key_event =
> layout_engine->BuildMyInterpretedKey(DomCode::KEY_K);

Yeah, that's not a bad way to look at it. Though I'd want state maintenance to
be a separate issue, i.e.
  layout_engine->BuildMyInterpretedKey(DomCode::KEY_K, EF_SHIFT_DOWN);

(A stateless layout engine is a practical require for accurately determining
Windows VKEY, unfortunately.)

Otherwise perfectly reasonable. I had been thinking in terms of
   layout_engine->GetAllTheStuffForTheContstructor(DomCode::KEY_K,
EF_SHIFT_DOWN, &larry, &curly, &moe);
   KeyEvent(DomCode::KEY_K, EF_SHIFT_DOWN, larry, curly, moe);
but your suggestion is kinder to callers.

> would be very readable. Or
> 
>   ui::KeyEvent rewrite_me;
>   layout_engine->RelayoutKey(DomCode::KEY_K, &rewrite_me);

I think we can get away without that. EventRewriter at least builds new events.

[kpschoedel, 2014-11-26 20:32:25]

Rearranged in light of the comments above.

This remains a non-committable prototype, as some tests fail.

[kpschoedel, 2014-11-27 23:27:21]

On 2014/11/21 21:25:25, sadrul wrote:
> On 2014/11/20 16:18:38, kpschoedel wrote:
> > Will need +ui/ozone dependency.
> 
> //ui/ozone has deps on //ui/events/, so we can't currently do this.

This is looking like it could be a problem — not with events coming from Ozone,
but all the other code that constructs |KeyEvent|s assuming they are constructed
without a character interpretation and that layout will be called from the
constructor or from GetCharacter() later. It's all structured with KeyEvent
depending on the layout engine.

To stop KeyEvent depending on the layout engine would require removing the two
constructors
  KeyEvent(EventType type, KeyboardCode key_code, int flags);
  KeyEvent(EventType type, KeyboardCode key_code, DomCode code, int flags);
and supplying a layout utility interface (which would depend on Ozone or X or
... as the case may be). There are about 200 calls to these constructors, but
only a few that clearly need to be made dependent on layout, the most
significant being IME and Mojo (which is a nuisance since it's out of tree). 

An alternative could be to move the layout engine out of OzonePlatform into a
separate component that both Events and Ozone depend on. This would essentially
be a NotTheOzonePlatform singleton to give access to the current engine.

[spang, 2014-11-28 19:35:28]

On 2014/11/27 23:27:21, kpschoedel wrote:
> On 2014/11/21 21:25:25, sadrul wrote:
> > On 2014/11/20 16:18:38, kpschoedel wrote:
> > > Will need +ui/ozone dependency.
> > 
> > //ui/ozone has deps on //ui/events/, so we can't currently do this.
> 
> This is looking like it could be a problem — not with events coming from
Ozone,
> but all the other code that constructs |KeyEvent|s assuming they are
constructed
> without a character interpretation and that layout will be called from the
> constructor or from GetCharacter() later. It's all structured with KeyEvent
> depending on the layout engine.
> 
> To stop KeyEvent depending on the layout engine would require removing the two
> constructors
>   KeyEvent(EventType type, KeyboardCode key_code, int flags);
>   KeyEvent(EventType type, KeyboardCode key_code, DomCode code, int flags);
> and supplying a layout utility interface (which would depend on Ozone or X or
> ... as the case may be). There are about 200 calls to these constructors, but
> only a few that clearly need to be made dependent on layout, the most
> significant being IME and Mojo (which is a nuisance since it's out of tree). 
> 
> An alternative could be to move the layout engine out of OzonePlatform into a
> separate component that both Events and Ozone depend on. This would
essentially
> be a NotTheOzonePlatform singleton to give access to the current engine.

Copying my answer here: I think all of those disparate places would have a
LayoutEngine* ideally.

In the shorter term we may need to do something like PlatformEventSource with a
global pointer because that allows us to implement the current API while working
toward a better and less magic one.

[kpschoedel, 2014-12-01 20:00:34]

Patchset #8 (id:140001) has been deleted

[kpschoedel, 2014-12-01 20:34:55]

Patchset #9 (id:180001) has been deleted

[kpschoedel, 2014-12-01 20:35:01]

Patchset #8 (id:160001) has been deleted

[kpschoedel, 2014-12-01 22:44:02]

Works on link_freon, using the hard-coded 'stub' layout. A little cleanup left
to do.

[Wez, 2014-12-02 05:45:06]

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

[Wez, 2014-12-02 05:45:08]

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

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:636: character_(0) {
Does Blink allow delegated constructors yet?

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:739: GetInterpretationFromKeyCode(key_code_, flags(), &key_,
&character_);
Is "interpretation" a term we're using elsewhere? I'd suggest "meaning" if not,
since in principle that's what the |key| field represents.

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

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.h#newco...
ui/events/event.h:702: DomKey GetDomKey() const;
Indentation looks wrong?

It's a shame that we cannot name this consistently with the |code| members,
above (i.e. most likely to re-name those).

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.h#newco...
ui/events/event.h:744: mutable DomKey key_;
Why does this need mutable?

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/BUILD.gn
File ui/events/ozone/BUILD.gn (right):

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/BUILD.g...
ui/events/ozone/BUILD.gn:127: "EVENTS_OZONE_LAYOTU_IMPLEMENTATION",
typo?

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:93: if (!static_cast<int>(dom_code))
nit: Explicitly compare against the invalid |code| value constant?

[kpschoedel, 2014-12-02 16:45:45]

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

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:636: character_(0) {
On 2014/12/02 05:45:07, Wez wrote:
> Does Blink allow delegated constructors yet?

Don't know about Blink, but they're allowed here. As above, I'd like to remove
the two following constructors later, but don't want the churn now.

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:739: GetInterpretationFromKeyCode(key_code_, flags(), &key_,
&character_);
On 2014/12/02 05:45:07, Wez wrote:
> Is "interpretation" a term we're using elsewhere? I'd suggest "meaning" if
not,
> since in principle that's what the |key| field represents.

Works for me.

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

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.h#newco...
ui/events/event.h:702: DomKey GetDomKey() const;
On 2014/12/02 05:45:08, Wez wrote:
> Indentation looks wrong?
> 
> It's a shame that we cannot name this consistently with the |code| members,
> above (i.e. most likely to re-name those).

Acknowledged.

https://codereview.chromium.org/742103002/diff/220001/ui/events/event.h#newco...
ui/events/event.h:744: mutable DomKey key_;
On 2014/12/02 05:45:07, Wez wrote:
> Why does this need mutable?

Same as character_; the existing code mutates character_ in GetCharacter(), so
it's practical to handle key_ the same way. There's a bit of discussion in the
earlier comments — I agree on moving away from this (for character_ and key_
both) but I think it's too risky to make that the highest priority.

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/BUILD.gn
File ui/events/ozone/BUILD.gn (right):

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/BUILD.g...
ui/events/ozone/BUILD.gn:127: "EVENTS_OZONE_LAYOTU_IMPLEMENTATION",
On 2014/12/02 05:45:08, Wez wrote:
> typo?

Done.

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/742103002/diff/220001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:93: if (!static_cast<int>(dom_code))
On 2014/12/02 05:45:08, Wez wrote:
> nit: Explicitly compare against the invalid |code| value constant?

Caught! The problem is that this file includes <linux/input.h> (via
keyboard_evdev.h), which has preprocessor macros named KEY_A etc., which
conflict with the shoutycase transliterations of KeyA etc. in DomCode. All the
workarounds are unappealing, including this one, so I won't object to a
different one.

[spang, 2014-12-02 18:38:26]

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:101: if
(!KeyboardLayoutEngineManager::GetKeyboardLayoutEngine()->Lookup(
Can you add a KeyboardLayoutEngineManager* member variable and plumb it in
through the constructor?

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:18: CHECK(!instance_)
<< "Only one platform event source can be created.";
forgot to update the message

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:36: if (!instance_)
Why do we need to lazily construct this? Is there an init ordering problem
somewhere?

[kpschoedel, 2014-12-02 19:18:43]

Patchset #11 (id:260001) has been deleted

[kpschoedel, 2014-12-02 19:24:11]

Patchset #11 (id:280001) has been deleted

[kpschoedel, 2014-12-02 19:34:20]

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:101: if
(!KeyboardLayoutEngineManager::GetKeyboardLayoutEngine()->Lookup(
On 2014/12/02 18:38:26, spang wrote:
> Can you add a KeyboardLayoutEngineManager* member variable and plumb it in
> through the constructor?

Done.

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:18: CHECK(!instance_)
<< "Only one platform event source can be created.";
On 2014/12/02 18:38:26, spang wrote:
> forgot to update the message

Done.

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:36: if (!instance_)
On 2014/12/02 18:38:26, spang wrote:
> Why do we need to lazily construct this? Is there an init ordering problem
> somewhere?

Yes, all the tests that think they can get away with using a KeyEvent without
having explicitly initialized the
KeyboardLayoutEngineManagerFactoryFloorSingletonImpl.

[spang, 2014-12-02 20:28:00]

lgtm

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:36: if (!instance_)
On 2014/12/02 19:34:20, kpschoedel wrote:
> On 2014/12/02 18:38:26, spang wrote:
> > Why do we need to lazily construct this? Is there an init ordering problem
> > somewhere?
> 
> Yes, all the tests that think they can get away with using a KeyEvent without
> having explicitly initialized the
> KeyboardLayoutEngineManagerFactoryFloorSingletonImpl.
> 

maybe TODO(): Fix tests that depend on layout engine to instantiate one.

https://codereview.chromium.org/742103002/diff/300001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/742103002/diff/300001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:210: *dom_key = DomKey::CLEAR;
missing break?

https://codereview.chromium.org/742103002/diff/300001/ui/events/ozone/events_...
File ui/events/ozone/events_ozone.gyp (right):

https://codereview.chromium.org/742103002/diff/300001/ui/events/ozone/events_...
ui/events/ozone/events_ozone.gyp:123: '../../../base/base.gyp:base',
I think there's some missing deps here. Should go through and add everything you
have an #include for.

[kpschoedel, 2014-12-02 21:52:35]

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/240001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:36: if (!instance_)
On 2014/12/02 20:27:59, spang wrote:
> On 2014/12/02 19:34:20, kpschoedel wrote:
> > On 2014/12/02 18:38:26, spang wrote:
> > > Why do we need to lazily construct this? Is there an init ordering problem
> > > somewhere?
> > 
> > Yes, all the tests that think they can get away with using a KeyEvent
without
> > having explicitly initialized the
> > KeyboardLayoutEngineManagerFactoryFloorSingletonImpl.
> > 
> 
> maybe TODO(): Fix tests that depend on layout engine to instantiate one.

Done.

https://codereview.chromium.org/742103002/diff/300001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/742103002/diff/300001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:210: *dom_key = DomKey::CLEAR;
On 2014/12/02 20:28:00, spang wrote:
> missing break?

Done.

https://codereview.chromium.org/742103002/diff/300001/ui/events/ozone/events_...
File ui/events/ozone/events_ozone.gyp (right):

https://codereview.chromium.org/742103002/diff/300001/ui/events/ozone/events_...
ui/events/ozone/events_ozone.gyp:123: '../../../base/base.gyp:base',
On 2014/12/02 20:28:00, spang wrote:
> I think there's some missing deps here. Should go through and add everything
you
> have an #include for.

Done; also ran component and GN builds.

[kpschoedel, 2014-12-03 19:14:36]

sadrul@, wez@ — please take another look

[sadrul, 2014-12-03 21:46:33]

On 2014/12/03 19:14:36, kpschoedel wrote:
> sadrul@, wez@ — please take another look

wez@ Please review the changes to KeyEvent in event.cc|h. I will rubberstamp
with your review.

[kpschoedel, 2014-12-05 17:49:31]

wez@ PTAL - despite Reitveld keeping the old email subject, this is ready to go.

[Wez, 2014-12-09 01:39:10]

On 2014/12/05 17:49:31, kpschoedel wrote:
> wez@ PTAL - despite Reitveld keeping the old email subject, this is ready to
go.

Can you update the CL description to describe the basic structure of the layout
engine changes?

[Wez, 2014-12-09 07:00:34]

https://codereview.chromium.org/742103002/diff/380001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/380001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:25: // system layout, so there
is no standalone 'KeyboardLayout' object.
This comment is not very self-explanatory; IIUC you're saying we can't express
platform keyboard layouts in terms of tables of e.g. DomKey values per physical
key, since that's not what all platforms expose?

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

https://codereview.chromium.org/742103002/diff/400001/ui/events/event.h#newco...
ui/events/event.h:588: // -- key_, possibly in conjuction with character_, is
the meaning
Clarify that the character meaning is explained below? Or explain it here?

https://codereview.chromium.org/742103002/diff/400001/ui/events/event.h#newco...
ui/events/event.h:755: // the event has been transformed.
What does it mean for it to be "lazily evaluated"?

https://codereview.chromium.org/742103002/diff/400001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:135: switch (key_code) {
The switches in these functions feels like they'd read more clearly as tables
akin to the keycode_converter_data table.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:35: virtual bool
SetCurrentLayoutByName(const std::string& layout_name) = 0;
What's the format of the layout name? Or is that platform specific?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:38: virtual bool
UsesISOLevel5Shift() const = 0;
Do you mean Level 5 shift, or Level 3?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:52: // *dom_key to UNIDENTIFIED,
*character to 0, and *key_code to VKEY_UNKNOWN.
Do all implementations have to set all of the fields, or can they opt to e.g.
not set the KeyboardCode?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:58: int flags,
What are the contents of |flags|?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:60: base::char16* character,
There isn't a 1:1 correspondence between keys and characters, e.g. due to dead
keys, IMEs, etc. Have you considered breaking apart the Code->Key conversion
from the sequence-of-Key->character conversion engines?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:13:
KeyboardLayoutEngineManager* KeyboardLayoutEngineManager::instance_ = nullptr;
Seems that this needs to be a leaky singleton scoped_ptr?

Does the manager do anything other than contain the global layout engine? If not
then can we replace this class with a pair of set/get APIs back by a singleton?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:19: static void
Create(scoped_ptr<KeyboardLayoutEngine> engine);
Why does the Create method take a KeyboardLayoutEngine and not actually create
anything?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:20: static
KeyboardLayoutEngine* GetKeyboardLayoutEngine();
Does this return a global singleton?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:27: };
DISALLOW_COPY_AND_ASSIGN

[kpschoedel, 2014-12-09 19:58:44]

https://codereview.chromium.org/742103002/diff/380001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/380001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:25: // system layout, so there
is no standalone 'KeyboardLayout' object.
On 2014/12/09 07:00:33, Wez wrote:
> This comment is not very self-explanatory; IIUC you're saying we can't express
> platform keyboard layouts in terms of tables of e.g. DomKey values per
physical
> key, since that's not what all platforms expose?

Indeed, took me a bit of pondering to remember what I was trying to say:

// This interface does not expose individual layouts because it must support
// platforms that only provide for one active system layout, and/or platforms
// where layouts have no accessible representation.

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

https://codereview.chromium.org/742103002/diff/400001/ui/events/event.h#newco...
ui/events/event.h:588: // -- key_, possibly in conjuction with character_, is
the meaning
On 2014/12/09 07:00:33, Wez wrote:
> Clarify that the character meaning is explained below? Or explain it here?

Done.

https://codereview.chromium.org/742103002/diff/400001/ui/events/event.h#newco...
ui/events/event.h:755: // the event has been transformed.
On 2014/12/09 07:00:33, Wez wrote:
> What does it mean for it to be "lazily evaluated"?

Not set until required by an accessor. Revised the comment.

https://codereview.chromium.org/742103002/diff/400001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:135: switch (key_code) {
On 2014/12/09 07:00:33, Wez wrote:
> The switches in these functions feels like they'd read more clearly as tables
> akin to the keycode_converter_data table.

OK. I agree tables are generally clearer. (Historically I've used switches where
the values aren't user-orderable.)

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:35: virtual bool
SetCurrentLayoutByName(const std::string& layout_name) = 0;
On 2014/12/09 07:00:33, Wez wrote:
> What's the format of the layout name? Or is that platform specific?

This is currently a drop-in replacement for
ImeKeyboard::SetCurrentKeyboardLayoutByName(), which means the argument is (1)
not explicitly defined, and (2) in practice an XKB RMLVO name. The interface
here currently open for revision with the IME folks.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:38: virtual bool
UsesISOLevel5Shift() const = 0;
On 2014/12/09 07:00:33, Wez wrote:
> Do you mean Level 5 shift, or Level 3?

Level 5. This again is a drop-in for ImeKeyboard::IsISOLevel5ShiftAvailable(),
slightly renamed because I could never remember whether 'Available' means the
layout uses it or doesn't use it.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:52: // *dom_key to UNIDENTIFIED,
*character to 0, and *key_code to VKEY_UNKNOWN.
On 2014/12/09 07:00:33, Wez wrote:
> Do all implementations have to set all of the fields, or can they opt to e.g.
> not set the KeyboardCode?

This was up in the air for a while, but I've settled on requiring it, as
determining it can require multiple calls to the underlying layout mechanism (as
in ui/events/keycodes/keyboard_code_conversion_x.cc) and I think it's better
done here than in the caller(s). A likely near-term plan is to factor out the
XkbKeyboardLayoutEngine code for this.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:58: int flags,
On 2014/12/09 07:00:33, Wez wrote:
> What are the contents of |flags|?

ui::EventFlags, which is an unscoped enum. Most existing uses of it are typed
|int|.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:60: base::char16* character,
On 2014/12/09 07:00:33, Wez wrote:
> There isn't a 1:1 correspondence between keys and characters, e.g. due to dead
> keys, IMEs, etc. Have you considered breaking apart the Code->Key conversion
> from the sequence-of-Key->character conversion engines?

Not in the short term; it's currently wrapped up with IME. For now this is just
a slight abstraction of core X11 XKB.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:13:
KeyboardLayoutEngineManager* KeyboardLayoutEngineManager::instance_ = nullptr;
On 2014/12/09 07:00:33, Wez wrote:
> Seems that this needs to be a leaky singleton scoped_ptr?
> 
> Does the manager do anything other than contain the global layout engine? If
not
> then can we replace this class with a pair of set/get APIs back by a
singleton?

Intended after the TODO at line 33 below is done. This CL is explicitly part of
a series of incremental steps.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:19: static void
Create(scoped_ptr<KeyboardLayoutEngine> engine);
On 2014/12/09 07:00:34, Wez wrote:
> Why does the Create method take a KeyboardLayoutEngine and not actually create
> anything?

Not sure now (it may have been some offline comment), so I'll change it back.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:20: static
KeyboardLayoutEngine* GetKeyboardLayoutEngine();
On 2014/12/09 07:00:33, Wez wrote:
> Does this return a global singleton?

Not generally; it's normally owned by an OzonePlatform instance. This -Manager
just exists to break an otherwise circular dependency between ui/events/ozone
and ui/ozone, and the plan is to remove it by no longer calling layout from
within KeyEvent — see spang's previous comments.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:27: };
On 2014/12/09 07:00:33, Wez wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Wez, 2014-12-12 20:04:18]

lgtm

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:35: virtual bool
SetCurrentLayoutByName(const std::string& layout_name) = 0;
On 2014/12/09 19:58:44, kpschoedel wrote:
> On 2014/12/09 07:00:33, Wez wrote:
> > What's the format of the layout name? Or is that platform specific?
> 
> This is currently a drop-in replacement for
> ImeKeyboard::SetCurrentKeyboardLayoutByName(), which means the argument is (1)
> not explicitly defined, and (2) in practice an XKB RMLVO name. The interface
> here currently open for revision with the IME folks.

OK, SGTM; consider referencing the corresponding bug for that in the comment.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:52: // *dom_key to UNIDENTIFIED,
*character to 0, and *key_code to VKEY_UNKNOWN.
On 2014/12/09 19:58:44, kpschoedel wrote:
> On 2014/12/09 07:00:33, Wez wrote:
> > Do all implementations have to set all of the fields, or can they opt to
e.g.
> > not set the KeyboardCode?
> 
> This was up in the air for a while, but I've settled on requiring it, as
> determining it can require multiple calls to the underlying layout mechanism
(as
> in ui/events/keycodes/keyboard_code_conversion_x.cc) and I think it's better
> done here than in the caller(s). A likely near-term plan is to factor out the
> XkbKeyboardLayoutEngine code for this.
> 

OK, sounds reasonable.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:58: int flags,
On 2014/12/09 19:58:44, kpschoedel wrote:
> On 2014/12/09 07:00:33, Wez wrote:
> > What are the contents of |flags|?
> 
> ui::EventFlags, which is an unscoped enum. Most existing uses of it are typed
> |int|.

Suggest clarifying that it's ui::EventFlags in the comment.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:13:
KeyboardLayoutEngineManager* KeyboardLayoutEngineManager::instance_ = nullptr;
On 2014/12/09 19:58:44, kpschoedel wrote:
> On 2014/12/09 07:00:33, Wez wrote:
> > Seems that this needs to be a leaky singleton scoped_ptr?
> > 
> > Does the manager do anything other than contain the global layout engine? If
> not
> > then can we replace this class with a pair of set/get APIs back by a
> singleton?
> 
> Intended after the TODO at line 33 below is done. This CL is explicitly part
of
> a series of incremental steps.

OK; in general it's best to leave comments referring to the bug # that describes
the work in these interim CLs - more lines-per-CL of churn, but at least if you
get stalled then the state of the code is obvious to the reader.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:19: static void
Create(scoped_ptr<KeyboardLayoutEngine> engine);
On 2014/12/09 19:58:44, kpschoedel wrote:
> On 2014/12/09 07:00:34, Wez wrote:
> > Why does the Create method take a KeyboardLayoutEngine and not actually
create
> > anything?
> 
> Not sure now (it may have been some offline comment), so I'll change it back.

If both of the methods are static, do you need the class in the header any more?

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:20: static
KeyboardLayoutEngine* GetKeyboardLayoutEngine();
On 2014/12/09 19:58:44, kpschoedel wrote:
> On 2014/12/09 07:00:33, Wez wrote:
> > Does this return a global singleton?
> 
> Not generally; it's normally owned by an OzonePlatform instance. This -Manager
> just exists to break an otherwise circular dependency between ui/events/ozone
> and ui/ozone, and the plan is to remove it by no longer calling layout from
> within KeyEvent — see spang's previous comments.

Agreed w/ spang's comments re KeyEvent becoming just data!

Not sure what the issue is with this being a class vs a pair of static
functions, but if it's necessary then OK.

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

https://codereview.chromium.org/742103002/diff/420001/ui/events/event.h#newco...
ui/events/event.h:587: // -- key_ and character_ provide the meaning of the key
event. Together they
nit: suggest "in the context of the active layout and modifiers"?

https://codereview.chromium.org/742103002/diff/420001/ui/events/event.h#newco...
ui/events/event.h:592: // -- key_code_ is a VKEY_ value associated with the key.
For printable
nit: Worth mentioning that this supports the legacy keyCode field, with values
chosen to match Windows/IE for compatibility?

https://codereview.chromium.org/742103002/diff/420001/ui/events/event.h#newco...
ui/events/event.h:603: // -- key_ is DomKey::CHARACTER and character_ is a
UTF-16 code point.
Setting |key_| for character events feels wrong, even though it makes no real
difference.

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:45: {VKEY_CLEAR, DomKey::CLEAR},
Won't C extend the DomKey::CLEAR value out into the plain_character and
shift_character fields, if you don't specify NULL explicitly?

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:249: // Isolated characters.
Needs a better description - how would you go about describing these characters?
Seems basically to be non-alphanumeric, non-numpad, non-function and
non-ctrl-modified?

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.h (right):

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.h:41: base::char16* character);
I think it'd be best to include "US" somewhere in these API names, e.g.
GetUsLayoutCharacterFromKeyCode and GetUsLayoutMeaningFromkeyCode, so that
editors of call-sites are clear on the API limitations.

https://codereview.chromium.org/742103002/diff/420001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/420001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:57: // and *key_code to
VKEY_UNKNOWN.
What does it set key_code to?

Do you need the return-value, or could callers just check the |dom_key| value,
for example?

[kpschoedel, 2014-12-12 21:20:39]

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:35: virtual bool
SetCurrentLayoutByName(const std::string& layout_name) = 0;
On 2014/12/12 20:04:17, Wez wrote:
> On 2014/12/09 19:58:44, kpschoedel wrote:
> > On 2014/12/09 07:00:33, Wez wrote:
> > > What's the format of the layout name? Or is that platform specific?
> > 
> > This is currently a drop-in replacement for
> > ImeKeyboard::SetCurrentKeyboardLayoutByName(), which means the argument is
(1)
> > not explicitly defined, and (2) in practice an XKB RMLVO name. The interface
> > here currently open for revision with the IME folks.
> 
> OK, SGTM; consider referencing the corresponding bug for that in the comment.

Done.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:58: int flags,
On 2014/12/12 20:04:17, Wez wrote:
> On 2014/12/09 19:58:44, kpschoedel wrote:
> > On 2014/12/09 07:00:33, Wez wrote:
> > > What are the contents of |flags|?
> > 
> > ui::EventFlags, which is an unscoped enum. Most existing uses of it are
typed
> > |int|.
> 
> Suggest clarifying that it's ui::EventFlags in the comment.

Done.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.cc (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.cc:13:
KeyboardLayoutEngineManager* KeyboardLayoutEngineManager::instance_ = nullptr;
On 2014/12/12 20:04:17, Wez wrote:
> On 2014/12/09 19:58:44, kpschoedel wrote:
> > On 2014/12/09 07:00:33, Wez wrote:
> > > Seems that this needs to be a leaky singleton scoped_ptr?
> > > 
> > > Does the manager do anything other than contain the global layout engine?
If
> > not
> > > then can we replace this class with a pair of set/get APIs back by a
> > singleton?
> > 
> > Intended after the TODO at line 33 below is done. This CL is explicitly part
> of
> > a series of incremental steps.
> 
> OK; in general it's best to leave comments referring to the bug # that
describes
> the work in these interim CLs - more lines-per-CL of churn, but at least if
you
> get stalled then the state of the code is obvious to the reader.

Done.

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine_manager.h (right):

https://codereview.chromium.org/742103002/diff/400001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine_manager.h:20: static
KeyboardLayoutEngine* GetKeyboardLayoutEngine();
On 2014/12/12 20:04:17, Wez wrote:
> Agreed w/ spang's comments re KeyEvent becoming just data!

I've started on this but didn't want it in simultaneously with the first Ozone
release, since it could have unexpected distracting side effects on other
platforms.

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

https://codereview.chromium.org/742103002/diff/420001/ui/events/event.h#newco...
ui/events/event.h:587: // -- key_ and character_ provide the meaning of the key
event. Together they
On 2014/12/12 20:04:18, Wez wrote:
> nit: suggest "in the context of the active layout and modifiers"?

Done.

https://codereview.chromium.org/742103002/diff/420001/ui/events/event.h#newco...
ui/events/event.h:603: // -- key_ is DomKey::CHARACTER and character_ is a
UTF-16 code point.
On 2014/12/12 20:04:18, Wez wrote:
> Setting |key_| for character events feels wrong, even though it makes no real
> difference.

The reason for doing so here is that some event-consuming code is vague about
keystroke vs character events, so I want to avoid risk from something operating
on a character event as if it were a keystroke event, by giving them the same
(key, character) pair. Ideally having the cleaner platform-independent DOM Level
3 model here in KeyEvent will encourage other code to move away from the Windows
conflated model.

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.cc (right):

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:45: {VKEY_CLEAR, DomKey::CLEAR},
On 2014/12/12 20:04:18, Wez wrote:
> Won't C extend the DomKey::CLEAR value out into the plain_character and
> shift_character fields, if you don't specify NULL explicitly?

Uninitialized fields are 0, but I'll make it explicit.

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.cc:249: // Isolated characters.
On 2014/12/12 20:04:18, Wez wrote:
> Needs a better description - how would you go about describing these
characters?
> Seems basically to be non-alphanumeric, non-numpad, non-function and
> non-ctrl-modified?

Changed to ‘Other keys.’

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion.h (right):

https://codereview.chromium.org/742103002/diff/420001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion.h:41: base::char16* character);
On 2014/12/12 20:04:18, Wez wrote:
> I think it'd be best to include "US" somewhere in these API names, e.g.
> GetUsLayoutCharacterFromKeyCode and GetUsLayoutMeaningFromkeyCode, so that
> editors of call-sites are clear on the API limitations.

Good idea. I'll do that in a separate CL, since I don't want to add all the
calling files to this one now.

https://codereview.chromium.org/742103002/diff/420001/ui/events/ozone/layout/...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/742103002/diff/420001/ui/events/ozone/layout/...
ui/events/ozone/layout/keyboard_layout_engine.h:57: // and *key_code to
VKEY_UNKNOWN.
On 2014/12/12 20:04:18, Wez wrote:
> What does it set key_code to?

The Windows-compatible code for the ‘equivalent’ layout. For the initial
Ozone/XKB implementation, I've based this on the tables in
keyboard_code_conversion_x.cc, but I'm not entirely happy with its current
state.

> Do you need the return-value, or could callers just check the |dom_key| value,
> for example?

They could. I'll leave it as is for today, because fengyuan@ has some work in
progress using patches with this signature.

[sadrul, 2014-12-15 17:29:20]

lgtm

[kpschoedel, 2014-12-15 17:30:47]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:31:59]

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

[commit-bot: I haz the power, 2014-12-15 19:35:16]

Committed patchset #19 (id:460001)

[commit-bot: I haz the power, 2014-12-15 19:36:02]

Patchset 19 (id:??) landed as
https://crrev.com/763ff21d56e31d5b18a43c635f233de89a2ad9f0
Cr-Commit-Position: refs/heads/master@{#308390}

[spang, 2014-12-16 04:05:20]

https://codereview.chromium.org/742103002/diff/300001/ui/events/ozone/events_...
File ui/events/ozone/events_ozone.gyp (right):

https://codereview.chromium.org/742103002/diff/300001/ui/events/ozone/events_...
ui/events/ozone/events_ozone.gyp:123: '../../../base/base.gyp:base',
On 2014/12/02 21:52:34, kpschoedel wrote:
> On 2014/12/02 20:28:00, spang wrote:
> > I think there's some missing deps here. Should go through and add everything
> you
> > have an #include for.
> 
> Done; also ran component and GN builds.

They seem to have gotten lost.