Implement sync of keyboard layout between Ozone and Wayland clients

components/exo/wayland/server.cc

Index: components/exo/wayland/server.cc
diff --git a/components/exo/wayland/server.cc b/components/exo/wayland/server.cc
index 84c17c1bfb9acfb2ff99527cd600b6093f3763ac..61792899721f7ff655b423f1581c600a7f795286 100644
--- a/components/exo/wayland/server.cc
+++ b/components/exo/wayland/server.cc
@@ -69,12 +69,15 @@
 #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"
@@ -2737,13 +2740,13 @@ const struct wl_pointer_interface pointer_implementation = {pointer_set_cursor,
 
 // 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)),
@@ -2759,6 +2762,22 @@ class WaylandKeyboardDelegate : public KeyboardDelegate {
     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:
@@ -2816,6 +2835,36 @@ class WaylandKeyboardDelegate : public KeyboardDelegate {
     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 {