XKB implementation of Ozone key layout.

Partial XKB implementation of Ozone key layout.

No layout switching; uses a hard-coded US layout.

BUG=430194
Committed: https://crrev.com/80093bd012a04094ec2c4d889f0bd6c0a8a94837
Cr-Commit-Position: refs/heads/master@{#308447}

[kpschoedel, 2014-12-02 23:24:57]

FYI Feng Yuan — this works on link_freon (on top of
https://codereview.chromium.org/742103002/) but I still need to do some work on
it.

[kpschoedel, 2014-12-03 19:12:49]

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

[kpschoedel, 2014-12-03 19:13:17]

Depends on https://codereview.chromium.org/742103002/

[spang, 2014-12-05 20:10:48]

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/events_o...
File ui/events/ozone/events_ozone.gyp (right):

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/events_o...
ui/events/ozone/events_ozone.gyp:156: '-lxkbcommon',
I think we'll want a

use_xkbcommon gyp variable

in order to avoid adding more mandatory dependencies.

As-is, even if you only build no-layout platforms, you'll need to install & link
this library.

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/layout/l...
File ui/events/ozone/layout/layout_util.h (right):

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/layout/l...
ui/events/ozone/layout/layout_util.h:26: // Determine the non-located VKEY
corresponding to a located VKEY.
Above says "positional" but here says "located". Can we make the comments
consistent?

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

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:78: xkb_context*
xkb_context_;
How about doing

// scoped_xkb.h

struct XkbContextDeleter {
  void operator()(xkb_context* context) const;
};

typedef scoped_ptr<xkb_context, XkbContextDeleter> ScopedXkbContextPtr;

// scoped_xkb.cc

void XkbContextDeleter::operator()(xkb_context* context) const {
  xkb_context_unref(context);
}

[kpschoedel, 2014-12-08 19:54:38]

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/events_o...
File ui/events/ozone/events_ozone.gyp (right):

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/events_o...
ui/events/ozone/events_ozone.gyp:156: '-lxkbcommon',
On 2014/12/05 20:10:47, spang wrote:
> I think we'll want a use_xkbcommon gyp variable

Changed to make building the xkb conditional.

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/layout/l...
File ui/events/ozone/layout/layout_util.h (right):

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/layout/l...
ui/events/ozone/layout/layout_util.h:26: // Determine the non-located VKEY
corresponding to a located VKEY.
On 2014/12/05 20:10:47, spang wrote:
> Above says "positional" but here says "located". Can we make the comments
> consistent?

Done.

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

https://codereview.chromium.org/778503002/diff/40001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:78: xkb_context*
xkb_context_;
On 2014/12/05 20:10:47, spang wrote:
> How about doing
> 
> // scoped_xkb.h
> 
> struct XkbContextDeleter {
>   void operator()(xkb_context* context) const;
> };
> 
> typedef scoped_ptr<xkb_context, XkbContextDeleter> ScopedXkbContextPtr;
> 
> // scoped_xkb.cc
> 
> void XkbContextDeleter::operator()(xkb_context* context) const {
>   xkb_context_unref(context);
> }

Done.

[kpschoedel, 2014-12-08 20:44:53]

Patchset #6 (id:100001) has been deleted

[spang, 2014-12-08 22:47:29]

I made a separate CL for some build bits

https://codereview.chromium.org/751623003/

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

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/BUILD.gn...
ui/events/ozone/BUILD.gn:135: if (ozone_platform_dri || ozone_platform_gbm) {
use_xkbcommon?

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
File ui/events/ozone/layout/xkb/xkb_keyboard_code_conversion.cc (right):

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_code_conversion.cc:14: }  // anonymous
namespace
remove this

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

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:250: xkb_keymap* keymap
= xkb_keymap_new_from_string(
Is this deleted anywhere?

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h (right):

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:18: using XkbKeyCode =
decltype(XKeyEvent.keycode);
remove dead code

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

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/BUILD.g...
ui/events/ozone/BUILD.gn:135: if (ozone_platform_dri || ozone_platform_gbm) {
if (use_xkbcommon)

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/BUILD.g...
ui/events/ozone/BUILD.gn:147: libs = [ "xkbcommon" ]
configs = [ "//build/config/linux:xkbcommon" ]

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

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/events_...
ui/events/ozone/events_ozone.gyp:158: 'link_settings': {
        'dependencies': [
          '../../../build/linux/system.gyp:xkbcommon',
        ],

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right):

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:304: static struct {
const

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h (right):

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:18: class
EVENTS_OZONE_LAYOUT_EXPORT XkbKeyCodeConverter {
move to separate header

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:65: // libxkbcommon uses
explicit reference counting for its structures,
Can you move these general-purpose typedefs to a new header e.g. scoped_xkb.h?

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri.gypi (right):

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri.gypi:17: 'use_xkbcommon': 1,
remove

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gbm.gypi (right):

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gbm.gypi:13: 'use_xkbcommon': 1,
remove

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_dri.cc (right):

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:13: #include
"ui/events/ozone/layout/xkb/xkb_evdev_codes.h"
#if defined(USE_XKBCOMMON)
#include "ui/events/ozone/layout/xkb/xkb_evdev_codes.h"
#include "ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h"
#else
#include "ui/events/ozone/layout/stub/stub_keyboard_layout_engine.h"
#endif

(put it below the unconditional headers)

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:104: new
XkbKeyboardLayoutEngine(xkb_evdev_code_converter_)));
#if defined(USE_XKBCOMMON)
  // create xkb thing
#else
  // create stub thing
#endif

[kpschoedel, 2014-12-08 23:15:26]

USE_X11 removal was in patch set 6.

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

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/BUILD.gn...
ui/events/ozone/BUILD.gn:135: if (ozone_platform_dri || ozone_platform_gbm) {
On 2014/12/08 22:47:28, spang wrote:
> use_xkbcommon?

Couldn't figure out how to set that under the platforms that use it.

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
File ui/events/ozone/layout/xkb/xkb_keyboard_code_conversion.cc (right):

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_code_conversion.cc:14: }  // anonymous
namespace
On 2014/12/08 22:47:28, spang wrote:
> remove this

Oops, USE_X11 was in there.

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

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:250: xkb_keymap* keymap
= xkb_keymap_new_from_string(
On 2014/12/08 22:47:28, spang wrote:
> Is this deleted anywhere?

xkb_state_unref() calls xkb_keymap_unref().

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h (right):

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/layout/x...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:18: using XkbKeyCode =
decltype(XKeyEvent.keycode);
On 2014/12/08 22:47:28, spang wrote:
> remove dead code

Done.

[spang, 2014-12-08 23:19:10]

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

https://codereview.chromium.org/778503002/diff/80001/ui/events/ozone/BUILD.gn...
ui/events/ozone/BUILD.gn:135: if (ozone_platform_dri || ozone_platform_gbm) {
On 2014/12/08 23:15:26, kpschoedel wrote:
> On 2014/12/08 22:47:28, spang wrote:
> > use_xkbcommon?
> 
> Couldn't figure out how to set that under the platforms that use it.

try on top of https://codereview.chromium.org/751623003/

[kpschoedel, 2014-12-15 19:03:57]

Using https://codereview.chromium.org/751623003/

[spang, 2014-12-15 19:16:22]

still unaddressed comments on #6

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

https://codereview.chromium.org/778503002/diff/180001/ui/events/ozone/events_...
ui/events/ozone/events_ozone.gyp:39: ['use_xkbcommon==1', {
remove this.

[kpschoedel, 2014-12-15 19:48:38]

On 2014/12/15 19:16:22, spang wrote:
> still unaddressed comments on #6

Sorry, stupid Reitveld didn't rub them in my face after the next patch set.

[kpschoedel, 2014-12-15 19:48:47]

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right):

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:304: static struct {
On 2014/12/08 22:47:29, spang wrote:
> const

Done.

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h (right):

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:18: class
EVENTS_OZONE_LAYOUT_EXPORT XkbKeyCodeConverter {
On 2014/12/08 22:47:29, spang wrote:
> move to separate header

Done.

https://codereview.chromium.org/778503002/diff/120001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:65: // libxkbcommon uses
explicit reference counting for its structures,
On 2014/12/08 22:47:29, spang wrote:
> Can you move these general-purpose typedefs to a new header e.g. scoped_xkb.h?

OK, though I wouldn't call them general-purpose — it's unlikely they will ever
be used more than once each.

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_dri.cc (right):

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:13: #include
"ui/events/ozone/layout/xkb/xkb_evdev_codes.h"
On 2014/12/08 22:47:29, spang wrote:
> #if defined(USE_XKBCOMMON)
> #include "ui/events/ozone/layout/xkb/xkb_evdev_codes.h"
> #include "ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h"
> #else
> #include "ui/events/ozone/layout/stub/stub_keyboard_layout_engine.h"
> #endif
> 
> (put it below the unconditional headers)

Done.

https://codereview.chromium.org/778503002/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:104: new
XkbKeyboardLayoutEngine(xkb_evdev_code_converter_)));
On 2014/12/08 22:47:29, spang wrote:
> #if defined(USE_XKBCOMMON)
>   // create xkb thing
> #else
>   // create stub thing
> #endif

Done.

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

https://codereview.chromium.org/778503002/diff/180001/ui/events/ozone/events_...
ui/events/ozone/events_ozone.gyp:39: ['use_xkbcommon==1', {
On 2014/12/15 19:16:22, spang wrote:
> remove this.

Done.

[spang, 2014-12-15 19:53:44]

lgtm with a nit

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_dri.cc (right):

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:16: #else
conditional #includes go below the unconditional ones

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:35: 
put them here after a blank line

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:18: #if defined(USE_XKBCOMMON)
move downwards

[kpschoedel, 2014-12-15 20:10:23]

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_dri.cc (right):

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:16: #else
On 2014/12/15 19:53:44, spang wrote:
> conditional #includes go below the unconditional ones

Done.

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_dri.cc:35: 
On 2014/12/15 19:53:44, spang wrote:
> put them here after a blank line

Acknowledged.

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/778503002/diff/200001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:18: #if defined(USE_XKBCOMMON)
On 2014/12/15 19:53:44, spang wrote:
> move downwards

Done.

[kpschoedel, 2014-12-15 21:13:39]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-12-15 21:14:06]

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

[commit-bot: I haz the power, 2014-12-15 21:20:39]

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

[commit-bot: I haz the power, 2014-12-15 21:20:40]

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

[kpschoedel, 2014-12-15 21:31:40]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-12-15 21:33:08]

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

[commit-bot: I haz the power, 2014-12-15 22:47:14]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2014-12-15 22:48:15]

Patchset 13 (id:??) landed as
https://crrev.com/80093bd012a04094ec2c4d889f0bd6c0a8a94837
Cr-Commit-Position: refs/heads/master@{#308447}