Delayed loading of the virtual keyboard.

Delayed loading of the virtual keyboard.

Don't create the virtual keyboard until it is first shown.  This helps
prevent some start-up races.

Also clean-up some of the object ownership between KeyboardController
and KeyboardLayoutManager.  Each pointer exists in only one or the
other object now.

BUG=none

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

[sadrul, 2013-04-15 03:48:44]

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:150:
container_->parent()->StackChildAtTop(container_);
Could you have just checked that container_->children().empty() to determine
that the keyboard hadn't been initialized?

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:139: input_method_->AddObserver(this);
I don't think having |input_method_| is useful here. We can just keep using the
proxy_ to get to the IM

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:156: return layout_manager_->owner();
I think having the |container_| in the KC is easier to understand. Going through
the LayoutManager to get the container feels twisty.

[bryeung, 2013-04-15 15:02:22]

Thanks: ptal.

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:150:
container_->parent()->StackChildAtTop(container_);
On 2013/04/15 03:48:44, sadrul wrote:
> Could you have just checked that container_->children().empty() to determine
> that the keyboard hadn't been initialized?

Done.

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:139: input_method_->AddObserver(this);
On 2013/04/15 03:48:44, sadrul wrote:
> I don't think having |input_method_| is useful here. We can just keep using
the
> proxy_ to get to the IM

Fetching the InputMethod from the Window properties via the Proxy was causing a
crash on shutdown, which is why I'm now holding a reference to it.

I'd also prefer not to have to hold the reference.  Do you have any suggestions
for other ways to fix the crash?

https://codereview.chromium.org/13932030/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:156: return layout_manager_->owner();
On 2013/04/15 03:48:44, sadrul wrote:
> I think having the |container_| in the KC is easier to understand. Going
through
> the LayoutManager to get the container feels twisty.

Okay.  I didn't like having two different references to the same thing in
different places, with different names in the same file.

I've renamed owner_ to container_ in the KeyboardLayoutManager to avoid this
instead.  wdyt?

[sadrul, 2013-04-15 16:49:37]

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller.cc:121: container_->RemoveObserver(this);
The KC does not own the |container_|. So it is possible that the KC owner
deletes the KC, but keeps the container_ around. In that case, the KC should
remove itself as an observer of the container. So this code (and setting
container_ to NULL in OnWindowDestroying) should still be here, I think.

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller.cc:122:
proxy_->GetInputMethod()->RemoveObserver(this);
I suppose it is possible that the root-window and/or the input-method has been
destroyed by the time the KC is destroyed? Perhaps there could be an
InputMethodObserver::OnInputMethodDestroyed()?

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller.cc:173: layout_manager_->ShowKeyboard();
I am not sure having |layout_manager_| is all that useful. Directly manipulating
container_ visibility should be sufficient? (unless you plan on using
|layout_manager_| for other things in the future)

[bryeung, 2013-04-15 17:42:36]

+nona for IME changes

Sadrul: all changes made as we discussed.  ptal.

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller.cc:121: container_->RemoveObserver(this);
On 2013/04/15 16:49:37, sadrul wrote:
> The KC does not own the |container_|. So it is possible that the KC owner
> deletes the KC, but keeps the container_ around. In that case, the KC should
> remove itself as an observer of the container. So this code (and setting
> container_ to NULL in OnWindowDestroying) should still be here, I think.

Oops!  Good catch.

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller.cc:122:
proxy_->GetInputMethod()->RemoveObserver(this);
On 2013/04/15 16:49:37, sadrul wrote:
> I suppose it is possible that the root-window and/or the input-method has been
> destroyed by the time the KC is destroyed? Perhaps there could be an
> InputMethodObserver::OnInputMethodDestroyed()?

Done.

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/13932030/diff/8001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller.cc:173: layout_manager_->ShowKeyboard();
On 2013/04/15 16:49:37, sadrul wrote:
> I am not sure having |layout_manager_| is all that useful. Directly
manipulating
> container_ visibility should be sufficient? (unless you plan on using
> |layout_manager_| for other things in the future)

Done.

[Seigo Nonaka, 2013-04-16 01:25:19]

lgtm

[bryeung, 2013-04-16 14:08:26]

+ben for OWNERS

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

LGTM

[Ben Goodger (Google), 2013-04-16 16:38:29]

lgtm

[bryeung, 2013-04-17 12:32:55]

Committed patchset #4 manually as r194580 (presubmit successful).