Use chromeos::Process in libchromeos.a instead of g_spawn_async.

chromeos_keyboard.cc

Index: chromeos_keyboard.cc
diff --git a/chromeos_keyboard.cc b/chromeos_keyboard.cc
index 2b8014fa85420f50c7e82be97e16f2568b1f7985..15a0e80dd5678ffbbe9741a3aa1e70b6064749aa 100644
--- a/chromeos_keyboard.cc
+++ b/chromeos_keyboard.cc
@@ -16,11 +16,14 @@
 #include "base/logging.h"
 #include "base/singleton.h"
 #include "base/string_util.h"
+#include "chromeos/process.h"
 
 namespace {
 
-// The command we use to set/get the current XKB layout and modifier key
-// mapping.
+// The default keyboard layout name in the xorg config file.
+const char kDefaultLayoutName[] = "us";
+// The command we use to set the current XKB layout and modifier key mapping.
+// TODO(yusukes): Use libxkbfile.so instead of the command (crosbug.com/13105)
 const char kSetxkbmapCommand[] = "/usr/bin/setxkbmap";
 // See the comment at ModifierKey in the .h file.
 chromeos::ModifierKey kCustomizableKeys[] = {
@@ -67,62 +70,40 @@ class XKeyboard {
   // Sets the current keyboard layout to |layout_name|. This function does not
   // change the current mapping of the modifier keys. Returns true on success.
   bool SetLayout(const std::string& layout_name) {
-    // TODO(yusukes): write auto tests for the function.
-    chromeos::ModifierMap modifier_map;
-    if (!GetModifierMapping(&modifier_map)) {
-      LOG(ERROR) << "Failed to get modifier mapping.";
-      return false;
-    }
-    if (SetLayoutInternal(layout_name, modifier_map, true)) {
+    if (SetLayoutInternal(layout_name, current_modifier_map_, true)) {
+      current_layout_name_ = layout_name;
       return true;
     }
+    // TODO(satorux,yusukes): Remove +version hack.
     LOG(ERROR) << "SetLayoutInternal failed. Retrying without +version option";
-    return SetLayoutInternal(layout_name, modifier_map, false);
+    if (SetLayoutInternal(layout_name, current_modifier_map_, false)) {
+      current_layout_name_ = layout_name;
+      return true;
+    }
+    return false;
   }
 
   // Remaps modifier keys. This function does not change the current keyboard
   // layout. Returns true on success.
   bool RemapModifierKeys(const chromeos::ModifierMap& modifier_map) {
     // TODO(yusukes): write auto tests for the function.
-    modifier_keys_are_remapped_ = false;
-    const std::string layout_name = GetLayout();
-    if (layout_name.empty()) {
-      return false;
-    }
-    if (SetLayoutInternal(layout_name, modifier_map, true)) {
-      modifier_keys_are_remapped_ = true;
+    if (SetLayoutInternal(current_layout_name_, modifier_map, true)) {
+      current_modifier_map_ = modifier_map;
       return true;
     }
+    // TODO(satorux,yusukes): Remove +version hack.
     LOG(ERROR) << "SetLayoutInternal failed. Retrying without +version option";
-    if (SetLayoutInternal(layout_name, modifier_map, false)) {
-      modifier_keys_are_remapped_ = true;
+    if (SetLayoutInternal(current_layout_name_, modifier_map, false)) {
+      current_modifier_map_ = modifier_map;
       return true;
     }
     return false;
   }
 
-  // Returns the current layout name like "us". On error, returns "".
-  std::string GetLayout() {
-    // TODO(yusukes): write auto tests for the function.
-    std::string command_output = last_full_layout_name_;
-
-    if (command_output.empty()) {
-      // Cache is not available. Execute setxkbmap to get the current layout.
-      if (!ExecuteGetLayoutCommand(&command_output)) {
-        return "";
-      }
-    }
-
-    const std::string layout_name =
-        chromeos::ExtractLayoutNameFromFullXkbLayoutName(command_output);
-    LOG(INFO) << "Current XKB layout name: " << layout_name;
-    return layout_name;
-  }
-
   // Gets the current auto-repeat mode of the keyboard. The result is stored in
   // |mode|. Returns true on success.
+  // TODO(yusukes): Remove this function.
   bool GetAutoRepeatEnabled(bool* enabled) {
-    // TODO(yusukes): write auto tests for the function.
     DCHECK(enabled);
     ScopedDisplay display(XOpenDisplay(NULL));
     if (!display.get()) {
@@ -139,8 +120,8 @@ class XKeyboard {
   }
 
   // Turns on and off the auto-repeat of the keyboard. Returns true on success.
+  // TODO(yusukes): Remove this function.
   bool SetAutoRepeatEnabled(bool enabled) {
-    // TODO(yusukes): write auto tests for the function.
     ScopedDisplay display(XOpenDisplay(NULL));
     if (!display.get()) {
       return false;
@@ -156,8 +137,8 @@ class XKeyboard {
 
   // Gets the current auto-repeat rate of the keyboard. The result is stored in
   // |out_rate|. Returns true on success.
+  // TODO(yusukes): Remove this function.
   bool GetAutoRepeatRate(chromeos::AutoRepeatRate* out_rate) {
-    // TODO(yusukes): write auto tests for the function.
     ScopedDisplay display(XOpenDisplay(NULL));
     if (!display.get()) {
       return false;
@@ -174,6 +155,7 @@ class XKeyboard {
 
   // Sets the auto-repeat rate of the keyboard, initial delay in ms, and repeat
   // interval in ms.  Returns true on success.
+  // TODO(yusukes): Call this function in non-UI thread or in an idle callback.
   bool SetAutoRepeatRate(const chromeos::AutoRepeatRate& rate) {
     // TODO(yusukes): write auto tests for the function.
     ScopedDisplay display(XOpenDisplay(NULL));
@@ -196,40 +178,15 @@ class XKeyboard {
  private:
   friend struct DefaultSingletonTraits<XKeyboard>;
 
-  XKeyboard() : modifier_keys_are_remapped_(false) {
-    InitializeStringToModifierMap(&string_to_modifier_map_);
+  XKeyboard() : current_layout_name_(kDefaultLayoutName) {
+    for (size_t i = 0; i < arraysize(kCustomizableKeys); ++i) {
+      chromeos::ModifierKey key = kCustomizableKeys[i];
+      current_modifier_map_.push_back(chromeos::ModifierKeyPair(key, key));
+    }
   }
   ~XKeyboard() {
   }
 
-  // Gets the current modifier mapping and stores them on |out_modifier_map|.
-  bool GetModifierMapping(chromeos::ModifierMap* out_modifier_map) {
-    out_modifier_map->clear();
-
-    // If modifier keys are not remapped, return a map that doesn't change
-    // any key mappings.
-    if (!modifier_keys_are_remapped_) {
-      for (size_t i = 0; i < arraysize(kCustomizableKeys); ++i) {
-        chromeos::ModifierKey key = kCustomizableKeys[i];
-        out_modifier_map->push_back(chromeos::ModifierKeyPair(key, key));
-      }
-      return true;
-    }
-
-    std::string command_output = last_full_layout_name_;
-    if (command_output.empty()) {
-      // Cache is not available. Execute setxkbmap to get the current mapping.
-      if (!ExecuteGetLayoutCommand(&command_output)) {
-        return false;
-      }
-    }
-    if (!chromeos::ExtractModifierMapFromFullXkbLayoutName(
-            command_output, string_to_modifier_map_, out_modifier_map)) {
-      return false;
-    }
-    return true;
-  }
-
   // This function is used by SetLayout() and RemapModifierKeys(). Calls
   // setxkbmap command if needed, and updates the last_full_layout_name_ cache.
   // If |use_version| is false, the function does not add "+version(...)" to the
@@ -243,10 +200,9 @@ class XKeyboard {
       return false;
     }
 
-    // Since executing setxkbmap takes more than 200 ms on EeePC, and this
-    // function is called on every focus-in event, try to reduce the number of
-    // the fork/exec calls.
-    if (last_full_layout_name_ == layouts_to_set) {
+    const std::string current_layout = chromeos::CreateFullXkbLayoutName(
+        current_layout_name_, current_modifier_map_, use_version);
+    if (current_layout == layouts_to_set) {
       DLOG(INFO) << "The requested layout is already set: " << layouts_to_set;
       return true;
     }
@@ -257,106 +213,36 @@ class XKeyboard {
       chromeos::SetCapsLockEnabled(false);
     }
 
-    gint exit_status = -1;
-    const gboolean successful =
-        ExecuteSetLayoutCommand(layouts_to_set, &exit_status);
-
-    // On success, update the cache and return true.
-    if (successful && (exit_status == 0)) {
-      last_full_layout_name_ = layouts_to_set;
-      DLOG(INFO) << "XKB layout is changed to " << layouts_to_set;
-      return true;
-    }
-
-    LOG(ERROR) << "Failed to change XKB layout to: " << layouts_to_set;
-    last_full_layout_name_.clear();  // invalidate the cache.
-    return false;
+    ExecuteSetLayoutCommand(layouts_to_set);
+    return true;
   }
 
-  // Executes 'setxkbmap -layout ...' command. Returns true if execve suceeds.
-  bool ExecuteSetLayoutCommand(const std::string& layouts_to_set,
-                               gint* out_exit_status) {
-    *out_exit_status = -1;
-
-    gchar* argv[] = {
-      g_strdup(kSetxkbmapCommand), g_strdup("-layout"),
-      g_strdup(layouts_to_set.c_str()), NULL
-    };
-    GError* error = NULL;
-    const gboolean successful = g_spawn_sync(NULL, argv, NULL,
-                                             static_cast<GSpawnFlags>(0),
-                                             NULL, NULL, NULL, NULL,
-                                             out_exit_status, &error);
-    for (size_t i = 0; argv[i] != NULL; ++i) {
-      g_free(argv[i]);
-    }
-
-    if (!successful) {
-      if (error && error->message) {
-        LOG(ERROR) << "Failed to execute setxkbmap: " << error->message;
-      }
-      g_error_free(error);
-    }
-    return (successful == TRUE);
+  // Executes 'setxkbmap -layout ...' command asynchronously.
+  // TODO(yusukes): Use libxkbfile.so instead of the command (crosbug.com/13105)
+  void ExecuteSetLayoutCommand(const std::string& layouts_to_set) {
+    chromeos::ProcessImpl process;
+    process.AddArg(kSetxkbmapCommand);
+    process.AddStringOption("-layout", layouts_to_set);
+    if (!process.Start()) {
+      LOG(ERROR) << "Failed to execute setxkbmap: " << layouts_to_set;
+      return;
+    }
+    // g_child_watch_add is necessary to prevent the process from becoming a
+    // zombie.
+    g_child_watch_add(process.pid(),
+                      reinterpret_cast<GChildWatchFunc>(OnSetLayoutFinish),
+                      this);
+    process.Release();  // do not kill the setxkbmap process on function exit.
   }
 
-  // Executes 'setxkbmap -print' command and parses the output (stdout) from the
-  // command. Returns true if both execve and parsing the output succeed.
-  // On success, it stores a string like "us+chromeos(..)+version(..)+inet(..)"
-  // on |out_command_output|.
-  bool ExecuteGetLayoutCommand(std::string* out_command_output) {
-    out_command_output->clear();
-
-    gint exit_status = -1;
-    gchar* argv[] = { g_strdup(kSetxkbmapCommand), g_strdup("-print"), NULL };
-    gchar* raw_command_output = NULL;
-    GError* error = NULL;
-    const gboolean successful = g_spawn_sync(NULL, argv, NULL,
-                                             static_cast<GSpawnFlags>(0),
-                                             NULL, NULL,
-                                             &raw_command_output, NULL,
-                                             &exit_status, &error);
-    for (size_t i = 0; argv[i] != NULL; ++i) {
-      g_free(argv[i]);
-    }
-
-    if (!successful) {
-      if (error && error->message) {
-        LOG(ERROR) << "Failed to execute setxkbmap: " << error->message;
-      }
-      g_error_free(error);
-      return false;
-    }
-
-    // g_spawn_sync succeeded.
-    std::string command_output = raw_command_output ? raw_command_output : "";
-    g_free(raw_command_output);
-    raw_command_output = NULL;  // DO NOT USE |raw_command_output| below.
-
-    if (exit_status != 0) {
-      return false;
-    }
-    // Parse a line like:
-    //   "xkb_symbols { include "pc+us+chromeos(..)+version(..)+inet(pc105)" };"
-    const size_t cursor = command_output.find("pc+");
-    if (cursor == std::string::npos) {
-      LOG(ERROR) << "pc+ is not found: " << command_output;
-      return false;
-    }
-    *out_command_output = command_output.substr(cursor + 3);  // Skip "pc+".
-    return true;
+  static void OnSetLayoutFinish(GPid pid, gint status, XKeyboard* self) {
+    DLOG(INFO) << "OnSetLayoutFinish: pid=" << pid;
   }
 
-  // The XKB layout name which we set last time like
-  // "us+chromeos(search_leftcontrol_leftalt)".
-  std::string last_full_layout_name_;
-
-  // A std::map that holds mappings like: "leftcontrol_disabled_leftalt" ->
-  //   { LEFT_CONTROL_KEY, VOID_KEY, LEFT_ALT_KEY }.
-  chromeos::StringToModifierMap string_to_modifier_map_;
-
-  // True if modifier keys are remapped.
-  bool modifier_keys_are_remapped_;
+  // The XKB layout name which we set last time like "us" and "us(dvorak)".
+  std::string current_layout_name_;
+  // The mapping of modifier keys we set last time.
+  chromeos::ModifierMap current_modifier_map_;
 
   DISALLOW_COPY_AND_ASSIGN(XKeyboard);
 };
@@ -440,76 +326,7 @@ std::string CreateFullXkbLayoutName(const std::string& layout_name,
   return full_xkb_layout_name;
 }
 
-std::string ExtractLayoutNameFromFullXkbLayoutName(
-    const std::string& full_xkb_layout_name) {
-  const size_t next_plus_pos = full_xkb_layout_name.find('+');
-  if (next_plus_pos == std::string::npos) {
-    LOG(ERROR) << "Bad layout name: " << full_xkb_layout_name;
-    return "";
-  }
-  return full_xkb_layout_name.substr(0, next_plus_pos);
-}
-
-void InitializeStringToModifierMap(StringToModifierMap* out_map) {
-  DCHECK(out_map);
-  out_map->clear();
-
-  for (int i = 0; i < static_cast<int>(kNumModifierKeys); ++i) {
-    for (int j = 0; j < static_cast<int>(kNumModifierKeys); ++j) {
-      for (int k = 0; k < static_cast<int>(kNumModifierKeys); ++k) {
-        const std::string string_rep = StringPrintf(
-            "%s_%s_%s",
-            ModifierKeyToString(ModifierKey(i)).c_str(),
-            ModifierKeyToString(ModifierKey(j)).c_str(),
-            ModifierKeyToString(ModifierKey(k)).c_str());
-        ModifierMap modifier_map;
-        modifier_map.push_back(ModifierKeyPair(kSearchKey, ModifierKey(i)));
-        modifier_map.push_back(
-            ModifierKeyPair(kLeftControlKey, ModifierKey(j)));
-        modifier_map.push_back(ModifierKeyPair(kLeftAltKey, ModifierKey(k)));
-        out_map->insert(make_pair(string_rep, modifier_map));
-      }
-    }
-  }
-}
-
-bool ExtractModifierMapFromFullXkbLayoutName(
-    const std::string& full_xkb_layout_name,
-    const StringToModifierMap& string_to_modifier_map,
-    ModifierMap* out_modifier_map) {
-  static const char kMark[] = "+chromeos(";
-  const size_t kMarkLen = strlen(kMark);
-
-  out_modifier_map->clear();
-  const size_t mark_pos = full_xkb_layout_name.find(kMark);
-  if (mark_pos == std::string::npos) {
-    LOG(ERROR) << "Bad layout name: " << full_xkb_layout_name;
-    return false;
-  }
-
-  const std::string tmp =  // e.g. "leftcontrol_disabled_leftalt), us"
-      full_xkb_layout_name.substr(mark_pos + kMarkLen);
-  const size_t next_paren_pos = tmp.find(')');
-  if (next_paren_pos == std::string::npos) {
-    LOG(ERROR) << "Bad layout name: " << full_xkb_layout_name;
-    return false;
-  }
-
-  const std::string modifier_map_string = tmp.substr(0, next_paren_pos);
-  DLOG(INFO) << "Modifier mapping is: " << modifier_map_string;
-
-  StringToModifierMap::const_iterator iter =
-      string_to_modifier_map.find(modifier_map_string);
-  if (iter == string_to_modifier_map.end()) {
-    LOG(ERROR) << "Bad mapping name '" << modifier_map_string
-               << "' in layout name '" << full_xkb_layout_name << "'";
-    return false;
-  }
-
-  *out_modifier_map = iter->second;
-  return true;
-}
-
+// This function is only for unittest.
 bool CapsLockIsEnabled() {
   ScopedDisplay display(XOpenDisplay(NULL));
   if (!display.get()) {
@@ -520,6 +337,7 @@ bool CapsLockIsEnabled() {
   return status.locked_mods & LockMask;
 }
 
+// TODO(yusukes): Call this function in non-UI thread or in an idle callback.
 void SetCapsLockEnabled(bool enable_caps_lock) {
   ScopedDisplay display(XOpenDisplay(NULL));
   if (!display.get()) {
@@ -555,16 +373,20 @@ bool ChromeOSRemapModifierKeys(const chromeos::ModifierMap& modifier_map) {
   return XKeyboard::Get()->RemapModifierKeys(modifier_map);
 }
 
+// TODO(yusukes): Remove this function.
 extern "C"
 bool ChromeOSGetAutoRepeatEnabled(bool* enabled) {
   return XKeyboard::Get()->GetAutoRepeatEnabled(enabled);
 }
 
+// TODO(yusukes): We can remove this function since the default setting of the
+// repeat mode is true, and we don't change the default.
 extern "C"
 bool ChromeOSSetAutoRepeatEnabled(bool enabled) {
   return XKeyboard::Get()->SetAutoRepeatEnabled(enabled);
 }
 
+// TODO(yusukes): Remove this function.
 extern "C"
 bool ChromeOSGetAutoRepeatRate(chromeos::AutoRepeatRate* out_rate) {
   return XKeyboard::Get()->GetAutoRepeatRate(out_rate);