Whitelist virtual keyboard container to process events at login screen

Whitelist virtual keyboard container to process events at login screen


When user session is blocked (login screen, lock etc.), virtual keyboard container
window should still be able to process events to function properly.

BUG=263599
TEST=
1. Enable virtual keyboard
2. go to login screen
verify if you can use virtual keyboard to type you password and login

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

[bshe, 2013-08-06 23:22:37]

Hi Nikita.

Would you mind to take a look at this CL? It tries to fix the regression that
introduced by https://chromiumcodereview.appspot.com/18850003.

Thanks!

[Nikita (slow), 2013-08-07 08:40:57]

+Pavel for review.

Please add ash_unittest that covers this case.

[dzhioev (left Google), 2013-08-07 12:26:17]

On 2013/08/06 23:22:37, bshe wrote:
> Hi Nikita.
> 
> Would you mind to take a look at this CL? It tries to fix the regression that
> introduced by https://chromiumcodereview.appspot.com/18850003.
> 
> Thanks!

LGTM

[bshe, 2013-08-08 16:09:26]

I added ash unit test for virtual keyboard container. Also, changed the check in
event_client_impl to restrict to virtual keyboard only. Would you guys mind to
do another round of review? Thanks!

Also adding owners:
+oshima for root_window_controller*
+jamescook for ash/*

[oshima, 2013-08-08 17:19:29]

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:513: }
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:520: UpdateDisplay("600x600");
do you need to change the size of the display for this test?

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:521: aura::RootWindow* root_window =
Shell::GetInstance()->GetPrimaryRootWindow();
GetPrimaryRootWindow is static

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.cc:32: return;
nuke return;

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.cc:35: }  // namespace keyboard
ash

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
File ash/shell/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.h:22: 
nit: no new lines for overridden methods.

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.h:32: }  // namespace keyboard
nit: namespace ash

[James Cook, 2013-08-08 17:29:44]

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:107: int mouse_presses() const { return
mouse_presses_; }
nit: blank line after this

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:522: aura::Window* vk_container =
Shell::GetContainer(root_window,
no abbreviations: vk_container -> keyboard_container or similar please

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:524: CHECK(vk_container);
optional nit: ASSERT_TRUE ?

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:528: scoped_ptr<aura::Window> vk_window(
vk_window -> keyboard_window or similar

https://codereview.chromium.org/22465007/diff/10001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell_window_ids.h#ne...
ash/shell_window_ids.h:106: const int kShellWindowId_VirtualKeyboardContainer =
24;
Where does this container actually sit in the hierarchy of containers?  Unless
it is actually on top of the power button animation container, this line should
go in the middle of the file somewhere and the IDs all adjusted.

https://codereview.chromium.org/22465007/diff/10001/ash/wm/event_client_impl.cc
File ash/wm/event_client_impl.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/wm/event_client_impl....
ash/wm/event_client_impl.cc:27: if
(Shell::GetInstance()->session_state_delegate()->IsUserSessionBlocked()) {
nit: this function might be clearer with an early return:

if (!IsUserSessionBlocked())
  return true;

[bshe, 2013-08-08 19:37:01]

Thanks for quick review!

Address comments separately, this one is for oshima@'s comment

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:513: }
On 2013/08/08 17:19:29, oshima wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:520: UpdateDisplay("600x600");
No. I was just following the above example. It is not necessary for this test.

On 2013/08/08 17:19:29, oshima wrote:
> do you need to change the size of the display for this test?

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:521: aura::RootWindow* root_window =
Shell::GetInstance()->GetPrimaryRootWindow();
On 2013/08/08 17:19:29, oshima wrote:
> GetPrimaryRootWindow is static 

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.cc:32: return;
On 2013/08/08 17:19:29, oshima wrote:
> nuke return;

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.cc:35: }  // namespace keyboard
On 2013/08/08 17:19:29, oshima wrote:
> ash

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
File ash/shell/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.h:22: 
On 2013/08/08 17:19:29, oshima wrote:
> nit: no new lines for overridden methods.

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.h:32: }  // namespace keyboard
Done
On 2013/08/08 17:19:29, oshima wrote:
> nit: namespace ash

[bshe, 2013-08-08 19:39:23]

And this is for jamescook@

Thanks!

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:107: int mouse_presses() const { return
mouse_presses_; }
On 2013/08/08 17:29:44, James Cook wrote:
> nit: blank line after this

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:522: aura::Window* vk_container =
Shell::GetContainer(root_window,
On 2013/08/08 17:29:44, James Cook wrote:
> no abbreviations: vk_container -> keyboard_container or similar please

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:524: CHECK(vk_container);
On 2013/08/08 17:29:44, James Cook wrote:
> optional nit: ASSERT_TRUE ?

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:528: scoped_ptr<aura::Window> vk_window(
On 2013/08/08 17:29:44, James Cook wrote:
> vk_window -> keyboard_window or similar

Done.

https://codereview.chromium.org/22465007/diff/10001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell_window_ids.h#ne...
ash/shell_window_ids.h:106: const int kShellWindowId_VirtualKeyboardContainer =
24;
I am sure what's the rule to order the IDs. Keyboard container is a direct
children of root_window (the same level as DesktopBackgroundContainer). My best
guess is to put it under it but I could be wrong. Do you have suggestion?

On 2013/08/08 17:29:44, James Cook wrote:
> Where does this container actually sit in the hierarchy of containers?  Unless
> it is actually on top of the power button animation container, this line
should
> go in the middle of the file somewhere and the IDs all adjusted.

https://codereview.chromium.org/22465007/diff/10001/ash/wm/event_client_impl.cc
File ash/wm/event_client_impl.cc (right):

https://codereview.chromium.org/22465007/diff/10001/ash/wm/event_client_impl....
ash/wm/event_client_impl.cc:27: if
(Shell::GetInstance()->session_state_delegate()->IsUserSessionBlocked()) {
Merged !root_window and !IsUserSessionBlocked to early return.

On 2013/08/08 17:29:44, James Cook wrote:
> nit: this function might be clearer with an early return:
> 
> if (!IsUserSessionBlocked())
>   return true;

[James Cook, 2013-08-08 19:52:39]

LGTM

https://codereview.chromium.org/22465007/diff/10001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/22465007/diff/10001/ash/shell_window_ids.h#ne...
ash/shell_window_ids.h:106: const int kShellWindowId_VirtualKeyboardContainer =
24;
On 2013/08/08 19:39:24, bshe wrote:
> I am sure what's the rule to order the IDs. Keyboard container is a direct
> children of root_window (the same level as DesktopBackgroundContainer). My
best
> guess is to put it under it but I could be wrong. Do you have suggestion?
> 
> On 2013/08/08 17:29:44, James Cook wrote:
> > Where does this container actually sit in the hierarchy of containers? 
Unless
> > it is actually on top of the power button animation container, this line
> should
> > go in the middle of the file somewhere and the IDs all adjusted.
> 

This seems fine.

[oshima, 2013-08-08 20:03:07]

lgtm with nit

https://codereview.chromium.org/22465007/diff/14002/ash/shell/keyboard_contro...
File ash/shell/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/22465007/diff/14002/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.h:23: 
nuke new line

[bshe, 2013-08-12 18:31:31]

Thanks for review!

jamescook@
would you mind to do another round of review for patchset 6.
I moved keyboard_controller_proxy_stub.* from ash/shell to ash. And these files
are now source files for target ash.
The reason is because that ash_unittests and ash_test_support depend on the stub
implementation. And ash seems like the best place to include the implementation.
(similar to caps_lock_delegate_stub.*)

https://codereview.chromium.org/22465007/diff/14002/ash/shell/keyboard_contro...
File ash/shell/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/22465007/diff/14002/ash/shell/keyboard_contro...
ash/shell/keyboard_controller_proxy_stub.h:23: 
On 2013/08/08 20:03:07, oshima wrote:
> nuke new line

Done.

[James Cook, 2013-08-12 20:37:47]

lgtm

[commit-bot: I haz the power, 2013-08-12 20:51:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/22465007/68001

[commit-bot: I haz the power, 2013-08-13 01:45:53]

Change committed as 217159