Remove g_spawn_async. Use base/process_utils.h instead.

chrome/browser/chromeos/cros/input_method_library.cc

Index: chrome/browser/chromeos/cros/input_method_library.cc
diff --git a/chrome/browser/chromeos/cros/input_method_library.cc b/chrome/browser/chromeos/cros/input_method_library.cc
index d02f23397d017603845fa31a251a8e2e1f5fc196..9f6027622166af1f2cd50fc5aa792da0498cd648 100644
--- a/chrome/browser/chromeos/cros/input_method_library.cc
+++ b/chrome/browser/chromeos/cros/input_method_library.cc
@@ -5,12 +5,13 @@
 #include "chrome/browser/chromeos/cros/input_method_library.h"
 
 #include <glib.h>
-#include <signal.h>
 
 #include "unicode/uloc.h"
 
 #include "base/basictypes.h"
 #include "base/message_loop.h"
+#include "base/process_util.h"
+#include "base/string_split.h"
 #include "base/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
@@ -62,7 +63,7 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
         defer_ime_startup_(false),
         enable_auto_ime_shutdown_(true),
         should_change_input_method_(false),
-        ibus_daemon_process_id_(0),
+        ibus_daemon_process_handle_(base::kNullProcessHandle),
         initialized_successfully_(false),
         candidate_window_controller_(NULL) {
     // Here, we use the fallback input method descriptor but
@@ -170,7 +171,7 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
     tentative_current_input_method_id_ = input_method_id;
     // If the input method daemon is not running and the specified input
     // method is a keyboard layout, switch the keyboard directly.
-    if (ibus_daemon_process_id_ == 0 &&
+    if (ibus_daemon_process_handle_ == base::kNullProcessHandle &&
         chromeos::input_method::IsKeyboardLayout(input_method_id)) {
       // We shouldn't use SetCurrentKeyboardLayoutByName() here. See
       // comments at ChangeCurrentInputMethod() for details.
@@ -597,39 +598,33 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
   }
 
   // Launches an input method procsess specified by the given command
-  // line. On success, returns true and stores the process ID in
-  // |process_id|. Otherwise, returns false, and the contents of
-  // |process_id| is untouched. OnImeShutdown will be called when the
+  // line. On success, returns true and stores the process handle in
+  // |process_handle|. Otherwise, returns false, and the contents of
+  // |process_handle| is untouched. OnImeShutdown will be called when the
   // process terminates.
   bool LaunchInputMethodProcess(const std::string& command_line,
-                                int* process_id) {
-    GError *error = NULL;
-    gchar **argv = NULL;
-    gint argc = NULL;
-    // TODO(zork): export "LD_PRELOAD=/usr/lib/libcrash.so"
-    if (!g_shell_parse_argv(command_line.c_str(), &argc, &argv, &error)) {
-      LOG(ERROR) << "Could not parse command: " << error->message;
-      g_error_free(error);
-      return false;
-    }
+                                base::ProcessHandle* process_handle) {
+    std::vector<std::string> argv;
+    base::file_handle_mapping_vector fds_to_remap;
+    base::ProcessHandle handle = base::kNullProcessHandle;
 
-    int pid = 0;
-    const GSpawnFlags kFlags = G_SPAWN_DO_NOT_REAP_CHILD;
-    const gboolean result = g_spawn_async(NULL, argv, NULL,
-                                          kFlags, NULL, NULL,
-                                          &pid, &error);
-    g_strfreev(argv);
+    // TODO(zork): export "LD_PRELOAD=/usr/lib/libcrash.so"
+    base::SplitString(command_line, ' ', &argv);
+    const bool result = base::LaunchApp(argv,
+                                        fds_to_remap,  // no remapping
+                                        false,  // wait
+                                        &handle);
     if (!result) {
-      LOG(ERROR) << "Could not launch: " << command_line << ": "
-                 << error->message;
-      g_error_free(error);
+      LOG(ERROR) << "Could not launch: " << command_line;
       return false;
     }
-    g_child_watch_add(pid, reinterpret_cast<GChildWatchFunc>(OnImeShutdown),
+    g_child_watch_add(base::GetProcId(handle),

[Zachary Kuznia, 2011/03/21 10:43:52]

Perhaps we should add a version of this function to process utils, or add a TODO
to switch if one is written?  I think it was the lack of this function that
initially caused me to use glib's function instead.

[satorux1, 2011/03/21 11:52:11]

I agree. Let's add a TODO here.

[Yusuke Sato, 2011/03/21 12:08:17]

Sure, added TODO.

On 2011/03/21 10:43:52, Zachary Kuznia wrote:

[Yusuke Sato, 2011/03/21 12:08:17]

Done.

+                      reinterpret_cast<GChildWatchFunc>(OnImeShutdown),
                       this);
 
-    *process_id = pid;
-    VLOG(1) << command_line << " (PID=" << pid << ") is started";
+    *process_handle = handle;
+    VLOG(1) << command_line << " (PID=" << base::GetProcId(handle)
+            << ") is started";
     return  true;
   }
 
@@ -651,13 +646,13 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
       }
     }
 
-    if (ibus_daemon_process_id_ == 0) {
+    if (ibus_daemon_process_handle_ == base::kNullProcessHandle) {
       // TODO(zork): Send output to /var/log/ibus.log
       const std::string ibus_daemon_command_line =
           StringPrintf("%s --panel=disable --cache=none --restart --replace",
                        kIBusDaemonPath);
       if (!LaunchInputMethodProcess(
-              ibus_daemon_command_line, &ibus_daemon_process_id_)) {
+              ibus_daemon_command_line, &ibus_daemon_process_handle_)) {
         LOG(ERROR) << "Failed to launch " << ibus_daemon_command_line;
       }
     }
@@ -667,9 +662,9 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
   static void OnImeShutdown(int pid,
                             int status,
                             InputMethodLibraryImpl* library) {
-    g_spawn_close_pid(pid);
-    if (library->ibus_daemon_process_id_ == pid) {
-      library->ibus_daemon_process_id_ = 0;
+    if (base::GetProcId(library->ibus_daemon_process_handle_) == pid) {

[Zachary Kuznia, 2011/03/21 10:43:52]

Should we check that we have a handle before calling this, or is it safe with a
null handle?

[Yusuke Sato, 2011/03/21 12:08:17]

Added the check.
(right now, calling GetProcId(kNullProcessHandle) is safe, but just in case.)

On 2011/03/21 10:43:52, Zachary Kuznia wrote:

+      base::CloseProcessHandle(library->ibus_daemon_process_handle_);
+      library->ibus_daemon_process_handle_ = base::kNullProcessHandle;
     }
 
     // Restart input method daemon if needed.
@@ -684,15 +679,17 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
       return;
 
     should_launch_ime_ = false;
-    if (ibus_daemon_process_id_) {
+    if (ibus_daemon_process_handle_ != base::kNullProcessHandle) {
       if (!chromeos::StopInputMethodProcess(input_method_status_connection_)) {
         LOG(ERROR) << "StopInputMethodProcess IPC failed. Sending SIGTERM to "
-                   << "PID " << ibus_daemon_process_id_;
-        kill(ibus_daemon_process_id_, SIGTERM);
+                   << "PID " << base::GetProcId(ibus_daemon_process_handle_);
+        base::KillProcess(ibus_daemon_process_handle_, -1, false /* wait */);
       }
-      VLOG(1) << "ibus-daemon (PID=" << ibus_daemon_process_id_ << ") is "
-              << "terminated";
-      ibus_daemon_process_id_ = 0;
+      VLOG(1) << "ibus-daemon (PID="
+              << base::GetProcId(ibus_daemon_process_handle_)
+              << ") is terminated";
+      base::CloseProcessHandle(ibus_daemon_process_handle_);

[Zachary Kuznia, 2011/03/21 10:43:52]

Shouldn't this already be called in OnImeShutdown, or do we need to call it
twice?

[Yusuke Sato, 2011/03/21 12:08:17]

Let me remove all CloseProcessHandle calls since base/process_utils.h explicitly
says that the function is NOP on Linux.

On 2011/03/21 10:43:52, Zachary Kuznia wrote:

+      ibus_daemon_process_handle_ = base::kNullProcessHandle;
     }
   }
 
@@ -765,9 +762,9 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
   // requests becomes empty.
   bool should_change_input_method_;
 
-  // The process id of the IBus daemon. 0 if it's not running. The process
-  // ID 0 is not used in Linux, hence it's safe to use 0 for this purpose.
-  int ibus_daemon_process_id_;
+  // The process handle of the IBus daemon. kNullProcessHandle if it's not
+  // running.
+  base::ProcessHandle ibus_daemon_process_handle_;
 
   // True if initialization is successfully done, meaning that libcros is
   // loaded and input method status monitoring is started. This value