IME refactoring: ChromeOS introduce input methods State.

IME refactoring: ChromeOS introduce input methods State.

We need input methods "config" object to switch between IME states.

This CL also fixed issue with incorrect input method after
screen unlock in multiprofile mode.

BUG=292774, 399129
TEST=manual

TBR=nona@chromium.org,asargent@chromium.org,davemoore@chromium.org

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

[Alexander Alekseev, 2014-07-26 00:42:59]

This CL is not finished, I need some help with it.

The problem:

IME doesn't have "config" object, that would completely define its state. (Like
system::TouchpadSettings or system::MouseSettings). It means that IME state has
to be updated step by step using different methods, and it breaks
when chromeos flow changes.

There are (al least) two issues which I need to fix:

1) Lock Screen is broken in multiprofile mode. Because on entering lock screen,
previous input method is remembered in a static variable, and restored after
unlock. But in multiprofile mode, we allow lock by one user and unlock by other.
Therefore, previous input method is restored from other user.

2) Adding new user to the session should allow per-user layouts (just like on
signin screen), and previous input method should be correctly handled.


Also, switching between sessions in multiprofile mode is slow, because full
re-initialization of IME happens on each switch. It seems weird, because
individual user settings are not usually changed.


The idea: create IME State object, which can be remembered outside of IME. This
way we can remember this State for each user, and quickly switch between them.
Also, for lock screen, signin screen, and other cases when we need temporary IME
state, we can remember the previous state completely, and then switch between
remembered states.

What is done in this CL:

1) Old IMM "enum State" is renamed to UIState to better describe its meaning.

2) Created IMM State object. All session-dependent IMM data moved to State
object.

3) Partially implemented fast state switching on active user changed.

4) Removed (most) calls to GetActiveUser() from IMM.



Questions:

1) Now we have several user IMM states, but only state for active user can be
modified. Pending Preferences updates for non-active users should be remembered
and applied when user becomes active.

But it seems that IMM State should have methods like "ChangeInputMethod" (i.e.
those methods should be moved from IMM to State object), which can modify State
even if user is not active. (And State actually knows if it is active.)

What do you think about moving IMM methods ChangeInputMethod,
ReplaceEnabledInputMethods, etc... to IMM State? So that
"Preferences::ime_state_outdated_" will not be needed.

2) It looks like AutoRepeat settings should also be moved to State.

3) I don't know how to correctly modify extensions list. It seems that
IMM::enabled_extension_imes_ should still be global (i.e. a part of IMM, not IMM
State). Am I right?

The same question for all the other data members of IMM.

4) InputMethodPersistence : it seems that current implementation tries to figure
out where to store input method setting, so it must also be updated when control
flow changes. But we could pass callback to ChangeInputMethod which would update
proper setting. Though this can be done in a separate CL.

5) What else should I modify to enable switching between states?

[Nikita (slow), 2014-07-30 19:39:28]

On 2014/07/26 00:42:59, alemate wrote:
> IME doesn't have "config" object, that would completely define its state.
(Like
> system::TouchpadSettings or system::MouseSettings). It means that IME state
has
> to be updated step by step using different methods, and it breaks
> when chromeos flow changes.

What does "chromeos flow changes" means?

> 
> There are (al least) two issues which I need to fix:
> 
> 1) Lock Screen is broken in multiprofile mode.

Please file a separate issue even if it will be addressed in this CL.

> 
> 2) Adding new user to the session should allow per-user layouts (just like on
> signin screen), and previous input method should be correctly handled.

does the http://crbug.com/292774 fully describe this issue?

> 
> Also, switching between sessions in multiprofile mode is slow, because full
> re-initialization of IME happens on each switch. It seems weird, because
> individual user settings are not usually changed.

Is this the existing behavior? Please file a separate bug.

> The idea: create IME State object, which can be remembered outside of IME.
This
> way we can remember this State for each user, and quickly switch between them.
> Also, for lock screen, signin screen, and other cases when we need temporary
IME
> state, we can remember the previous state completely, and then switch between
> remembered states.

sounds good

[Nikita (slow), 2014-07-30 19:41:49]

On 2014/07/26 00:42:59, alemate wrote:
> Questions:
> 
> 2) It looks like AutoRepeat settings should also be moved to State.

Let's handle them separetely. New issue would be useful to keep track on that
work item.

All other questions should be answered by shuchen@

[Alexander Alekseev, 2014-07-31 00:45:43]

On 2014/07/30 19:39:28, Nikita Kostylev wrote:
> On 2014/07/26 00:42:59, alemate wrote:
> > IME doesn't have "config" object, that would completely define its state.
> (Like
> > system::TouchpadSettings or system::MouseSettings). It means that IME state
> has
> > to be updated step by step using different methods, and it breaks
> > when chromeos flow changes.
> 
> What does "chromeos flow changes" means?

When IME adds new option, it is not obvious where is should be updated in
existing code.

Also, when we create another way to switch between interface states, we
might break some IME settings, because there is no complete list of them.

For example the previous lock-unlock flow "locked device can be unlocked
by the session owner" has changed to "in multiprofile mode, unlock can be
activated by any logged-in users in the same session" and broke the list
of active keyboards. This happened because there was a hidden lock screen
dependency in IME, which was not obvious in lock/unlock code.

> > There are (al least) two issues which I need to fix:
> > 
> > 1) Lock Screen is broken in multiprofile mode.
> 
> Please file a separate issue even if it will be addressed in this CL.

I've created crbug.com/399129 .

> > 2) Adding new user to the session should allow per-user layouts (just like
on
> > signin screen), and previous input method should be correctly handled.
> 
> does the http://crbug.com/292774 fully describe this issue?

Yes, though the description is probably cryptic.

> > Also, switching between sessions in multiprofile mode is slow, because full
> > re-initialization of IME happens on each switch. It seems weird, because
> > individual user settings are not usually changed.
> 
> Is this the existing behavior? Please file a separate bug.

I've created crbug.com/399136

> > The idea: create IME State object, which can be remembered outside of IME.
> This
> > way we can remember this State for each user, and quickly switch between
them.
> > Also, for lock screen, signin screen, and other cases when we need temporary
> IME
> > state, we can remember the previous state completely, and then switch
between
> > remembered states.
> 
> sounds good

[Shu Chen, 2014-08-04 14:55:24]

This cl is kind of old and needs to be rebased. Sorry for delayed review.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: void
InputMethodManagerImpl::SetState(
I guess this is for fast user switching, correct? I'm concerned that this may
breaks 3rd party IMEs, as enabled_extension_imes_ is not included in State.

I'm not familiar with the design purpose of enabled_extension_imes_. nona@ may
know better.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:135:
ChangeInputMethodInternal(state_->current_input_method_, false);
Is there particular reason of calling ChangeInputMethodInternal instead of
LookupAndChangeInputMethodInternal?

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:422: bool
InputMethodManagerImpl::LookupAndChangeInputMethodInternal(
looks like the bool return value never gets used.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:604:
state_->engine_map_[id] = engine;
I think this will break something. GetProfile() is necessary to match profile
map in InputMethodEventRouter.
If you want to remove the profiled map here, you have to remove the profiled map
in InputMethodEventRouter.
And that would be an even bigger change. Please refer to cl:
https://codereview.chromium.org/433163005/.

So, this flow will be broken, if only removing profiled map in
InputMethodManagerImpl.
Engine contains Observer which contains profile which is used to dispatch events
to extension.
Suppose there are 2 users: A and B, and they both use input method X.
InputMethodManagerImpl::AddInputMethodExtension() will be called for both A and
B.
 - AddInputMethodExtension(<engineX with observer to IME extension loaded under
user A>)
 - AddInputMethodExtension(<engineX with observer to IME extension loaded under
user B>)
Now <engineX with observer to IME extension loaded under user B> will overwrite
<engineX with observer to IME extension loaded under user A> in
state_->engine_map_.
So under user A, InputMethodManagerImpl::ChangeInputMethod() cannot successfully
send onActivate event to IME extension because the engine it can get is for user
B. And user B's IME extension will get onActivate unexpectedly.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (left):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:219:
std::string pending_input_method_;
this is already gone, please rebase your cl.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:656: if (!ime_state_) {
I don't understand. Will there be single InputMethodManager::State shared for
multiple users?

https://codereview.chromium.org/419293002/diff/1/chromeos/ime/input_method_ma...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/419293002/diff/1/chromeos/ime/input_method_ma...
chromeos/ime/input_method_manager.h:33: enum UIState {
Maybe better to name it as "UISession" or "UIScreen".

[Alexander Alekseev, 2014-08-06 23:39:46]

+nona@

nona@, please review this. The long description and a list of questions to
reviewers is in my first comment.

Also, I have a special question to you: why enabled_extension_imes_ is shared
between users? Can I move it to IME::State (and split between users)? Or it
should stay shared?

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: void
InputMethodManagerImpl::SetState(
On 2014/08/04 14:55:23, Shu Chen wrote:
> I guess this is for fast user switching, correct? I'm concerned that this may
> breaks 3rd party IMEs, as enabled_extension_imes_ is not included in State.
> 
> I'm not familiar with the design purpose of enabled_extension_imes_. nona@ may
> know better.

Yes, this was one of my concerns, when I've uploaded this early version of CL. I
also do not understand why enabled_extension_imes_ have always
been shared between users.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:135:
ChangeInputMethodInternal(state_->current_input_method_, false);
On 2014/08/04 14:55:23, Shu Chen wrote:
> Is there particular reason of calling ChangeInputMethodInternal instead of
> LookupAndChangeInputMethodInternal?

The idea is that SetState should be lightweight, because it is always
consistent. So LookupAndChangeInputMethodInternal() would be doing
extra job of re-checking already consistent configuration.

Also, ReconfigureIMEFramework seems to be too heavy to be re-applied
each time active user is changed. But I haven't yet investigated how
to implement it. Are all users extensions always loaded in multiprofile
mode? If it is so, probably we just need to properly route events to
the current active set. What do you think?

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:422: bool
InputMethodManagerImpl::LookupAndChangeInputMethodInternal(
On 2014/08/04 14:55:23, Shu Chen wrote:
> looks like the bool return value never gets used.

Done.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:604:
state_->engine_map_[id] = engine;
On 2014/08/04 14:55:23, Shu Chen wrote:
> I think this will break something. GetProfile() is necessary to match profile
> map in InputMethodEventRouter.
> If you want to remove the profiled map here, you have to remove the profiled
map
> in InputMethodEventRouter.
> And that would be an even bigger change. Please refer to cl:
> https://codereview.chromium.org/433163005/.
> 
> So, this flow will be broken, if only removing profiled map in
> InputMethodManagerImpl.
> Engine contains Observer which contains profile which is used to dispatch
events
> to extension.
> Suppose there are 2 users: A and B, and they both use input method X.
> InputMethodManagerImpl::AddInputMethodExtension() will be called for both A
and
> B.
>  - AddInputMethodExtension(<engineX with observer to IME extension loaded
under
> user A>)
>  - AddInputMethodExtension(<engineX with observer to IME extension loaded
under
> user B>)
> Now <engineX with observer to IME extension loaded under user B> will
overwrite
> <engineX with observer to IME extension loaded under user A> in
> state_->engine_map_.
> So under user A, InputMethodManagerImpl::ChangeInputMethod() cannot
successfully
> send onActivate event to IME extension because the engine it can get is for
user
> B. And user B's IME extension will get onActivate unexpectedly.
> 

state_ is per-user, so this would never happen.

I'm going to remove profile map in Router also. The idea is that
Router should route events according to current IME state.
I haven't done it because I do not know why enabled_input_methods_
is shared.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (left):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:219:
std::string pending_input_method_;
On 2014/08/04 14:55:23, Shu Chen wrote:
> this is already gone, please rebase your cl.

Done.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:656: if (!ime_state_) {
On 2014/08/04 14:55:24, Shu Chen wrote:
> I don't understand. Will there be single InputMethodManager::State shared for
> multiple users?

No. Preferences belong to particular user, so this is lazy initialization of
per-user State object.

https://codereview.chromium.org/419293002/diff/1/chromeos/ime/input_method_ma...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/419293002/diff/1/chromeos/ime/input_method_ma...
chromeos/ime/input_method_manager.h:33: enum UIState {
On 2014/08/04 14:55:24, Shu Chen wrote:
> Maybe better to name it as "UISession" or "UIScreen".

I've renamed it to UISessionState, because set method named 'SetSession' doesn't
explain what is happenng. So I named them SetSessionState, and enum
UISessionState.

On the other hand, there are a lot of "enum UIState" all around chromium code.

[Shu Chen, 2014-08-07 15:40:19]

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: void
InputMethodManagerImpl::SetState(
On 2014/08/06 23:39:45, alemate wrote:
> On 2014/08/04 14:55:23, Shu Chen wrote:
> > I guess this is for fast user switching, correct? I'm concerned that this
may
> > breaks 3rd party IMEs, as enabled_extension_imes_ is not included in State.
> > 
> > I'm not familiar with the design purpose of enabled_extension_imes_. nona@
may
> > know better.
> 
> Yes, this was one of my concerns, when I've uploaded this early version of CL.
I
> also do not understand why enabled_extension_imes_ have always
> been shared between users.

I think enabled_extension_imes_ should not be shared among users.
Different users may install different 3rd party IME extensions.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:135:
ChangeInputMethodInternal(state_->current_input_method_, false);
On 2014/08/06 23:39:45, alemate wrote:
> On 2014/08/04 14:55:23, Shu Chen wrote:
> > Is there particular reason of calling ChangeInputMethodInternal instead of
> > LookupAndChangeInputMethodInternal?
> 
> The idea is that SetState should be lightweight, because it is always
> consistent. So LookupAndChangeInputMethodInternal() would be doing
> extra job of re-checking already consistent configuration.
> 
> Also, ReconfigureIMEFramework seems to be too heavy to be re-applied
> each time active user is changed. But I haven't yet investigated how
> to implement it. Are all users extensions always loaded in multiprofile
> mode? If it is so, probably we just need to properly route events to
> the current active set. What do you think?

I think ReconfigureIMEFramework() is not heavy, because:
LoadNecessaryComponentExtensions() calls
ComponentExtensionIMEManager::LoadComponentExtensionIME() calls
ComponentExtensionIMEManagerImpl::Load() which checks for existing extension:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

There are only 3 component IME extensions, and most of users would only use 1 of
them (XKB extension).
How many calls to GetExtensionById() check depend on how many input methods user
enabled.
But most of users would only enable 1 or 2 input methods.

Have you done a perf profiling to narrow down the real bottle neck?

I'm not familiar with multi-user code, but I have a feeling that when user
switching from A to B, system will unload all extensions of A and install the
extensions of B, which may hurt performance (but that may be a CPU/RAM trade
off).

Can you please briefly introduce how fast user switching works? (or any
documents I get read?)

Thanks

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

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:124:
LOG(ERROR) << "InputMethodManagerImpl::SetState(): Called.";
I suppose these logs will be removed before submitting, right?

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:497: return;
useless return;

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

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:58: //   {
input_method_id : Engine }.
s/input_method_id/extension_id

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:60: EngineMap
engine_map_;
The engine_map_ is not profile based now, and it's shared for multiple users.
So please keep this out of State.

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

https://codereview.chromium.org/419293002/diff/40001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.cc:217: //     << ", out_engine="
<< out_engine << "): Called.";
pls clean up all the logs.

[Alexander Alekseev, 2014-08-07 17:26:19]

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: void
InputMethodManagerImpl::SetState(
On 2014/08/07 15:40:19, Shu Chen wrote:
> On 2014/08/06 23:39:45, alemate wrote:
> > On 2014/08/04 14:55:23, Shu Chen wrote:
> > > I guess this is for fast user switching, correct? I'm concerned that this
> may
> > > breaks 3rd party IMEs, as enabled_extension_imes_ is not included in
State.
> > > 
> > > I'm not familiar with the design purpose of enabled_extension_imes_. nona@
> may
> > > know better.
> > 
> > Yes, this was one of my concerns, when I've uploaded this early version of
CL.
> I
> > also do not understand why enabled_extension_imes_ have always
> > been shared between users.
> 
> I think enabled_extension_imes_ should not be shared among users.
> Different users may install different 3rd party IME extensions.
> 

But it is still shared. (I don't understand why.) So I haven't changed anything
here.

https://codereview.chromium.org/419293002/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:135:
ChangeInputMethodInternal(state_->current_input_method_, false);
On 2014/08/07 15:40:19, Shu Chen wrote:
> On 2014/08/06 23:39:45, alemate wrote:
> > On 2014/08/04 14:55:23, Shu Chen wrote:
> > > Is there particular reason of calling ChangeInputMethodInternal instead of
> > > LookupAndChangeInputMethodInternal?
> > 
> > The idea is that SetState should be lightweight, because it is always
> > consistent. So LookupAndChangeInputMethodInternal() would be doing
> > extra job of re-checking already consistent configuration.
> > 
> > Also, ReconfigureIMEFramework seems to be too heavy to be re-applied
> > each time active user is changed. But I haven't yet investigated how
> > to implement it. Are all users extensions always loaded in multiprofile
> > mode? If it is so, probably we just need to properly route events to
> > the current active set. What do you think?
> 
> I think ReconfigureIMEFramework() is not heavy, because:
> LoadNecessaryComponentExtensions() calls
> ComponentExtensionIMEManager::LoadComponentExtensionIME() calls
> ComponentExtensionIMEManagerImpl::Load() which checks for existing extension:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> There are only 3 component IME extensions, and most of users would only use 1
of
> them (XKB extension).
> How many calls to GetExtensionById() check depend on how many input methods
user
> enabled.
> But most of users would only enable 1 or 2 input methods.
> 
> Have you done a perf profiling to narrow down the real bottle neck?
> 
> I'm not familiar with multi-user code, but I have a feeling that when user
> switching from A to B, system will unload all extensions of A and install the
> extensions of B, which may hurt performance (but that may be a CPU/RAM trade
> off).
> 
> Can you please briefly introduce how fast user switching works? (or any
> documents I get read?)
> 
> Thanks


Extensions are not unloaded. From the extensions point of view,
multiprofile works just like on a desktop. Each extension is
linked with user Profile, so extensions with the same profile
belong to the same user. They continue to work in background.

We've discussed the performance of LoadNecessaryComponentExtensions() +
FindEngineEntry() off this thread, so I hope you'll look into it ;-)

How User switching worked for IMs before your refactoring:

Method chromeos::Preferences::ActiveUserChanged() is called on each
user switch. (Just before it is called, GetActiveUser() starts returning
new active user). It invokes chromeos::Preferences::ApplyPrefes(), which
reinitializes IME settings according to the user profile. (And it is relatively
slow, because converting user settings to IME configuration
takes time). So that, when IME methods are called (like ChangeInputMethod()),
GetActiveUserProfile() already returns new user profile. It means that all
static data in IME is shared between users.
The only exception was the list of extensions which was per-user.

As far as I understand your previous refactoring of IME, you've left
all shared data shared, but moved GetActiveUserProfile() to
ComponentExtensionsIMEManagerImpl. It is definitely better than the
previous code, because you reduced the number of calls to
GetActiveUserProfile().

On the other hand, what I am trying to achieve: I'll put Profile* to
IME::State, so that GetActiveUser would be never needed in IME.

I think, I'll upload a new version soon.

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

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:124:
LOG(ERROR) << "InputMethodManagerImpl::SetState(): Called.";
On 2014/08/07 15:40:19, Shu Chen wrote:
> I suppose these logs will be removed before submitting, right?

Yes. As this CL is still in "concept" state, here is a lot of debug code.
I'll drop it in final version.

[Shu Chen, 2014-08-09 03:42:47]

Thanks for elaboration of how user switch works. I will investigate perf issue
in FindEntry().

Please address my comments for patch set #3.

[Shu Chen, 2014-08-09 15:49:22]

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

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:131:
ReconfigureIMFramework();
Considering user A and B. ReconfigureIMFramework() is only necessary when A
firstly adds B. After that, fast switching between A and B should not call
ReconfigureIMFramework() to improve performance.

I'm not expert for preferences.cc, so can you please confirm whether SetState()
will be called only for fast switching and not for the first user adding?

Ideally, we should remain the original code flow (through
ReplaceEnabledInputMethod) for the first user adding, and call SetState() for
fast user switching. And SetState() method should not call
ReconfigureIMFramework().

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:403:
LookupAndChangeInputMethodInternal(input_method_id, false);
Per our offline discussion, there is no need to have
LookupAndChangeInputMethodInternal().

[Alexander Alekseev, 2014-08-13 10:14:16]

This CL enables modifications of IME settings for inactive
users. So we do not need to run full IME re-initialization
on each active user change in multiprofile mode.

Also, this CL fixes IM issue on the Lock screen in multiprofile mode. (Probably
changes to screen_locker.cc are the best illustration why we need State object.)

Later I plan to allow per-user input methods for all user-POD screens (just as
it works on Login screen now).

Most changes look like converting calls from:
 input_method_manager->Method()
to
 input_method_manager->GetActiveIMEState()->Method()

This is actually the result of complete removal of calls to
GetActiveUserProfile() from IME.
Previously InputMethodManager performed most operations for
active user (silently). But now caller should specify
whether operation should be done for active user (like GetActiveIMEState()), or
for some other user (and it is implemented in Preferences and a few other parts
of code).

So additional call to GetActiveIMEState() actually changes nothing in observable
behavior.


Please review:

shuchen@, nona@, nkostylev@: all

asargent@ : chrome/browser/extensions/*

davemoore@ : chrome/browser/profiles/*

oshima@ : chrome/browser/ui/ash/*

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

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:124:
LOG(ERROR) << "InputMethodManagerImpl::SetState(): Called.";
On 2014/08/07 17:26:19, alemate wrote:
> On 2014/08/07 15:40:19, Shu Chen wrote:
> > I suppose these logs will be removed before submitting, right?
> 
> Yes. As this CL is still in "concept" state, here is a lot of debug code.
> I'll drop it in final version.

Done.

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:131:
ReconfigureIMFramework();
On 2014/08/09 15:49:22, Shu Chen wrote:
> Considering user A and B. ReconfigureIMFramework() is only necessary when A
> firstly adds B. After that, fast switching between A and B should not call
> ReconfigureIMFramework() to improve performance.
> 
> I'm not expert for preferences.cc, so can you please confirm whether
SetState()
> will be called only for fast switching and not for the first user adding?
> 
> Ideally, we should remain the original code flow (through
> ReplaceEnabledInputMethod) for the first user adding, and call SetState() for
> fast user switching. And SetState() method should not call
> ReconfigureIMFramework().

Done.

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:403:
LookupAndChangeInputMethodInternal(input_method_id, false);
On 2014/08/09 15:49:22, Shu Chen wrote:
> Per our offline discussion, there is no need to have
> LookupAndChangeInputMethodInternal().

I had to split "Lookup" and "Change" to enable modify inactive State.

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:497: return;
On 2014/08/07 15:40:19, Shu Chen wrote:
> useless return;

Done.

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

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:58: //   {
input_method_id : Engine }.
On 2014/08/07 15:40:19, Shu Chen wrote:
> s/input_method_id/extension_id

Done.

https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:60: EngineMap
engine_map_;
On 2014/08/07 15:40:19, Shu Chen wrote:
> The engine_map_ is not profile based now, and it's shared for multiple users.
> So please keep this out of State.

Done.

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

https://codereview.chromium.org/419293002/diff/40001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.cc:217: //     << ", out_engine="
<< out_engine << "): Called.";
On 2014/08/07 15:40:19, Shu Chen wrote:
> pls clean up all the logs.

Done.

[Nikita (slow), 2014-08-13 10:56:41]

lgtm with nits

I've looked on whole CL except:
chrome/browser/chromeos/input_method/*
chromeos/ime/*

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

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:33: class State
: public InputMethodManager::State {
As discussed, makes sense to rename as StateImpl.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:228: if
(UserManager::IsInitialized())
nit: Please add TODO(nkostylev) comment why this is needed.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:548: }
nit: drop {}

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:492: // FIXME(alemate):
Is this change correct?
Delete this comment, as we've checked that this conversion is still correct.

[Shu Chen, 2014-08-13 16:15:34]

I have a feeling that InputMethodManagerImpl and InputMethodManagerImpl::State
are coupled together too tightly.
InputMethodManagerImpl and State are holding each other's reference.

Do you have any ideas of decouple them?

I think it may be better to have State::Observer interface and
InputMethodManagerImpl can "observe" state changes, and do actions like
reconfig, etc.

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

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:412:
std::vector<std::string> new_active_input_method_idsfiltered;
why rename this?

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:667:
ChangeInputMethod(current_input_method.id(), false);
why remove the check: if (state_ != STATE_TERMINATING)?

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:953: void
InputMethodManagerImpl::InitializeComponentExtension(Profile* profile) {
This method will be gone in cl: https://codereview.chromium.org/467073002/.

InitializeComponentExtension() method only initialize the input method list, and
it actually won't loading any extensions (sorry for confusing naming though). So
passing profile into it seems weird.

Also, why need to call ChangeInputMethodInternal() in this method?

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

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:222:
UISessionState ui_session_;
pls update the comment.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:247:
base::WeakPtrFactory<InputMethodManagerImpl> weak_ptr_factory_;
add a blank line.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:253:
std::map<Profile*, scoped_refptr<State> > default_states_;
Can you please add comments to explain for what does "default" mean?

Also, holding a profile'd map for states in InputMethodManagerImpl seems
confusing.
Suppose states should be hold in other places which call SetState() to change
the active state.

This map seems only be used as a "cache", and I don't see any code to remove a
profile'd state from this map if a profile is deleted.

[oshima, 2014-08-13 22:59:19]

On 2014/08/13 10:14:16, alemate wrote:
> This CL enables modifications of IME settings for inactive
> users. So we do not need to run full IME re-initialization
> on each active user change in multiprofile mode.
> 
> Also, this CL fixes IM issue on the Lock screen in multiprofile mode.
(Probably
> changes to screen_locker.cc are the best illustration why we need State
object.)
> 
> Later I plan to allow per-user input methods for all user-POD screens (just as
> it works on Login screen now).
> 
> Most changes look like converting calls from:
>  input_method_manager->Method()
> to
>  input_method_manager->GetActiveIMEState()->Method()
> 
> This is actually the result of complete removal of calls to
> GetActiveUserProfile() from IME.
> Previously InputMethodManager performed most operations for
> active user (silently). But now caller should specify
> whether operation should be done for active user (like GetActiveIMEState()),
or
> for some other user (and it is implemented in Preferences and a few other
parts
> of code).
> 
> So additional call to GetActiveIMEState() actually changes nothing in
observable
> behavior.
> 
> 
> Please review:
> 
> shuchen@, nona@, nkostylev@: all
> 
> asargent@ : chrome/browser/extensions/*
> 
> davemoore@ : chrome/browser/profiles/*
> 
> oshima@ : chrome/browser/ui/ash/*

I'l wait until you and shuchen agree on the design.

> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
(right):
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:124:
> LOG(ERROR) << "InputMethodManagerImpl::SetState(): Called.";
> On 2014/08/07 17:26:19, alemate wrote:
> > On 2014/08/07 15:40:19, Shu Chen wrote:
> > > I suppose these logs will be removed before submitting, right?
> > 
> > Yes. As this CL is still in "concept" state, here is a lot of debug code.
> > I'll drop it in final version.
> 
> Done.
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:131:
> ReconfigureIMFramework();
> On 2014/08/09 15:49:22, Shu Chen wrote:
> > Considering user A and B. ReconfigureIMFramework() is only necessary when A
> > firstly adds B. After that, fast switching between A and B should not call
> > ReconfigureIMFramework() to improve performance.
> > 
> > I'm not expert for preferences.cc, so can you please confirm whether
> SetState()
> > will be called only for fast switching and not for the first user adding?
> > 
> > Ideally, we should remain the original code flow (through
> > ReplaceEnabledInputMethod) for the first user adding, and call SetState()
for
> > fast user switching. And SetState() method should not call
> > ReconfigureIMFramework().
> 
> Done.
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:403:
> LookupAndChangeInputMethodInternal(input_method_id, false);
> On 2014/08/09 15:49:22, Shu Chen wrote:
> > Per our offline discussion, there is no need to have
> > LookupAndChangeInputMethodInternal().
> 
> I had to split "Lookup" and "Change" to enable modify inactive State.
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:497: return;
> On 2014/08/07 15:40:19, Shu Chen wrote:
> > useless return;
> 
> Done.
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.h:58: //   {
> input_method_id : Engine }.
> On 2014/08/07 15:40:19, Shu Chen wrote:
> > s/input_method_id/extension_id
> 
> Done.
> 
>
https://codereview.chromium.org/419293002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.h:60: EngineMap
> engine_map_;
> On 2014/08/07 15:40:19, Shu Chen wrote:
> > The engine_map_ is not profile based now, and it's shared for multiple
users.
> > So please keep this out of State.
> 
> Done.
> 
>
https://codereview.chromium.org/419293002/diff/40001/chromeos/ime/component_e...
> File chromeos/ime/component_extension_ime_manager.cc (right):
> 
>
https://codereview.chromium.org/419293002/diff/40001/chromeos/ime/component_e...
> chromeos/ime/component_extension_ime_manager.cc:217: //     << ", out_engine="
> << out_engine << "): Called.";
> On 2014/08/07 15:40:19, Shu Chen wrote:
> > pls clean up all the logs.
> 
> Done.

[Alexander Alekseev, 2014-08-13 23:29:33]

On 2014/08/13 16:15:34, Shu Chen wrote:
> I have a feeling that InputMethodManagerImpl and InputMethodManagerImpl::State
> are coupled together too tightly.
> InputMethodManagerImpl and State are holding each other's reference.
> 
> Do you have any ideas of decouple them?
> 
> I think it may be better to have State::Observer interface and
> InputMethodManagerImpl can "observe" state changes, and do actions like
> reconfig, etc.


It is too difficult to untie them without rewriting a lot
of code: State implementation knows what is the exact implementation of
InputMethodManager is requires. So they are now paired, because most StateImpl
methods are just old
methods of IMMImpl.

But after removing of |default_states_|, the only reference
to StateImpl from IMMImpl would be |state_| member.

I would plan to move it out of IMMImpl completely when
we would have UserSession object, so that all callers would
be able to identify their session, and would behave independently. But by now it
is the simplest way to "help callers find appropriate UserSession data for IME".


So my idea was that IMM should stay a global singleton, controlling physical
input devices. But UI would be able
to create initial (empty) states, clone existing states
and switch between them. And most interaction between UI and IME would be though
State API.

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

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:412:
std::vector<std::string> new_active_input_method_idsfiltered;
On 2014/08/13 16:15:33, Shu Chen wrote:
> why rename this?

Fixed.
This was a result of mass rename.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:667:
ChangeInputMethod(current_input_method.id(), false);
On 2014/08/13 16:15:33, Shu Chen wrote:
> why remove the check: if (state_ != STATE_TERMINATING)?

The idea was to move these checks to IMM. But you're right, it is probably a
slowdown. Re-added (to StateImpl::ChangeInputMethod())

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:953: void
InputMethodManagerImpl::InitializeComponentExtension(Profile* profile) {
On 2014/08/13 16:15:33, Shu Chen wrote:
> This method will be gone in cl: https://codereview.chromium.org/467073002/.
> 
> InitializeComponentExtension() method only initialize the input method list,
and
> it actually won't loading any extensions (sorry for confusing naming though).
So
> passing profile into it seems weird.
> 
> Also, why need to call ChangeInputMethodInternal() in this method?

This call happens after PostProfileInit for signin profile, so State probably
has some data.

But actually I am waiting for your CL to land, to remove this.

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

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:33: class State
: public InputMethodManager::State {
On 2014/08/13 10:56:41, Nikita Kostylev wrote:
> As discussed, makes sense to rename as StateImpl.

Done.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:222:
UISessionState ui_session_;
On 2014/08/13 16:15:34, Shu Chen wrote:
> pls update the comment.

Done.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:247:
base::WeakPtrFactory<InputMethodManagerImpl> weak_ptr_factory_;
On 2014/08/13 16:15:33, Shu Chen wrote:
> add a blank line.

Done.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:253:
std::map<Profile*, scoped_refptr<State> > default_states_;
On 2014/08/13 16:15:34, Shu Chen wrote:
> Can you please add comments to explain for what does "default" mean?
> 
> Also, holding a profile'd map for states in InputMethodManagerImpl seems
> confusing.
> Suppose states should be hold in other places which call SetState() to change
> the active state.

Before your recent CL, there were two independant ways to initialize
State :
1) From ChromeOS UI code, when we need to apply settings.
2) From Component loader, when extension was loaded.

So I had to put this map in IMM, because this was the only reachable
place for both.

I am ready to move it to UserSessionManager, where it probably should be as soon
as you commit https://codereview.chromium.org/467073002/ .

Actually it should be a map like { UserSession => State }, but we do
not have UserSession object yet.


> This map seems only be used as a "cache", and I don't see any code to remove a
> profile'd state from this map if a profile is deleted.

This is done on purpose (for now), as profiles are never deleted.
But I'll move it out of IMM after your CL.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:228: if
(UserManager::IsInitialized())
On 2014/08/13 10:56:41, Nikita Kostylev wrote:
> nit: Please add TODO(nkostylev) comment why this is needed.

Done.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:548: }
On 2014/08/13 10:56:41, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/419293002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:492: // FIXME(alemate):
Is this change correct?
On 2014/08/13 10:56:41, Nikita Kostylev wrote:
> Delete this comment, as we've checked that this conversion is still correct.

Done.

[Alexander Alekseev, 2014-08-14 01:59:26]

I've moved default_ime_states_ to UserSessionManager.

PTAL.

[Shu Chen, 2014-08-14 15:26:57]

lgtm

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

https://codereview.chromium.org/419293002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:156: }
nit: can you please sort the code for StateImpl together?

[Alexander Alekseev, 2014-08-14 18:06:12]

oshima@
nona@
asargent@

- what do you think?

[oshima, 2014-08-14 18:09:44]

c/b/ui/ash lgtm

[Nikita (slow), 2014-08-15 09:05:37]

Please link to https://code.google.com/p/chromium/issues/detail?id=399129

[Alexander Alekseev, 2014-08-15 16:04:33]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-08-15 16:05:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/419293002/260001

[Nikita (slow), 2014-08-15 18:07:50]

You've got a real crash in unit_test:

http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

PreferencesTest.TestUpdatePrefOnBrowserScreenDetails (run #1):
[ RUN      ] PreferencesTest.TestUpdatePrefOnBrowserScreenDetails
Received signal 11 SEGV_MAPERR 000000000000
#0 0x0000034178da base::debug::StackTrace::StackTrace()
#1 0x0000034173f6 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fdc1d9a5cb0 \u003Cunknown>
#3 0x000005b9e195 chromeos::Preferences::SetLanguageConfigStringListAsCSV()
#4 0x000005b9d9d9 chromeos::Preferences::SetInputMethodList()
#5 0x000000c36d96
chromeos::PreferencesTest_TestUpdatePrefOnBrowserScreenDetails_Test::TestBody()
#6 0x000002fa394f testing::Test::Run()
#7 0x000002fa48ce testing::TestInfo::Run()
#8 0x000002fa4d8f testing::TestCase::Run()
#9 0x000002facab6 testing::internal::UnitTestImpl::RunAllTests()
#10 0x000002fac75e testing::UnitTest::Run()
#11 0x0000065985ed base::TestSuite::Run()
#12 0x0000065951cd base::(anonymous namespace)::LaunchUnitTestsInternal()
#13 0x000006594fa3 base::LaunchUnitTests()
#14 0x00000305a192 main
#15 0x7fdc1bebe76d __libc_start_main
#16 0x0000004b9ac1 \u003Cunknown>
  r8: 0000000007bf2c90  r9: 00007fdc1bfdff80 r10: 676175676e616c2e r11:
00007fdc1c020450
 r12: 00007fffab1df9e8 r13: 00007fffab1dfe58 r14: 0000000007bf2c90 r15:
0000000007bf2c88
  di: 0000000000000000  si: 0000347177147be0  bp: 0000000008f4dee0  bx:
00007fffab1df501
  dx: 0000347176c53000  ax: 0000000000000b9f  cx: 0000000000000001  sp:
00007fffab1df590
  ip: 0000000005b9e195 efl: 0000000000010202 cgf: 0000000000000033 erf:
0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000000

[commit-bot: I haz the power, 2014-08-15 18:43:47]

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-15 19:21:08]

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

[commit-bot: I haz the power, 2014-08-15 19:21:10]

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_...)

[Alexander Alekseev, 2014-08-15 19:46:08]

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

https://codereview.chromium.org/419293002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:156: }
On 2014/08/14 15:26:57, Shu Chen wrote:
> nit: can you please sort the code for StateImpl together?

Done.

[Alexander Alekseev, 2014-08-15 19:47:00]

On 2014/08/15 18:07:50, Nikita Kostylev wrote:
> You've got a real crash in unit_test:
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> PreferencesTest.TestUpdatePrefOnBrowserScreenDetails (run #1):
> [ RUN      ] PreferencesTest.TestUpdatePrefOnBrowserScreenDetails
> Received signal 11 SEGV_MAPERR 000000000000
> #0 0x0000034178da base::debug::StackTrace::StackTrace()
> #1 0x0000034173f6 base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7fdc1d9a5cb0 \u003Cunknown>
> #3 0x000005b9e195 chromeos::Preferences::SetLanguageConfigStringListAsCSV()
> #4 0x000005b9d9d9 chromeos::Preferences::SetInputMethodList()
> #5 0x000000c36d96
>
chromeos::PreferencesTest_TestUpdatePrefOnBrowserScreenDetails_Test::TestBody()
> #6 0x000002fa394f testing::Test::Run()
> #7 0x000002fa48ce testing::TestInfo::Run()
> #8 0x000002fa4d8f testing::TestCase::Run()
> #9 0x000002facab6 testing::internal::UnitTestImpl::RunAllTests()
> #10 0x000002fac75e testing::UnitTest::Run()
> #11 0x0000065985ed base::TestSuite::Run()
> #12 0x0000065951cd base::(anonymous namespace)::LaunchUnitTestsInternal()
> #13 0x000006594fa3 base::LaunchUnitTests()
> #14 0x00000305a192 main
> #15 0x7fdc1bebe76d __libc_start_main
> #16 0x0000004b9ac1 \u003Cunknown>
>   r8: 0000000007bf2c90  r9: 00007fdc1bfdff80 r10: 676175676e616c2e r11:
> 00007fdc1c020450
>  r12: 00007fffab1df9e8 r13: 00007fffab1dfe58 r14: 0000000007bf2c90 r15:
> 0000000007bf2c88
>   di: 0000000000000000  si: 0000347177147be0  bp: 0000000008f4dee0  bx:
> 00007fffab1df501
>   dx: 0000347176c53000  ax: 0000000000000b9f  cx: 0000000000000001  sp:
> 00007fffab1df590
>   ip: 0000000005b9e195 efl: 0000000000010202 cgf: 0000000000000033 erf:
> 0000000000000004
>  trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000000

Thank you. Fixed.

[Alexander Alekseev, 2014-08-15 19:48:17]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-08-15 19:50:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/419293002/280001

[commit-bot: I haz the power, 2014-08-15 23:31:50]

Committed patchset #15 (280001) as 290048