Fix UI-thread blocking issue in SetImeConfig.

Do not kill ibus-daemon before FlushImeConfig() since it can lead the ibus_config_set_value() call to block for 25 seconds (i.e. GDBus default timeout) when ibus-1.4 and GDBus libraries are in use.

BUG=chromium-os:9685
TEST=manually with ibus-1.3 and 1.4.

Review URL: http://codereview.chromium.org/6032005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@70598 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/cros/input_method_library.cc

@@ -63,6 +63,9 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
         should_change_input_method_(false),
         ibus_daemon_process_id_(0),
         candidate_window_process_id_(0) {
+    // TODO(yusukes): Using both CreateFallbackInputMethodDescriptors and
+    // chromeos::GetHardwareKeyboardLayoutName doesn't look clean. Probably
+    // we should unify these APIs.
     scoped_ptr<InputMethodDescriptors> input_method_descriptors(
         CreateFallbackInputMethodDescriptors());
     current_input_method_ = input_method_descriptors->at(0);
@@ -148,19 +151,22 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
     return false;
   }
 
-  bool GetImeConfig(const char* section, const char* config_name,
+  bool GetImeConfig(const std::string& section, const std::string& config_name,
                     ImeConfigValue* out_value) {
     bool success = false;
     if (EnsureLoadedAndStarted()) {
       success = chromeos::GetImeConfig(input_method_status_connection_,
-                                       section, config_name, out_value);
+                                       section.c_str(),
+                                       config_name.c_str(),
+                                       out_value);
     }
     return success;
   }
 
-  bool SetImeConfig(const char* section, const char* config_name,
+  bool SetImeConfig(const std::string& section, const std::string& config_name,
                     const ImeConfigValue& value) {
-    MaybeStartOrStopInputMethodProcesses(section, config_name, value);
+    // Before calling FlushImeConfig(), start input method process if necessary.
+    MaybeStartInputMethodProcesses(section, config_name, value);
 
     const ConfigKeyType key = std::make_pair(section, config_name);
     current_config_values_[key] = value;
@@ -168,6 +174,9 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
       pending_config_requests_[key] = value;
       FlushImeConfig();
     }
+
+    // Stop input method process if necessary.
+    MaybeStopInputMethodProcesses(section, config_name, value);
     return pending_config_requests_.empty();
   }
 
@@ -183,24 +192,52 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
   }
 
  private:
-  // Starts or stops the input method processes based on the current state.
-  void MaybeStartOrStopInputMethodProcesses(
-      const char* section,
-      const char* config_name,
-      const ImeConfigValue& value) {
-    if (!strcmp(language_prefs::kGeneralSectionName, section) &&
-        !strcmp(language_prefs::kPreloadEnginesConfigName, config_name)) {
+  // Starts input method processes based on the |defer_ime_startup_| flag and
+  // input method configuration being updated. |section| is a section name of
+  // the input method configuration (e.g. "general", "general/hotkey").
+  // |config_name| is a name of the configuration (e.g. "preload_engines",
+  // "previous_engine"). |value| is the configuration value to be set.
+  void MaybeStartInputMethodProcesses(const std::string& section,
+                                      const std::string& config_name,
+                                      const ImeConfigValue& value) {
+    if (section == language_prefs::kGeneralSectionName &&
+        config_name == language_prefs::kPreloadEnginesConfigName) {
+      if (EnsureLoadedAndStarted()) {
+        const std::string hardware_layout_name =
+            chromeos::GetHardwareKeyboardLayoutName();  // e.g. "xkb:us::eng"
+        if (!(value.type == ImeConfigValue::kValueTypeStringList &&
+              value.string_list_value.size() == 1 &&
+              value.string_list_value[0] == hardware_layout_name) &&
+            !defer_ime_startup_) {
+          // If there are no input methods other than one for the hardware
+          // keyboard, we don't start the input method processes.
+          // When |defer_ime_startup_| is true, we don't start it either.
+          StartInputMethodProcesses();
+        }
+        chromeos::SetActiveInputMethods(input_method_status_connection_, value);
+      }
+    }
+  }
+
+  // Stops input method processes based on the |enable_auto_ime_shutdown_| flag
+  // and input method configuration being updated.
+  // See also: MaybeStartInputMethodProcesses().
+  void MaybeStopInputMethodProcesses(const std::string& section,
+                                     const std::string& config_name,
+                                     const ImeConfigValue& value) {
+    if (section == language_prefs::kGeneralSectionName &&
+        config_name == language_prefs::kPreloadEnginesConfigName) {
       if (EnsureLoadedAndStarted()) {
-        // If there are no input methods other than one for the hardware
-        // keyboard, we'll stop the input method processes.
+        const std::string hardware_layout_name =
+            chromeos::GetHardwareKeyboardLayoutName();  // e.g. "xkb:us::eng"
         if (value.type == ImeConfigValue::kValueTypeStringList &&
             value.string_list_value.size() == 1 &&
-            value.string_list_value[0] ==
-            chromeos::GetHardwareKeyboardLayoutName()) {
-          if (enable_auto_ime_shutdown_)
-            StopInputMethodProcesses();
-        } else if (!defer_ime_startup_) {
-          StartInputMethodProcesses();
+            value.string_list_value[0] == hardware_layout_name &&
+            enable_auto_ime_shutdown_) {
+          // If there are no input methods other than one for the hardware
+          // keyboard, and |enable_auto_ime_shutdown_| is true, we'll stop the
+          // input method processes.
+          StopInputMethodProcesses();
         }
         chromeos::SetActiveInputMethods(input_method_status_connection_, value);
       }
@@ -304,7 +341,7 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
       const chromeos::InputMethodDescriptor& current_input_method) {
     // The handler is called when the input method method change is
     // notified via a DBus connection. Since the DBus notificatiosn are
-    // handled in the UI thread, we can assume that this functionalways
+    // handled in the UI thread, we can assume that this function always
     // runs on the UI thread, but just in case.
     if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
       LOG(ERROR) << "Not on UI thread";
@@ -586,6 +623,8 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
   // If true, we'll defer the startup until a non-default method is
   // activated.
   bool defer_ime_startup_;
+  // True if we should stop input method processes when there are no input
+  // methods other than one for the hardware keyboard.
   bool enable_auto_ime_shutdown_;
   // The ID of the current input method (ex. "mozc").
   std::string current_input_method_id_;
@@ -637,14 +676,14 @@ class InputMethodLibraryStubImpl : public InputMethodLibrary {
     return true;
   }
 
-  bool GetImeConfig(const char* section,
-                    const char* config_name,
+  bool GetImeConfig(const std::string& section,
+                    const std::string& config_name,
                     ImeConfigValue* out_value) {
     return false;
   }
 
-  bool SetImeConfig(const char* section,
-                    const char* config_name,
+  bool SetImeConfig(const std::string& section,
+                    const std::string& config_name,
                     const ImeConfigValue& value) {
     return false;
   }

chrome/browser/chromeos/cros/input_method_library.h

@@ -84,9 +84,9 @@ class InputMethodLibrary {
   // |out_value|. Returns true if |out_value| is successfully updated.
   // When you would like to retrieve 'panel/custom_font', |section| should
   // be "panel", and |config_name| should be "custom_font".
-  virtual bool GetImeConfig(
-      const char* section, const char* config_name,
-      ImeConfigValue* out_value) = 0;
+  virtual bool GetImeConfig(const std::string& section,
+                            const std::string& config_name,
+                            ImeConfigValue* out_value) = 0;
 
   // Updates a configuration of ibus-daemon or IBus engines with |value|.
   // Returns true if the configuration (and all pending configurations, if any)
@@ -97,8 +97,8 @@ class InputMethodLibrary {
   // Notice: This function might call the Observer::ActiveInputMethodsChanged()
   // callback function immediately, before returning from the SetImeConfig
   // function. See also http://crosbug.com/5217.
-  virtual bool SetImeConfig(const char* section,
-                            const char* config_name,
+  virtual bool SetImeConfig(const std::string& section,
+                            const std::string& config_name,
                             const ImeConfigValue& value) = 0;
 
   // Sets the IME state to enabled, and launches its processes if needed.

chrome/browser/chromeos/cros/mock_input_method_library.h

@@ -27,8 +27,9 @@ class MockInputMethodLibrary : public InputMethodLibrary {
   MOCK_METHOD1(ChangeInputMethod, void(const std::string&));
   MOCK_METHOD2(SetImePropertyActivated, void(const std::string&, bool));
   MOCK_METHOD1(InputMethodIsActivated, bool(const std::string&));
-  MOCK_METHOD3(GetImeConfig, bool(const char*, const char*, ImeConfigValue*));
-  MOCK_METHOD3(SetImeConfig, bool(const char*, const char*,
+  MOCK_METHOD3(GetImeConfig, bool(const std::string&, const std::string&,
+                                  ImeConfigValue*));
+  MOCK_METHOD3(SetImeConfig, bool(const std::string&, const std::string&,
                                   const ImeConfigValue&));
   MOCK_CONST_METHOD0(previous_input_method, const InputMethodDescriptor&(void));
   MOCK_CONST_METHOD0(current_input_method, const InputMethodDescriptor&(void));