Add a full screen virtual keyboard to virtual keyboard root window

Add a full screen virtual keyboard to virtual keyboard root window


BUG=310331
TEST=
1. add keyboard-usability-experiment flag
2. hook up an external screen
3. boot device
expected: a full screen keyboard on one of the screen

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

[bshe, 2013-10-30 17:16:20]

Hi Oshima. Do you mind to take a look at this CL?

+sadrul for ui/keyboard/* owner.

[oshima, 2013-10-30 17:19:23]

would you mind adding a simple test so that we can catch changes that break VK
feature? It doesn't have to be thorough.

[sadrul, 2013-10-30 20:58:27]

https://codereview.chromium.org/47873003/diff/110001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/47873003/diff/110001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:206: if (!type == ui::TEXT_INPUT_TYPE_NONE)
type != ...

[oshima, 2013-10-30 21:05:29]

https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
File ash/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
ash/keyboard_controller_proxy_stub.h:19: class ASH_EXPORT
KeyboardControllerProxyStub
Can you move this to ash/shell directory so that regular build won't build this?

(I'm going to clean up other stubs)

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
ash/root_window_controller.cc:277: }
ditto

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
ash/root_window_controller.cc:617: }
nuke {}

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
ash/root_window_controller.h:286: RootWindowType root_window_type_;
you don't need this with my patch.

[bshe, 2013-10-30 22:40:50]

On 2013/10/30 21:05:29, oshima wrote:
>
https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
> File ash/keyboard_controller_proxy_stub.h (right):
> 
>
https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
> ash/keyboard_controller_proxy_stub.h:19: class ASH_EXPORT
> KeyboardControllerProxyStub
> Can you move this to ash/shell directory so that regular build won't build
this?
> 
> (I'm going to clean up other stubs)
> 
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> File ash/root_window_controller.cc (right):
> 
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> ash/root_window_controller.cc:277: }
> ditto
> 
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> ash/root_window_controller.cc:617: }
> nuke {}
> 
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> File ash/root_window_controller.h (right):
> 
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> ash/root_window_controller.h:286: RootWindowType root_window_type_;
> you don't need this with my patch.

dooh. Sorry. The latest patch was just an local experiment patch and I didn't
mean to put it up for
review. But thanks for review anyway and sorry for the extremely bad style. :p I
will keep the experiment
patch and address reviews in a new patch.

[oshima, 2013-10-30 23:33:32]

On 2013/10/30 22:40:50, bshe wrote:
> On 2013/10/30 21:05:29, oshima wrote:
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
> > File ash/keyboard_controller_proxy_stub.h (right):
> > 
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
> > ash/keyboard_controller_proxy_stub.h:19: class ASH_EXPORT
> > KeyboardControllerProxyStub
> > Can you move this to ash/shell directory so that regular build won't build
> this?
> > 
> > (I'm going to clean up other stubs)
> > 
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> > File ash/root_window_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> > ash/root_window_controller.cc:277: }
> > ditto
> > 
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> > ash/root_window_controller.cc:617: }
> > nuke {}
> > 
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> > File ash/root_window_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
> > ash/root_window_controller.h:286: RootWindowType root_window_type_;
> > you don't need this with my patch.
> 
> dooh. Sorry. The latest patch was just an local experiment patch and I didn't
> mean to put it up for
> review. But thanks for review anyway and sorry for the extremely bad style. :p
I
> will keep the experiment
> patch and address reviews in a new patch.

oh, sorry I should have noticed it's "experiment".

[bshe, 2013-10-31 16:45:44]

Added a simple test that verify VK container is activated on the vk's root
window controller.

Could you please take another look?

https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
File ash/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/47873003/diff/110001/ash/keyboard_controller_...
ash/keyboard_controller_proxy_stub.h:19: class ASH_EXPORT
KeyboardControllerProxyStub
On 2013/10/30 21:05:30, oshima wrote:
> Can you move this to ash/shell directory so that regular build won't build
this?
> 
> (I'm going to clean up other stubs)

Done.

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
ash/root_window_controller.cc:277: }
On 2013/10/30 21:05:30, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
ash/root_window_controller.cc:617: }
On 2013/10/30 21:05:30, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/110001/ash/root_window_controll...
ash/root_window_controller.h:286: RootWindowType root_window_type_;
On 2013/10/30 21:05:30, oshima wrote:
> you don't need this with my patch.

Done.

https://codereview.chromium.org/47873003/diff/110001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/47873003/diff/110001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:206: if (!type == ui::TEXT_INPUT_TYPE_NONE)
On 2013/10/30 20:58:27, sadrul wrote:
> type != ...

Done.

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
ash/root_window_controller_unittest.cc:699: // Mock a login user profile change
to reinitialize the keyboard.
This OnLoginUserProfilePrepared is normally called after receiving
chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED notification. Here I just
called it directly in this test. Not sure how ash unit test mock the
notification or maybe I should move this unittest to a browser test? Or this is
fine?

[Ben Goodger (Google), 2013-10-31 17:11:13]

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
File ash/display/virtual_keyboard_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller.h:19: // virtual keyboard.
Why?

[oshima, 2013-11-01 00:09:02]

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
ash/display/display_controller.cc:346: }
can you call 

CloseNonDesktopDisplay()

at the end of Shutdown instead?

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
File ash/display/virtual_keyboard_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller.h:27: }
then you can remove this.

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
File ash/display/virtual_keyboard_window_controller_unittest.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller_unittest.cc:30: }
TearDown is not necessary.

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller_unittest.cc:40: #define
MAYBE_VirtualKeyboardWindowTest DISABLED_VirtualKeyboardWindowTest
can you check if this works on win7 aura? (you can use SupportsMultiDisplays()
to skip this test on win8)

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
ash/root_window_controller.cc:424: }
can you rebase?

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
ash/root_window_controller.cc:610: if
(!keyboard::IsKeyboardUsabilityTestEnabled())
The name of this flag is confusing. Can we rename to something like
KeyboardDisplayTestEnabled() ?

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
ash/root_window_controller_unittest.cc:699: // Mock a login user profile change
to reinitialize the keyboard.
On 2013/10/31 16:45:44, bshe wrote:
> This OnLoginUserProfilePrepared is normally called after receiving
> chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED notification. Here I just
> called it directly in this test. Not sure how ash unit test mock the
> notification or maybe I should move this unittest to a browser test? Or this
is
> fine?

I'm not sure. I'll investigate and get back to you.

https://codereview.chromium.org/47873003/diff/150002/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/shell.cc#newcode386
ash/shell.cc:386:
virtual_keyboard_window_controller()->root_window_controller();
Can't you create the keyboard for this mode in VirtualkeyboardWindowController?

https://codereview.chromium.org/47873003/diff/150002/ash/shell/keyboard_contr...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/shell/keyboard_contr...
ash/shell/keyboard_controller_proxy_stub.cc:24: return window;
who owns this?

https://codereview.chromium.org/47873003/diff/150002/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/47873003/diff/150002/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:214:
ash::Shell::GetInstance()->CreateLauncher();
will it make sense to move CreateLauncher to OnLoginUserProfilePrepared?

[bshe, 2013-11-12 15:28:33]

oshima@ Sorry for the delay.
I was in China last week and having trouble access my desktop. Would you mind to
take another look?

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
ash/display/display_controller.cc:346: }
This close child window currently depends on event_client_ (not sure if this is
expected). CloseNonDesktopDisplay is called after event_client_ is deleted in
the shutdown code path (dctor of Shell). So I called close child windows here.

On 2013/11/01 00:09:02, oshima wrote:
> can you call 
> 
> CloseNonDesktopDisplay()
> 
> at the end of Shutdown instead?

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
File ash/display/virtual_keyboard_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller.h:19: // virtual keyboard.
discussed offline. Thanks!
On 2013/10/31 17:11:13, Ben Goodger (Google) wrote:
> Why?

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller.h:27: }
It may be needed by display_controller if we want to call CloseChildWindows
(assume it is the right approach, if not, I will do the change accordingly).

On 2013/11/01 00:09:02, oshima wrote:
> then you can remove this.

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
File ash/display/virtual_keyboard_window_controller_unittest.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller_unittest.cc:30: }
On 2013/11/01 00:09:02, oshima wrote:
> TearDown is not necessary.

Done.

https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
ash/display/virtual_keyboard_window_controller_unittest.cc:40: #define
MAYBE_VirtualKeyboardWindowTest DISABLED_VirtualKeyboardWindowTest
On 2013/11/01 00:09:02, oshima wrote:
> can you check if this works on win7 aura? (you can use SupportsMultiDisplays()
> to skip this test on win8)

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
ash/root_window_controller.cc:424: }
On 2013/11/01 00:09:02, oshima wrote:
> can you rebase?

Done.

https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
ash/root_window_controller.cc:610: if
(!keyboard::IsKeyboardUsabilityTestEnabled())
Do you mean change --keyboard-usability-test to --keyboard-display-test and the
corresponding function name? It looks like the usability test flag has been used
from the beginning. It is a bit confusing in this case but it seems better
describe our purpose. I can definitely do the change if you are strong on the
name change.

On 2013/11/01 00:09:02, oshima wrote:
> The name of this flag is confusing. Can we rename to something like
> KeyboardDisplayTestEnabled() ?

https://codereview.chromium.org/47873003/diff/150002/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/shell.cc#newcode386
ash/shell.cc:386:
virtual_keyboard_window_controller()->root_window_controller();
On 2013/11/01 00:09:02, oshima wrote:
> Can't you create the keyboard for this mode in
VirtualkeyboardWindowController?

Done.

https://codereview.chromium.org/47873003/diff/150002/ash/shell/keyboard_contr...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/shell/keyboard_contr...
ash/shell/keyboard_controller_proxy_stub.cc:24: return window;
It looks like the keyboard container owns this window.
https://code.google.com/p/chromium/codesearch/#chromium/src/ui/keyboard/keybo...

On 2013/11/01 00:09:02, oshima wrote:
> who owns this?

https://codereview.chromium.org/47873003/diff/150002/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/47873003/diff/150002/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:214:
ash::Shell::GetInstance()->CreateLauncher();
On 2013/11/01 00:09:02, oshima wrote:
> will it make sense to move CreateLauncher to OnLoginUserProfilePrepared?

Done.

[bshe, 2013-11-13 18:05:54]

On 2013/11/12 15:28:33, bshe wrote:
> oshima@ Sorry for the delay.
> I was in China last week and having trouble access my desktop. Would you mind
to
> take another look?
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
> File ash/display/display_controller.cc (right):
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
> ash/display/display_controller.cc:346: }
> This close child window currently depends on event_client_ (not sure if this
is
> expected). CloseNonDesktopDisplay is called after event_client_ is deleted in
> the shutdown code path (dctor of Shell). So I called close child windows here.
> 
> On 2013/11/01 00:09:02, oshima wrote:
> > can you call 
> > 
> > CloseNonDesktopDisplay()
> > 
> > at the end of Shutdown instead?
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
> File ash/display/virtual_keyboard_window_controller.h (right):
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
> ash/display/virtual_keyboard_window_controller.h:19: // virtual keyboard.
> discussed offline. Thanks!
> On 2013/10/31 17:11:13, Ben Goodger (Google) wrote:
> > Why?
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
> ash/display/virtual_keyboard_window_controller.h:27: }
> It may be needed by display_controller if we want to call CloseChildWindows
> (assume it is the right approach, if not, I will do the change accordingly).
> 
> On 2013/11/01 00:09:02, oshima wrote:
> > then you can remove this.
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
> File ash/display/virtual_keyboard_window_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
> ash/display/virtual_keyboard_window_controller_unittest.cc:30: }
> On 2013/11/01 00:09:02, oshima wrote:
> > TearDown is not necessary.
> 
> Done.
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/display/virtual_keyb...
> ash/display/virtual_keyboard_window_controller_unittest.cc:40: #define
> MAYBE_VirtualKeyboardWindowTest DISABLED_VirtualKeyboardWindowTest
> On 2013/11/01 00:09:02, oshima wrote:
> > can you check if this works on win7 aura? (you can use
SupportsMultiDisplays()
> > to skip this test on win8)
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
> File ash/root_window_controller.cc (right):
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
> ash/root_window_controller.cc:424: }
> On 2013/11/01 00:09:02, oshima wrote:
> > can you rebase?
> 
> Done.
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/root_window_controll...
> ash/root_window_controller.cc:610: if
> (!keyboard::IsKeyboardUsabilityTestEnabled())
> Do you mean change --keyboard-usability-test to --keyboard-display-test and
the
> corresponding function name? It looks like the usability test flag has been
used
> from the beginning. It is a bit confusing in this case but it seems better
> describe our purpose. I can definitely do the change if you are strong on the
> name change.
> 
> On 2013/11/01 00:09:02, oshima wrote:
> > The name of this flag is confusing. Can we rename to something like
> > KeyboardDisplayTestEnabled() ?
> 
> https://codereview.chromium.org/47873003/diff/150002/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/47873003/diff/150002/ash/shell.cc#newcode386
> ash/shell.cc:386:
> virtual_keyboard_window_controller()->root_window_controller();
> On 2013/11/01 00:09:02, oshima wrote:
> > Can't you create the keyboard for this mode in
> VirtualkeyboardWindowController?
> 
> Done.
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/shell/keyboard_contr...
> File ash/shell/keyboard_controller_proxy_stub.cc (right):
> 
>
https://codereview.chromium.org/47873003/diff/150002/ash/shell/keyboard_contr...
> ash/shell/keyboard_controller_proxy_stub.cc:24: return window;
> It looks like the keyboard container owns this window.
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ui/keyboard/keybo...
> 
> On 2013/11/01 00:09:02, oshima wrote:
> > who owns this?
> 
>
https://codereview.chromium.org/47873003/diff/150002/chrome/browser/ui/ash/ch...
> File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):
> 
>
https://codereview.chromium.org/47873003/diff/150002/chrome/browser/ui/ash/ch...
> chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:214:
> ash::Shell::GetInstance()->CreateLauncher();
> On 2013/11/01 00:09:02, oshima wrote:
> > will it make sense to move CreateLauncher to OnLoginUserProfilePrepared?
> 
> Done.

Friendly ping?

[oshima, 2013-11-13 21:22:48]

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
ash/display/display_controller.cc:346: }
On 2013/11/12 15:28:33, bshe wrote:
> This close child window currently depends on event_client_ (not sure if this
is

Let me look into this dependency.

> expected). CloseNonDesktopDisplay is called after event_client_ is deleted in
> the shutdown code path (dctor of Shell). So I called close child windows here.
> 
> On 2013/11/01 00:09:02, oshima wrote:
> > can you call 
> > 
> > CloseNonDesktopDisplay()
> > 
> > at the end of Shutdown instead?
>

[oshima, 2013-11-13 22:23:36]

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
ash/display/display_controller.cc:346: }
On 2013/11/13 21:22:49, oshima wrote:
> On 2013/11/12 15:28:33, bshe wrote:
> > This close child window currently depends on event_client_ (not sure if this
> is
> 
> Let me look into this dependency.
> 
> > expected). CloseNonDesktopDisplay is called after event_client_ is deleted
in
> > the shutdown code path (dctor of Shell). So I called close child windows
here.

What kind of problem you have seen? I just tried and saw no issue.

> > 
> > On 2013/11/01 00:09:02, oshima wrote:
> > > can you call 
> > > 
> > > CloseNonDesktopDisplay()
> > > 
> > > at the end of Shutdown instead?
> > 
>

https://codereview.chromium.org/47873003/diff/940001/ash/shell/keyboard_contr...
File ash/shell/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/47873003/diff/940001/ash/shell/keyboard_contr...
ash/shell/keyboard_controller_proxy_stub.h:25: aura::Window* GetKeyboardWindow()
OVERRIDE;
virtual

https://codereview.chromium.org/47873003/diff/940001/ui/keyboard/keyboard_swi...
File ui/keyboard/keyboard_switches.h (right):

https://codereview.chromium.org/47873003/diff/940001/ui/keyboard/keyboard_swi...
ui/keyboard/keyboard_switches.h:20: extern const char kKeyboardUsabilityTest[];
Don't you still need export? (component build seems to fail)

[bshe, 2013-11-14 00:11:28]

Done. Thanks for review and suggestions.

Please see the latest patch. Sorry I did another rebase because of some other
stuff that I am working on. Only two files are of your interest:
ash/shell.cc  --> add the CloseNonDesktopDisplay call.
ash/shell/keyboard_controller_proxy_stub.h  --> add virtual
Hope it wont cause too much trouble for you.

Could you take another look?

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/47873003/diff/150002/ash/display/display_cont...
ash/display/display_controller.cc:346: }
Talked on hangout. I misunderstood you, sorry.
Changed to your suggestion.

On 2013/11/13 22:23:37, oshima wrote:
> On 2013/11/13 21:22:49, oshima wrote:
> > On 2013/11/12 15:28:33, bshe wrote:
> > > This close child window currently depends on event_client_ (not sure if
this
> > is
> > 
> > Let me look into this dependency.
> > 
> > > expected). CloseNonDesktopDisplay is called after event_client_ is deleted
> in
> > > the shutdown code path (dctor of Shell). So I called close child windows
> here.
> 
> What kind of problem you have seen? I just tried and saw no issue.
> 
> > > 
> > > On 2013/11/01 00:09:02, oshima wrote:
> > > > can you call 
> > > > 
> > > > CloseNonDesktopDisplay()
> > > > 
> > > > at the end of Shutdown instead?
> > > 
> > 
>

https://codereview.chromium.org/47873003/diff/940001/ash/shell/keyboard_contr...
File ash/shell/keyboard_controller_proxy_stub.h (right):

https://codereview.chromium.org/47873003/diff/940001/ash/shell/keyboard_contr...
ash/shell/keyboard_controller_proxy_stub.h:25: aura::Window* GetKeyboardWindow()
OVERRIDE;
On 2013/11/13 22:23:37, oshima wrote:
> virtual

Done.

[oshima, 2013-11-14 00:37:06]

lgtm with a nit

https://codereview.chromium.org/47873003/diff/1290002/ash/display/virtual_key...
File ash/display/virtual_keyboard_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/1290002/ash/display/virtual_key...
ash/display/virtual_keyboard_window_controller.h:30: return
root_window_controller_.get();
Is this used only for test?

If so, can you rename to _for_test and move to private + friend and rename

[bshe, 2013-11-14 17:01:50]

Done. Thanks for review!

sadrul@ would you mind to take another look for ui/keyboard/*

+jamescook for chrome/browser/ui/views/ash OWNER

https://codereview.chromium.org/47873003/diff/1290002/ash/display/virtual_key...
File ash/display/virtual_keyboard_window_controller.h (right):

https://codereview.chromium.org/47873003/diff/1290002/ash/display/virtual_key...
ash/display/virtual_keyboard_window_controller.h:30: return
root_window_controller_.get();
On 2013/11/14 00:37:06, oshima wrote:
> Is this used only for test?
> 
> If so, can you rename to _for_test and move to private + friend and rename

Done.

[sadrul, 2013-11-14 17:03:41]

lgtm

[James Cook, 2013-11-14 17:15:50]

https://codereview.chromium.org/47873003/diff/1360001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/47873003/diff/1360001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:100: if
(!keyboard::IsKeyboardUsabilityTestEnabled()) {
This sounds like "keyboard usability test" is running some kind of test code.  I
would either call this KeyboardUsabilityExperiment or KeyboardUsabilityTrial, or
at least add documentation for the flag that explains that this isn't a
unit_test or browser_test mode.

[bshe, 2013-11-14 17:35:41]

Done. Would you mind to take another look?

https://codereview.chromium.org/47873003/diff/1360001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/47873003/diff/1360001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:100: if
(!keyboard::IsKeyboardUsabilityTestEnabled()) {
renamed to KeyboardUsabilityExperiment and added comment.

On 2013/11/14 17:15:51, James Cook wrote:
> This sounds like "keyboard usability test" is running some kind of test code. 
I
> would either call this KeyboardUsabilityExperiment or KeyboardUsabilityTrial,
or
> at least add documentation for the flag that explains that this isn't a
> unit_test or browser_test mode.

[James Cook, 2013-11-14 17:40:19]

LGTM

[commit-bot: I haz the power, 2013-11-14 18:54:14]

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

[commit-bot: I haz the power, 2013-11-14 19:19:00]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-14 19:35:27]

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

[commit-bot: I haz the power, 2013-11-14 21:46:41]

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

[commit-bot: I haz the power, 2013-11-15 01:13:38]

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

[commit-bot: I haz the power, 2013-11-15 03:47:46]

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

[commit-bot: I haz the power, 2013-11-15 06:44:53]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 20:47:26]

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

[commit-bot: I haz the power, 2013-11-15 20:52:26]

Failed to apply patch for ash/display/virtual_keyboard_window_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/display/virtual_keyboard_window_controller.cc
  Hunk #3 FAILED at 59.
  1 out of 3 hunks FAILED -- saving rejects to file
ash/display/virtual_keyboard_window_controller.cc.rej

Patch:       ash/display/virtual_keyboard_window_controller.cc
Index: ash/display/virtual_keyboard_window_controller.cc
diff --git a/ash/display/virtual_keyboard_window_controller.cc
b/ash/display/virtual_keyboard_window_controller.cc
index
098c3ce6e7e2946d7a7ef41e52b2f482ae8be487..9786aa2695f0cb5527bde0643320557050fe544c
100644
--- a/ash/display/virtual_keyboard_window_controller.cc
+++ b/ash/display/virtual_keyboard_window_controller.cc
@@ -16,6 +16,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "ui/aura/env.h"
 #include "ui/aura/root_window.h"
+#include "ui/keyboard/keyboard_controller.h"
 
 namespace ash {
 namespace internal {
@@ -28,6 +29,11 @@
VirtualKeyboardWindowController::~VirtualKeyboardWindowController() {
   Close();
 }
 
+void VirtualKeyboardWindowController::ActivateKeyboard(
+    keyboard::KeyboardController* keyboard_controller) {
+  root_window_controller_->ActivateKeyboard(keyboard_controller);
+}
+
 void VirtualKeyboardWindowController::UpdateWindow(
     const DisplayInfo& display_info) {
   static int virtual_keyboard_root_window_count = 0;
@@ -53,6 +59,8 @@ void VirtualKeyboardWindowController::UpdateWindow(
     root_window_controller_.reset(GetRootWindowController(
         root_window->window()));
     root_window_controller_->dispatcher()->ShowRootWindow();
+    root_window_controller_->ActivateKeyboard(
+        Shell::GetInstance()->keyboard_controller());
   } else {
     aura::RootWindow* root_window = root_window_controller_->dispatcher();
     GetRootWindowSettings(root_window->window())->display_id =

[commit-bot: I haz the power, 2013-11-15 22:52:04]

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

[commit-bot: I haz the power, 2013-11-16 00:17:05]

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

[commit-bot: I haz the power, 2013-11-16 02:56:16]

Change committed as 235502