Refactoring for InputMethodEngine and InputMethodEventRouter.

Refactoring for InputMethodEngine and InputMethodEventRouter.

1) Makes InputMethodEngine extension based instead of input component based.
2) Makes InputMethodEngine independent to user profile.
3) Makes InputMethodEventRouter independent to user profile, except
   forwarding events to extension.

BUG=342336
TEST=Verified on sandbox.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287781

[Shu Chen, 2014-08-04 13:58:35]

Nona/Yuki/Alex, can you please review this cl?

Nona for overall refactoring solution and potential breakages to existing
features of IMF.

Yuki for implementation details, coding conventions, etc.

Alex for integration with profile/login/etc., as well as the refactoring
solution which may not be aligned with some planned refactoring.

Thanks,
Shu

[Shu Chen, 2014-08-04 13:58:36]

Nona/Yuki/Alex, can you please review this cl?

Nona for overall refactoring solution and potential breakages to existing
features of IMF.

Yuki for implementation details, coding conventions, etc.

Alex for integration with profile/login/etc., as well as the refactoring
solution which may not be aligned with some planned refactoring.

Thanks,
Shu

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

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:479:
input_method::InputMethodManager::Get();
Here could just use
input_method::InputMethodManager::Get()->GetCurrentInputMethod().

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:471: void
InputMethodManagerImpl::RemoveInputMethodExtension(
I'm not sure whether the below code to remove things in active_input_method_ids_
and extra_input_methods_ look ok. Maybe use swap() is better?

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:19: #include
"chromeos/ime/input_method_manager.h"
Nona, I guess the original design was to avoid dependency to InputMethodManager,
but I don't know why.
This cl would introduce dependency to InputMethodManager. With reasonable
justification, I can move the dependency to InputMethodEngine.

[Seigo Nonaka, 2014-08-04 23:29:48]

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:479:
input_method::InputMethodManager::Get();
Seems yes.

On 2014/08/04 15:59:31, Shu Chen wrote:
> Here could just use
> input_method::InputMethodManager::Get()->GetCurrentInputMethod().

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:459: DVLOG(1)
<< "AddInputMethodExtension: alread added: "
nit: /alread/already/ I'm sorry this is not your mistake but please fix.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:471: void
InputMethodManagerImpl::RemoveInputMethodExtension(
Maybe true. could you fix?


On 2014/08/04 15:59:31, Shu Chen wrote:
> I'm not sure whether the below code to remove things in
active_input_method_ids_
> and extra_input_methods_ look ok. Maybe use swap() is better?

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:19: #include
"chromeos/ime/input_method_manager.h"
Probably, it is historical reasons. It used to be planed that this API would be
used also in other platforms.
However, that plan is no longer active and already chrome os specific dependency
was introduced.
So I think we no longer consider the dependency to chrome os specific headers.

On 2014/08/04 15:59:31, Shu Chen wrote:
> Nona, I guess the original design was to avoid dependency to
InputMethodManager,
> but I don't know why.
> This cl would introduce dependency to InputMethodManager. With reasonable
> justification, I can move the dependency to InputMethodEngine.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:406: if
(engine_map_.find(extension_id) != engine_map_.end()) {
nit: it is totally up to you but I prefer

if (engine_map_.find(extension_id) != engine_map_.end() &&
  engine_map_[extension_id]->GetActiveEngineId() == engine_id)
  return engine_map_[extension_id];
else
  return NULL;

or

if (engine_map_.find(extension_id) == engine_map_.end())
  return NULL;

if (engine_map_[extension_id]->GetActiveEngineId() == engine _id)
  return engine_map_[extension_id];

return NULL;

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:415: if
(engine_map_.find(extension_id) != engine_map_.end()) {
same as above.

[Shu Chen, 2014-08-05 01:23:10]

Thanks for your review. Can you please take another look?

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:479:
input_method::InputMethodManager::Get();
On 2014/08/04 23:29:47, Seigo Nonaka wrote:
> Seems yes.
> 
> On 2014/08/04 15:59:31, Shu Chen wrote:
> > Here could just use
> > input_method::InputMethodManager::Get()->GetCurrentInputMethod().
> 

Done.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:459: DVLOG(1)
<< "AddInputMethodExtension: alread added: "
On 2014/08/04 23:29:47, Seigo Nonaka wrote:
> nit: /alread/already/ I'm sorry this is not your mistake but please fix.

Done.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:471: void
InputMethodManagerImpl::RemoveInputMethodExtension(
On 2014/08/04 23:29:47, Seigo Nonaka wrote:
> Maybe true. could you fix?
> 
> 
> On 2014/08/04 15:59:31, Shu Chen wrote:
> > I'm not sure whether the below code to remove things in
> active_input_method_ids_
> > and extra_input_methods_ look ok. Maybe use swap() is better?
> 

Done.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:19: #include
"chromeos/ime/input_method_manager.h"
On 2014/08/04 23:29:47, Seigo Nonaka wrote:
> Probably, it is historical reasons. It used to be planed that this API would
be
> used also in other platforms.
> However, that plan is no longer active and already chrome os specific
dependency
> was introduced.
> So I think we no longer consider the dependency to chrome os specific headers.
> 
> On 2014/08/04 15:59:31, Shu Chen wrote:
> > Nona, I guess the original design was to avoid dependency to
> InputMethodManager,
> > but I don't know why.
> > This cl would introduce dependency to InputMethodManager. With reasonable
> > justification, I can move the dependency to InputMethodEngine.
> 

Done.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:406: if
(engine_map_.find(extension_id) != engine_map_.end()) {
On 2014/08/04 23:29:48, Seigo Nonaka wrote:
> nit: it is totally up to you but I prefer
> 
> if (engine_map_.find(extension_id) != engine_map_.end() &&
>   engine_map_[extension_id]->GetActiveEngineId() == engine_id)
>   return engine_map_[extension_id];
> else
>   return NULL;
> 
> or
> 
> if (engine_map_.find(extension_id) == engine_map_.end())
>   return NULL;
> 
> if (engine_map_[extension_id]->GetActiveEngineId() == engine _id)
>   return engine_map_[extension_id];
> 
> return NULL;

Done.

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:415: if
(engine_map_.find(extension_id) != engine_map_.end()) {
On 2014/08/04 23:29:47, Seigo Nonaka wrote:
> same as above.

Done.

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

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:479:
input_method::InputMethodManager::Get();
On 2014/08/04 23:29:47, Seigo Nonaka wrote:
> Seems yes.
> 
> On 2014/08/04 15:59:31, Shu Chen wrote:
> > Here could just use
> > input_method::InputMethodManager::Get()->GetCurrentInputMethod().
> 

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?

[Seigo Nonaka, 2014-08-05 02:11:25]

lgtm.

Could you kindly double check this patch works on Chrome OS devices.

I'd like you to try with debug build or dcheck_alwasy_on=1 config if debug build
is hard to test on the device.

Thank you.

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

Mostly minor nits, but one important comment I have.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:134: observer_ =
observer.Pass();
IIUC, we don't need Pass() in this case.
    observer_ = observer;
should do.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.h (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.h:110: // This IME ID
in Chrome Extension.
Could you brush up this comment?  This is unclear.

Could this be something like the following?  I'm not sure what this is.  The
definition of "IME ID" is very unclear to me while "extension id" is crystal
clear and well-defined.
// Input method engine's ID, aka ComponentInfo's ID????

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:373:
std::string extension_id =
Please use const references.
const std::string& extension_id = ...;

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:376:
std::string engine_id = extension_ime_util::GetEngineIDByInputMethodID(
Ditto.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:381:
engine->Enable(engine_id);
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?

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:205:
std::map<std::string, InputMethodEngineInterface*> engine_map_;
The original comment was very useful, and it's still useful, I think.
Could you describe what the key is in a comment?  I can guess the value because
it's type of InputMethodEngineInterface*, but std::string is very vague.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:107: }
This line can be on the same line above.
    : extension_id_(extension_id) {}

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:365: std::string
input_method_id =
Use a const reference.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:396: if
(engine_map_.find(extension_id) != engine_map_.end()) {
Could you optimize the code a little more?
find() and operator[] do the same look-up.  You can store the result of find()
in a temporary variable and re-use it.

Ditto for the following methods.

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

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:134: observer_ =
observer.Pass();
On 2014/08/05 07:57:18, Yuki wrote:
> IIUC, we don't need Pass() in this case.
>     observer_ = observer;
> should do.

Done.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.h (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.h:110: // This IME ID
in Chrome Extension.
On 2014/08/05 07:57:18, Yuki wrote:
> Could you brush up this comment?  This is unclear.
> 
> Could this be something like the following?  I'm not sure what this is.  The
> definition of "IME ID" is very unclear to me while "extension id" is crystal
> clear and well-defined.
> // Input method engine's ID, aka ComponentInfo's ID????

Done. Yes, it is input_component ID in IME extension's manifest.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:373:
std::string extension_id =
On 2014/08/05 07:57:18, Yuki wrote:
> Please use const references.
> const std::string& extension_id = ...;

Done.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:376:
std::string engine_id = extension_ime_util::GetEngineIDByInputMethodID(
On 2014/08/05 07:57:18, Yuki wrote:
> Ditto.

Done.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:381:
engine->Enable(engine_id);
On 2014/08/05 07:57:18, Yuki wrote:
> 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?

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.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:205:
std::map<std::string, InputMethodEngineInterface*> engine_map_;
On 2014/08/05 07:57:18, Yuki wrote:
> The original comment was very useful, and it's still useful, I think.
> Could you describe what the key is in a comment?  I can guess the value
because
> it's type of InputMethodEngineInterface*, but std::string is very vague.

Done.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:107: }
On 2014/08/05 07:57:18, Yuki wrote:
> This line can be on the same line above.
>     : extension_id_(extension_id) {}

Done.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:365: std::string
input_method_id =
On 2014/08/05 07:57:18, Yuki wrote:
> Use a const reference.

Done.

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:396: if
(engine_map_.find(extension_id) != engine_map_.end()) {
On 2014/08/05 07:57:18, Yuki wrote:
> Could you optimize the code a little more?
> find() and operator[] do the same look-up.  You can store the result of find()
> in a temporary variable and re-use it.
> 
> Ditto for the following methods.

Done.

[Shu Chen, 2014-08-05 15:36:00]

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine.cc:134: observer_ =
observer.Pass();
On 2014/08/05 14:04:31, Shu Chen wrote:
> On 2014/08/05 07:57:18, Yuki wrote:
> > IIUC, we don't need Pass() in this case.
> >     observer_ = observer;
> > should do.
> 
> Done.

"observer_ = observer;" causes compiling error: 

../../chrome/browser/chromeos/input_method/input_method_engine.cc:135:13: error:
'operator=' is a private member of
'scoped_ptr<chromeos::InputMethodEngineInterface::Observer,
base::DefaultDeleter<chromeos::InputMethodEngineInterface::Observer> >'
  observer_ = observer;
  ~~~~~~~~~ ^ ~~~~~~~~


Reverted it back.

[Alexander Alekseev, 2014-08-05 16:01:02]

lgtm

[Yuki, 2014-08-06 04:42:24]

lgtm

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_engine.cc:542:
active_component_id_ = "";
active_component_id_.clear() could be slightly better.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_engine_interface.h
(right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_engine_interface.h:229: //
Returns the current active engine id.
s/engine/component/ at least.
Better to mention to input_component id.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc
(right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:214:
engine_->Enable(kTestImeEngineId);
s/Engine/Component/

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:377:
extension_ime_util::GetEngineIDByInputMethodID(input_method_id_to_switch);
s/GetEngineID/GetComponentID/

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:475: //
Removes the active input methods with |extension_id|.
s/Removes/Remove/

Descriptive for class/function/variable declaration comments, and imperative for
implementation comments.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:476:
std::vector<std::string> new_active_input_method_ids_;
s/ids_/ids/

This variable is a local variable, isn't it?  Then, don't put the last
underscore.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:484: //
Removes the extra input methods with |extension_id|.
Ditto.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:485:
std::map<std::string, InputMethodDescriptor> new_extra_input_methods_;
Ditto.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:207: // The
engine map: { extension_id : { Engine } }.
{ extension_id : Engine }, I think.

Or, // The engine map from extension_id to an engine.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_engine.cc:109:
active_component_id_ = "";
active_compoenent_id_.clear() could be slightly better.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:322: it !=
listener_list.end(); it++) {
s/it++/++it/ for iterators.
it++ may be slower than ++it.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/input_ime/input_ime_api.h (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/input_ime/input_ime_api.h:67:
std::map<std::string, chromeos::InputMethodEngineInterface*> engine_map_;
Better to have a comment here, too.
Say that the key is extension_id.

https://codereview.chromium.org/433163005/diff/120001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.h (right):

https://codereview.chromium.org/433163005/diff/120001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:88: GetEngineIDByInputMethodID(const
std::string& input_method_id);
IIRC,
std::string CHROMEOS_EXPORT GetEngineIDByInputMethodID(
    const std::string& input_method_id);
is preferable.

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_bridge.h (right):

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_bridge.h:75: virtual void Enable(const std::string&
engine_id) = 0;
s/engine_id/component_id/

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_ime_engine_handler.cc (right):

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_ime_engine_handler.cc:36: void
MockIMEEngineHandler::Enable(const std::string& engine_id) {
s/engine_id/component_id/

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_ime_engine_handler.h (right):

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_ime_engine_handler.h:21: virtual void Enable(const
std::string& engine_id) OVERRIDE;
s/engine_id/component_id/

[Shu Chen, 2014-08-06 05:45:04]

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_engine.cc:542:
active_component_id_ = "";
On 2014/08/06 04:42:23, Yuki wrote:
> active_component_id_.clear() could be slightly better.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_engine_interface.h
(right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_engine_interface.h:229: //
Returns the current active engine id.
On 2014/08/06 04:42:23, Yuki wrote:
> s/engine/component/ at least.
> Better to mention to input_component id.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc
(right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:214:
engine_->Enable(kTestImeEngineId);
On 2014/08/06 04:42:23, Yuki wrote:
> s/Engine/Component/

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:377:
extension_ime_util::GetEngineIDByInputMethodID(input_method_id_to_switch);
On 2014/08/06 04:42:23, Yuki wrote:
> s/GetEngineID/GetComponentID/

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:475: //
Removes the active input methods with |extension_id|.
On 2014/08/06 04:42:23, Yuki wrote:
> s/Removes/Remove/
> 
> Descriptive for class/function/variable declaration comments, and imperative
for
> implementation comments.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:476:
std::vector<std::string> new_active_input_method_ids_;
On 2014/08/06 04:42:23, Yuki wrote:
> s/ids_/ids/
> 
> This variable is a local variable, isn't it?  Then, don't put the last
> underscore.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:484: //
Removes the extra input methods with |extension_id|.
On 2014/08/06 04:42:23, Yuki wrote:
> Ditto.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:485:
std::map<std::string, InputMethodDescriptor> new_extra_input_methods_;
On 2014/08/06 04:42:24, Yuki wrote:
> Ditto.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:207: // The
engine map: { extension_id : { Engine } }.
On 2014/08/06 04:42:24, Yuki wrote:
> { extension_id : Engine }, I think.
> 
> Or, // The engine map from extension_id to an engine.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_engine.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_engine.cc:109:
active_component_id_ = "";
On 2014/08/06 04:42:24, Yuki wrote:
> active_compoenent_id_.clear() could be slightly better.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:322: it !=
listener_list.end(); it++) {
On 2014/08/06 04:42:24, Yuki wrote:
> s/it++/++it/ for iterators.
> it++ may be slower than ++it.

Done.

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/input_ime/input_ime_api.h (right):

https://codereview.chromium.org/433163005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/input_ime/input_ime_api.h:67:
std::map<std::string, chromeos::InputMethodEngineInterface*> engine_map_;
On 2014/08/06 04:42:24, Yuki wrote:
> Better to have a comment here, too.
> Say that the key is extension_id.

Done.

https://codereview.chromium.org/433163005/diff/120001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.h (right):

https://codereview.chromium.org/433163005/diff/120001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:88: GetEngineIDByInputMethodID(const
std::string& input_method_id);
On 2014/08/06 04:42:24, Yuki wrote:
> IIRC,
> std::string CHROMEOS_EXPORT GetEngineIDByInputMethodID(
>     const std::string& input_method_id);
> is preferable.

Done.

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_bridge.h (right):

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_bridge.h:75: virtual void Enable(const std::string&
engine_id) = 0;
On 2014/08/06 04:42:24, Yuki wrote:
> s/engine_id/component_id/

Done.

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_ime_engine_handler.cc (right):

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_ime_engine_handler.cc:36: void
MockIMEEngineHandler::Enable(const std::string& engine_id) {
On 2014/08/06 04:42:24, Yuki wrote:
> s/engine_id/component_id/

Done.

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_ime_engine_handler.h (right):

https://codereview.chromium.org/433163005/diff/120001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_ime_engine_handler.h:21: virtual void Enable(const
std::string& engine_id) OVERRIDE;
On 2014/08/06 04:42:24, Yuki wrote:
> s/engine_id/component_id/

Done.

[Shu Chen, 2014-08-06 05:45:09]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-08-06 05:49:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/433163005/140001

[commit-bot: I haz the power, 2014-08-06 08:33:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-06 09:16:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 09:16:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2014-08-06 14:05:45]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-08-06 14:06:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/433163005/160001

[commit-bot: I haz the power, 2014-08-06 16:34:58]

Change committed as 287781

[Alexander Alekseev, 2014-08-06 20:48:29]

https://codereview.chromium.org/433163005/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/433163005/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:205:
base::WeakPtrFactory<InputMethodManagerImpl> weak_ptr_factory_;
This should be the last member.

[Shu Chen, 2014-08-07 00:44:28]

https://codereview.chromium.org/433163005/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/433163005/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:205:
base::WeakPtrFactory<InputMethodManagerImpl> weak_ptr_factory_;
On 2014/08/06 20:48:28, alemate wrote:
> This should be the last member.

I don't see any code is using it. Probably it should be removed?