Create and show the virtual keyboard.

Create and show the virtual keyboard.

Introduces ui/keyboard/ which contains generic code for the control of the keyboard (right now, just creation of the container window).

Also includes the necessary changes to ash so that the keyboard is (always) displayed on ChromeOS builds when the --enable-virtual-keyboard flag is present.

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

[sadrul, 2013-03-28 19:17:00]

If I understand correctly, the way it works now:

 * The VK consumer creates a proxy. This proxy creates the necessary WebContents
that hosts the keyboard.

 * The VK consumer creates the VKController, and provides the proxy created in
step 1.

 * The VK consumer receives an aura::Window from the VKC. The consumer shows it
in the right place.

I think this overall design makes sense. I was thinking that if we can have the
VK depend directly on content/, then we can get rid of the proxy, and have the
VK itself create the WebContents for the keyboard. But I don't think this is
possible at the moment, since the chrome implementation depends on extensions
for this. So having a delegate/proxy for creating/setting-up the WebContents
makes good sense.

https://codereview.chromium.org/13164002/diff/1/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13164002/diff/1/ash/root_window_controller.cc...
ash/root_window_controller.cc:640: SetUsesScreenCoordinates(overlay_container);
I think you can get the keyboard-container here, and add it to the
overlay_container.

The overlay-container can have other windows. So the keyboard-controller will
need to make sure the window is stacked on top whenever the keyboard shows up.

https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h#newcode30
ash/shell_delegate.h:30: class WebContents;
This doesn't seem necessary.

https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h#newcode163
ash/shell_delegate.h:163: CreateVirtualKeyboardControllerProxy() = 0;
You will likely need another dummy implementation for this for
ash_unittests/ash_shell.

Also, indent 4 spaces.

https://codereview.chromium.org/13164002/diff/1/ash/virtual_keyboard/ash_virt...
File ash/virtual_keyboard/ash_virtual_keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/1/ash/virtual_keyboard/ash_virt...
ash/virtual_keyboard/ash_virtual_keyboard_controller_proxy.h:18: class
AshVirtualKeyboardControllerProxy
Remove this since this isn't used?

https://codereview.chromium.org/13164002/diff/1/chrome/browser/ui/ash/ash_vir...
File chrome/browser/ui/ash/ash_virtual_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/13164002/diff/1/chrome/browser/ui/ash/ash_vir...
chrome/browser/ui/ash/ash_virtual_keyboard_controller_proxy.cc:21: const char
kVirtualKeyboardUrl[] =
"chrome-extension://mppnpdlheglhdfmldimlhpnegondlapf/index.html";
break after =

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
File ui/virtual_keyboard/virtual_keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.cc:14: class
VirtualKeyboardLayoutManager : public aura::LayoutManager {
doc

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.cc:19: void OnWindowResized()
OVERRIDE {
virtual (and other overrides too)

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
File ui/virtual_keyboard/virtual_keyboard_controller.h (right):

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.h:20: class
VIRTUAL_KEYBOARD_EXPORT VirtualKeyboardController {
doc

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.h:28: aura::Window* Init();
Perhaps Init() and GetContainerWindow() should be separate.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.h:34: // container_ once I've
added it to the RootWindow?
By default, an aura window is owned by the parent. Perhaps in this case, you
want to set_owned_by_parent(false)

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
File ui/virtual_keyboard/virtual_keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller_proxy.h:16: class
VIRTUAL_KEYBOARD_EXPORT VirtualKeyboardControllerProxy {
Doc

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller_proxy.h:18: virtual
aura::Window* GetVirtualKeyboard() = 0;
Doc. Perhaps rename to GetVirtualKeyboardWindow (or just GetNativeWindow)?

And the destructor should come first.

[sadrul, 2013-03-28 19:18:19]

Oh, and 'virtual_keyboard' is a veeryy loong prefix :( Can this be just
'keyboard'?

[bryeung, 2013-03-28 19:38:28]

Thanks for looking this over.

On Thu, Mar 28, 2013 at 3:17 PM,  <sadrul@chromium.org> wrote:
> If I understand correctly, the way it works now:
>
>  * The VK consumer creates a proxy. This proxy creates the necessary
> WebContents
> that hosts the keyboard.
>
>  * The VK consumer creates the VKController, and provides the proxy created
> in
> step 1.
>
>  * The VK consumer receives an aura::Window from the VKC. The consumer shows
> it
> in the right place.
>
> I think this overall design makes sense. I was thinking that if we can have
> the
> VK depend directly on content/, then we can get rid of the proxy, and have
> the
> VK itself create the WebContents for the keyboard. But I don't think this is
> possible at the moment, since the chrome implementation depends on
> extensions
> for this. So having a delegate/proxy for creating/setting-up the WebContents
> makes good sense.

Yes, that's all correct.  And I had the proxy-less solution
implemented first, and then moved to this in order to a) get rid of
the dependency on content from ui, b) make the creation and ownership
of the keyboard host (WebContents and necessary extensions stuff) a
little more encapsulated.



>
>
> https://codereview.chromium.org/13164002/diff/1/ash/root_window_controller.cc
> File ash/root_window_controller.cc (right):
>
>
https://codereview.chromium.org/13164002/diff/1/ash/root_window_controller.cc...
> ash/root_window_controller.cc:640:
> SetUsesScreenCoordinates(overlay_container);
> I think you can get the keyboard-container here, and add it to the
> overlay_container.
>
> The overlay-container can have other windows. So the keyboard-controller
> will need to make sure the window is stacked on top whenever the
> keyboard shows up.
>
> https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h
> File ash/shell_delegate.h (right):
>
>
https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h#newcode30
> ash/shell_delegate.h:30: class WebContents;
> This doesn't seem necessary.
>
>
https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h#newcode163
> ash/shell_delegate.h:163: CreateVirtualKeyboardControllerProxy() = 0;
> You will likely need another dummy implementation for this for
> ash_unittests/ash_shell.
>
> Also, indent 4 spaces.
>
>
https://codereview.chromium.org/13164002/diff/1/ash/virtual_keyboard/ash_virt...
> File ash/virtual_keyboard/ash_virtual_keyboard_controller_proxy.h
> (right):
>
>
https://codereview.chromium.org/13164002/diff/1/ash/virtual_keyboard/ash_virt...
> ash/virtual_keyboard/ash_virtual_keyboard_controller_proxy.h:18: class
> AshVirtualKeyboardControllerProxy
> Remove this since this isn't used?
>
>
https://codereview.chromium.org/13164002/diff/1/chrome/browser/ui/ash/ash_vir...
> File chrome/browser/ui/ash/ash_virtual_keyboard_controller_proxy.cc
> (right):
>
>
https://codereview.chromium.org/13164002/diff/1/chrome/browser/ui/ash/ash_vir...
> chrome/browser/ui/ash/ash_virtual_keyboard_controller_proxy.cc:21: const
> char kVirtualKeyboardUrl[] =
> "chrome-extension://mppnpdlheglhdfmldimlhpnegondlapf/index.html";
> break after =
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> File ui/virtual_keyboard/virtual_keyboard_controller.cc (right):
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller.cc:14: class
> VirtualKeyboardLayoutManager : public aura::LayoutManager {
> doc
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller.cc:19: void
> OnWindowResized() OVERRIDE {
> virtual (and other overrides too)
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> File ui/virtual_keyboard/virtual_keyboard_controller.h (right):
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller.h:20: class
> VIRTUAL_KEYBOARD_EXPORT VirtualKeyboardController {
> doc
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller.h:28: aura::Window*
> Init();
> Perhaps Init() and GetContainerWindow() should be separate.
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller.h:34: // container_ once
> I've added it to the RootWindow?
> By default, an aura window is owned by the parent. Perhaps in this case,
> you want to set_owned_by_parent(false)
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> File ui/virtual_keyboard/virtual_keyboard_controller_proxy.h (right):
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller_proxy.h:16: class
> VIRTUAL_KEYBOARD_EXPORT VirtualKeyboardControllerProxy {
> Doc
>
>
https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
> ui/virtual_keyboard/virtual_keyboard_controller_proxy.h:18: virtual
> aura::Window* GetVirtualKeyboard() = 0;
> Doc. Perhaps rename to GetVirtualKeyboardWindow (or just
> GetNativeWindow)?
>
> And the destructor should come first.
>
> https://codereview.chromium.org/13164002/

[bryeung, 2013-04-02 15:56:53]

I think this covers your comments.

I'm working on adding the switch to turn it off to this CL as well, but would
you mind taking at look at this basic stuff in the meantime?

https://codereview.chromium.org/13164002/diff/1/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13164002/diff/1/ash/root_window_controller.cc...
ash/root_window_controller.cc:640: SetUsesScreenCoordinates(overlay_container);
On 2013/03/28 19:17:00, sadrul wrote:
> I think you can get the keyboard-container here, and add it to the
> overlay_container.
> 
> The overlay-container can have other windows. So the keyboard-controller will
> need to make sure the window is stacked on top whenever the keyboard shows up.

This can't go here yet, as we're creating the Keyboard window, which ends up
needing the workspace_controller_, which isn't setup yet.

When the keyboard controller is able to defer creation until a more appropriate
time, then we'll be able to move it here.

https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h#newcode30
ash/shell_delegate.h:30: class WebContents;
On 2013/03/28 19:17:00, sadrul wrote:
> This doesn't seem necessary.

Leftovers.  Good catch.

https://codereview.chromium.org/13164002/diff/1/ash/shell_delegate.h#newcode163
ash/shell_delegate.h:163: CreateVirtualKeyboardControllerProxy() = 0;
On 2013/03/28 19:17:00, sadrul wrote:
> You will likely need another dummy implementation for this for
> ash_unittests/ash_shell.
> 
> Also, indent 4 spaces.

Done.

https://codereview.chromium.org/13164002/diff/1/chrome/browser/ui/ash/ash_vir...
File chrome/browser/ui/ash/ash_virtual_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/13164002/diff/1/chrome/browser/ui/ash/ash_vir...
chrome/browser/ui/ash/ash_virtual_keyboard_controller_proxy.cc:21: const char
kVirtualKeyboardUrl[] =
"chrome-extension://mppnpdlheglhdfmldimlhpnegondlapf/index.html";
On 2013/03/28 19:17:00, sadrul wrote:
> break after =

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
File ui/virtual_keyboard/virtual_keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.cc:14: class
VirtualKeyboardLayoutManager : public aura::LayoutManager {
On 2013/03/28 19:17:00, sadrul wrote:
> doc

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.cc:19: void OnWindowResized()
OVERRIDE {
On 2013/03/28 19:17:00, sadrul wrote:
> virtual (and other overrides too)

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
File ui/virtual_keyboard/virtual_keyboard_controller.h (right):

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.h:20: class
VIRTUAL_KEYBOARD_EXPORT VirtualKeyboardController {
On 2013/03/28 19:17:00, sadrul wrote:
> doc

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.h:28: aura::Window* Init();
On 2013/03/28 19:17:00, sadrul wrote:
> Perhaps Init() and GetContainerWindow() should be separate.

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller.h:34: // container_ once I've
added it to the RootWindow?
On 2013/03/28 19:17:00, sadrul wrote:
> By default, an aura window is owned by the parent. Perhaps in this case, you
> want to set_owned_by_parent(false)

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
File ui/virtual_keyboard/virtual_keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller_proxy.h:16: class
VIRTUAL_KEYBOARD_EXPORT VirtualKeyboardControllerProxy {
On 2013/03/28 19:17:00, sadrul wrote:
> Doc

Done.

https://codereview.chromium.org/13164002/diff/1/ui/virtual_keyboard/virtual_k...
ui/virtual_keyboard/virtual_keyboard_controller_proxy.h:18: virtual
aura::Window* GetVirtualKeyboard() = 0;
On 2013/03/28 19:17:00, sadrul wrote:
> Doc. Perhaps rename to GetVirtualKeyboardWindow (or just GetNativeWindow)?
> 
> And the destructor should come first.

Done.

[sadrul, 2013-04-02 16:10:31]

Some nits. Other than that, looks good!

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:22: virtual void OnWindowResized() OVERRIDE {
// Overridden from aura::LayoutManager:

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.h (right):

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.h:25:
KeyboardController(KeyboardControllerProxy* proxy);
explicit

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.h:39: }  // namespace message_center
keyboard

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller_proxy.h:26: }  // namespace message_center
keyboard

[sadrul, 2013-04-02 20:55:26]

https://codereview.chromium.org/13164002/diff/13001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/13001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:74: aura::Window* keyboard =
proxy_->GetKeyboardWindow();
I think it makes sense to keyboard->Show() here.

https://codereview.chromium.org/13164002/diff/13001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:79: container_->Show();
The caller might want to set the window up in some way before making it visible.
It makes sense to not Show() the container here. If we make this change, we
should also update the comment in GetContainerWindow() that the caller needs to
explicitly Show() [if the caller doesn't, the window will still show up
correctly when the summoning code is done]

[bryeung, 2013-04-03 14:46:03]

I've left it as a child of the root window: the overlay container seems to end
up the wrong size.

This also still has the shutdown crash.  I suspect it is related to how the
WebContents is being created.  I'm investigating...

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:22: virtual void OnWindowResized() OVERRIDE {
On 2013/04/02 16:10:31, sadrul wrote:
> // Overridden from aura::LayoutManager:

Done.

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.h (right):

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.h:25:
KeyboardController(KeyboardControllerProxy* proxy);
On 2013/04/02 16:10:31, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.h:39: }  // namespace message_center
On 2013/04/02 16:10:31, sadrul wrote:
> keyboard

Done.

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/11001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller_proxy.h:26: }  // namespace message_center
On 2013/04/02 16:10:31, sadrul wrote:
> keyboard

Done.

https://codereview.chromium.org/13164002/diff/13001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/13001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:74: aura::Window* keyboard =
proxy_->GetKeyboardWindow();
On 2013/04/02 20:55:27, sadrul wrote:
> I think it makes sense to keyboard->Show() here.

Done.

https://codereview.chromium.org/13164002/diff/13001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:79: container_->Show();
On 2013/04/02 20:55:27, sadrul wrote:
> The caller might want to set the window up in some way before making it
visible.
> It makes sense to not Show() the container here. If we make this change, we
> should also update the comment in GetContainerWindow() that the caller needs
to
> explicitly Show() [if the caller doesn't, the window will still show up
> correctly when the summoning code is done]

Done.

[bryeung, 2013-04-04 21:26:15]

I think all of the caveats have been removed and this is now landable.  PTAL :-)

[sadrul, 2013-04-04 22:08:25]

Some nits. other than that, lgtm.

Please update the CL description with a brief summary of the change. And there
should be a non-null BUG

https://codereview.chromium.org/13164002/diff/25001/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13164002/diff/25001/ash/root_window_controlle...
ash/root_window_controller.cc:505:
Shell::GetInstance()->delegate()->CreateKeyboardControllerProxy();
indent 2 more spaces

https://codereview.chromium.org/13164002/diff/25001/ash/root_window_controlle...
ash/root_window_controller.cc:509: keyboard_controller_->GetContainerWindow();
ditto

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1267: #if defined(OS_CHROMEOS)
Should this be USE_ASH?

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/ui/ash/ash...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:25: : public
keyboard::KeyboardControllerProxy,
2 more indent. (it looks like some code does use 2-space indent in this
scenario, but I think a majority of the code uses 4-space indent)

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:42: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
overrides after non-overrides

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:62: : proxy_(proxy), container_(NULL) {
2 more spaces.

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller_unittest.cc:49: scoped_ptr<aura::Window>
window_;
DISALLOW_COPY...

[bryeung, 2013-04-05 12:29:38]

Thanks sadrul!

https://codereview.chromium.org/13164002/diff/25001/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13164002/diff/25001/ash/root_window_controlle...
ash/root_window_controller.cc:505:
Shell::GetInstance()->delegate()->CreateKeyboardControllerProxy();
On 2013/04/04 22:08:25, sadrul wrote:
> indent 2 more spaces

Done.

https://codereview.chromium.org/13164002/diff/25001/ash/root_window_controlle...
ash/root_window_controller.cc:509: keyboard_controller_->GetContainerWindow();
On 2013/04/04 22:08:25, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1267: #if defined(OS_CHROMEOS)
On 2013/04/04 22:08:25, sadrul wrote:
> Should this be USE_ASH?

I've restricted it to kOsCrOS below.  Should I allow Windows too and change this
to USE_ASH?  (None of this has been tested on Windows, which is why I kept it to
CrOS.)

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/ui/ash/ash...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.h (right):

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:25: : public
keyboard::KeyboardControllerProxy,
On 2013/04/04 22:08:25, sadrul wrote:
> 2 more indent. (it looks like some code does use 2-space indent in this
> scenario, but I think a majority of the code uses 4-space indent)

Okay.  Done.

https://codereview.chromium.org/13164002/diff/25001/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:42: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
On 2013/04/04 22:08:25, sadrul wrote:
> overrides after non-overrides

Done.

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:62: : proxy_(proxy), container_(NULL) {
On 2013/04/04 22:08:25, sadrul wrote:
> 2 more spaces.

Done.

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/13164002/diff/25001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller_unittest.cc:49: scoped_ptr<aura::Window>
window_;
On 2013/04/04 22:08:25, sadrul wrote:
> DISALLOW_COPY...

Good catch.  Thanks.

[bryeung, 2013-04-05 12:30:19]

Hi Ben: can you have a look?

[Ben Goodger (Google), 2013-04-05 16:01:48]

lgtm

https://codereview.chromium.org/13164002/diff/33001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/33001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:32: 
remove nls around this method:

[bryeung, 2013-04-05 17:22:24]

Thanks Ben!

https://codereview.chromium.org/13164002/diff/33001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13164002/diff/33001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:32: 
On 2013/04/05 16:01:48, Ben Goodger (Google) wrote:
> remove nls around this method:

Done.

[commit-bot: I haz the power, 2013-04-05 17:22:58]

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

[commit-bot: I haz the power, 2013-04-05 17:29:53]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-05 17:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/13164002/53001

[commit-bot: I haz the power, 2013-04-05 17:45:10]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-08 18:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/13164002/70001

[commit-bot: I haz the power, 2013-04-08 18:33:30]

Failed to apply patch for chrome/browser/about_flags.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/about_flags.cc
  Hunk #2 FAILED at 1302.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/about_flags.cc.rej

Patch:       chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
c7614770138773613affd0be1cc791ae4cb661e1..405898577edbb3cf53f756e90ffe9105e6411945
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -35,6 +35,7 @@
 #include "ui/base/ui_base_switches.h"
 #include "ui/gfx/switches.h"
 #include "ui/gl/gl_switches.h"
+#include "ui/keyboard/keyboard_switches.h"
 #include "ui/surface/surface_switches.h"
 
 #if defined(ENABLE_MESSAGE_CENTER)
@@ -1301,6 +1302,15 @@ const Experiment kExperiments[] = {
     SINGLE_VALUE_TYPE(switches::kEnableGoogleNowIntegration)
   },
 #endif
+#if defined(OS_CHROMEOS)
+  {
+    "enable-virtual-keyboard",
+    IDS_FLAGS_ENABLE_VIRTUAL_KEYBOARD_NAME,
+    IDS_FLAGS_ENABLE_VIRTUAL_KEYBOARD_DESCRIPTION,
+    kOsCrOS,
+    SINGLE_VALUE_TYPE(keyboard::switches::kEnableVirtualKeyboard)
+  },
+#endif
 };
 
 const Experiment* experiments = kExperiments;

[commit-bot: I haz the power, 2013-04-08 18:49:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/13164002/72002

[commit-bot: I haz the power, 2013-04-09 06:26:06]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-09 17:57:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/13164002/83001

[commit-bot: I haz the power, 2013-04-09 19:25:15]

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

[bryeung, 2013-04-10 13:39:28]

Committed patchset #16 manually as r193378 (presubmit successful).