Make virtual keyboard work on supervised user creation

Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too.

BUG=712873
TEST=Added unit test. Manually tested VK works:
- on supervised user creation window
- after multiprofile switch

Review-Url: https://codereview.chromium.org/2852403002
Cr-Commit-Position: refs/heads/master@{#480878}
Committed: https://chromium.googlesource.com/chromium/src/+/c21941d2b11067e23073143692fe5968af3875f7

[oka, 2017-05-02 14:33:43]

Description was changed from

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACITVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why the keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too. To avoid needless recreation, previous state is stored and recreation
doesn't happen when the state moves from LOGGED_IN_ACTIVE to ACTIVE or vise
versa.

BUG=712873
TEST=manually tested VK works on supervised user creation.
==========

to

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACITVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too. To avoid needless recreation, previous state is stored and recreation
doesn't happen when the state moves from LOGGED_IN_ACTIVE to ACTIVE or vise
versa.

BUG=712873
TEST=manually tested VK works on supervised user creation.
==========

[oka, 2017-05-02 14:34:40]

oka@chromium.org changed reviewers:
+ oshima@chromium.org

[oka, 2017-05-02 14:34:40]

oshima@

Could you take a look?
Thank you!

[oka, 2017-05-02 14:34:48]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 14:35:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oka, 2017-05-02 14:35:16]

Description was changed from

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACITVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too. To avoid needless recreation, previous state is stored and recreation
doesn't happen when the state moves from LOGGED_IN_ACTIVE to ACTIVE or vise
versa.

BUG=712873
TEST=manually tested VK works on supervised user creation.
==========

to

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too. To avoid needless recreation, previous state is stored and recreation
doesn't happen when the state moves from LOGGED_IN_ACTIVE to ACTIVE or vise
versa.

BUG=712873
TEST=manually tested VK works on supervised user creation.
==========

[commit-bot: I haz the power, 2017-05-02 15:43:49]

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

[commit-bot: I haz the power, 2017-05-02 15:43:50]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-05-03 14:49:05]

oshima@chromium.org changed reviewers:
+ alemate@chromium.org

[oshima, 2017-05-03 14:49:05]

I'm not familiar with supervisor user creation process.
alemate@, can you review this?

oka-san, can you add a unit test for this?

[Alexander Alekseev, 2017-05-12 04:05:21]

alemate@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Alexander Alekseev, 2017-05-12 04:05:21]

+xiyuan

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc#newcode1230
ash/shell.cc:1230: if (GetAshConfig() != Config::MASH) {
Did you verify that multiprofile mode works with this change?

[xiyuan, 2017-05-12 17:30:03]

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc#newcode1229
ash/shell.cc:1229: previous_state_ == session_manager::SessionState::ACTIVE)) {
How about using KeyboardController::GetInstance() to suppress duplicate calls
instead of using a |previous_state_|?

e.g.

  if ((state == SessionState::LOGGED_IN_NOT_ACTIVE ||
       state == SessionState::ACTIVE) &&
       keyboard::IsKeyboardEnabled()) &&
       !KeyboardController::GetInstance()) {
  }

[oka, 2017-06-03 02:26:03]

Wip: address comments

[oka, 2017-06-03 02:26:59]

Wip2: address comments

[oka, 2017-06-03 02:28:58]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-03 02:29:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-03 02:31:51]

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

[commit-bot: I haz the power, 2017-06-03 02:31:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[oka, 2017-06-03 02:33:31]

Rebase.

[oka, 2017-06-03 02:35:53]

Rebase.

[oka, 2017-06-03 02:36:03]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-03 02:36:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-03 02:55:26]

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

[commit-bot: I haz the power, 2017-06-03 02:55:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oka, 2017-06-03 02:56:11]

Compile.

[oka, 2017-06-06 12:45:37]

Make ActiveKeyboard do nothing if keyboard_controller is not initialized.

[oka, 2017-06-06 12:45:46]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 12:45:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 13:35:27]

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

[commit-bot: I haz the power, 2017-06-06 13:35:28]

Dry run: This issue passed the CQ dry run.

[oka, 2017-06-07 07:17:20]

.

[oka, 2017-06-07 12:38:30]

Description was changed from

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too. To avoid needless recreation, previous state is stored and recreation
doesn't happen when the state moves from LOGGED_IN_ACTIVE to ACTIVE or vise
versa.

BUG=712873
TEST=manually tested VK works on supervised user creation.
==========

to

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too, whenever the keyboard is enabled and the controller is not initialized.

BUG=712873
TEST=manually tested VK works on supervised user creation. Added unit test.
==========

[oka, 2017-06-07 12:38:44]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 12:38:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oka, 2017-06-07 12:39:54]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 12:40:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oka, 2017-06-07 12:41:54]

Description was changed from

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too, whenever the keyboard is enabled and the controller is not initialized.

BUG=712873
TEST=manually tested VK works on supervised user creation. Added unit test.
==========

to

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too, whenever the keyboard is enabled and the controller is not initialized.

BUG=712873
TEST=Added unit test. Manually tested VK works:
- on supervised user creation window
- after multiprofile switch
==========

[oka, 2017-06-07 12:42:37]

PTAL. (sorry for belated response).

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc#newcode1229
ash/shell.cc:1229: previous_state_ == session_manager::SessionState::ACTIVE)) {
On 2017/05/12 17:30:03, xiyuan wrote:
> How about using KeyboardController::GetInstance() to suppress duplicate calls
> instead of using a |previous_state_|?
> 
> e.g.
> 
>   if ((state == SessionState::LOGGED_IN_NOT_ACTIVE ||
>        state == SessionState::ACTIVE) &&
>        keyboard::IsKeyboardEnabled()) &&
>        !KeyboardController::GetInstance()) {
>   }

Updated.

https://codereview.chromium.org/2852403002/diff/1/ash/shell.cc#newcode1230
ash/shell.cc:1230: if (GetAshConfig() != Config::MASH) {
On 2017/05/12 04:05:21, Alexander Alekseev wrote:
> Did you verify that multiprofile mode works with this change?

I manually confirmed keyboard works after multiprofile switch.

[commit-bot: I haz the power, 2017-06-07 13:27:43]

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

[commit-bot: I haz the power, 2017-06-07 13:27:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2017-06-07 15:08:13]

https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:179: keyboard::KeyboardController::GetInstance());
If only that test fails without this, should this be part of that test's SetUp
instead?

ash_test_helper_->SetUp() call above should set ACTIVE session state and
CreateKeyboard should be exercised. Not sure why we need to do this now.

[oka, 2017-06-07 15:45:20]

https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:179: keyboard::KeyboardController::GetInstance());
On 2017/06/07 15:08:13, xiyuan wrote:
> If only that test fails without this, should this be part of that test's SetUp
> instead?
> 
> ash_test_helper_->SetUp() call above should set ACTIVE session state and
> CreateKeyboard should be exercised. Not sure why we need to do this now.

ash_test_helper_->SetUp() initializes the keyboard controller instance from
RootWindowController::Init(), before the state becomes active. The same thing
happens on real OS on start up.
However, on real workflow, RootWindowController::ActivateKeyboard is called
after that from ChromeBrowserMainExtraPartsAsh::PostProfileInit and the
controller  becomes a sane state.

[xiyuan, 2017-06-07 16:18:13]

https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:179: keyboard::KeyboardController::GetInstance());
On 2017/06/07 15:45:19, oka wrote:
> On 2017/06/07 15:08:13, xiyuan wrote:
> > If only that test fails without this, should this be part of that test's
SetUp
> > instead?
> > 
> > ash_test_helper_->SetUp() call above should set ACTIVE session state and
> > CreateKeyboard should be exercised. Not sure why we need to do this now.
> 
> ash_test_helper_->SetUp() initializes the keyboard controller instance from
> RootWindowController::Init(), before the state becomes active. The same thing
> happens on real OS on start up.
> However, on real workflow, RootWindowController::ActivateKeyboard is called
> after that from ChromeBrowserMainExtraPartsAsh::PostProfileInit and the
> controller  becomes a sane state.

But Shell::CreateKeyboard calls ActivateKeyboard too. So if session state
changes to ACTIVE, Shell::CreateKeyboard should do that already. I don't
understand why we need to do this explicitlyl again here.

[oka, 2017-06-08 00:01:42]

On 2017/06/07 16:18:13, xiyuan wrote:
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
> File ash/test/ash_test_base.cc (right):
> 
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
> ash/test/ash_test_base.cc:179: keyboard::KeyboardController::GetInstance());
> On 2017/06/07 15:45:19, oka wrote:
> > On 2017/06/07 15:08:13, xiyuan wrote:
> > > If only that test fails without this, should this be part of that test's
> SetUp
> > > instead?
> > > 
> > > ash_test_helper_->SetUp() call above should set ACTIVE session state and
> > > CreateKeyboard should be exercised. Not sure why we need to do this now.
> > 
> > ash_test_helper_->SetUp() initializes the keyboard controller instance from
> > RootWindowController::Init(), before the state becomes active. The same
thing
> > happens on real OS on start up.
> > However, on real workflow, RootWindowController::ActivateKeyboard is called
> > after that from ChromeBrowserMainExtraPartsAsh::PostProfileInit and the
> > controller  becomes a sane state.
> 
> But Shell::CreateKeyboard calls ActivateKeyboard too. So if session state
> changes to ACTIVE, Shell::CreateKeyboard should do that already. I don't
> understand why we need to do this explicitlyl again here.

RootWindowController::Init() only calls Shell::InitKeyboard [1], but doesn't
call RootWindowController::ActivateKeyboard (I don't know why. Maybe we should
call Shell::CreateKeyboard instead there).
Shell::CreateKeyboard calls both Shell::InitKeyboard and
RootWindowController::ActivateKeyboard.
On start up workflow, when the session state becomes active, the keyboard
instance is already created by [1], so OnSessionStateChanged is no-op for
keyboard. However, on the test in question,
RootWindowController::ActivateKeyboard in not called as aforementioned. Thus the
test failure happens. 

[1]
https://cs.chromium.org/chromium/src/ash/root_window_controller.cc?type=cs&q=...

[xiyuan, 2017-06-08 15:14:40]

On 2017/06/08 00:01:42, oka wrote:
> On 2017/06/07 16:18:13, xiyuan wrote:
> >
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
> > File ash/test/ash_test_base.cc (right):
> > 
> >
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
> > ash/test/ash_test_base.cc:179: keyboard::KeyboardController::GetInstance());
> > On 2017/06/07 15:45:19, oka wrote:
> > > On 2017/06/07 15:08:13, xiyuan wrote:
> > > > If only that test fails without this, should this be part of that test's
> > SetUp
> > > > instead?
> > > > 
> > > > ash_test_helper_->SetUp() call above should set ACTIVE session state and
> > > > CreateKeyboard should be exercised. Not sure why we need to do this now.
> > > 
> > > ash_test_helper_->SetUp() initializes the keyboard controller instance
from
> > > RootWindowController::Init(), before the state becomes active. The same
> thing
> > > happens on real OS on start up.
> > > However, on real workflow, RootWindowController::ActivateKeyboard is
called
> > > after that from ChromeBrowserMainExtraPartsAsh::PostProfileInit and the
> > > controller  becomes a sane state.
> > 
> > But Shell::CreateKeyboard calls ActivateKeyboard too. So if session state
> > changes to ACTIVE, Shell::CreateKeyboard should do that already. I don't
> > understand why we need to do this explicitlyl again here.
> 
> RootWindowController::Init() only calls Shell::InitKeyboard [1], but doesn't
> call RootWindowController::ActivateKeyboard (I don't know why. Maybe we should
> call Shell::CreateKeyboard instead there).
> Shell::CreateKeyboard calls both Shell::InitKeyboard and
> RootWindowController::ActivateKeyboard.
> On start up workflow, when the session state becomes active, the keyboard
> instance is already created by [1], so OnSessionStateChanged is no-op for
> keyboard. However, on the test in question,
> RootWindowController::ActivateKeyboard in not called as aforementioned. Thus
the
> test failure happens. 
> 
> [1]
>
https://cs.chromium.org/chromium/src/ash/root_window_controller.cc?type=cs&q=...

I still don't understand why ActivateKeyboard is not called for the test case.

Quote from your previous response:

> > ash_test_helper_->SetUp() initializes the keyboard controller instance from
> > RootWindowController::Init(), before the state becomes active.

Whether keyboard is initialized or not, when session state becomes ACTIVE, shell
should call CreateKeyboard, right? Wouldn't that ActivateKeyboard? What did I
miss?

[oka, 2017-06-14 07:49:52]

On 2017/06/08 15:14:40, xiyuan wrote:
> On 2017/06/08 00:01:42, oka wrote:
> > On 2017/06/07 16:18:13, xiyuan wrote:
> > >
> >
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
> > > File ash/test/ash_test_base.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
> > > ash/test/ash_test_base.cc:179:
keyboard::KeyboardController::GetInstance());
> > > On 2017/06/07 15:45:19, oka wrote:
> > > > On 2017/06/07 15:08:13, xiyuan wrote:
> > > > > If only that test fails without this, should this be part of that
test's
> > > SetUp
> > > > > instead?
> > > > > 
> > > > > ash_test_helper_->SetUp() call above should set ACTIVE session state
and
> > > > > CreateKeyboard should be exercised. Not sure why we need to do this
now.
> > > > 
> > > > ash_test_helper_->SetUp() initializes the keyboard controller instance
> from
> > > > RootWindowController::Init(), before the state becomes active. The same
> > thing
> > > > happens on real OS on start up.
> > > > However, on real workflow, RootWindowController::ActivateKeyboard is
> called
> > > > after that from ChromeBrowserMainExtraPartsAsh::PostProfileInit and the
> > > > controller  becomes a sane state.
> > > 
> > > But Shell::CreateKeyboard calls ActivateKeyboard too. So if session state
> > > changes to ACTIVE, Shell::CreateKeyboard should do that already. I don't
> > > understand why we need to do this explicitlyl again here.
> > 
> > RootWindowController::Init() only calls Shell::InitKeyboard [1], but doesn't
> > call RootWindowController::ActivateKeyboard (I don't know why. Maybe we
should
> > call Shell::CreateKeyboard instead there).
> > Shell::CreateKeyboard calls both Shell::InitKeyboard and
> > RootWindowController::ActivateKeyboard.
> > On start up workflow, when the session state becomes active, the keyboard
> > instance is already created by [1], so OnSessionStateChanged is no-op for
> > keyboard. However, on the test in question,
> > RootWindowController::ActivateKeyboard in not called as aforementioned. Thus
> the
> > test failure happens. 
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/ash/root_window_controller.cc?type=cs&q=...
> 
> I still don't understand why ActivateKeyboard is not called for the test case.
> 
> Quote from your previous response:
> 
> > > ash_test_helper_->SetUp() initializes the keyboard controller instance
from
> > > RootWindowController::Init(), before the state becomes active.
> 
> Whether keyboard is initialized or not, when session state becomes ACTIVE,
shell
> should call CreateKeyboard, right? Wouldn't that ActivateKeyboard? What did I
> miss?

I mentioned the following three functions:
(A) CreateKeyboard
  (1) InitKeyboard
  (2) ActivateKeyboard

(A) calls both (1) and (2).
Both (1) and (2) need be called to set up keyboard controller properly.

Session state change to ACTIVE triggers (A) **If (1) is not called before in the
same session (i.e. if there is no keyboard controller instance).**
What happens on real device is:
- On start up:
  (1) is called from RootWindowController::Init.
  (2) is called from ChromeBrowserMainExtraPartsAsh::PostProfileInit.
  (A) is NOT called when session state becomes ACTIVE because (1) has been
called.
- On profile change:
  keyboard controller is destructed and then (A) is called when session state
becomes ACTIVE.

However in the test in question, only (1) is called and (2) is not called,
leading test failure due to inconsistent controller state.
(Previously, (A) was always called and thus the test didn't fail.)
My proposal is to fix the test by explicitly calling (2) on the test setup.

Hope it makes sense. Thanks.

[oka, 2017-06-14 08:35:50]

Fix mash test failure: move virtual keyboard specific set up from ash_test_best
to a particular test.

[oka, 2017-06-14 14:55:10]

On 2017/06/14 07:49:52, oka wrote:
> On 2017/06/08 15:14:40, xiyuan wrote:
> > On 2017/06/08 00:01:42, oka wrote:
> > > On 2017/06/07 16:18:13, xiyuan wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base.cc
> > > > File ash/test/ash_test_base.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2852403002/diff/220001/ash/test/ash_test_base...
> > > > ash/test/ash_test_base.cc:179:
> keyboard::KeyboardController::GetInstance());
> > > > On 2017/06/07 15:45:19, oka wrote:
> > > > > On 2017/06/07 15:08:13, xiyuan wrote:
> > > > > > If only that test fails without this, should this be part of that
> test's
> > > > SetUp
> > > > > > instead?
> > > > > > 
> > > > > > ash_test_helper_->SetUp() call above should set ACTIVE session state
> and
> > > > > > CreateKeyboard should be exercised. Not sure why we need to do this
> now.
> > > > > 
> > > > > ash_test_helper_->SetUp() initializes the keyboard controller instance
> > from
> > > > > RootWindowController::Init(), before the state becomes active. The
same
> > > thing
> > > > > happens on real OS on start up.
> > > > > However, on real workflow, RootWindowController::ActivateKeyboard is
> > called
> > > > > after that from ChromeBrowserMainExtraPartsAsh::PostProfileInit and
the
> > > > > controller  becomes a sane state.
> > > > 
> > > > But Shell::CreateKeyboard calls ActivateKeyboard too. So if session
state
> > > > changes to ACTIVE, Shell::CreateKeyboard should do that already. I don't
> > > > understand why we need to do this explicitlyl again here.
> > > 
> > > RootWindowController::Init() only calls Shell::InitKeyboard [1], but
doesn't
> > > call RootWindowController::ActivateKeyboard (I don't know why. Maybe we
> should
> > > call Shell::CreateKeyboard instead there).
> > > Shell::CreateKeyboard calls both Shell::InitKeyboard and
> > > RootWindowController::ActivateKeyboard.
> > > On start up workflow, when the session state becomes active, the keyboard
> > > instance is already created by [1], so OnSessionStateChanged is no-op for
> > > keyboard. However, on the test in question,
> > > RootWindowController::ActivateKeyboard in not called as aforementioned.
Thus
> > the
> > > test failure happens. 
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/ash/root_window_controller.cc?type=cs&q=...
> > 
> > I still don't understand why ActivateKeyboard is not called for the test
case.
> > 
> > Quote from your previous response:
> > 
> > > > ash_test_helper_->SetUp() initializes the keyboard controller instance
> from
> > > > RootWindowController::Init(), before the state becomes active.
> > 
> > Whether keyboard is initialized or not, when session state becomes ACTIVE,
> shell
> > should call CreateKeyboard, right? Wouldn't that ActivateKeyboard? What did
I
> > miss?
> 
> I mentioned the following three functions:
> (A) CreateKeyboard
>   (1) InitKeyboard
>   (2) ActivateKeyboard
> 
> (A) calls both (1) and (2).
> Both (1) and (2) need be called to set up keyboard controller properly.
> 
> Session state change to ACTIVE triggers (A) **If (1) is not called before in
the
> same session (i.e. if there is no keyboard controller instance).**
> What happens on real device is:
> - On start up:
>   (1) is called from RootWindowController::Init.
>   (2) is called from ChromeBrowserMainExtraPartsAsh::PostProfileInit.
>   (A) is NOT called when session state becomes ACTIVE because (1) has been
> called.
> - On profile change:
>   keyboard controller is destructed and then (A) is called when session state
> becomes ACTIVE.
> 
> However in the test in question, only (1) is called and (2) is not called,
> leading test failure due to inconsistent controller state.
> (Previously, (A) was always called and thus the test didn't fail.)
> My proposal is to fix the test by explicitly calling (2) on the test setup.
> 
> Hope it makes sense. Thanks.

Hi. I will submit https://chromium-review.googlesource.com/c/535221/ first and
then the test specific set up will not be needed.
Thanks.

[oka, 2017-06-14 15:04:22]

Rebase on top of the change to inline InitKeyboard

[oka, 2017-06-14 15:08:02]

PTAL.

[oka, 2017-06-14 15:08:13]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:08:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 16:36:34]

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

[commit-bot: I haz the power, 2017-06-14 16:36:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2017-06-14 16:47:23]

Thanks for the investigation. It makes sense to me now. I missed the added
!keyboard::KeyboardController::GetInstance() check that skips CreateKeyboard
(along with ActivateKeyboard) when there is an existing controller instance.

In this case, I think it is okay to add that in test setup. So #12 lgtm now.

If the other CL (https://chromium-review.googlesource.com/c/535221/) does not
have problem with profiles (e.g. RootWindowController::Init is invoked too early
before profile is loaded etc), #15 is even better.

[oka, 2017-06-14 18:34:37]

Back to 13.

[Alexander Alekseev, 2017-06-16 20:29:23]

alemate@chromium.org changed reviewers:
- alemate@chromium.org

[oka, 2017-06-18 23:20:57]

Remove !GetInstance check to fix tests (and real problem)

[oka, 2017-06-18 23:22:59]

On 2017/06/14 18:34:37, oka wrote:
> Back to 13.

I removed the !keyboard::KeyboardController::GetInstance() check because
KeyboardController should be re-created on profile change even if there is an
existing controller, so that the correct Profile is associated with keyboardUI
instance in [1].
I somehow misunderstood that KeyboardController is deconstructed on profile
change, but it was not true. That's why some tests, including
KeyboardEndToEndTest.OpenIfFocusedOnClick were failing.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/chrome_shell_deleg...

[oka, 2017-06-18 23:53:32]

Fix compile error

[oka, 2017-06-19 01:50:21]

Fix ShellTest.KeyboardCreation.

[oka, 2017-06-19 09:32:37]

xiyuan@ Could you take another look?

[xiyuan, 2017-06-19 15:31:06]

still lgtm

[oka, 2017-06-19 22:58:57]

The CQ bit was checked by oka@chromium.org

[commit-bot: I haz the power, 2017-06-19 22:59:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 23:09:23]

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

[commit-bot: I haz the power, 2017-06-19 23:09:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oka, 2017-06-19 23:15:57]

oshima@ could you take a look?
I need your OWNERs approval.

[oka, 2017-06-20 17:23:46]

Description was changed from

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too, whenever the keyboard is enabled and the controller is not initialized.

BUG=712873
TEST=Added unit test. Manually tested VK works:
- on supervised user creation window
- after multiprofile switch
==========

to

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too.

BUG=712873
TEST=Added unit test. Manually tested VK works:
- on supervised user creation window
- after multiprofile switch
==========

[oshima, 2017-06-20 17:45:09]

lgtm

[oka, 2017-06-20 17:49:20]

The CQ bit was checked by oka@chromium.org

[commit-bot: I haz the power, 2017-06-20 17:49:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 17:54:49]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1497980960557970,
"parent_rev": "14deaa4b7ea427d2a9e013dec4e4a9d6d8d7e58c", "commit_rev":
"c21941d2b11067e23073143692fe5968af3875f7"}

[commit-bot: I haz the power, 2017-06-20 17:55:02]

Description was changed from

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too.

BUG=712873
TEST=Added unit test. Manually tested VK works:
- on supervised user creation window
- after multiprofile switch
==========

to

==========
Make virtual keyboard work on supervised user creation

Keyboard should be recreated with Shell::CreateKeyboard per user sign
in/out so to associate the new user to keyboard extensions. If the
keyboard extensions were associated with the previous user, it in turn
associated with an inactive IME and thus chrome.input.ime.sendKeyEvents
does nothing.

Currently the keyboard is recreated in Shell::OnSessionStateChanged when
the session state becomes ACTIVE. However, on the supervised user
creation window, the session state is not ACTIVE, but
LOGGED_IN_NOT_ACTIVE, which represents the state where the user
has signed in but the login UI is not hidden yet.
That's why virtual keyboard was not working there.

This CL fixes the issue by recreating the keyboard on LOGGED_IN_NOT_ACTIVE
too.

BUG=712873
TEST=Added unit test. Manually tested VK works:
- on supervised user creation window
- after multiprofile switch

Review-Url: https://codereview.chromium.org/2852403002
Cr-Commit-Position: refs/heads/master@{#480878}
Committed:
https://chromium.googlesource.com/chromium/src/+/c21941d2b11067e23073143692fe...
==========

[commit-bot: I haz the power, 2017-06-20 17:55:03]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/c21941d2b11067e23073143692fe...