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..57262934f64f60295b0583197f556baceb5b668a 100644
--- a/components/exo/wayland/server.cc
+++ b/components/exo/wayland/server.cc
@@ -85,6 +85,11 @@
 #include <drm_fourcc.h>
 #include <linux-dmabuf-unstable-v1-server-protocol.h>
 #include <wayland-drm-server-protocol.h>
+#if defined(OS_CHROMEOS)

[sadrul, 2017/06/14 03:16:53]

Do you need this? Are there other platforms where exo is built?

[jclinton, 2017/06/14 15:49:04]

This was already reviewed and it is necessary. See review thread.

[sadrul, 2017/06/15 06:45:21]

Link please?

+#include "ui/base/ime/chromeos/ime_keyboard.h"
+#include "ui/base/ime/chromeos/input_method_manager.h"
+#include "ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h"
+#endif
 #endif
 
 #if BUILDFLAG(USE_XKBCOMMON)
@@ -2737,29 +2742,34 @@ 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
+#if defined(USE_OZONE) && defined(OS_CHROMEOS)

[sadrul, 2017/06/14 03:16:53]

Do you need either of the define checks?

[jclinton, 2017/06/14 15:49:04]

This was already reviewed and it is necessary. See review thread.

[sadrul, 2017/06/15 06:57:27]

I am requesting more information here to understand why we need these checks,
because if we do need these, then it would be better to organize this code
differently. For example, have a separate a separate independent
ImeKeyboard::Observer object, owned by WaylandKeyboardDelegate, instead of
having to litter the ifdefs everywhere in WaylandKeyboardDelegate, which makes
this code difficult to read. Or an ozone+cros specific subclass of
WaylandKeyboardDelegate.

Also, USE_OZONE is turned on by default for chromeos [1], so you shouldn't need
both checks. And exo/wayland has a dependency on ash [2, 3], and ash is
supported only on chromeos [4]. That's why I want to understand better when we
will be building exo on other platforms.

[1] https://cs.chromium.org/chromium/src/build/config/ui.gni?l=30
[2]
https://cs.chromium.org/chromium/src/components/exo/BUILD.gn?sq=package:chrom...
[3]
https://cs.chromium.org/chromium/src/components/exo/wayland/BUILD.gn?sq=packa...
[4]
https://cs.chromium.org/chromium/src/ash/BUILD.gn?sq=package:chromium&dr&l=12

[reveman, 2017/06/15 11:44:15]

We're not making the assumption that exo will only be built on chromeos and with
ozone today. When exo was created we decided to not limit it to chromeos builds
but also support ash on linux but now that this is no longer a supported build
config we might want to change that design. Ozone was not used by default for
workstation cros builds when we created exo so ifdefs for that was necessary.
Organizing the code differently is fine with me if it improves readability but
I'm also OK with current version (prefer it if we intend to remove some of these
ifdefs in the near future).
Are non-ozone chromeos builds not supported at all anymore?

I don't mind changing these build assumptions but if we do that then we should
do it throughout all of exo and not just for this code. The current patch is
consistent with rest of exo today.

[sadrul, 2017/06/15 13:18:11]

The linux_chromium_chromeos_rel_ng still runs non-ozone config. But the support
is in the process of being removed
(https://bugs.chromium.org/p/chromium/issues/detail?id=671355).
Sounds reasonable. Thanks.

+      , public chromeos::input_method::ImeKeyboard::Observer
+#endif
+    {
  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);
+        xkb_context_(xkb_context_new(XKB_CONTEXT_NO_FLAGS)) {
+#if defined(USE_OZONE) && defined(OS_CHROMEOS)
+    chromeos::input_method::ImeKeyboard* keyboard =
+        chromeos::input_method::InputMethodManager::Get()->GetImeKeyboard();
+    if (keyboard)
+      keyboard->AddObserver(this);
+    SendNamedLayout(keyboard->GetCurrentKeyboardLayoutName());

[sadrul, 2017/06/14 03:16:53]

Looks like you don't actually need the null-check in line 2758?

[jclinton, 2017/06/14 15:49:04]

This was already reviewed and it is necessary; null is returned from the fake.
See review thread.

[sadrul, 2017/06/15 13:18:11]

If |keyboard| can be null, won't this line cause a crash?

[jclinton, 2017/06/15 15:46:46]

Done.

+#else
+    SendLayout(nullptr);
+#endif
   }
+#if defined(USE_OZONE) && defined(OS_CHROMEOS)
+  ~WaylandKeyboardDelegate() override {
+    chromeos::input_method::ImeKeyboard* keyboard =
+        chromeos::input_method::InputMethodManager::Get()->GetImeKeyboard();
+    if (keyboard)
+      keyboard->RemoveObserver(this);
+  }
+#endif
 
   // Overridden from KeyboardDelegate:
   void OnKeyboardDestroying(Keyboard* keyboard) override { delete this; }
@@ -2816,6 +2826,14 @@ class WaylandKeyboardDelegate : public KeyboardDelegate {
     wl_client_flush(client());
   }
 
+#if defined(USE_OZONE) && defined(OS_CHROMEOS)
+  // Overridden from input_method::ImeKeyboard::Observer:
+  void OnCapsLockChanged(bool enabled) override {}
+  void OnLayoutChanging(const std::string& layout_name) override {
+    SendNamedLayout(layout_name);
+  }
+#endif
+
  private:
   // Returns the corresponding key given a dom code.
   uint32_t DomCodeToKey(ui::DomCode code) const {
@@ -2853,6 +2871,40 @@ class WaylandKeyboardDelegate : public KeyboardDelegate {
     return xkb_modifiers;
   }
 
+
+#if defined(USE_OZONE) && defined(OS_CHROMEOS)
+  // Send the named keyboard layout to the client.
+  void SendNamedLayout(const std::string& layout_name) {
+    std::string layout_id, layout_variant;
+    ui::XkbKeyboardLayoutEngine::ParseLayoutName(layout_name, &layout_id,
+                                                 &layout_variant);
+    xkb_rule_names names = {.rules = nullptr,
+                            .model = "pc101",
+                            .layout = layout_id.c_str(),
+                            .variant = layout_variant.c_str(),
+                            .options = ""};
+    SendLayout(&names);
+  }
+#endif
+
+  // Send the keyboard layout named by XKB rules to the client.
+  void SendLayout(const xkb_rule_names* names) {
+    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);
+  }
+
   // The client who own this keyboard instance.
   wl_client* client() const {
     return wl_resource_get_client(keyboard_resource_);