[VK] Clarify KeyboardUI interface.

Currently, KeyboardUI has a weird interface where |GetKeyboardWindow|
will start loading the keyboard web page if it is called the first time.
So this resulted in a lot of places where we were calling
GetKeyboardWindow and didn't know whether it would cause a load or not.

We change the KeyboardUI interface a bit to explicitly state when we
are creating / loading a keyboard window, vs just getting an already
loaded one.

This removes ChromeKeyboardUIWebContent's dependency on
KeyboardController as well.

We tried to keep the same behaviour for existing KeyboardUI subclasses.

Bug: 845780
Change-Id: I3178c81c382f2c3cd8217eda5a0c9b9d489df5be
Reviewed-on: https://chromium-review.googlesource.com/c/1264336
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599772}

ash/keyboard/test_keyboard_ui.cc

@@ -14,17 +14,20 @@
 namespace ash {
 
 TestKeyboardUI::TestKeyboardUI() = default;
+
 TestKeyboardUI::~TestKeyboardUI() = default;
 
-bool TestKeyboardUI::HasKeyboardWindow() const {
-  return !!keyboard_window_;
+aura::Window* TestKeyboardUI::LoadKeyboardWindow(LoadCallback callback) {
+  DCHECK(!keyboard_window_);
+  keyboard_window_ = window_factory::NewWindow(&delegate_);
+  keyboard_window_->Init(ui::LAYER_NOT_DRAWN);
+
+  // TODO(https://crbug.com/849995): Call |callback| instead of having tests
+  // call |NotifyKeyboardWindowLoaded|.
+  return keyboard_window_.get();
 }
 
-aura::Window* TestKeyboardUI::GetKeyboardWindow() {
-  if (!keyboard_window_) {
-    keyboard_window_ = window_factory::NewWindow(&delegate_);
-    keyboard_window_->Init(ui::LAYER_NOT_DRAWN);
-  }
+aura::Window* TestKeyboardUI::GetKeyboardWindow() const {
   return keyboard_window_.get();
 }
 

ash/keyboard/test_keyboard_ui.h

@@ -23,8 +23,9 @@ class TestKeyboardUI : public keyboard::KeyboardUI {
   TestKeyboardUI();
   ~TestKeyboardUI() override;
 
-  bool HasKeyboardWindow() const override;
-  aura::Window* GetKeyboardWindow() override;
+  // Overridden from KeyboardUI:
+  aura::Window* LoadKeyboardWindow(LoadCallback callback) override;
+  aura::Window* GetKeyboardWindow() const override;
 
  private:
   // Overridden from keyboard::KeyboardUI:

chrome/browser/ui/ash/chrome_keyboard_ui.cc

@@ -107,7 +107,7 @@ ChromeKeyboardUI::~ChromeKeyboardUI() {
 }
 
 void ChromeKeyboardUI::UpdateInsetsForWindow(aura::Window* window) {
-  if (!HasKeyboardWindow() || !ShouldWindowOverscroll(window))
+  if (!GetKeyboardWindow() || !ShouldWindowOverscroll(window))
     return;
 
   std::unique_ptr<content::RenderWidgetHostIterator> widgets(
@@ -127,13 +127,12 @@ void ChromeKeyboardUI::UpdateInsetsForWindow(aura::Window* window) {
   }
 }
 
-aura::Window* ChromeKeyboardUI::GetKeyboardWindow() {
-  if (keyboard_contents_)
-    return keyboard_contents_->web_contents()->GetNativeView();
+aura::Window* ChromeKeyboardUI::LoadKeyboardWindow(LoadCallback callback) {
+  DCHECK(!keyboard_contents_);
 
   GURL keyboard_url = GetVirtualKeyboardUrl();
   keyboard_contents_ = std::make_unique<ChromeKeyboardWebContents>(
-      browser_context_, keyboard_url);
+      browser_context_, keyboard_url, std::move(callback));
 
   aura::Window* keyboard_window =
       keyboard_contents_->web_contents()->GetNativeView();
@@ -166,8 +165,10 @@ aura::Window* ChromeKeyboardUI::GetKeyboardWindow() {
   return keyboard_window;
 }
 
-bool ChromeKeyboardUI::HasKeyboardWindow() const {
-  return !!keyboard_contents_;
+aura::Window* ChromeKeyboardUI::GetKeyboardWindow() const {
+  return keyboard_contents_
+             ? keyboard_contents_->web_contents()->GetNativeView()
+             : nullptr;
 }
 
 ui::InputMethod* ChromeKeyboardUI::GetInputMethod() {
@@ -286,10 +287,10 @@ bool ChromeKeyboardUI::ShouldWindowOverscroll(aura::Window* window) {
   if (!root_window)
     return true;
 
-  if (!HasKeyboardWindow())
+  aura::Window* keyboard_window = GetKeyboardWindow();
+  if (!keyboard_window)
     return false;
 
-  aura::Window* keyboard_window = GetKeyboardWindow();
   if (root_window != keyboard_window->GetRootWindow())
     return false;
 
@@ -310,8 +311,9 @@ void ChromeKeyboardUI::AddBoundsChangedObserver(aura::Window* window) {
 }
 
 void ChromeKeyboardUI::SetShadowAroundKeyboard() {
-  DCHECK(HasKeyboardWindow());
   aura::Window* contents_window = GetKeyboardWindow();
+  DCHECK(contents_window);
+
   if (!shadow_) {
     shadow_ = std::make_unique<ui::Shadow>();
     shadow_->Init(kShadowElevationVirtualKeyboard);

chrome/browser/ui/ash/chrome_keyboard_ui.h

@@ -52,8 +52,8 @@ class ChromeKeyboardUI : public keyboard::KeyboardUI,
   void UpdateInsetsForWindow(aura::Window* window);
 
   // Overridden from KeyboardUI:
-  aura::Window* GetKeyboardWindow() override;
-  bool HasKeyboardWindow() const override;
+  aura::Window* LoadKeyboardWindow(LoadCallback callback) override;
+  aura::Window* GetKeyboardWindow() const override;
   ui::InputMethod* GetInputMethod() override;
   void ReloadKeyboardIfNeeded() override;
   void InitInsets(const gfx::Rect& new_bounds) override;

chrome/browser/ui/ash/chrome_keyboard_ui_unittest.cc

@@ -37,8 +37,9 @@ class ChromeKeyboardUITest : public ChromeRenderViewHostTestHarness {
 }  // namespace
 
 // Ensure ChromeKeyboardContentsDelegate is successfully constructed and has
-// a valid aura::Window when GetKeyboardWindow() is called.
+// a valid aura::Window after calling LoadKeyboardWindow().
 TEST_F(ChromeKeyboardUITest, ChromeKeyboardContentsDelegate) {
-  aura::Window* window = chrome_keyboard_ui_->GetKeyboardWindow();
-  ASSERT_TRUE(window);
+  aura::Window* window =
+      chrome_keyboard_ui_->LoadKeyboardWindow(base::DoNothing());
+  EXPECT_TRUE(window);
 }

chrome/browser/ui/ash/chrome_keyboard_web_contents.cc

@@ -20,8 +20,6 @@
 #include "third_party/blink/public/platform/web_gesture_event.h"
 #include "ui/aura/window.h"
 #include "ui/base/ui_base_features.h"
-#include "ui/keyboard/keyboard_controller.h"
-#include "ui/keyboard/keyboard_resource_util.h"
 
 namespace {
 
@@ -124,7 +122,9 @@ class ChromeKeyboardContentsDelegate : public content::WebContentsDelegate,
 
 ChromeKeyboardWebContents::ChromeKeyboardWebContents(
     content::BrowserContext* context,
-    const GURL& url) {
+    const GURL& url,
+    LoadCallback callback)
+    : callback_(std::move(callback)) {
   DCHECK(context);
   content::WebContents::CreateParams web_contents_params(
       context, content::SiteInstance::CreateForURL(context, url));
@@ -171,9 +171,7 @@ void ChromeKeyboardWebContents::RenderViewCreated(
 void ChromeKeyboardWebContents::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
-  // TODO(mash): Support virtual keyboard. https://crbug.com/843332.
-  if (!features::IsMultiProcessMash())
-    keyboard::KeyboardController::Get()->NotifyKeyboardWindowLoaded();
+  std::move(callback_).Run();
 }
 
 void ChromeKeyboardWebContents::LoadContents(const GURL& url) {

chrome/browser/ui/ash/chrome_keyboard_web_contents.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "content/public/browser/web_contents_observer.h"
 
@@ -20,11 +21,15 @@ class WebContents;
 // contents, manages the content::HostZoomMap, and informs the virtual
 // keyboard controller when the contents have loaded. It also provides a
 // WebContentsDelegate implementation.
-// TODO(mash): Remove keyboard::KeyboardController dependency to support
-// multi process mash.
 class ChromeKeyboardWebContents : public content::WebContentsObserver {
  public:
-  ChromeKeyboardWebContents(content::BrowserContext* context, const GURL& url);
+  using LoadCallback = base::OnceCallback<void()>;
+
+  // Immediately starts loading |url| in a WebContents. |callback| is called
+  // when the WebContents finishes loading.
+  ChromeKeyboardWebContents(content::BrowserContext* context,
+                            const GURL& url,
+                            LoadCallback callback);
   ~ChromeKeyboardWebContents() override;
 
   // Updates the keyboard URL if |url| does not match the existing url.
@@ -45,6 +50,7 @@ class ChromeKeyboardWebContents : public content::WebContentsObserver {
   void LoadContents(const GURL& url);
 
   std::unique_ptr<content::WebContents> web_contents_;
+  LoadCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeKeyboardWebContents);
 };

chrome/browser/ui/ash/chrome_keyboard_web_contents_unittest.cc

@@ -27,9 +27,10 @@ class ChromeKeyboardWebContentsTest : public ChromeRenderViewHostTestHarness {
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
-  void CreateWebContents(const GURL& url) {
-    chrome_keyboard_web_contents_ =
-        std::make_unique<ChromeKeyboardWebContents>(profile(), url);
+  void CreateWebContents(const GURL& url,
+                         ChromeKeyboardWebContents::LoadCallback callback) {
+    chrome_keyboard_web_contents_ = std::make_unique<ChromeKeyboardWebContents>(
+        profile(), url, std::move(callback));
   }
 
  protected:
@@ -61,7 +62,7 @@ class TestDelegate : public content::WebContentsDelegate {
 
 // Calling SetKeyboardUrl with a different URL should open the new page.
 TEST_F(ChromeKeyboardWebContentsTest, SetKeyboardUrl) {
-  CreateWebContents(GURL("http://foo.com"));
+  CreateWebContents(GURL("http://foo.com"), base::DoNothing());
   ASSERT_TRUE(chrome_keyboard_web_contents_->web_contents());
 
   // Override the delegate to test that OpenURLFromTab gets called.

ui/keyboard/keyboard_controller.cc

@@ -283,7 +283,7 @@ void KeyboardController::DeactivateKeyboard() {
 }
 
 aura::Window* KeyboardController::GetKeyboardWindow() const {
-  return ui_ && ui_->HasKeyboardWindow() ? ui_->GetKeyboardWindow() : nullptr;
+  return ui_ ? ui_->GetKeyboardWindow() : nullptr;
 }
 
 aura::Window* KeyboardController::GetRootWindow() {
@@ -710,10 +710,13 @@ void KeyboardController::PopulateKeyboardContent(
 
   if (parent_container_->children().empty()) {
     DCHECK_EQ(state_, KeyboardControllerState::INITIAL);
-    // TODO(https://crbug.com/845780): This call will create and load the
-    // virtual keyboard window. Redesign the KeyboardUI interface so that
-    // loading is explicit.
-    aura::Window* keyboard_window = ui_->GetKeyboardWindow();
+    // For now, using Unretained is safe here because the |ui_| is owned by
+    // |this| and the callback does not outlive |ui_|.
+    // TODO(https://crbug.com/845780): Use a weak ptr here in case this
+    // assumption changes.
+    aura::Window* keyboard_window = ui_->LoadKeyboardWindow(
+        base::BindOnce(&KeyboardController::NotifyKeyboardWindowLoaded,
+                       base::Unretained(this)));
     keyboard_window->AddPreTargetHandler(&event_filter_);
     keyboard_window->AddObserver(this);
     parent_container_->AddChild(keyboard_window);

ui/keyboard/keyboard_ui.cc

@@ -18,15 +18,17 @@ KeyboardUI::KeyboardUI() = default;
 KeyboardUI::~KeyboardUI() = default;
 
 void KeyboardUI::ShowKeyboardWindow() {
-  if (HasKeyboardWindow()) {
+  aura::Window* window = GetKeyboardWindow();
+  if (window) {
     TRACE_EVENT0("vk", "ShowKeyboardWindow");
-    GetKeyboardWindow()->Show();
+    window->Show();
   }
 }
 
 void KeyboardUI::HideKeyboardWindow() {
-  if (HasKeyboardWindow())
-    GetKeyboardWindow()->Hide();
+  aura::Window* window = GetKeyboardWindow();
+  if (window)
+    window->Hide();
 }
 
 void KeyboardUI::SetController(KeyboardController* controller) {

ui/keyboard/keyboard_ui.h

@@ -5,6 +5,7 @@
 #ifndef UI_KEYBOARD_KEYBOARD_UI_H_
 #define UI_KEYBOARD_KEYBOARD_UI_H_
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "ui/base/ime/text_input_type.h"
 #include "ui/keyboard/keyboard_export.h"
@@ -26,17 +27,21 @@ class KeyboardController;
 // Interface representing a window containing virtual keyboard UI.
 class KEYBOARD_EXPORT KeyboardUI {
  public:
+  using LoadCallback = base::OnceCallback<void()>;
+
   KeyboardUI();
   virtual ~KeyboardUI();
 
-  // Gets the virtual keyboard window i.e. the WebContents window where
-  // keyboard extensions are loaded. May return null if the window has not yet
-  // been created.
-  // This class owns the window.
-  virtual aura::Window* GetKeyboardWindow() = 0;
+  // Begin loading the virtual keyboard window asynchronously.
+  // Returns a window immediately, but the UI within the window is not
+  // guaranteed to be fully loaded until |callback| is called.
+  // This function can only be called once.
+  virtual aura::Window* LoadKeyboardWindow(LoadCallback callback) = 0;
 
-  // Whether the keyboard window has been created.
-  virtual bool HasKeyboardWindow() const = 0;
+  // Gets the virtual keyboard window i.e. the WebContents window where
+  // keyboard extensions are loaded. Returns null if the window has not started
+  // loading.
+  virtual aura::Window* GetKeyboardWindow() const = 0;
 
   // Gets the InputMethod that will provide notifications about changes in the
   // text input context.

ui/keyboard/test/keyboard_test_util.cc

@@ -124,21 +124,23 @@ TestKeyboardUI::~TestKeyboardUI() {
   window_.reset();
 }
 
-aura::Window* TestKeyboardUI::GetKeyboardWindow() {
-  if (!window_) {
-    window_.reset(new aura::Window(&delegate_));
-    window_->Init(ui::LAYER_NOT_DRAWN);
-    window_->set_owned_by_parent(false);
-  }
+aura::Window* TestKeyboardUI::LoadKeyboardWindow(LoadCallback callback) {
+  DCHECK(!window_);
+  window_ = std::make_unique<aura::Window>(&delegate_);
+  window_->Init(ui::LAYER_NOT_DRAWN);
+  window_->set_owned_by_parent(false);
+
+  // TODO(https://crbug.com/849995): Call |callback| instead of having tests
+  // call |NotifyKeyboardWindowLoaded|.
   return window_.get();
 }
 
-ui::InputMethod* TestKeyboardUI::GetInputMethod() {
-  return input_method_;
+aura::Window* TestKeyboardUI::GetKeyboardWindow() const {
+  return window_.get();
 }
 
-bool TestKeyboardUI::HasKeyboardWindow() const {
-  return !!window_;
+ui::InputMethod* TestKeyboardUI::GetInputMethod() {
+  return input_method_;
 }
 
 }  // namespace keyboard

ui/keyboard/test/keyboard_test_util.h

@@ -43,8 +43,8 @@ class TestKeyboardUI : public KeyboardUI {
   ~TestKeyboardUI() override;
 
   // Overridden from KeyboardUI:
-  bool HasKeyboardWindow() const override;
-  aura::Window* GetKeyboardWindow() override;
+  aura::Window* LoadKeyboardWindow(LoadCallback callback) override;
+  aura::Window* GetKeyboardWindow() const override;
   ui::InputMethod* GetInputMethod() override;
   void ReloadKeyboardIfNeeded() override {}
   void InitInsets(const gfx::Rect& keyboard_bounds) override {}