Update keyboard browser tests to use ChromeKeyboardControllerClient

This includes:
* keyboard_end_to_end_browsertest.cc
* keyboard_controller_browsertest.cc

Note: This renames the tests in keyboard_controller_browsertest from
VirtualKeyboard* to KeyboardController* to better match the file name
and the specific area of test focus.

This also contains some minor cleanup to keyboard::KeyboardController
that primarily impacts the in-progress SingleProcessMash implementation,
but should not affect the classic Ash implementation (except to clean
it up some). It is useful to isolate these changes from the larger CL
introducing AshKeyboardUI.

Bug: 876138
Change-Id: I8aa7e5da1f04f0d8b17a58e9582608e6159bee29
Reviewed-on: https://chromium-review.googlesource.com/c/1336250
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Shu Chen <shuchen@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608638}

chrome/browser/ui/ash/chrome_keyboard_controller_client.cc

@@ -100,6 +100,12 @@ void ChromeKeyboardControllerClient::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void ChromeKeyboardControllerClient::NotifyKeyboardLoaded() {
+  is_keyboard_loaded_ = true;
+  for (auto& observer : observers_)
+    observer.OnKeyboardLoaded();
+}
+
 keyboard::mojom::KeyboardConfig
 ChromeKeyboardControllerClient::GetKeyboardConfig() {
   if (!cached_keyboard_config_) {

chrome/browser/ui/ash/chrome_keyboard_controller_client.h

@@ -37,7 +37,12 @@ class ChromeKeyboardControllerClient
 
     // Forwards the 'OnKeyboardVisibilityChanged' mojo observer method.
     // This is used by oobe and login to adjust the UI.
-    virtual void OnKeyboardVisibilityChanged(bool visible) = 0;
+    virtual void OnKeyboardVisibilityChanged(bool visible) {}
+
+    // Notifies observers when the keyboard content (i.e. the extension) has
+    // loaded. Note: if the content is already loaded when the observer is
+    // added, this will not be triggered, but see is_keyboard_loaded().
+    virtual void OnKeyboardLoaded() {}
   };
 
   // This class uses a static getter and only supports a single instance.
@@ -54,6 +59,10 @@ class ChromeKeyboardControllerClient
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // In Classic Ash, notifies this that the contents have loaded, triggering
+  // OnKeyboardLoaded.
+  void NotifyKeyboardLoaded();
+
   // Returns the cached KeyboardConfig value.
   keyboard::mojom::KeyboardConfig GetKeyboardConfig();
 
@@ -94,6 +103,7 @@ class ChromeKeyboardControllerClient
   aura::Window* GetKeyboardWindow() const;
 
   bool is_keyboard_enabled() { return is_keyboard_enabled_; }
+  bool is_keyboard_loaded() { return is_keyboard_loaded_; }
   bool is_keyboard_visible() { return is_keyboard_visible_; }
 
   void FlushForTesting();
@@ -130,6 +140,9 @@ class ChromeKeyboardControllerClient
   // Tracks the enabled state of the keyboard.
   bool is_keyboard_enabled_ = false;
 
+  // Tracks when the keyboard content has loaded.
+  bool is_keyboard_loaded_ = false;
+
   // Tracks the visible state of the keyboard.
   bool is_keyboard_visible_ = false;
 

chrome/browser/ui/ash/chrome_keyboard_ui.cc

@@ -46,7 +46,12 @@ aura::Window* ChromeKeyboardUI::LoadKeyboardWindow(LoadCallback callback) {
   keyboard_contents_ = std::make_unique<ChromeKeyboardWebContents>(
       browser_context_,
       ChromeKeyboardControllerClient::Get()->GetVirtualKeyboardUrl(),
-      std::move(callback));
+      base::BindOnce(
+          [](LoadCallback callback) {
+            ChromeKeyboardControllerClient::Get()->NotifyKeyboardLoaded();
+            std::move(callback).Run();
+          },
+          std::move(callback)));
 
   aura::Window* keyboard_window =
       keyboard_contents_->web_contents()->GetNativeView();

chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc

@@ -5,7 +5,9 @@
 #include "ash/shell.h"
 #include "base/command_line.h"
 #include "base/files/file.h"
+#include "base/run_loop.h"
 #include "chrome/browser/chromeos/input_method/textinput_test_helper.h"
+#include "chrome/browser/ui/ash/chrome_keyboard_controller_client.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -15,18 +17,56 @@
 #include "content/public/test/browser_test_utils.h"
 #include "ui/aura/test/mus/change_completion_waiter.h"
 #include "ui/aura/window_tree_host.h"
-#include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/public/keyboard_switches.h"
 #include "ui/keyboard/resources/keyboard_resource_util.h"
-#include "ui/keyboard/test/keyboard_test_util.h"
 
-namespace keyboard {
+namespace {
+
+class KeyboardVisibleWaiter : public ChromeKeyboardControllerClient::Observer {
+ public:
+  explicit KeyboardVisibleWaiter(bool visible) : visible_(visible) {
+    ChromeKeyboardControllerClient::Get()->AddObserver(this);
+  }
+  ~KeyboardVisibleWaiter() override {
+    ChromeKeyboardControllerClient::Get()->RemoveObserver(this);
+  }
+
+  void Wait() { run_loop_.Run(); }
+
+  // ChromeKeyboardControllerClient::Observer
+  void OnKeyboardVisibilityChanged(bool visible) override {
+    if (visible == visible_)
+      run_loop_.QuitWhenIdle();
+  }
+
+ private:
+  base::RunLoop run_loop_;
+  const bool visible_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyboardVisibleWaiter);
+};  // namespace
+
+bool WaitUntilShown() {
+  if (ChromeKeyboardControllerClient::Get()->is_keyboard_visible())
+    return true;
+  KeyboardVisibleWaiter(true).Wait();
+  return ChromeKeyboardControllerClient::Get()->is_keyboard_visible();
+}
+
+bool WaitUntilHidden() {
+  if (!ChromeKeyboardControllerClient::Get()->is_keyboard_visible())
+    return true;
+  KeyboardVisibleWaiter(false).Wait();
+  return !ChromeKeyboardControllerClient::Get()->is_keyboard_visible();
+}
+
+}  // namespace
 
 class KeyboardEndToEndTest : public InProcessBrowserTest {
  public:
   // Ensure that the virtual keyboard is enabled.
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitch(switches::kEnableVirtualKeyboard);
+    command_line->AppendSwitch(keyboard::switches::kEnableVirtualKeyboard);
   }
 
   void SetUpOnMainThread() override {
@@ -36,17 +76,15 @@ class KeyboardEndToEndTest : public InProcessBrowserTest {
     web_contents = browser()->tab_strip_model()->GetActiveWebContents();
     ASSERT_TRUE(web_contents);
 
-    ASSERT_TRUE(KeyboardController::Get());
-    ASSERT_TRUE(KeyboardController::Get()->IsEnabled());
-    EXPECT_FALSE(IsKeyboardVisible());
-  }
+    base::RunLoop().RunUntilIdle();
 
- protected:
-  bool IsKeyboardVisible() {
-    auto* keyboard_controller = keyboard::KeyboardController::Get();
-    return keyboard_controller->IsKeyboardVisible();
+    auto* client = ChromeKeyboardControllerClient::Get();
+    ASSERT_TRUE(client);
+    ASSERT_TRUE(client->is_keyboard_enabled());
+    EXPECT_FALSE(client->is_keyboard_visible());
   }
 
+ protected:
   // Initialized in |SetUpOnMainThread|.
   content::WebContents* web_contents;
 
@@ -133,7 +171,8 @@ IN_PROC_BROWSER_TEST_F(KeyboardEndToEndFormTest,
                       "document.getElementById('username').type = 'password'")
           .error.empty());
 
-  EXPECT_FALSE(IsKeyboardHiding());
+  base::RunLoop().RunUntilIdle();  // Allow async operations to complete.
+  EXPECT_TRUE(ChromeKeyboardControllerClient::Get()->is_keyboard_visible());
 }
 
 IN_PROC_BROWSER_TEST_F(KeyboardEndToEndFormTest,
@@ -172,7 +211,8 @@ IN_PROC_BROWSER_TEST_F(KeyboardEndToEndFormTest,
                               "setAttribute('inputmode', 'numeric')")
                   .error.empty());
 
-  EXPECT_FALSE(IsKeyboardHiding());
+  base::RunLoop().RunUntilIdle();  // Allow async operations to complete.
+  EXPECT_TRUE(ChromeKeyboardControllerClient::Get()->is_keyboard_visible());
 }
 
 IN_PROC_BROWSER_TEST_F(KeyboardEndToEndFormTest,
@@ -215,7 +255,8 @@ IN_PROC_BROWSER_TEST_F(KeyboardEndToEndFocusTest,
       content::EvalJs(web_contents, "document.getElementById('text').focus()")
           .error.empty());
 
-  EXPECT_FALSE(IsKeyboardShowing());
+  base::RunLoop().RunUntilIdle();  // Allow async operations to complete.
+  EXPECT_FALSE(ChromeKeyboardControllerClient::Get()->is_keyboard_visible());
 }
 
 IN_PROC_BROWSER_TEST_F(KeyboardEndToEndFocusTest,
@@ -230,7 +271,8 @@ IN_PROC_BROWSER_TEST_F(
     TriggerAsyncInputFocusFromUserGestureDoesNotShowKeyboard) {
   ClickElementWithId(web_contents, "async");
 
-  EXPECT_FALSE(IsKeyboardShowing());
+  base::RunLoop().RunUntilIdle();  // Allow async operations to complete.
+  EXPECT_FALSE(ChromeKeyboardControllerClient::Get()->is_keyboard_visible());
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -263,7 +305,8 @@ IN_PROC_BROWSER_TEST_F(
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(3501));
 
   ClickElementWithId(web_contents, "async");
-  EXPECT_FALSE(IsKeyboardShowing());
+  base::RunLoop().RunUntilIdle();  // Allow async operations to complete.
+  EXPECT_FALSE(ChromeKeyboardControllerClient::Get()->is_keyboard_visible());
 }
 
 class KeyboardEndToEndOverscrollTest : public KeyboardEndToEndTest {
@@ -274,7 +317,11 @@ class KeyboardEndToEndOverscrollTest : public KeyboardEndToEndTest {
 
   void FocusAndShowKeyboard() { ClickElementWithId(web_contents, "username"); }
 
-  void HideKeyboard() { KeyboardController::Get()->HideKeyboardByUser(); }
+  void HideKeyboard() {
+    auto* controller = ChromeKeyboardControllerClient::Get();
+    controller->HideKeyboard(ash::mojom::HideReason::kUser);
+    controller->FlushForTesting();
+  }
 
  protected:
   int GetViewportHeight(content::WebContents* web_contents) {
@@ -386,5 +433,3 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ(browser()->window()->GetBounds(), old_browser_bounds);
   EXPECT_EQ(GetViewportHeight(web_contents), old_height);
 }
-
-}  // namespace keyboard

testing/buildbot/filters/chromeos.mash.fyi.browser_tests.filter

@@ -193,13 +193,13 @@
 # Virtual keyboard not supported.
 -AccessibilityManagerLoginTest.ResumeSavedPref
 -DefaultKeyboardExtensionBrowserTest.*
+-KeyboardControllerAppWindowTest.*
+-KeyboardControllerStateTest.*
+-KeyboardControllerWebContentTest.*
 -KeyboardEndToEndFocusTest.*
 -KeyboardEndToEndFormTest.*
 -KeyboardEndToEndOverscrollTest.*
 -KioskVirtualKeyboardTest.*
--VirtualKeyboardAppWindowTest.*
--VirtualKeyboardStateTest.*
--VirtualKeyboardWebContentTest.*
 -PolicyTest.VirtualKeyboardEnabled
 
 # Also depends on keyboard. Test crashes in Mash. https://crbug.com/897012

testing/buildbot/filters/chromeos.single_process_mash.browser_tests.filter

@@ -3,14 +3,14 @@
 
 # Virtual keyboard not supported. https://crbug.com/646565
 -DefaultKeyboardExtensionBrowserTest.*
+-KeyboardControllerAppWindowTest.*
+-KeyboardControllerStateTest.*
+-KeyboardControllerWebContentTest.*
 -KeyboardEndToEndFocusTest.*
 -KeyboardEndToEndFormTest.*
 -KeyboardEndToEndOverscrollTest.*
 -KioskVirtualKeyboardTest.*
 -PolicyTest.VirtualKeyboardEnabled
--VirtualKeyboardAppWindowTest.*
--VirtualKeyboardStateTest.*
--VirtualKeyboardWebContentTest.*
 
 # Also depends on keyboard. Test hangs in single process mash crbug.com/897012
 -SelectFileDialogExtensionBrowserTest.SelectFileVirtualKeyboard_TabletMode

ui/keyboard/keyboard_util_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/macros.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/ime/dummy_input_method.h"
 #include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/keyboard_ui.h"
 #include "ui/keyboard/keyboard_util.h"

ui/keyboard/test/keyboard_test_util.cc

@@ -7,8 +7,6 @@
 #include "base/run_loop.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
-#include "ui/aura/window.h"
-#include "ui/aura/window_observer.h"
 #include "ui/display/screen.h"
 #include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/keyboard_controller_observer.h"
@@ -53,20 +51,6 @@ bool WaitVisibilityChangesTo(bool wait_until) {
 
 }  // namespace
 
-bool WaitUntilLoaded() {
-  auto* controller = KeyboardController::Get();
-  while (controller->GetStateForTest() == KeyboardControllerState::INITIAL ||
-         controller->GetStateForTest() ==
-             KeyboardControllerState::LOADING_EXTENSION) {
-    base::RunLoop run_loop;
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, run_loop.QuitClosure(),
-        base::TimeDelta::FromMilliseconds(100));
-    run_loop.Run();
-  }
-  return true;
-}
-
 bool WaitUntilShown() {
   // KeyboardController send a visibility update once the show animation
   // finishes.

ui/keyboard/test/keyboard_test_util.h

@@ -6,9 +6,6 @@
 #define UI_KEYBOARD_TEST_KEYBOARD_TEST_UTIL_H_
 
 #include "ui/aura/test/test_window_delegate.h"
-#include "ui/aura/window.h"
-#include "ui/base/ime/dummy_input_method.h"
-#include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/keyboard_ui.h"
 
 namespace gfx {
@@ -23,9 +20,6 @@ bool WaitUntilShown();
 // Waits until the keyboard starts to hide, with possible pending animations.
 bool WaitUntilHidden();
 
-// Waits until the keyboard web contents has loaded.
-bool WaitUntilLoaded();
-
 // Returns true if the keyboard is about to show or already shown.
 bool IsKeyboardShowing();