Fix UI-thread blocking issue in SetImeConfig

chrome/browser/chromeos/cros/input_method_library.cc

 // Copyright (c) 2010 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"
@@ skipping 45 matching lines @@
       : 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),
         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);
     if (CrosLibrary::Get()->EnsureLoaded()) {
       current_input_method_id_ = chromeos::GetHardwareKeyboardLayoutName();
     }
     // Observe APP_EXITING to stop input method processes gracefully.
     // Note that even if we fail to stop input method processes from
     // Chrome in case of a sudden crash, we have a way to do it from an
     // upstart script. See crosbug.com/6515 and crosbug.com/6995 for
@@ skipping 77 matching lines @@
     bool success = false;
     if (EnsureLoadedAndStarted()) {
       success = chromeos::GetImeConfig(input_method_status_connection_,
                                        section, config_name, out_value);
     }
     return success;
   }
 
   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;
     if (ime_connected_) {
       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();
   }
 
   virtual const InputMethodDescriptor& previous_input_method() const {
     return previous_input_method_;
   }
   virtual const InputMethodDescriptor& current_input_method() const {
     return current_input_method_;
   }
 
   virtual const ImePropertyList& current_ime_properties() const {
     return current_ime_properties_;
   }
 
  private:
   // Starts or stops the input method processes based on the current state.

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

Please add comments about parameters using this opportunity.

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

Done.

   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()) {
         // If there are no input methods other than one for the hardware
         // keyboard, we'll stop the input method processes.
         if (value.type == ImeConfigValue::kValueTypeStringList &&
             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();
         }
         chromeos::SetActiveInputMethods(input_method_status_connection_, value);
       }
     }
   }
 
   // Changes the current input method to |input_method_id|. If the id is not in
@@ skipping 86 matching lines @@
                                                   current_input_method_,
                                                   num_active_input_methods));
     }
   }
 
   static void InputMethodChangedHandler(
       void* object,
       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";
       return;
     }
 
     InputMethodLibraryImpl* input_method_library =
         static_cast<InputMethodLibraryImpl*>(object);
     input_method_library->ChangeCurrentInputMethod(current_input_method);
   }
@@ skipping 261 matching lines @@
   // This is used to register this object to APP_EXITING notification.
   NotificationRegistrar notification_registrar_;
 
   // True if we should launch the input method processes.
   bool should_launch_ime_;
   // True if the connection to the IBus daemon is alive.
   bool ime_connected_;
   // If true, we'll defer the startup until a non-default method is
   // activated.
   bool defer_ime_startup_;
   bool enable_auto_ime_shutdown_;

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

Could you write a comment about this?

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

Done.

   // The ID of the current input method (ex. "mozc").
   std::string current_input_method_id_;
   // True if we should change the input method 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 id of the candidate window. 0 if it's not running.
@@ skipping 208 matching lines @@
     return new InputMethodLibraryStubImpl();
   else
     return new InputMethodLibraryImpl();
 }
 
 }  // 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);