Improve perforance for component IME extensions initialization by using map instead of vector in Co…

Improve perforance for component IME extensions initialization.

ComponentExtensionIMEManager::FindEngineEntry() only needs to find for extension.

BUG=402222
TEST=Verified on linux_chromeos.

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

[Shu Chen, 2014-08-09 08:37:40]

Hi Nona/Alex, can you please review this cl? Thanks

[Seigo Nonaka, 2014-08-11 16:41:06]

lgtm with one nit

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/extension_i...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/extension_i...
chromeos/ime/extension_ime_util.cc:59: input_method_id.size() >=
kComponentExtensionIMEPrefixLength +
Is this check necessary?
If so, how about moving this check into IsComponentExtensionIME?

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/extension_i...
chromeos/ime/extension_ime_util.cc:64: input_method_id.size() >=
kExtensionIMEPrefixLength +
same as above.

[Alexander Alekseev, 2014-08-12 00:01:12]

lgtm with nits

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:109: std::map<std::string,
ComponentExtensionIME> component_extension_imes_;
nit: add comment (mostly on lifetime of this).

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:112: std::set<std::string>
input_method_id_set_;
nit: what is the lifetime of this?

[Shu Chen, 2014-08-12 04:44:07]

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:109: std::map<std::string,
ComponentExtensionIME> component_extension_imes_;
On 2014/08/12 00:01:12, alemate wrote:
> nit: add comment (mostly on lifetime of this).

Done.

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:112: std::set<std::string>
input_method_id_set_;
On 2014/08/12 00:01:12, alemate wrote:
> nit: what is the lifetime of this?

Done.

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/extension_i...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/extension_i...
chromeos/ime/extension_ime_util.cc:59: input_method_id.size() >=
kComponentExtensionIMEPrefixLength +
On 2014/08/11 16:41:06, Seigo Nonaka wrote:
> Is this check necessary?
> If so, how about moving this check into IsComponentExtensionIME?

Done.

https://codereview.chromium.org/457943002/diff/20001/chromeos/ime/extension_i...
chromeos/ime/extension_ime_util.cc:64: input_method_id.size() >=
kExtensionIMEPrefixLength +
On 2014/08/11 16:41:06, Seigo Nonaka wrote:
> same as above.

Done.

[Shu Chen, 2014-08-12 04:47:13]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-08-12 04:48:39]

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

[Shu Chen, 2014-08-12 05:36:47]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-08-12 05:38:17]

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

[Alexander Alekseev, 2014-08-12 13:47:23]

https://codereview.chromium.org/457943002/diff/40001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/457943002/diff/40001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:110: // It's filled by
Initialize() method and never changed during runtime.
This is wrong ;-)

ComponentExtensionIMEManager is a singleton.
InputMethodManagerImpl::InitializeComponentExtension() is called for every new
user. It calls ComponentExtensionIMEManager::Initialize() and settings from the
new user are added to the current set ;-)

(And it all works because they are added, but not replaced.)

[Shu Chen, 2014-08-12 14:30:58]

https://codereview.chromium.org/457943002/diff/40001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/457943002/diff/40001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:110: // It's filled by
Initialize() method and never changed during runtime.
On 2014/08/12 13:47:22, alemate wrote:
> This is wrong ;-)
> 
> ComponentExtensionIMEManager is a singleton.
> InputMethodManagerImpl::InitializeComponentExtension() is called for every new
> user. It calls ComponentExtensionIMEManager::Initialize() and settings from
the
> new user are added to the current set ;-)
> 
> (And it all works because they are added, but not replaced.)

But the whitelisted IME extensions are the same, with
ComponentExtensionIMEManager::Initialize() being called multiple times for
different users, the contents of |component_extension_imes_| remains the same.

Maybe it's necessary to speed it up to avoid reinitializing for different users.
;-)

[Alexander Alekseev, 2014-08-12 14:42:31]

https://codereview.chromium.org/457943002/diff/40001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/457943002/diff/40001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:110: // It's filled by
Initialize() method and never changed during runtime.
On 2014/08/12 14:30:58, Shu Chen wrote:
> On 2014/08/12 13:47:22, alemate wrote:
> > This is wrong ;-)
> > 
> > ComponentExtensionIMEManager is a singleton.
> > InputMethodManagerImpl::InitializeComponentExtension() is called for every
new
> > user. It calls ComponentExtensionIMEManager::Initialize() and settings from
> the
> > new user are added to the current set ;-)
> > 
> > (And it all works because they are added, but not replaced.)
> 
> But the whitelisted IME extensions are the same, with
> ComponentExtensionIMEManager::Initialize() being called multiple times for
> different users, the contents of |component_extension_imes_| remains the same.
> 
> Maybe it's necessary to speed it up to avoid reinitializing for different
users.
> ;-)

Yes. I think so ;-)

[commit-bot: I haz the power, 2014-08-12 15:27:20]

Change committed as 288979