[ozone/wayland] Implement basic keyboard handling support

[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing.

Modifiers work as follows: when the wayland compositor calls out the
client hook notifying of a modifier key state changed, we update
XkbKeyboardLayoutEngine::xkb_state_ with it.

Based on the original work of Michael Forney <forney@google.com>;, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890
Review-Url: https://codereview.chromium.org/2639053002
Cr-Commit-Position: refs/heads/master@{#446529}
Committed: https://chromium.googlesource.com/chromium/src/+/f304472fe1220599ab63ef2a51ad5cfdedca805f

[tonikitoo, 2017-01-17 21:03:53]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:04:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2017-01-17 21:04:33]

Description was changed from

==========
[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing. It still needs proper modifier support, which I will work
on in a follow up, in order to unblock inputing.

Based on the original work of Michael Forney <forney@google.com>, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890
==========

to

==========
[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing. It still needs proper modifier support, which I will work
on in a follow up, in order to unblock inputing.

Based on the original work of Michael Forney <forney@google.com>, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890
==========

[tonikitoo, 2017-01-17 21:04:33]

tonikitoo@igalia.com changed reviewers:
+ rjkroege@chromium.org

[commit-bot: I haz the power, 2017-01-17 21:34:31]

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

[commit-bot: I haz the power, 2017-01-17 21:34:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tonikitoo, 2017-01-17 23:00:48]

On 2017/01/17 21:34:32, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

It fails in chromeos_ozone_rel because of the absence of libxkbcommon0 /
libxkbcommon-dev packages in the debian-wheezy sysroot, but are on
debian-jessie. This is bug
https://bugs.chromium.org/p/chromium/issues/detail?id=681562

FAILED: obj/ui/ozone/platform/wayland/wayland/wayland_keyboard.o 
(..)
In file included from ../../ui/ozone/platform/wayland/wayland_keyboard.cc:11:
../../ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:9:10: fatal error:
'xkbcommon/xkbcommon.h' file not found
#include <xkbcommon/xkbcommon.h>
         ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[6870/12429] CXX

[tonikitoo, 2017-01-17 23:01:15]

PTAL

[tonikitoo, 2017-01-17 23:23:31]

tonikitoo@igalia.com changed reviewers:
+ spang@chromium.org

[Michael Forney, 2017-01-17 23:44:55]

forney@google.com changed reviewers:
+ forney@google.com

[Michael Forney, 2017-01-17 23:44:56]

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_keyboard.cc:55:
keyboard->evdev_.SetCurrentLayoutByName(map_str);
map_str might not be NULL terminated.

This is why libxkbcommon added xkb_keymap_new_from_buffer. You might need to
create a temporary copy.

Also, do we have any plan for making this better? Overloading
SetCurrentLayoutByName seems pretty bad.

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_keyboard.cc:85: key, state ==
WL_KEYBOARD_KEY_STATE_PRESSED false,
Missing comma?

[tonikitoo, 2017-01-18 00:02:28]

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_keyboard.cc:55:
keyboard->evdev_.SetCurrentLayoutByName(map_str);
On 2017/01/17 23:44:56, Michael Forney wrote:
> map_str might not be NULL terminated.
> 
> This is why libxkbcommon added xkb_keymap_new_from_buffer. You might need to
> create a temporary copy.
> 
> Also, do we have any plan for making this better? Overloading
> SetCurrentLayoutByName seems pretty bad.

That is a good. I can certainly try that out. Not sure if before this patch, or
it is fine to be done as a follow up. wdyt?

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_keyboard.cc:85: key, state ==
WL_KEYBOARD_KEY_STATE_PRESSED false,
On 2017/01/17 23:44:56, Michael Forney wrote:
> Missing comma?

Done.

'git cl format' did me this favor. I should file a bug..

[Michael Forney, 2017-01-18 00:16:05]

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_keyboard.cc:55:
keyboard->evdev_.SetCurrentLayoutByName(map_str);
On 2017/01/18 00:02:28, tonikitoo wrote:
> On 2017/01/17 23:44:56, Michael Forney wrote:
> > map_str might not be NULL terminated.
> > 
> > This is why libxkbcommon added xkb_keymap_new_from_buffer. You might need to
> > create a temporary copy.
> > 
> > Also, do we have any plan for making this better? Overloading
> > SetCurrentLayoutByName seems pretty bad.
> 
> That is a good. I can certainly try that out. Not sure if before this patch,
or
> it is fine to be done as a follow up. wdyt?

I think we should change this CL to pass std::string(map_str, size) so it makes
a NULL-terminated copy, and maybe consider using the appropriate libxkbcommon
API in a future CL.

I don't think we should check-in code that could potentially cause libxkbcommon
to read memory passed the end of our keymap string, even if it's just temporary.

[spang, 2017-01-18 00:59:48]

https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_keyboard.h (right):

https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_keyboard.h:55: KeyboardEvdev evdev_;
I don't think using KeyboardEvdev and EventModifiersEvdev (or, in general,
anything from events/ozone/evdev) is likely to be the right approach for wayland
keyboard integration. Those classes implement low level keyboard handling for
Chrome OS, but the wayland compositor is responsible for that. For example
active modifiers are deliver to you here in Modifiers(), whereas KeyboardEvdev
will try to calculate that itself and things are likely to get out of sync.

[tonikitoo, 2017-01-18 05:46:09]

On 2017/01/18 00:59:48, spang wrote:
>
https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
> File ui/ozone/platform/wayland/wayland_keyboard.h (right):
> 
>
https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
> ui/ozone/platform/wayland/wayland_keyboard.h:55: KeyboardEvdev evdev_;
> I don't think using KeyboardEvdev and EventModifiersEvdev (or, in general,
> anything from events/ozone/evdev) is likely to be the right approach for
wayland
> keyboard integration. Those classes implement low level keyboard handling for
> Chrome OS, but the wayland compositor is responsible for that. For example
> active modifiers are deliver to you here in Modifiers(), whereas KeyboardEvdev
> will try to calculate that itself and things are likely to get out of sync.

Thanks spang@. I believe mouse events do something similar to what you described
(no KeyboardEvdev et al). Will try it.

[tonikitoo, 2017-01-18 18:34:22]

On 2017/01/18 00:16:05, Michael Forney wrote:
> 
> I think we should change this CL to pass std::string(map_str, size) so it
makes
> a NULL-terminated copy, and maybe consider using the appropriate libxkbcommon
> API in a future CL.
> 
> I don't think we should check-in code that could potentially cause
libxkbcommon
> to read memory passed the end of our keymap string, even if it's just
temporary.

Ok, I tried it. Please check.


On 2017/01/18 00:16:05, Michael Forney wrote:
> 
> I think we should change this CL to pass std::string(map_str, size) so it
makes
> a NULL-terminated copy, and maybe consider using the appropriate libxkbcommon
> API in a future CL.
> 
> I don't think we should check-in code that could potentially cause
libxkbcommon
> to read memory passed the end of our keymap string, even if it's just
temporary.

Ok, I tried it. Please check.


On 2017/01/18 00:59:48, spang wrote:
>
https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
> File ui/ozone/platform/wayland/wayland_keyboard.h (right):
> 
>
https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
> ui/ozone/platform/wayland/wayland_keyboard.h:55: KeyboardEvdev evdev_;
> I don't think using KeyboardEvdev and EventModifiersEvdev (or, in general,
> anything from events/ozone/evdev) is likely to be the right approach for
wayland
> keyboard integration. Those classes implement low level keyboard handling for
> Chrome OS, but the wayland compositor is responsible for that. For example
> active modifiers are deliver to you here in Modifiers(), whereas KeyboardEvdev
> will try to calculate that itself and things are likely to get out of sync.

So, I could eliminate most of the evdev usage. In wayland_keyboard.cc/h, for
instance. I am now instantiating the event manually and dispatching, similar to
how mouse events are dispatched (see WaylandPointer).
As a consequence, some stuff that KeyboardEvDev implemented were not working
yet, notably auto-repeat and modifiers. If this approach is accepted, I can work
on it in a follow up.

libxkbcommon is still being used though, so that XkbKeyboardLayoutEngine is can
be used in wayland_connection.cc/h.
XkbKeyboardLayoutEngine takes a XkbEvdevCodes instance to get constructed, so
there is still a dependency on Evdev of some sort.

WDYT?

[tonikitoo, 2017-01-18 20:39:21]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 20:40:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Forney, 2017-01-18 21:33:51]

On 2017/01/18 18:34:22, tonikitoo wrote:
> On 2017/01/18 00:16:05, Michael Forney wrote:
> > 
> > I think we should change this CL to pass std::string(map_str, size) so it
> makes
> > a NULL-terminated copy, and maybe consider using the appropriate
libxkbcommon
> > API in a future CL.
> > 
> > I don't think we should check-in code that could potentially cause
> libxkbcommon
> > to read memory passed the end of our keymap string, even if it's just
> temporary.
> 
> Ok, I tried it. Please check.

Thanks.

> On 2017/01/18 00:59:48, spang wrote:
> >
>
https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
> > File ui/ozone/platform/wayland/wayland_keyboard.h (right):
> > 
> >
>
https://codereview.chromium.org/2639053002/diff/20001/ui/ozone/platform/wayla...
> > ui/ozone/platform/wayland/wayland_keyboard.h:55: KeyboardEvdev evdev_;
> > I don't think using KeyboardEvdev and EventModifiersEvdev (or, in general,
> > anything from events/ozone/evdev) is likely to be the right approach for
> wayland
> > keyboard integration. Those classes implement low level keyboard handling
for
> > Chrome OS, but the wayland compositor is responsible for that. For example
> > active modifiers are deliver to you here in Modifiers(), whereas
KeyboardEvdev
> > will try to calculate that itself and things are likely to get out of sync.
> 
> So, I could eliminate most of the evdev usage. In wayland_keyboard.cc/h, for
> instance. I am now instantiating the event manually and dispatching, similar
to
> how mouse events are dispatched (see WaylandPointer).
> As a consequence, some stuff that KeyboardEvDev implemented were not working
> yet, notably auto-repeat and modifiers. If this approach is accepted, I can
work
> on it in a follow up.
> 
> libxkbcommon is still being used though, so that XkbKeyboardLayoutEngine is
can
> be used in wayland_connection.cc/h.
> XkbKeyboardLayoutEngine takes a XkbEvdevCodes instance to get constructed, so
> there is still a dependency on Evdev of some sort.
> 
> WDYT?

I think most of the problems we are running in to here are due to
XkbKeyboardLayoutEngine doing all of keycode translation, xkb state management,
and xkb keymap management. In our case, ozone/wayland can much more easily and
correctly keep xkb state and keymap objects up to date, but then we are unable
to use the rest of XkbKeyboardLayoutEngine.

I wonder if we could separate the two pieces in XkbKeyboardLayoutEngine so that
we end up with a function that can turn a keycode, keymap, and state into the
appropriate dom code and key, and use that to implement a KeyboardLayoutEngine
for ozone/wayland.

A while ago, I asked about the possibility of making the keyboard layout engine
owned by the platform[0]. I think that might make this easier.

[0] https://groups.google.com/a/chromium.org/forum/#!topic/ozone-dev/ZHDJ_nAA2Bo

[tonikitoo, 2017-01-18 21:38:50]

@forney/spang: BTW, to start with, I decided to not use XKB (and
XkbKeyboardLayoutEngine et al) on this first iteration. Hence, the primarily
goal of this CL became allow basic keyboard inputting, which is not possible at
all today.

We can add a proper XKB integration (including the refactory @forney described
above) in follow ups, as we mature up our implementation.

How does that sound?

[commit-bot: I haz the power, 2017-01-18 21:42:50]

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

[commit-bot: I haz the power, 2017-01-18 21:42:50]

Dry run: This issue passed the CQ dry run.

[spang, 2017-01-19 16:28:04]

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

[spang, 2017-01-19 16:28:18]

On 2017/01/18 21:38:50, tonikitoo wrote:
> @forney/spang: BTW, to start with, I decided to not use XKB (and
> XkbKeyboardLayoutEngine et al) on this first iteration. Hence, the primarily
> goal of this CL became allow basic keyboard inputting, which is not possible
at
> all today.
> 
> We can add a proper XKB integration (including the refactory @forney described
> above) in follow ups, as we mature up our implementation.
> 
> How does that sound?

Sure. If you want to ignore XKB integration for now just set the layout to "us".

[tonikitoo, 2017-01-20 14:43:39]

On 2017/01/17 23:44:56, Michael Forney wrote:
>
https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
> File ui/ozone/platform/wayland/wayland_keyboard.cc (right):
> 
>
https://codereview.chromium.org/2639053002/diff/1/ui/ozone/platform/wayland/w...
> ui/ozone/platform/wayland/wayland_keyboard.cc:55:
> keyboard->evdev_.SetCurrentLayoutByName(map_str);
> map_str might not be NULL terminated.
> 
> This is why libxkbcommon added xkb_keymap_new_from_buffer. You might need to
> create a temporary copy.
> 
> Also, do we have any plan for making this better? Overloading
> SetCurrentLayoutByName seems pretty bad.

I implemented the second request in https://codereview.chromium.org/2645703005/
. I tried with doing the first one (switch to using xkb_keymap_new_from_string)
as part of it, but I started to get 

  xkbcommon: ERROR: (input string): 1598:1 unrecognized token
  xkbcommon: ERROR: (input string): 1598:1 syntax error
  xkbcommon: ERROR: failed to parse input xkb string

I will try it separated, on its own CL later.

[tonikitoo, 2017-01-24 16:29:10]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 16:29:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 16:33:44]

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

[commit-bot: I haz the power, 2017-01-24 16:33:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tonikitoo, 2017-01-24 18:34:53]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 18:35:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2017-01-24 19:17:43]

Description was changed from

==========
[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing. It still needs proper modifier support, which I will work
on in a follow up, in order to unblock inputing.

Based on the original work of Michael Forney <forney@google.com>, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890
==========

to

==========
[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing.

Modifiers work as follows: when the wayland compositor calls out the
client hook notifying of a modifier key state changed, we update
XkbKeyboardLayoutEngine::xkb_state_ with it.

Based on the original work of Michael Forney <forney@google.com>, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890
==========

[tonikitoo, 2017-01-24 19:22:32]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 19:23:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spang, 2017-01-24 20:41:50]

Kevin should review the keyboard layout bits.

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

https://codereview.chromium.org/2639053002/diff/120001/ui/events/ozone/layout...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:129: }
xkb_mod_indexes_{0, 0, 0};
= {0, 0, 0};

Actually, probably even better to just do

struct {
  control = 0;
  alt = 0;
  shift = 0;
} xkb_mod_indexes_;

See
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab...
for advice on which syntax to use for initializers.

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/BUILD.gn:46: "//ui/events/ozone:events_ozone_layout",
Remove. This component is for Chrome OS (or other desktopless) builds.

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:45: uint32_t size) {
ScopedFD keymap_fd(fd);

and remove all the manual close() calls.

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:58:
->SetCurrentLayoutFromBuffer(keymap_str, strlen(keymap_str));
Please don't strlen a mmap'd file - this is unsafe.

strlen(keymap_str) -> size

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:100: // TODO(tonikitoo): handle
repeat here.
What happens when you get a repeat?

[Michael Forney, 2017-01-24 20:47:54]

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:100: // TODO(tonikitoo): handle
repeat here.
On 2017/01/24 20:41:50, spang wrote:
> What happens when you get a repeat?

The wayland server doesn't send repeats, and expects clients to generate them
based on the rate and delay sent in RepeatInfo. This TODO is about implementing
this functionality.

[spang, 2017-01-24 21:22:13]

On 2017/01/24 20:47:54, Michael Forney wrote:
>
https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
> File ui/ozone/platform/wayland/wayland_keyboard.cc (right):
> 
>
https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
> ui/ozone/platform/wayland/wayland_keyboard.cc:100: // TODO(tonikitoo): handle
> repeat here.
> On 2017/01/24 20:41:50, spang wrote:
> > What happens when you get a repeat?
> 
> The wayland server doesn't send repeats, and expects clients to generate them
> based on the rate and delay sent in RepeatInfo. This TODO is about
implementing
> this functionality.

Huh, interesting. Client side repeats don't work well if your keyboard is across
a network link (e.g. RDP). Oh well.

[Michael Forney, 2017-01-24 21:38:11]

On 2017/01/24 21:22:13, spang wrote:
> On 2017/01/24 20:47:54, Michael Forney wrote:
> > The wayland server doesn't send repeats, and expects clients to generate
them
> > based on the rate and delay sent in RepeatInfo. This TODO is about
> implementing
> > this functionality.
> 
> Huh, interesting. Client side repeats don't work well if your keyboard is
across
> a network link (e.g. RDP). Oh well.

How so? Because the application would repeat based on the configured rate/delay
in the RDP server instead of the display server the RDP client is running under?
That actually seems more correct to me, and it avoids having to send the repeat
key events across the connection.

[spang, 2017-01-24 21:48:23]

On 2017/01/24 21:38:11, Michael Forney wrote:
> On 2017/01/24 21:22:13, spang wrote:
> > On 2017/01/24 20:47:54, Michael Forney wrote:
> > > The wayland server doesn't send repeats, and expects clients to generate
> them
> > > based on the rate and delay sent in RepeatInfo. This TODO is about
> > implementing
> > > this functionality.
> > 
> > Huh, interesting. Client side repeats don't work well if your keyboard is
> across
> > a network link (e.g. RDP). Oh well.
> 
> How so? Because the application would repeat based on the configured
rate/delay
> in the RDP server instead of the display server the RDP client is running
under?
> That actually seems more correct to me, and it avoids having to send the
repeat
> key events across the connection.

A keyup can be arbitrarily delayed by the network. Sssooo tthiiss miight haappen
if you do client side repeats.

[tonikitoo, 2017-01-24 21:51:11]

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

https://codereview.chromium.org/2639053002/diff/120001/ui/events/ozone/layout...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:129: }
xkb_mod_indexes_{0, 0, 0};
On 2017/01/24 20:41:50, spang wrote:
> = {0, 0, 0};
> 
> Actually, probably even better to just do
> 
> struct {
>   control = 0;
>   alt = 0;
>   shift = 0;
> } xkb_mod_indexes_;
> 
> See
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab...
> for advice on which syntax to use for initializers.

Done.

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/BUILD.gn:46: "//ui/events/ozone:events_ozone_layout",
On 2017/01/24 20:41:50, spang wrote:
> Remove. This component is for Chrome OS (or other desktopless) builds.

'gn check <out>' actually complains (see below).
Basically, it is needed to include
"ui/events/ozone/layout/keyboard_layout_engine_manager.h", so that we call:

 36 #if BUILDFLAG(USE_XKBCOMMON)
 37   KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
 38       base::MakeUnique<XkbKeyboardLayoutEngine>(codes_));
 39 #else
 40   KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
 41       base::MakeUnique<StubKeyboardLayoutEngine>());
 42 #endif


Actual error:


Dependency chain (there may also be others):
  //ui/ozone/platform/wayland:wayland -->
  //ui/events:events --[private]-->
  //ui/events/ozone:events_ozone_layout

___________________
ERROR at //ui/ozone/platform/wayland/wayland_connection.cc:15:11: Can't include
this header from here.
#include "ui/events/ozone/layout/keyboard_layout_engine_manager.h"
          ^------------------------------------------------------
The target:
  //ui/ozone/platform/wayland:wayland
is including a file from the target:
  //ui/events/ozone:events_ozone_layout

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:45: uint32_t size) {
On 2017/01/24 20:41:50, spang wrote:
> ScopedFD keymap_fd(fd);
> 
> and remove all the manual close() calls.

Done.

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:58:
->SetCurrentLayoutFromBuffer(keymap_str, strlen(keymap_str));
On 2017/01/24 20:41:50, spang wrote:
> Please don't strlen a mmap'd file - this is unsafe.
> 
> strlen(keymap_str) -> size

Ok, I was willing to use 'size' to begin with BUT it was failing.

It turns out that my Wayland compositor (weston) sends 'size' with different
value than 'strlen(keymap_str)', and xkb_keymap_new_from_buffer fails down the
road.

The difference is '1', so I am using 'size - 1' here rather than 'size' so that
it succeeds.

WDYT?

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:100: // TODO(tonikitoo): handle
repeat here.
On 2017/01/24 20:47:53, Michael Forney wrote:
> On 2017/01/24 20:41:50, spang wrote:
> > What happens when you get a repeat?
> 
> The wayland server doesn't send repeats, and expects clients to generate them
> based on the rate and delay sent in RepeatInfo. This TODO is about
implementing
> this functionality.

Exactly.

[Michael Forney, 2017-01-24 22:11:47]

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:58:
->SetCurrentLayoutFromBuffer(keymap_str, strlen(keymap_str));
On 2017/01/24 21:51:11, tonikitoo wrote:
> On 2017/01/24 20:41:50, spang wrote:
> > Please don't strlen a mmap'd file - this is unsafe.
> > 
> > strlen(keymap_str) -> size
> 
> Ok, I was willing to use 'size' to begin with BUT it was failing.
> 
> It turns out that my Wayland compositor (weston) sends 'size' with different
> value than 'strlen(keymap_str)', and xkb_keymap_new_from_buffer fails down the
> road.
> 
> The difference is '1', so I am using 'size - 1' here rather than 'size' so
that
> it succeeds.
> 
> WDYT?

Hmm, so I looked into this a bit further.

All wayland compositors I looked at create a temporary file containing the
entire keymap string including a NULL-terminator. Clients use size for mmap and
assume that the mapped file is NULL-terminated, but the protocol specification
doesn't make it clear if that can be relied on.

Maybe the safest thing to do is pass strnlen(keymap_str, size) here, which
should handle both cases.

[spang, 2017-01-24 22:24:07]

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/BUILD.gn:46: "//ui/events/ozone:events_ozone_layout",
On 2017/01/24 21:51:11, tonikitoo wrote:
> On 2017/01/24 20:41:50, spang wrote:
> > Remove. This component is for Chrome OS (or other desktopless) builds.
> 
> 'gn check <out>' actually complains (see below).
> Basically, it is needed to include
> "ui/events/ozone/layout/keyboard_layout_engine_manager.h", so that we call:
> 
>  36 #if BUILDFLAG(USE_XKBCOMMON)
>  37   KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
>  38       base::MakeUnique<XkbKeyboardLayoutEngine>(codes_));
>  39 #else
>  40   KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
>  41       base::MakeUnique<StubKeyboardLayoutEngine>());
>  42 #endif
> 
> 
> Actual error:
> 
> 
> Dependency chain (there may also be others):
>   //ui/ozone/platform/wayland:wayland -->
>   //ui/events:events --[private]-->
>   //ui/events/ozone:events_ozone_layout
> 
> ___________________
> ERROR at //ui/ozone/platform/wayland/wayland_connection.cc:15:11: Can't
include
> this header from here.
> #include "ui/events/ozone/layout/keyboard_layout_engine_manager.h"
>           ^------------------------------------------------------
> The target:
>   //ui/ozone/platform/wayland:wayland
> is including a file from the target:
>   //ui/events/ozone:events_ozone_layout
> 
> 
> 

Er, I meant to comment on "//ui/events/ozone:events_ozone_evdev" not
"//ui/events/ozone:events_ozone_layout". You do need the latter.

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:58:
->SetCurrentLayoutFromBuffer(keymap_str, strlen(keymap_str));
On 2017/01/24 22:11:47, Michael Forney wrote:
> On 2017/01/24 21:51:11, tonikitoo wrote:
> > On 2017/01/24 20:41:50, spang wrote:
> > > Please don't strlen a mmap'd file - this is unsafe.
> > > 
> > > strlen(keymap_str) -> size
> > 
> > Ok, I was willing to use 'size' to begin with BUT it was failing.
> > 
> > It turns out that my Wayland compositor (weston) sends 'size' with different
> > value than 'strlen(keymap_str)', and xkb_keymap_new_from_buffer fails down
the
> > road.
> > 
> > The difference is '1', so I am using 'size - 1' here rather than 'size' so
> that
> > it succeeds.
> > 
> > WDYT?
> 
> Hmm, so I looked into this a bit further.
> 
> All wayland compositors I looked at create a temporary file containing the
> entire keymap string including a NULL-terminator. Clients use size for mmap
and
> assume that the mapped file is NULL-terminated, but the protocol specification
> doesn't make it clear if that can be relied on.
> 
> Maybe the safest thing to do is pass strnlen(keymap_str, size) here, which
> should handle both cases.

SGTM.

[spang, 2017-01-24 22:31:14]

https://codereview.chromium.org/2639053002/diff/140001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/140001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:68: // TODO(forney): The
KeyboardEvdev object should update its state using the
Remove this comment.

[tonikitoo, 2017-01-24 23:16:32]

On 2017/01/24 22:24:07, spang wrote:
>
https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
> File ui/ozone/platform/wayland/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
> ui/ozone/platform/wayland/BUILD.gn:46:
"//ui/events/ozone:events_ozone_layout",
> On 2017/01/24 21:51:11, tonikitoo wrote:
> > On 2017/01/24 20:41:50, spang wrote:
> > > Remove. This component is for Chrome OS (or other desktopless) builds.
> > 
> > 'gn check <out>' actually complains (see below).
> > Basically, it is needed to include
> > "ui/events/ozone/layout/keyboard_layout_engine_manager.h", so that we call:
> > 
> >  36 #if BUILDFLAG(USE_XKBCOMMON)
> >  37   KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
> >  38       base::MakeUnique<XkbKeyboardLayoutEngine>(codes_));
> >  39 #else
> >  40   KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
> >  41       base::MakeUnique<StubKeyboardLayoutEngine>());
> >  42 #endif
> > 
> > 
> > Actual error:
> > 
> > 
> > Dependency chain (there may also be others):
> >   //ui/ozone/platform/wayland:wayland -->
> >   //ui/events:events --[private]-->
> >   //ui/events/ozone:events_ozone_layout
> > 
> > ___________________
> > ERROR at //ui/ozone/platform/wayland/wayland_connection.cc:15:11: Can't
> include
> > this header from here.
> > #include "ui/events/ozone/layout/keyboard_layout_engine_manager.h"
> >           ^------------------------------------------------------
> > The target:
> >   //ui/ozone/platform/wayland:wayland
> > is including a file from the target:
> >   //ui/events/ozone:events_ozone_layout
> > 
> > 
> > 
> 
> Er, I meant to comment on "//ui/events/ozone:events_ozone_evdev" not
> "//ui/events/ozone:events_ozone_layout". You do need the latter.

So, this dependency is needed because XkbkeyboardLayoutEngine ctor takes a
XkbKeyCodeConverter instance.

I added a TODO(tonikitoo), to work on removing it in a follow up CL.

>
https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
> File ui/ozone/platform/wayland/wayland_keyboard.cc (right):
> 
>
https://codereview.chromium.org/2639053002/diff/120001/ui/ozone/platform/wayl...
> ui/ozone/platform/wayland/wayland_keyboard.cc:58:
> ->SetCurrentLayoutFromBuffer(keymap_str, strlen(keymap_str));
> On 2017/01/24 22:11:47, Michael Forney wrote:
> > On 2017/01/24 21:51:11, tonikitoo wrote:
> > > On 2017/01/24 20:41:50, spang wrote:
> > > > Please don't strlen a mmap'd file - this is unsafe.
> > > > 
> > > > strlen(keymap_str) -> size
> > > 
> > > Ok, I was willing to use 'size' to begin with BUT it was failing.
> > > 
> > > It turns out that my Wayland compositor (weston) sends 'size' with
different
> > > value than 'strlen(keymap_str)', and xkb_keymap_new_from_buffer fails down
> the
> > > road.
> > > 
> > > The difference is '1', so I am using 'size - 1' here rather than 'size' so
> > that
> > > it succeeds.
> > > 
> > > WDYT?
> > 
> > Hmm, so I looked into this a bit further.
> > 
> > All wayland compositors I looked at create a temporary file containing the
> > entire keymap string including a NULL-terminator. Clients use size for mmap
> and
> > assume that the mapped file is NULL-terminated, but the protocol
specification
> > doesn't make it clear if that can be relied on.
> > 
> > Maybe the safest thing to do is pass strnlen(keymap_str, size) here, which
> > should handle both cases.
> 
> SGTM.

Done.

[tonikitoo, 2017-01-24 23:16:48]

https://codereview.chromium.org/2639053002/diff/140001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_keyboard.cc (right):

https://codereview.chromium.org/2639053002/diff/140001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_keyboard.cc:68: // TODO(forney): The
KeyboardEvdev object should update its state using the
On 2017/01/24 22:31:14, spang wrote:
> Remove this comment.

Done.

[tonikitoo, 2017-01-24 23:16:56]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 23:17:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spang, 2017-01-24 23:27:03]

https://codereview.chromium.org/2639053002/diff/160001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/2639053002/diff/160001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/BUILD.gn:48: # TODO(tonikitoo): Remove this dependency
for non-ChromeOS builds.
I don't follow. XkbKeyCodeConverter is not in ui/events/ozone/evdev, but in
ui/events/ozone/layout. What's the error?

[tonikitoo, 2017-01-24 23:43:52]

On 2017/01/24 23:27:03, spang wrote:
>
https://codereview.chromium.org/2639053002/diff/160001/ui/ozone/platform/wayl...
> File ui/ozone/platform/wayland/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2639053002/diff/160001/ui/ozone/platform/wayl...
> ui/ozone/platform/wayland/BUILD.gn:48: # TODO(tonikitoo): Remove this
dependency
> for non-ChromeOS builds.
> I don't follow. XkbKeyCodeConverter is not in ui/events/ozone/evdev, but in
> ui/events/ozone/layout. What's the error?

Sorry, my bad. This is the error:



ERROR at //ui/ozone/platform/wayland/wayland_keyboard.cc:15:11: Can't include
this header from here.
#include "ui/events/ozone/evdev/keyboard_util_evdev.h"
          ^------------------------------------------
The target:
  //ui/ozone/platform/wayland:wayland
is including a file from the target:
  //ui/events/ozone:events_ozone_evdev




It happens because we need a one function in WaylandKeyboard.cc:

(..)
  DomCode dom_code =
      KeycodeConverter::NativeKeycodeToDomCode(EvdevCodeToNativeCode(key));
  if (dom_code == ui::DomCode::NONE)
(..)

'EvdevCodeToNativeCode' does a basic conversion. I can adapt the comment and
work on it as a follow on.

[tonikitoo, 2017-01-25 00:05:08]

On 2017/01/24 23:43:52, tonikitoo wrote:
> On 2017/01/24 23:27:03, spang wrote:
> >
>
https://codereview.chromium.org/2639053002/diff/160001/ui/ozone/platform/wayl...
> > File ui/ozone/platform/wayland/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2639053002/diff/160001/ui/ozone/platform/wayl...
> > ui/ozone/platform/wayland/BUILD.gn:48: # TODO(tonikitoo): Remove this
> dependency
> > for non-ChromeOS builds.
> > I don't follow. XkbKeyCodeConverter is not in ui/events/ozone/evdev, but in
> > ui/events/ozone/layout. What's the error?
> 
> Sorry, my bad. This is the error:
> 
> 
> 
> ERROR at //ui/ozone/platform/wayland/wayland_keyboard.cc:15:11: Can't include
> this header from here.
> #include "ui/events/ozone/evdev/keyboard_util_evdev.h"
>           ^------------------------------------------
> The target:
>   //ui/ozone/platform/wayland:wayland
> is including a file from the target:
>   //ui/events/ozone:events_ozone_evdev
> 
> 
> 
> 
> It happens because we need a one function in WaylandKeyboard.cc:
> 
> (..)
>   DomCode dom_code =
>       KeycodeConverter::NativeKeycodeToDomCode(EvdevCodeToNativeCode(key));
>   if (dom_code == ui::DomCode::NONE)
> (..)
> 
> 'EvdevCodeToNativeCode' does a basic conversion. I can adapt the comment and
> work on it as a follow on.

Well, just fixed it in ps10.

[spang, 2017-01-25 18:54:28]

lgtm

kpschoedel do you have any comments about the keyboard layout bits?

[tonikitoo, 2017-01-25 19:10:54]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 19:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kpschoedel, 2017-01-25 19:37:40]

On 2017/01/25 18:54:28, spang wrote:
> lgtm
> 
> kpschoedel do you have any comments about the keyboard layout bits?

I think the added XkbKeyboardLayoutEngine::UpdateModifiers() is likely to break
two things, as is. First, KeyboardLayoutEngine::Lookup() can't depend on
internal state (other than the selected keyboard layout) because it can be
called to handle synthetic events independent of the current physical keyboard
state. Second, one call to XkbKeyboardLayoutEngine::Lookup() may end up making
several calls to xkb_state_update_mask() and xkb_state_key_get_one_sym()
internally, with different modifier combinations, in order to determine the
correct VKEY_ (ui::KeyboardCode) — see
XkbKeyboardLayoutEngine::DifficultKeyboardCode(). Yes, this is a giant pain and
Windows/legacy codes should be taken out and shot, but we're stuck with them for
the foreseeable future.

[commit-bot: I haz the power, 2017-01-25 20:28:00]

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

[commit-bot: I haz the power, 2017-01-25 20:28:01]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-25 22:13:56]

On 2017/01/25 19:37:40, kpschoedel wrote:
> On 2017/01/25 18:54:28, spang wrote:
> > lgtm
> > 
> > kpschoedel do you have any comments about the keyboard layout bits?
> 
> I think the added XkbKeyboardLayoutEngine::UpdateModifiers() is likely to
break
> two things, as is. First, KeyboardLayoutEngine::Lookup() can't depend on
> internal state (other than the selected keyboard layout) because it can be
> called to handle synthetic events independent of the current physical keyboard
> state. Second, one call to XkbKeyboardLayoutEngine::Lookup() may end up making
> several calls to xkb_state_update_mask() and xkb_state_key_get_one_sym()
> internally, with different modifier combinations, in order to determine the
> correct VKEY_ (ui::KeyboardCode) — see
> XkbKeyboardLayoutEngine::DifficultKeyboardCode(). Yes, this is a giant pain
and
> Windows/legacy codes should be taken out and shot, but we're stuck with them
for
> the foreseeable future.

Thanks for you input!

Looking at the code more deeply I can understand your concern:

XkbKeyboardLayoutEngine::UpdateModifiers does call xkb_state_update_mask, with
the state parameters sent by the compositor. so it changes the internal state of
xkb_state.

Also, as you said, XkbKeyboardLayoutEngine::Lookup can indeed call ::XkbLookup
multiple times (even indirectly via ::DifficultKeyboardCode and
::XkbSubCharacter), but always when ::XkbLookup is called, it passes a "flags"
parameter that actually sets the xkb_state again, by calling
xkb_state_update_mask again.

This in the end, the behavior is not changed (and hence no tests fail neither).
But I do see your point that it is not ideal.

Something that I was actually looking for is a ::UpdateMotifiersDryRun(...) type
of method. It could basically return the ui_flags based on the current state of
the modifiers. So similar to UpdateMotifiersDryRun, but without actually
changing the internal xkb_state.



So in I understand the code correctly, XkbKeyboardLayoutEngine::Lookup (that can
call XkbKeyboardLayoutEngine::XkbLookup 
twice and

[tonikitoo, 2017-01-25 22:24:11]

> Something that I was actually looking for is a ::UpdateMotifiersDryRun(...)
type
> of method. It could basically return the ui_flags based on the current state
of
> the modifiers. So similar to UpdateMotifiersDryRun, but without actually
> changing the internal xkb_state.
> 

Correction: So similar to UpdateMotifiers, but without actually changing the
internal xkb_state.

> So in I understand the code correctly, XkbKeyboardLayoutEngine::Lookup (that
can
> call XkbKeyboardLayoutEngine::XkbLookup 
> twice and

trailing lines. please ignore them.

[tonikitoo, 2017-01-26 17:32:01]

kpschoedel: I am trying to address your feedback in comment #54.

Before, let me give you some quick background: We could certainly detect
modifiers and handle them somehow by the same way KeyboardEvdev class does. At
some point during the review cycle, spang (thanks for your reviews!) actually
suggested that we should use the modifiers "hook", called by the compositor. In
the patch this is WaylandKeyboard::Modifiers(..).

Then, I studied various wayland clients that implement this hook, and all of
them actually update the 'xkb_state' with a call to xkb_state_update_mask. This
includes xwayland, weston compositor itself, and different projects I found on
the internet, e.g. [1], [2], [3] .

[1] 
https://github.com/xbmc/xbmc/commit/df4277989f28d172e9d1be4492b4f1c801d1dc4b#...
[2] http://g3d.cs.williams.edu/g3d/G3D10/glfw.lib/src/wl_init.c (search for
"keyboardHandleModifiers")
[3]
https://git.merproject.org/mer-core/libsdl/blob/176c3826ce34d51662e1a8a2090cf...

So I am trying to figure how to implement it, in a way that it does not concern
you. One alternative is that we can add a WaylandXkbKeyboardLayoutEngine
(inherits from XkbKeyboardLayoutEngine), and actually implement it without
messing up with other users of XkbKeyboardLayoutEngine.

wdyt?

[kpschoedel, 2017-01-26 18:23:37]

I admit I don't know Wayland in any depth and in particular I don't (yet) know
how its keyboard handling is meant to work. I don't want to block progress on
this — consider my comments an LGTM with a heads-up that there have been many
issues in the past with modifier handling and legacy VK emulation.

On 2017/01/26 17:32:01, tonikitoo wrote:
> So I am trying to figure how to implement it, in a way that it does not
concern
> you. One alternative is that we can add a WaylandXkbKeyboardLayoutEngine
> (inherits from XkbKeyboardLayoutEngine), and actually implement it without
> messing up with other users of XkbKeyboardLayoutEngine.

Yes, I'd be happy with that.

[tonikitoo, 2017-01-26 21:03:45]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 21:04:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2017-01-26 21:04:31]

On 2017/01/26 18:23:37, kpschoedel wrote:
> I admit I don't know Wayland in any depth and in particular I don't (yet) know
> how its keyboard handling is meant to work. I don't want to block progress on
> this — consider my comments an LGTM with a heads-up that there have been many
> issues in the past with modifier handling and legacy VK emulation.

Thank you, kpschoedel. Really great feedback.
 
> On 2017/01/26 17:32:01, tonikitoo wrote:
> > So I am trying to figure how to implement it, in a way that it does not
> concern
> > you. One alternative is that we can add a WaylandXkbKeyboardLayoutEngine
> > (inherits from XkbKeyboardLayoutEngine), and actually implement it without
> > messing up with other users of XkbKeyboardLayoutEngine.
> 
> Yes, I'd be happy with that.

Done.

[commit-bot: I haz the power, 2017-01-26 22:45:21]

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

[commit-bot: I haz the power, 2017-01-26 22:45:22]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-26 23:07:22]

The CQ bit was checked by tonikitoo@igalia.com

[tonikitoo, 2017-01-26 23:07:23]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org,
kpschoedel@chromium.org
Link to the patchset: https://codereview.chromium.org/2639053002/#ps220001
(title: "addressed kpschoedel's review")

[commit-bot: I haz the power, 2017-01-26 23:08:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2017-01-26 23:16:38]

folks, I am sending this to CL. ps12 addresses an agreement with 'kpschoedel' in
comment #60. It essentially move a couple of class members from private to
protected so that they ca be accessible to a child class.

[commit-bot: I haz the power, 2017-01-26 23:19:22]

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

[commit-bot: I haz the power, 2017-01-26 23:19:24]

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

[tonikitoo, 2017-01-26 23:37:49]

tonikitoo@igalia.com changed reviewers:
+ sadrul@chromium.org

[tonikitoo, 2017-01-26 23:38:15]

On 2017/01/26 23:19:24, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

** Presubmit ERRORS **
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
    '+ui/base/ui_features.h',

+sadrul for ui/ review.

[sadrul, 2017-01-27 01:15:54]

lgtm

[tonikitoo, 2017-01-27 01:44:55]

The CQ bit was checked by tonikitoo@igalia.com

[commit-bot: I haz the power, 2017-01-27 01:45:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 01:52:48]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485481495497870,
"parent_rev": "0fcc08c5d93f2e8128d5e583c236dd2a81e70a7b", "commit_rev":
"f304472fe1220599ab63ef2a51ad5cfdedca805f"}

[commit-bot: I haz the power, 2017-01-27 01:53:25]

Description was changed from

==========
[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing.

Modifiers work as follows: when the wayland compositor calls out the
client hook notifying of a modifier key state changed, we update
XkbKeyboardLayoutEngine::xkb_state_ with it.

Based on the original work of Michael Forney <forney@google.com>, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890
==========

to

==========
[ozone/wayland] Implement basic keyboard handling support

CL implements basic keyboard support on Ozone's Wayland backend as well
as some testing.

Modifiers work as follows: when the wayland compositor calls out the
client hook notifying of a modifier key state changed, we update
XkbKeyboardLayoutEngine::xkb_state_ with it.

Based on the original work of Michael Forney <forney@google.com>, in [1].

[1] https://codereview.chromium.org/1841083003/

BUG=578890

Review-Url: https://codereview.chromium.org/2639053002
Cr-Commit-Position: refs/heads/master@{#446529}
Committed:
https://chromium.googlesource.com/chromium/src/+/f304472fe1220599ab63ef2a51ad...
==========

[commit-bot: I haz the power, 2017-01-27 01:53:26]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/f304472fe1220599ab63ef2a51ad...