Fix UI-thread blocking issue in SetImeConfig

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 9b5316f1da56f338474c57627fef23ab16421d1f..bed135497a9b428f57af71c37832a8fd1fd42d93 100644
--- a/chrome/browser/chromeos/cros/input_method_library.cc
+++ b/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);
@@ -160,7 +163,10 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
 
   bool SetImeConfig(const char* section, const char* config_name,
                     const ImeConfigValue& value) {
-    MaybeStartOrStopInputMethodProcesses(section, config_name, value);
+    // Start input method process if necessary. We don't stop the process here
+    // since it can lead the successive FlushImeConfig() call to fail.
+    MaybeStartOrStopInputMethodProcesses(
+        section, config_name, value, false /* auto_ime_shutdown */);

[satorux1, 2011/01/05 06:52:31]

This is a bit hard to read. I think it would be easier to read if we have two
separate functions:

MaybeStartInputMethodProcesses(section, config_name, value);
MaybeStopInputMethodProcesses(section, config_name, value);

[Yusuke Sato, 2011/01/05 10:05:55]

Done.

 
     const ConfigKeyType key = std::make_pair(section, config_name);
     current_config_values_[key] = value;
@@ -168,6 +174,10 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
       pending_config_requests_[key] = value;
       FlushImeConfig();
     }
+
+    // Stop input method process if necessary.
+    MaybeStartOrStopInputMethodProcesses(
+        section, config_name, value, enable_auto_ime_shutdown_);
     return pending_config_requests_.empty();
   }
 
@@ -187,7 +197,8 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
   void MaybeStartOrStopInputMethodProcesses(
       const char* section,
       const char* config_name,
-      const ImeConfigValue& value) {
+      const ImeConfigValue& value,
+      bool auto_ime_shutdown) {
     if (!strcmp(language_prefs::kGeneralSectionName, section) &&

[satorux1, 2011/01/05 06:52:31]

You might want to change the parameters to const string& so you can use ==. If
you do, you probably want to change SetImeConfig() and GetImeConfig(). Other
functions seem to use const string& rather than const char*.

[Yusuke Sato, 2011/01/05 10:05:55]

Done.

         !strcmp(language_prefs::kPreloadEnginesConfigName, config_name)) {
       if (EnsureLoadedAndStarted()) {
@@ -197,7 +208,7 @@ class InputMethodLibraryImpl : public InputMethodLibrary,
             value.string_list_value.size() == 1 &&
             value.string_list_value[0] ==
             chromeos::GetHardwareKeyboardLayoutName()) {
-          if (enable_auto_ime_shutdown_)
+          if (auto_ime_shutdown)
             StopInputMethodProcesses();
         } else if (!defer_ime_startup_) {
           StartInputMethodProcesses();
@@ -304,7 +315,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";