Refactoring for InputMethodEngine and InputMethodEventRouter.

chrome/browser/chromeos/input_method/input_method_engine.cc

Index: chrome/browser/chromeos/input_method/input_method_engine.cc
diff --git a/chrome/browser/chromeos/input_method/input_method_engine.cc b/chrome/browser/chromeos/input_method/input_method_engine.cc
index 47a3c5a904eeccd9f4a942a4185fd6d4f9f5c0d8..fb298fa85de555c6d61638f366295d30b389d979 100644
--- a/chrome/browser/chromeos/input_method/input_method_engine.cc
+++ b/chrome/browser/chromeos/input_method/input_method_engine.cc
@@ -112,7 +112,6 @@ void GetExtensionKeyboardEventFromKeyEvent(
 
 InputMethodEngine::InputMethodEngine()
     : current_input_type_(ui::TEXT_INPUT_TYPE_NONE),
-      active_(false),
       context_id_(0),
       next_context_id_(1),
       composition_text_(new CompositionText()),
@@ -124,72 +123,29 @@ InputMethodEngine::InputMethodEngine()
 InputMethodEngine::~InputMethodEngine() {
   if (start_time_.ToInternalValue())
     RecordHistogram("WorkingTime", (end_time_ - start_time_).InSeconds());
-  input_method::InputMethodManager::Get()->RemoveInputMethodExtension(imm_id_);
 }
 
 void InputMethodEngine::Initialize(
     scoped_ptr<InputMethodEngineInterface::Observer> observer,
-    const char* engine_name,
-    const char* extension_id,
-    const char* engine_id,
-    const std::vector<std::string>& languages,
-    const std::vector<std::string>& layouts,
-    const GURL& options_page,
-    const GURL& input_view) {
+    const char* extension_id) {
   DCHECK(observer) << "Observer must not be null.";
 
   // TODO(komatsu): It is probably better to set observer out of Initialize.
   observer_ = observer.Pass();
-  engine_id_ = engine_id;
   extension_id_ = extension_id;
-
-  input_method::InputMethodManager* manager =
-      input_method::InputMethodManager::Get();
-  ComponentExtensionIMEManager* comp_ext_ime_manager =
-      manager->GetComponentExtensionIMEManager();
-
-  if (comp_ext_ime_manager->IsWhitelistedExtension(extension_id)) {
-    imm_id_ = comp_ext_ime_manager->GetId(extension_id, engine_id);
-  } else {
-    imm_id_ = extension_ime_util::GetInputMethodID(extension_id, engine_id);
-  }
-
-  input_view_url_ = input_view;
-  descriptor_ = input_method::InputMethodDescriptor(
-      imm_id_,
-      engine_name,
-      std::string(), // TODO(uekawa): Set short name.
-      layouts,
-      languages,
-      extension_ime_util::IsKeyboardLayoutExtension(
-          imm_id_), // is_login_keyboard
-      options_page,
-      input_view);
-
-  // TODO(komatsu): It is probably better to call AddInputMethodExtension
-  // out of Initialize.
-  manager->AddInputMethodExtension(imm_id_, this);
-}
-
-const input_method::InputMethodDescriptor& InputMethodEngine::GetDescriptor()
-    const {
-  return descriptor_;
 }
 
 void InputMethodEngine::RecordHistogram(const char* name, int count) {
   std::string histo_name =
-      base::StringPrintf("InputMethod.%s.%s", name, engine_id_.c_str());
+      base::StringPrintf("InputMethod.%s.%s", name, active_engine_id_.c_str());
   base::HistogramBase* counter = base::Histogram::FactoryGet(
       histo_name, 0, 1000000, 50, base::HistogramBase::kNoFlags);
   if (counter)
     counter->Add(count);
 }
 
-void InputMethodEngine::NotifyImeReady() {
-  input_method::InputMethodManager* manager =
-      input_method::InputMethodManager::Get();
-  if (manager && imm_id_ == manager->GetCurrentInputMethod().id())
-    Enable();
+const std::string& InputMethodEngine::GetActiveEngineId() const {
+  return active_engine_id_;
 }
 
 bool InputMethodEngine::SetComposition(
@@ -200,7 +156,7 @@ bool InputMethodEngine::SetComposition(
     int cursor,
     const std::vector<SegmentInfo>& segments,
     std::string* error) {
-  if (!active_) {
+  if (!IsActive()) {
     *error = kErrorNotActive;
     return false;
   }
@@ -244,7 +200,7 @@ bool InputMethodEngine::SetComposition(
 
 bool InputMethodEngine::ClearComposition(int context_id,
                                          std::string* error)  {
-  if (!active_) {
+  if (!IsActive()) {
     *error = kErrorNotActive;
     return false;
   }
@@ -261,7 +217,7 @@ bool InputMethodEngine::ClearComposition(int context_id,
 
 bool InputMethodEngine::CommitText(int context_id, const char* text,
                                    std::string* error) {
-  if (!active_) {
+  if (!IsActive()) {
     // TODO: Commit the text anyways.
     *error = kErrorNotActive;
     return false;
@@ -286,7 +242,7 @@ bool InputMethodEngine::CommitText(int context_id, const char* text,
 bool InputMethodEngine::SendKeyEvents(
     int context_id,
     const std::vector<KeyboardEvent>& events) {
-  if (!active_) {
+  if (!IsActive()) {
     return false;
   }
   // context_id  ==  0, means sending key events to non-input field.
@@ -355,7 +311,7 @@ void InputMethodEngine::SetCandidateWindowProperty(
   candidate_window_->SetProperty(dest_property);
   candidate_window_property_ = property;
 
-  if (active_) {
+  if (IsActive()) {
     IMECandidateWindowHandlerInterface* cw_handler =
         IMEBridge::Get()->GetCandidateWindowHandler();
     if (cw_handler)
@@ -365,7 +321,7 @@ void InputMethodEngine::SetCandidateWindowProperty(
 
 bool InputMethodEngine::SetCandidateWindowVisible(bool visible,
                                                   std::string* error) {
-  if (!active_) {
+  if (!IsActive()) {
     *error = kErrorNotActive;
     return false;
   }
@@ -382,7 +338,7 @@ bool InputMethodEngine::SetCandidates(
     int context_id,
     const std::vector<Candidate>& candidates,
     std::string* error) {
-  if (!active_) {
+  if (!IsActive()) {
     *error = kErrorNotActive;
     return false;
   }
@@ -410,7 +366,7 @@ bool InputMethodEngine::SetCandidates(
 
     candidate_window_->mutable_candidates()->push_back(entry);
   }
-  if (active_) {
+  if (IsActive()) {
     IMECandidateWindowHandlerInterface* cw_handler =
         IMEBridge::Get()->GetCandidateWindowHandler();
     if (cw_handler)
@@ -421,7 +377,7 @@ bool InputMethodEngine::SetCandidates(
 
 bool InputMethodEngine::SetCursorPosition(int context_id, int candidate_id,
                                           std::string* error) {
-  if (!active_) {
+  if (!IsActive()) {
     *error = kErrorNotActive;
     return false;
   }
@@ -451,7 +407,7 @@ bool InputMethodEngine::SetMenuItems(const std::vector<MenuItem>& items) {
 
 bool InputMethodEngine::UpdateMenuItems(
     const std::vector<MenuItem>& items) {
-  if (!active_)
+  if (!IsActive())
     return false;
 
   ash::ime::InputMethodMenuItemList menu_item_list;
@@ -469,14 +425,14 @@ bool InputMethodEngine::UpdateMenuItems(
 }
 
 bool InputMethodEngine::IsActive() const {
-  return active_;
+  return !active_engine_id_.empty();
 }
 
 bool InputMethodEngine::DeleteSurroundingText(int context_id,
                                               int offset,
                                               size_t number_of_chars,
                                               std::string* error) {
-  if (!active_) {
+  if (!IsActive()) {
     *error = kErrorNotActive;
     return false;
   }
@@ -508,7 +464,8 @@ void InputMethodEngine::HideInputView() {
 }
 
 void InputMethodEngine::EnableInputView(bool enabled) {
-  const GURL& url = enabled ? input_view_url_ : GURL();
+  const GURL& url = enabled && GetCurrentInputMethod() ?
+      GetCurrentInputMethod()->input_view_url() : GURL();
   keyboard::SetOverrideContentUrl(url);
   keyboard::KeyboardController* keyboard_controller =
       keyboard::KeyboardController::GetInstance();
@@ -516,11 +473,27 @@ void InputMethodEngine::EnableInputView(bool enabled) {
     keyboard_controller->Reload();
 }
 
+const input_method::InputMethodDescriptor*
+InputMethodEngine::GetCurrentInputMethod() {
+  input_method::InputMethodManager* manager =
+      input_method::InputMethodManager::Get();

[Shu Chen, 2014/08/04 15:59:31]

Here could just use
input_method::InputMethodManager::Get()->GetCurrentInputMethod().

[Seigo Nonaka, 2014/08/04 23:29:47]

Seems yes.

On 2014/08/04 15:59:31, Shu Chen wrote:

[Shu Chen, 2014/08/05 01:23:09]

Done.

[Shu Chen, 2014/08/05 01:59:03]

Changed and found a bug. Enable() method is called in
InputMethodManagerImpl::ChangeInputMethodInternal() before its
current_input_method_ is set to new input method.

I've fixed it by moving the  Enable() call after current_input_method_ is set to
new input method.
Do you think it's a reasonable fix?

+  ComponentExtensionIMEManager* comp_manager =
+      manager->GetComponentExtensionIMEManager();
+  std::string input_method_id;
+  if (comp_manager->IsWhitelistedExtension(extension_id_))
+    input_method_id = extension_ime_util::GetComponentInputMethodID(
+        extension_id_, active_engine_id_);
+  else
+    input_method_id = extension_ime_util::GetInputMethodID(
+        extension_id_, active_engine_id_);
+  return manager->GetInputMethodFromId(input_method_id);
+}
+
 void InputMethodEngine::FocusIn(
     const IMEEngineHandlerInterface::InputContext& input_context) {
   current_input_type_ = input_context.type;
 
-  if (!active_ || current_input_type_ == ui::TEXT_INPUT_TYPE_NONE)
+  if (!IsActive() || current_input_type_ == ui::TEXT_INPUT_TYPE_NONE)
     return;
 
   context_id_ = next_context_id_;
@@ -556,7 +529,7 @@ void InputMethodEngine::FocusIn(
 }
 
 void InputMethodEngine::FocusOut() {
-  if (!active_ || current_input_type_ == ui::TEXT_INPUT_TYPE_NONE)
+  if (!IsActive() || current_input_type_ == ui::TEXT_INPUT_TYPE_NONE)
     return;
 
   current_input_type_ = ui::TEXT_INPUT_TYPE_NONE;
@@ -566,9 +539,10 @@ void InputMethodEngine::FocusOut() {
   observer_->OnBlur(context_id);
 }
 
-void InputMethodEngine::Enable() {
-  active_ = true;
-  observer_->OnActivate(engine_id_);
+void InputMethodEngine::Enable(const std::string& engine_id) {
+  DCHECK(!engine_id.empty());
+  active_engine_id_ = engine_id;
+  observer_->OnActivate(engine_id);
   current_input_type_ = IMEBridge::Get()->GetCurrentTextInputType();
   FocusIn(IMEEngineHandlerInterface::InputContext(
       current_input_type_, ui::TEXT_INPUT_MODE_DEFAULT));
@@ -580,19 +554,19 @@ void InputMethodEngine::Enable() {
 }
 
 void InputMethodEngine::Disable() {
-  active_ = false;
-  observer_->OnDeactivated(engine_id_);
+  active_engine_id_ = "";
+  observer_->OnDeactivated(active_engine_id_);
 
   if (start_time_.ToInternalValue())
     RecordHistogram("WorkingTime", (end_time_ - start_time_).InSeconds());
 }
 
 void InputMethodEngine::PropertyActivate(const std::string& property_name) {
-  observer_->OnMenuItemActivated(engine_id_, property_name);
+  observer_->OnMenuItemActivated(active_engine_id_, property_name);
 }
 
 void InputMethodEngine::Reset() {
-  observer_->OnReset(engine_id_);
+  observer_->OnReset(active_engine_id_);
 }
 
 void InputMethodEngine::ProcessKeyEvent(
@@ -613,7 +587,7 @@ void InputMethodEngine::ProcessKeyEvent(
     ext_event.extension_id = extension_id_;
 
   observer_->OnKeyEvent(
-      engine_id_,
+      active_engine_id_,
       ext_event,
       reinterpret_cast<input_method::KeyEventHandle*>(handler));
 }
@@ -625,13 +599,13 @@ void InputMethodEngine::CandidateClicked(uint32 index) {
 
   // Only left button click is supported at this moment.
   observer_->OnCandidateClicked(
-      engine_id_, candidate_ids_.at(index), MOUSE_BUTTON_LEFT);
+      active_engine_id_, candidate_ids_.at(index), MOUSE_BUTTON_LEFT);
 }
 
 void InputMethodEngine::SetSurroundingText(const std::string& text,
                                            uint32 cursor_pos,
                                            uint32 anchor_pos) {
-  observer_->OnSurroundingTextChanged(engine_id_,
+  observer_->OnSurroundingTextChanged(active_engine_id_,
                                       text,
                                       static_cast<int>(cursor_pos),
                                       static_cast<int>(anchor_pos));