Refactoring for InputMethodEngine and InputMethodEventRouter.

chrome/browser/chromeos/input_method/input_method_manager_impl.cc

 // Copyright (c) 2012 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/input_method/input_method_manager_impl.h"
 
 #include <algorithm>  // std::find
 
 #include "ash/ime/input_method_menu_item.h"
 #include "ash/ime/input_method_menu_manager.h"
@@ skipping 313 matching lines @@
       DVLOG(1) << "Can't change the current input method to "
                << input_method_id << " since the engine is not enabled. "
                << "Switch to " << input_method_id_to_switch << " instead.";
     }
   }
 
   // Hide candidate window and info list.
   if (candidate_window_controller_.get())
     candidate_window_controller_->Hide();
 
-  // Disable the current engine handler.
-  IMEEngineHandlerInterface* engine =
-      IMEBridge::Get()->GetCurrentEngineHandler();
-  if (engine)
-    engine->Disable();
-
-  // Configure the next engine handler.
-  if (InputMethodUtil::IsKeyboardLayout(input_method_id_to_switch) &&
-      !extension_ime_util::IsKeyboardLayoutExtension(
-          input_method_id_to_switch)) {
-    IMEBridge::Get()->SetCurrentEngineHandler(NULL);
-  } else {
-    IMEEngineHandlerInterface* next_engine =
-        profile_engine_map_[GetProfile()][input_method_id_to_switch];
-    if (next_engine) {
-      IMEBridge::Get()->SetCurrentEngineHandler(next_engine);
-      next_engine->Enable();
-    }
-  }
-
   // TODO(komatsu): Check if it is necessary to perform the above routine
   // when the current input method is equal to |input_method_id_to_swich|.
   if (current_input_method_.id() != input_method_id_to_switch) {
     // Clear property list.  Property list would be updated by
     // extension IMEs via InputMethodEngine::(Set|Update)MenuItems.
     // If the current input method is a keyboard layout, empty
     // properties are sufficient.
     const ash::ime::InputMethodMenuItemList empty_menu_item_list;
     ash::ime::InputMethodMenuManager* input_method_menu_manager =
         ash::ime::InputMethodMenuManager::GetInstance();
@@ skipping 10 matching lines @@
           util_.GetInputMethodDescriptorFromId(input_method_id_to_switch);
       if (!descriptor)
         LOG(ERROR) << "Unknown input method id: " << input_method_id_to_switch;
     }
     DCHECK(descriptor);
 
     previous_input_method_ = current_input_method_;
     current_input_method_ = *descriptor;
   }
 
+  // Disable the current engine handler.
+  IMEEngineHandlerInterface* engine =
+      IMEBridge::Get()->GetCurrentEngineHandler();
+  if (engine)
+    engine->Disable();
+
+  // Configure the next engine handler.
+  // This must be after |current_input_method_| has been set to new input
+  // method, because engine's Enable() method needs to access it.
+  std::string extension_id =

[Yuki, 2014/08/05 07:57:18]

Please use const references.
const std::string& extension_id = ...;

[Shu Chen, 2014/08/05 14:04:31]

Done.

+      extension_ime_util::GetExtensionIDFromInputMethodID(
+          input_method_id_to_switch);
+  std::string engine_id = extension_ime_util::GetEngineIDByInputMethodID(

[Yuki, 2014/08/05 07:57:18]

Ditto.

[Shu Chen, 2014/08/05 14:04:31]

Done.

+      input_method_id_to_switch);
+  engine = engine_map_[extension_id];
+  IMEBridge::Get()->SetCurrentEngineHandler(engine);
+  if (engine)
+    engine->Enable(engine_id);

[Yuki, 2014/08/05 07:57:18]

I'm uneasy with this part of code (and InputMethodEngine::Initialize and Enable)
because of naming.

"ID" means an unique value corresponding to something, and it must be immutable.
 So, this code looks very strange to me.

  engine = engine_map_[extension_id];
  // You got an |engine| from the |extension_id|.
  // Then, the engine should know the ID of itself.
  if (engine)
    engine->Enable(engine_id);
    // ???
    // Why the engine needs to be set the ID?
    // Is the ID unique to the engine?  Or not?
    // Can we give a different ID to the engine?

The original design and naming seem reasonable.  It was ID.  The ID was given at
Initialize and never change.  Engine IDs were unique to engines.  That's the ID.

If a single engine can be enabled with different IDs, we shouldn't call it
"engine id".  If it's unique to engines, then we shouldn't give the id to
Enable.

The design and naming in your CL is confusing to me.
I'm also confused by what "engine id" is.  There seem extension id, input method
id, and engine id.  Do we really need all of them?

[Shu Chen, 2014/08/05 14:04:31]

Sorry for confusion. I've changed the naming engine_id to component_id.
This cl is to make InputMethodEngine extension-based instead of input_component
based.
So One InputMethodEngine maps one IME extension. So engine->Enable() needs to
provide which input_component it's going to enable.

+
   // Change the keyboard layout to a preferred layout for the input method.
   if (!keyboard_->SetCurrentKeyboardLayoutByName(
           current_input_method_.GetPreferredKeyboardLayout())) {
     LOG(ERROR) << "Failed to change keyboard layout to "
                << current_input_method_.GetPreferredKeyboardLayout();
   }
 
   // Update input method indicators (e.g. "US", "DV") in Chrome windows.
   FOR_EACH_OBSERVER(InputMethodManager::Observer,
                     observers_,
@@ skipping 32 matching lines @@
         IMEBridge::Get()->GetCurrentEngineHandler();
     if (engine)
       engine->PropertyActivate(key);
     return;
   }
 
   DVLOG(1) << "ActivateInputMethodMenuItem: unknown key: " << key;
 }
 
 void InputMethodManagerImpl::AddInputMethodExtension(
-    const std::string& id,
+    const std::string& extension_id,
+    const InputMethodDescriptors& descriptors,
     InputMethodEngineInterface* engine) {
   if (state_ == STATE_TERMINATING)
     return;
 
   DCHECK(engine);
 
-  profile_engine_map_[GetProfile()][id] = engine;
+  engine_map_[extension_id] = engine;
 
-  if (id == current_input_method_.id()) {
+  if (extension_id == extension_ime_util::GetExtensionIDFromInputMethodID(
+      current_input_method_.id())) {
     IMEBridge::Get()->SetCurrentEngineHandler(engine);
-    engine->Enable();
+    engine->Enable(extension_ime_util::GetEngineIDByInputMethodID(
+        current_input_method_.id()));
   }
 
-  if (extension_ime_util::IsComponentExtensionIME(id))
-    return;
+  bool contain = false;
+  for (size_t i = 0; i < descriptors.size(); i++) {
+    const InputMethodDescriptor& descriptor = descriptors[i];
+    const std::string& id = descriptor.id();
+    extra_input_methods_[id] = descriptor;
+    if (Contains(enabled_extension_imes_, id)) {
+      if (!Contains(active_input_method_ids_, id)) {
+        active_input_method_ids_.push_back(id);
+      } else {
+        DVLOG(1) << "AddInputMethodExtension: already added: "
+                 << id << ", " << descriptor.name();
+      }
+      contain = true;
+    }
+  }
 
-  CHECK(extension_ime_util::IsExtensionIME(id))
-      << id << "is not a valid extension input method ID";
-
-  const InputMethodDescriptor& descriptor = engine->GetDescriptor();
-  extra_input_methods_[id] = descriptor;
-
-  if (Contains(enabled_extension_imes_, id)) {
-    if (!Contains(active_input_method_ids_, id)) {
-      active_input_method_ids_.push_back(id);
-    } else {
-      DVLOG(1) << "AddInputMethodExtension: alread added: "
-               << id << ", " << descriptor.name();
-    }
-
-    // Ensure that the input method daemon is running.
+  // Ensure that the input method daemon is running.
+  if (contain)
     MaybeInitializeCandidateWindowController();
-  }
 }
 
-void InputMethodManagerImpl::RemoveInputMethodExtension(const std::string& id) {
-  if (!extension_ime_util::IsExtensionIME(id))
-    DVLOG(1) << id << " is not a valid extension input method ID.";
+void InputMethodManagerImpl::RemoveInputMethodExtension(
+    const std::string& extension_id) {
+  // Removes the active input methods with |extension_id|.
+  std::vector<std::string> new_active_input_method_ids_;
+  for (size_t i = 0; i < active_input_method_ids_.size(); ++i) {
+    if (extension_id !=
+        extension_ime_util::GetExtensionIDFromInputMethodID(
+            active_input_method_ids_[i]))
+      new_active_input_method_ids_.push_back(active_input_method_ids_[i]);
+  }
+  active_input_method_ids_.swap(new_active_input_method_ids_);
 
-  std::vector<std::string>::iterator i = std::find(
-      active_input_method_ids_.begin(), active_input_method_ids_.end(), id);
-  if (i != active_input_method_ids_.end())
-    active_input_method_ids_.erase(i);
-  extra_input_methods_.erase(id);
+  // Removes the extra input methods with |extension_id|.
+  std::map<std::string, InputMethodDescriptor> new_extra_input_methods_;
+  for (std::map<std::string, InputMethodDescriptor>::iterator i =
+       extra_input_methods_.begin(); i != extra_input_methods_.end(); ++i) {
+    if (extension_id !=
+        extension_ime_util::GetExtensionIDFromInputMethodID(i->first))
+      new_extra_input_methods_[i->first] = i->second;
+  }
+  extra_input_methods_.swap(new_extra_input_methods_);
 
-  EngineMap& engine_map = profile_engine_map_[GetProfile()];
-  if (IMEBridge::Get()->GetCurrentEngineHandler() == engine_map[id])
+  if (IMEBridge::Get()->GetCurrentEngineHandler() == engine_map_[extension_id])
     IMEBridge::Get()->SetCurrentEngineHandler(NULL);
-  engine_map.erase(id);
+  engine_map_.erase(extension_id);
 
   // No need to switch input method when terminating.
   if (state_ != STATE_TERMINATING) {
     // If |current_input_method| is no longer in |active_input_method_ids_|,
     // switch to the first one in |active_input_method_ids_|.
     ChangeInputMethod(current_input_method_.id());
   }
 }
 
 void InputMethodManagerImpl::GetInputMethodExtensions(
@@ skipping 362 matching lines @@
 
 void InputMethodManagerImpl::MaybeInitializeCandidateWindowController() {
   if (candidate_window_controller_.get())
     return;
 
   candidate_window_controller_.reset(
       CandidateWindowController::CreateCandidateWindowController());
   candidate_window_controller_->AddObserver(this);
 }
 
-Profile* InputMethodManagerImpl::GetProfile() const {
-  return ProfileManager::GetActiveUserProfile();
-}
-
 }  // namespace input_method
 }  // namespace chromeos