ozone: xkb: Load keymaps on worker thread & cache them

ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc

Index: ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc
diff --git a/ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc b/ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc
index a756db9131a69dfdc20747c23bf5f8514021a2f6..21083b76d742343c4c9dea8547a87a18c918ccda 100644
--- a/ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc
+++ b/ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc
@@ -6,7 +6,10 @@
 
 #include <xkbcommon/xkbcommon-names.h>
 
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
+#include "base/threading/worker_pool.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/dom3/dom_code.h"
 #include "ui/events/keycodes/dom3/dom_key.h"
@@ -618,6 +621,56 @@ const PrintableSimpleEntry kSimpleMap[] = {
 
 }  // anonymous namespace
 
+XkbKeyboardLayoutEngine::XkbKeymapLoader::XkbKeymapLoader(
+    XkbKeyboardLayoutEngine* layout_engine) : layout_engine_(layout_engine) {
+}
+
+void XkbKeyboardLayoutEngine::XkbKeymapLoader::loadKeymap(
+    const std::string& layout_name) {
+  size_t dash_index = layout_name.find('-');
+  size_t parentheses_index = layout_name.find('(');
+  std::string layout_id = layout_name;
+  std::string layout_variant = "";
+  if (parentheses_index != std::string::npos) {
+    layout_id = layout_name.substr(0, parentheses_index);
+    size_t close_index = layout_name.find(')', parentheses_index);
+    if (close_index == std::string::npos)
+      close_index = layout_name.size();
+    layout_variant = layout_name.substr(parentheses_index + 1,
+                                        close_index - parentheses_index - 1);
+  } else if (dash_index != std::string::npos) {
+    layout_id = layout_name.substr(0, dash_index);
+    layout_variant = layout_name.substr(dash_index + 1);
+  }
+  names = new xkb_rule_names {
+    .rules = NULL,
+        .model = "pc101",
+        .layout = layout_id.c_str(),
+        .variant = layout_variant.c_str(),
+        .options = ""
+  };
+  base::WorkerPool::PostTask(
+      FROM_HERE,
+      base::Bind(&XkbKeyboardLayoutEngine::XkbKeymapLoader::startLoadKeymap,
+                 base::Unretained(this),

[Shu Chen, 2014/12/23 08:46:49]

base::Owned(this)

[FengYuan, 2014/12/23 15:24:57]

Done.

+                 layout_name,
+                 base::Unretained(names)),
+      true);
+}
+
+void XkbKeyboardLayoutEngine::XkbKeymapLoader::startLoadKeymap(
+    const std::string& layout_name, xkb_rule_names* names) {

[Shu Chen, 2014/12/23 08:46:49]

You need to delete names in this method.

[FengYuan, 2014/12/23 15:24:57]

Done.

+  xkb_keymap* keymap = xkb_keymap_new_from_names(
+      layout_engine_->xkb_context_.get(),
+      names,
+      XKB_KEYMAP_COMPILE_NO_FLAGS);
+
+  if (keymap) {
+    layout_engine_->xkb_keymaps_[layout_name] = keymap;

[Shu Chen, 2014/12/23 08:46:49]

Please make sure these operations are executed in UI thread, otherwise, you need
to make sure it is thread safe, because it is possible for 2 threads to execute
operations on layout_engine_ at the same time.

I think using PostTaskAndReply may be better.

[FengYuan, 2014/12/23 15:24:57]

These operations are in the worker thread, and reply is not necessary.

+    layout_engine_->SetKeymap(keymap);
+  }
+}
+
 XkbKeyCodeConverter::XkbKeyCodeConverter() {
 }
 
@@ -634,6 +687,10 @@ XkbKeyboardLayoutEngine::XkbKeyboardLayoutEngine(
 }
 
 XkbKeyboardLayoutEngine::~XkbKeyboardLayoutEngine() {
+  std::map<std::string, xkb_keymap*>::const_iterator it = xkb_keymaps_.begin();
+  for(;it != xkb_keymaps_.end(); ++it) {
+    xkb_keymap_unref(it->second);
+  }
 }
 
 bool XkbKeyboardLayoutEngine::CanSetCurrentLayout() const {
@@ -647,35 +704,14 @@ bool XkbKeyboardLayoutEngine::CanSetCurrentLayout() const {
 bool XkbKeyboardLayoutEngine::SetCurrentLayoutByName(
     const std::string& layout_name) {
 #if defined(OS_CHROMEOS)
-  size_t dash_index = layout_name.find('-');
-  size_t parentheses_index = layout_name.find('(');
-  std::string layout_id = layout_name;
-  std::string layout_variant = "";
-  if (parentheses_index != std::string::npos) {
-    layout_id = layout_name.substr(0, parentheses_index);
-    size_t close_index = layout_name.find(')', parentheses_index);
-    if (close_index == std::string::npos)
-      close_index = layout_name.size();
-    layout_variant = layout_name.substr(parentheses_index + 1,
-                                        close_index - parentheses_index - 1);
-  } else if (dash_index != std::string::npos) {
-    layout_id = layout_name.substr(0, dash_index);
-    layout_variant = layout_name.substr(dash_index + 1);
-  }
-  struct xkb_rule_names names = {
-    .rules = NULL,
-    .model = "pc101",
-    .layout = layout_id.c_str(),
-    .variant = layout_variant.c_str(),
-    .options = ""
-  };
-  xkb_keymap* keymap = xkb_keymap_new_from_names(xkb_context_.get(),
-                                                 &names,
-                                                 XKB_KEYMAP_COMPILE_NO_FLAGS);
+  xkb_keymap* keymap = xkb_keymaps_[layout_name];
   if (keymap) {
     SetKeymap(keymap);
-    return true;
+  } else {
+    XkbKeymapLoader* keymap_loader = new XkbKeymapLoader(this);

[Shu Chen, 2014/12/23 08:46:49]

nowhere to delete keymap_loader? Suggest to make it as a member variable.

[FengYuan, 2014/12/23 15:24:57]

Done.

+    keymap_loader->loadKeymap(layout_name);
   }
+  return true;
 #endif  // defined(OS_CHROMEOS)
   return false;
 }