Remove g_spawn_async. Use base/process_utils.h instead.

chrome/browser/chromeos/cros/input_method_library.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/chromeos/cros/keyboard_library.h"
 #include "chrome/browser/chromeos/input_method/candidate_window.h"
 #include "chrome/browser/chromeos/input_method/input_method_util.h"
 #include "chrome/browser/chromeos/language_preferences.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/notification_observer.h"
 #include "content/common/notification_registrar.h"
@@ skipping 31 matching lines @@
  public:
   InputMethodLibraryImpl()
       : input_method_status_connection_(NULL),
         previous_input_method_("", "", "", ""),
         current_input_method_("", "", "", ""),
         should_launch_ime_(false),
         ime_connected_(false),
         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
     // |current_input_method_| will be updated as soon as the login screen
     // is shown or the user is logged in, so there is no problem.
     current_input_method_ = input_method::GetFallbackInputMethodDescriptor();
     active_input_method_ids_.push_back(current_input_method_.id);
     // Observe APP_TERMINATING to stop input method daemon gracefully.
     // We should not use APP_EXITING here since logout might be canceled by
     // JavaScript after APP_EXITING is sent (crosbug.com/11055).
@@ skipping 87 matching lines @@
     return chromeos::GetSupportedInputMethodDescriptors();
   }
 
   virtual void ChangeInputMethod(const std::string& input_method_id) {
     // Changing the input method isn't guaranteed to succeed here, but we
     // should remember the last one regardless. See comments in
     // FlushImeConfig() for details.
     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.
       ChangeCurrentInputMethodFromId(input_method_id);
     } else {
       // Otherwise, start the input method daemon, and change the input
       // method via the damon.
       StartInputMethodDaemon();
       // ChangeInputMethodViaIBus() fails if the IBus daemon is not
       // ready yet. In this case, we'll defer the input method change
@@ skipping 406 matching lines @@
   }
 
   // Updates the properties used by the current input method.
   void UpdateProperty(const ImePropertyList& prop_list) {
     for (size_t i = 0; i < prop_list.size(); ++i) {
       FindAndUpdateProperty(prop_list[i], &current_ime_properties_);
     }
   }
 
   // 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;
+                                base::ProcessHandle* process_handle) {
+    std::vector<std::string> argv;
+    base::file_handle_mapping_vector fds_to_remap;
+    base::ProcessHandle handle = base::kNullProcessHandle;
+
     // 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);
+    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;
       return false;
     }
-
-    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);
-    if (!result) {
-      LOG(ERROR) << "Could not launch: " << command_line << ": "
-                 << error->message;
-      g_error_free(error);
-      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;
   }
 
   // Launches input method daemon if these are not yet running.
   void MaybeLaunchInputMethodDaemon() {
     // CandidateWindowController requires libcros to be loaded. Besides,
     // launching ibus-daemon without libcros loaded doesn't make sense.
     if (!initialized_successfully_)
       return;
 
     if (!should_launch_ime_) {
       return;
     }
 
     if (!candidate_window_controller_.get()) {
       candidate_window_controller_.reset(new CandidateWindowController);
       if (!candidate_window_controller_->Init()) {
         LOG(WARNING) << "Failed to initialize the candidate window controller";
       }
     }
 
-    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;
       }
     }
   }
 
   // Called when the input method process is shut down.
   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.
     library->MaybeLaunchInputMethodDaemon();
   }
 
   // Stops the backend input method daemon. This function should also be
   // called from MaybeStopInputMethodDaemon(), except one case where we
   // stop the input method daemon at Chrome shutdown in Observe().
   void StopInputMethodDaemon() {
     if (!initialized_successfully_)
       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;
     }
   }
 
   void SetDeferImeStartup(bool defer) {
     VLOG(1) << "Setting DeferImeStartup to " << defer;
     defer_ime_startup_ = defer;
   }
 
   void SetEnableAutoImeShutdown(bool enable) {
     enable_auto_ime_shutdown_ = enable;
@@ skipping 52 matching lines @@
   bool enable_auto_ime_shutdown_;
   // The ID of the tentative current input method (ex. "mozc"). This value
   // can be different from the actual current input method, if
   // ChangeInputMethod() fails.
   std::string tentative_current_input_method_id_;
   // True if we should change the current input method to
   // |tentative_current_input_method_id_| once the queue of the pending config
   // 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
   // should be checked where we call libcros functions.
   bool initialized_successfully_;
 
   // The candidate window.  This will be deleted when the APP_TERMINATING
   // message is sent.
   scoped_ptr<CandidateWindowController> candidate_window_controller_;
 
@@ skipping 319 matching lines @@
     }
     return impl;
   }
 }
 
 }  // namespace chromeos
 
 // Allows InvokeLater without adding refcounting. This class is a Singleton and
 // won't be deleted until it's last InvokeLater is run.
 DISABLE_RUNNABLE_METHOD_REFCOUNT(chromeos::InputMethodLibraryImpl);