Delayed loading of the virtual keyboard.

ui/keyboard/keyboard_controller.cc

Index: ui/keyboard/keyboard_controller.cc
diff --git a/ui/keyboard/keyboard_controller.cc b/ui/keyboard/keyboard_controller.cc
index cdecdc36dfb5971d10b4bd14bd2cf23e53284acb..fc4af8f7543a1a92fea1b4f0d5bf107524d7c673 100644
--- a/ui/keyboard/keyboard_controller.cc
+++ b/ui/keyboard/keyboard_controller.cc
@@ -9,7 +9,6 @@
 #include "ui/aura/window_delegate.h"
 #include "ui/base/hit_test.h"
 #include "ui/base/ime/input_method.h"
-#include "ui/base/ime/input_method_base.h"
 #include "ui/base/ime/text_input_client.h"
 #include "ui/base/ime/text_input_type.h"
 #include "ui/gfx/path.h"
@@ -28,38 +27,6 @@ gfx::Rect KeyboardBoundsFromWindowBounds(const gfx::Rect& window_bounds) {
       window_bounds.height() * kKeyboardHeightRatio);
 }
 
-// LayoutManager for the virtual keyboard container.  Manages a single window
-// (the virtual keyboard) and keeps it positioned at the bottom of the
-// container window.
-class KeyboardLayoutManager : public aura::LayoutManager {
- public:
-  KeyboardLayoutManager(aura::Window* owner, aura::Window* keyboard)
-    : owner_(owner), keyboard_(keyboard) {}
-
-  // Overridden from aura::LayoutManager
-  virtual void OnWindowResized() OVERRIDE {
-    SetChildBoundsDirect(keyboard_,
-                         KeyboardBoundsFromWindowBounds(owner_->bounds()));
-  }
-  virtual void OnWindowAddedToLayout(aura::Window* child) OVERRIDE {
-    CHECK(child == keyboard_);
-  }
-  virtual void OnWillRemoveWindowFromLayout(aura::Window* child) OVERRIDE {}
-  virtual void OnWindowRemovedFromLayout(aura::Window* child) OVERRIDE {}
-  virtual void OnChildWindowVisibilityChanged(aura::Window* child,
-                                              bool visible) OVERRIDE {}
-  virtual void SetChildBounds(aura::Window* child,
-                              const gfx::Rect& requested_bounds) OVERRIDE {
-    // Drop these: the size should only be set in OnWindowResized.
-  }
-
- private:
-  aura::Window* owner_;
-  aura::Window* keyboard_;
-
-  DISALLOW_COPY_AND_ASSIGN(KeyboardLayoutManager);
-};
-
 // The KeyboardWindowDelegate makes sure the keyboard-window does not get focus.
 // This is necessary to make sure that the synthetic key-events reach the target
 // window.
@@ -110,47 +77,97 @@ class KeyboardWindowDelegate : public aura::WindowDelegate {
 
 namespace keyboard {
 
+// LayoutManager for the virtual keyboard container.  Manages a single window
+// (the virtual keyboard) and keeps it positioned at the bottom of the
+// owner window.
+class KeyboardLayoutManager : public aura::LayoutManager {
+ public:
+  KeyboardLayoutManager(aura::Window* owner) : owner_(owner), keyboard_(NULL) {
+    CHECK(owner_);
+  }
+
+  aura::Window* owner() { return owner_; }
+
+  void AddKeyboard(aura::Window* keyboard) {
+    keyboard->Show();
+    owner_->AddChild(keyboard);
+    OnWindowResized();
+  }
+
+  bool HasKeyboard() { return keyboard_ != NULL; }
+
+  void ShowKeyboard() {
+    owner_->parent()->StackChildAtTop(owner_);
+    owner_->Show();
+  }
+
+  void HideKeyboard() {
+    owner_->Hide();
+  }
+
+  // Overridden from aura::LayoutManager
+  virtual void OnWindowResized() OVERRIDE {
+    if (!keyboard_)
+      return;
+    SetChildBoundsDirect(keyboard_,
+                         KeyboardBoundsFromWindowBounds(owner_->bounds()));
+  }
+  virtual void OnWindowAddedToLayout(aura::Window* child) OVERRIDE {
+    DCHECK(!keyboard_);
+    keyboard_ = child;
+  }
+  virtual void OnWillRemoveWindowFromLayout(aura::Window* child) OVERRIDE {}
+  virtual void OnWindowRemovedFromLayout(aura::Window* child) OVERRIDE {}
+  virtual void OnChildWindowVisibilityChanged(aura::Window* child,
+                                              bool visible) OVERRIDE {}
+  virtual void SetChildBounds(aura::Window* child,
+                              const gfx::Rect& requested_bounds) OVERRIDE {
+    // Drop these: the size should only be set in OnWindowResized.
+  }
+
+ private:
+  aura::Window* owner_;
+  aura::Window* keyboard_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyboardLayoutManager);
+};
+
 KeyboardController::KeyboardController(KeyboardControllerProxy* proxy)
-    : proxy_(proxy), container_(NULL) {
+    : proxy_(proxy), layout_manager_(NULL), input_method_(NULL) {
   CHECK(proxy);
-  proxy_->GetInputMethod()->AddObserver(this);
+  input_method_ = proxy_->GetInputMethod();
+  input_method_->AddObserver(this);

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

I don't think having |input_method_| is useful here. We can just keep using the
proxy_ to get to the IM

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

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?

 }
 
 KeyboardController::~KeyboardController() {
-  if (container_)
-    container_->RemoveObserver(this);
-  proxy_->GetInputMethod()->RemoveObserver(this);
+  input_method_->RemoveObserver(this);
 }
 
 aura::Window* KeyboardController::GetContainerWindow() {
-  if (!container_) {
-    container_ = new aura::Window(new KeyboardWindowDelegate());
-    container_->SetName("KeyboardContainer");
-    container_->Init(ui::LAYER_NOT_DRAWN);
-    container_->AddObserver(this);
-
-    aura::Window* keyboard = proxy_->GetKeyboardWindow();
-    keyboard->Show();
-
-    container_->SetLayoutManager(
-        new KeyboardLayoutManager(container_, keyboard));
-    container_->AddChild(keyboard);
+  if (!layout_manager_) {
+    aura::Window* container = new aura::Window(new KeyboardWindowDelegate());
+    container->SetName("KeyboardContainer");
+    container->Init(ui::LAYER_NOT_DRAWN);
+    container->AddObserver(this);
+
+    layout_manager_ = new KeyboardLayoutManager(container);
+    container->SetLayoutManager(layout_manager_);
   }
-  return container_;
+  return layout_manager_->owner();

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

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]

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?

 }
 
 void KeyboardController::OnTextInputStateChanged(
     const ui::TextInputClient* client) {
-  if (!container_)
+  if (!layout_manager_)
     return;
 
   if (!client || client->GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE) {
-    container_->Hide();
+    layout_manager_->HideKeyboard();
   } else {
-    container_->parent()->StackChildAtTop(container_);

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

Could you have just checked that container_->children().empty() to determine
that the keyboard hadn't been initialized?

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

Done.

-    container_->Show();
+    if (!layout_manager_->HasKeyboard())
+      layout_manager_->AddKeyboard(proxy_->GetKeyboardWindow());
+    layout_manager_->ShowKeyboard();
   }
-
   // TODO(bryeung): whenever the TextInputClient changes we need to notify the
   // keyboard (with the TextInputType) so that it can reset it's state (e.g.
   // abandon compositions in progress)
@@ -162,8 +179,9 @@ void KeyboardController::OnWindowParentChanged(aura::Window* window,
 }
 
 void KeyboardController::OnWindowDestroying(aura::Window* window) {
-  DCHECK_EQ(container_, window);
-  container_ = NULL;
+  DCHECK(layout_manager_);
+  DCHECK_EQ(layout_manager_->owner(), window);
+  layout_manager_->owner()->RemoveObserver(this);
 }
 
 }  // namespace keyboard