Implement sync of keyboard layout between Ozone and Wayland clients

components/exo/wayland/server.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/wayland/server.h"
 
 #include <alpha-compositing-unstable-v1-server-protocol.h>
 #include <gaming-input-unstable-v1-server-protocol.h>
 #include <gaming-input-unstable-v2-server-protocol.h>
 #include <grp.h>
@@ skipping 51 matching lines @@
 #include "components/exo/shell_surface.h"
 #include "components/exo/sub_surface.h"
 #include "components/exo/surface.h"
 #include "components/exo/touch.h"
 #include "components/exo/touch_delegate.h"
 #include "components/exo/touch_stylus_delegate.h"
 #include "components/exo/wm_helper.h"
 #include "third_party/skia/include/core/SkRegion.h"
 #include "ui/base/class_property.h"
 #include "ui/base/hit_test.h"
+#include "ui/base/ime/chromeos/ime_keyboard.h"

[reveman, 2017/06/09 14:53:18]

we don't assume that this code is only built on chromeos. please move to
chromeos ifdef below

[jclinton, 2017/06/09 16:36:03]

Done.

+#include "ui/base/ime/chromeos/input_method_manager.h"
 #include "ui/base/ui_features.h"
 #include "ui/compositor/compositor_vsync_manager.h"
 #include "ui/display/display_observer.h"
 #include "ui/display/manager/managed_display_info.h"
 #include "ui/display/screen.h"
 #include "ui/events/keycodes/dom/keycode_converter.h"
+#include "ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h"

[reveman, 2017/06/09 14:53:18]

move this to #if defined(USE_OZONE) below

[jclinton, 2017/06/09 16:36:03]

Done.

 #include "ui/gfx/buffer_format_util.h"
 #include "ui/gfx/buffer_types.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 #include "ui/wm/core/coordinate_conversion.h"
 
 #if defined(USE_OZONE)
 #include <drm_fourcc.h>
 #include <linux-dmabuf-unstable-v1-server-protocol.h>
 #include <wayland-drm-server-protocol.h>
@@ skipping 2642 matching lines @@
 const struct wl_pointer_interface pointer_implementation = {pointer_set_cursor,
                                                             pointer_release};
 
 #if BUILDFLAG(USE_XKBCOMMON)
 
 ////////////////////////////////////////////////////////////////////////////////
 // wl_keyboard_interface:
 
 // Keyboard delegate class that accepts events for surfaces owned by the same
 // client as a keyboard resource.
-class WaylandKeyboardDelegate : public KeyboardDelegate {
+class WaylandKeyboardDelegate
+    : public KeyboardDelegate,
+      public chromeos::input_method::ImeKeyboard::Observer {

[reveman, 2017/06/09 14:53:19]

I think this needs to be protected by a chromeos ifdef

[jclinton, 2017/06/09 16:36:03]

Done.

  public:
   explicit WaylandKeyboardDelegate(wl_resource* keyboard_resource)
       : keyboard_resource_(keyboard_resource),
         xkb_context_(xkb_context_new(XKB_CONTEXT_NO_FLAGS)),
-        // TODO(reveman): Keep keymap synchronized with the keymap used by
-        // chromium and the host OS.
         xkb_keymap_(xkb_keymap_new_from_names(xkb_context_.get(),
                                               nullptr,
                                               XKB_KEYMAP_COMPILE_NO_FLAGS)),
         xkb_state_(xkb_state_new(xkb_keymap_.get())) {
     std::unique_ptr<char, base::FreeDeleter> keymap_string(
         xkb_keymap_get_as_string(xkb_keymap_.get(), XKB_KEYMAP_FORMAT_TEXT_V1));
     DCHECK(keymap_string.get());
     size_t keymap_size = strlen(keymap_string.get()) + 1;
     base::SharedMemory shared_keymap;
     bool rv = shared_keymap.CreateAndMapAnonymous(keymap_size);
     DCHECK(rv);
     memcpy(shared_keymap.memory(), keymap_string.get(), keymap_size);
     wl_keyboard_send_keymap(keyboard_resource_,
                             WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1,
                             shared_keymap.handle().GetHandle(), keymap_size);
+    chromeos::input_method::ImeKeyboard* keyboard =

[reveman, 2017/06/09 14:53:18]

needs a chromeos ifdef

[jclinton, 2017/06/09 16:36:03]

Done.

+        chromeos::input_method::InputMethodManager::Get()->GetImeKeyboard();
+

[reveman, 2017/06/09 14:53:18]

nit: no need for this blank line

[jclinton, 2017/06/09 16:36:03]

Done.

+    if (keyboard) {

[reveman, 2017/06/09 14:53:19]

when is this null? can we DCHECK instead?

[jclinton, 2017/06/09 16:36:03]

It returns null here:
https://cs.chromium.org/chromium/src/ui/base/ime/chromeos/mock_input_method_m...

[reveman, 2017/06/09 16:50:12]

Ack

+      keyboard->AddObserver(this);
+      keyboard->ReapplyCurrentKeyboardLayout();

[reveman, 2017/06/09 14:53:18]

Does this result in another wl_keyboard_send_keymap call? In that case please
avoid the call above.

[jclinton, 2017/06/09 16:36:03]

Yes, it does but we can no longer avoid it after the introduction of the #ifdef
requested above. Before, this was here to defensively attempt to preserve some
kind of consistent state in cases where they keyboard layout cannot be
determined. After the introduction of the #ifdef, that's still the case but now
it's mandatory.

[reveman, 2017/06/09 16:50:12]

Not sure I follow. Looks like we just need to refactor this a bit. See my
comments on latest patch.

+    }
+  }
+

[reveman, 2017/06/09 14:53:18]

nit: no need for this blank line

[jclinton, 2017/06/09 16:36:02]

Done.

+  ~WaylandKeyboardDelegate() {

[reveman, 2017/06/09 14:53:18]

~WaylandKeyboardDelegate() override ? as this inherits

[jclinton, 2017/06/09 16:36:02]

Done.

+    chromeos::input_method::ImeKeyboard* keyboard =

[reveman, 2017/06/09 14:53:18]

this needs ifdefs

[jclinton, 2017/06/09 16:36:02]

Done.

+        chromeos::input_method::InputMethodManager::Get()->GetImeKeyboard();
+

[reveman, 2017/06/09 14:53:18]

nit: not need for blank line

[jclinton, 2017/06/09 16:36:03]

Done.

+    if (keyboard) {

[reveman, 2017/06/09 14:53:18]

DCHECK instead if possible

[jclinton, 2017/06/09 16:36:03]

Acknowledged.

+      keyboard->RemoveObserver(this);
+    }

[reveman, 2017/06/09 14:53:18]

nit need for {}

[jclinton, 2017/06/09 16:36:02]

Done.

   }
 
   // Overridden from KeyboardDelegate:
   void OnKeyboardDestroying(Keyboard* keyboard) override { delete this; }
   bool CanAcceptKeyboardEventsForSurface(Surface* surface) const override {
     wl_resource* surface_resource = GetSurfaceResource(surface);
     // We can accept events for this surface if the client is the same as the
     // keyboard.
     return surface_resource &&
            wl_resource_get_client(surface_resource) == client();
@@ skipping 37 matching lines @@
     wl_keyboard_send_modifiers(
         keyboard_resource_, next_serial(),
         xkb_state_serialize_mods(xkb_state_.get(), XKB_STATE_MODS_DEPRESSED),
         xkb_state_serialize_mods(xkb_state_.get(), XKB_STATE_MODS_LOCKED),
         xkb_state_serialize_mods(xkb_state_.get(), XKB_STATE_MODS_LATCHED),
         xkb_state_serialize_layout(xkb_state_.get(),
                                    XKB_STATE_LAYOUT_EFFECTIVE));
     wl_client_flush(client());
   }
 
+  // input_method::ImeKeyboard::Observer: we are not interested in caps lock:

[reveman, 2017/06/09 14:53:18]

"// Overridden from input_method::ImeKeyboard::Observer:"

[jclinton, 2017/06/09 16:36:03]

Done.

+  void OnCapsLockChanged(bool enabled) override {}
+

[reveman, 2017/06/09 14:53:18]

remove this blank line. all input_method::ImeKeyboard::Observer overrides should
be in one group

[jclinton, 2017/06/09 16:36:02]

Done.

+  // input_method::ImeKeyboard::Observer implementation:

[reveman, 2017/06/09 14:53:18]

remove comment as we should have the "Overridden from.." above

[jclinton, 2017/06/09 16:36:03]

Done.

+  void OnLayoutChanged(const std::string& layout_name) override {
+    std::string layout_id;
+    std::string layout_variant;
+    ui::XkbKeyboardLayoutEngine::ParseLayoutName(layout_name, &layout_id,
+                                                 &layout_variant);
+    xkb_rule_names names = {.rules = NULL,
+                            .model = "pc101",
+                            .layout = layout_id.c_str(),
+                            .variant = layout_variant.c_str(),
+                            .options = ""};
+    xkb_keymap_.reset(xkb_keymap_new_from_names(xkb_context_.get(), &names,
+                                                XKB_KEYMAP_COMPILE_NO_FLAGS));
+    xkb_state_.reset(xkb_state_new(xkb_keymap_.get()));
+    std::unique_ptr<char, base::FreeDeleter> keymap_string(
+        xkb_keymap_get_as_string(xkb_keymap_.get(), XKB_KEYMAP_FORMAT_TEXT_V1));
+    DCHECK(keymap_string.get());
+    size_t keymap_size = strlen(keymap_string.get()) + 1;
+    base::SharedMemory shared_keymap;
+    bool rv = shared_keymap.CreateAndMapAnonymous(keymap_size);
+    DCHECK(rv);
+    memcpy(shared_keymap.memory(), keymap_string.get(), keymap_size);
+    wl_keyboard_send_keymap(keyboard_resource_,
+                            WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1,
+                            shared_keymap.handle().GetHandle(), keymap_size);
+  };

[reveman, 2017/06/09 14:53:18]

s/};/}/ and all this needs to be behind and ifdef

[jclinton, 2017/06/09 16:36:03]

Done.

+
  private:
   // Returns the corresponding key given a dom code.
   uint32_t DomCodeToKey(ui::DomCode code) const {
     // This assumes KeycodeConverter has been built with evdev/xkb codes.
     xkb_keycode_t xkb_keycode = static_cast<xkb_keycode_t>(
         ui::KeycodeConverter::DomCodeToNativeKeycode(code));
 
     // Keycodes are offset by 8 in Xkb.
     DCHECK_GE(xkb_keycode, 8u);
     return xkb_keycode - 8;
@@ skipping 1219 matching lines @@
   DCHECK(event_loop);
   wl_event_loop_dispatch(event_loop, timeout.InMilliseconds());
 }
 
 void Server::Flush() {
   wl_display_flush_clients(wl_display_.get());
 }
 
 }  // namespace wayland
 }  // namespace exo