|
|
Description[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
Patch Set 1 #
Total comments: 5
Patch Set 2 : fixed a missing comma #
Total comments: 1
Patch Set 3 : addressed forney's and spang's feedback #Patch Set 4 : no xkbcommon dependency to begin with #Patch Set 5 : Implement basic keyboard handling support #Patch Set 6 : Implement basic keyboard handling support #Patch Set 7 : Implement basic keyboard handling support #
Total comments: 14
Patch Set 8 : Addressed spang's feedback #
Total comments: 2
Patch Set 9 : addressed moar feedback from forney/spang #
Total comments: 1
Patch Set 10 : removed ozone:events_ozone_evdev dep #Patch Set 11 : fixed a strange line-wrap made by git cl format #Patch Set 12 : addressed kpschoedel's review #Messages
Total messages: 79 (39 generated)
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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@igalia.com changed reviewers: + rjkroege@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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
PTAL
tonikitoo@igalia.com changed reviewers: + spang@chromium.org
forney@google.com changed reviewers: + forney@google.com
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?
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..
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.
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.
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.
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?
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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
@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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
spang@chromium.org changed reviewers: + kpschoedel@chromium.org
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".
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.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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?
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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?
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.
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.
lgtm kpschoedel do you have any comments about the keyboard layout bits?
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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
> 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.
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?
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.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tonikitoo@igalia.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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@igalia.com changed reviewers: + sadrul@chromium.org
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.
lgtm
The CQ bit was checked by tonikitoo@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485481495497870, "parent_rev": "0fcc08c5d93f2e8128d5e583c236dd2a81e70a7b", "commit_rev": "f304472fe1220599ab63ef2a51ad5cfdedca805f"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/f304472fe1220599ab63ef2a51ad... |