Keyboard: Clean up bounds observer and tests

This CL:
* Adds some debug logging to the keyboard code (DVLOG).
* Cleans up ChromeKeyboardBoundsObserver and refactors
  ShouldWindowOverscroll to not be dependent on Ash.
* Removes Ash and KeyboardController dependencies from browser tests.

These changes are in preparation for enabling the virtual keyboard in
SingleProcessMash and should have no impact on existing tests or
behavior.

Bug: 843332
Change-Id: I8954b4640c39e670350adae776fe14b079e37f36
Reviewed-on: https://chromium-review.googlesource.com/c/1343557
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Shu Chen <shuchen@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609895}

chrome/browser/ui/ash/chrome_keyboard_bounds_observer.cc

@@ -2,16 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ui/aura/window.h"
+#include "chrome/browser/ui/ash/chrome_keyboard_bounds_observer.h"
 
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/root_window_controller.h"
-#include "chrome/browser/ui/ash/chrome_keyboard_bounds_observer.h"
+#include "chrome/browser/apps/platform_apps/app_window_registry_util.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
+#include "extensions/browser/app_window/app_window.h"
+#include "ui/aura/window.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/gfx/geometry/insets.h"
+#include "ui/views/view.h"
 
 ChromeKeyboardBoundsObserver::ChromeKeyboardBoundsObserver(
     aura::Window* keyboard_window)
@@ -38,6 +41,7 @@ void ChromeKeyboardBoundsObserver::OnKeyboardOccludedBoundsChanged(
 
 void ChromeKeyboardBoundsObserver::UpdateOccludedBounds(
     const gfx::Rect& occluded_bounds) {
+  DVLOG(1) << "UpdateOccludedBounds: " << occluded_bounds.ToString();
   occluded_bounds_ = occluded_bounds;
 
   // Adjust the height of the viewport for visible windows on the primary
@@ -67,13 +71,7 @@ void ChromeKeyboardBoundsObserver::UpdateOccludedBounds(
     if (!ShouldWindowOverscroll(window))
       continue;
 
-    gfx::Rect view_bounds = view->GetViewBounds();
-    gfx::Rect intersect = gfx::IntersectRects(view_bounds, occluded_bounds);
-    int overlap = intersect.height();
-    if (overlap > 0 && overlap < view_bounds.height())
-      view->SetInsets(gfx::Insets(0, 0, overlap, 0));
-    else
-      view->SetInsets(gfx::Insets());
+    UpdateInsetsForHostView(view);
     AddObservedWindow(window);
   }
 
@@ -101,6 +99,7 @@ void ChromeKeyboardBoundsObserver::OnWindowBoundsChanged(
     const gfx::Rect& old_bounds,
     const gfx::Rect& new_bounds,
     ui::PropertyChangeReason reason) {
+  DVLOG(1) << "OnWindowBoundsChanged: " << new_bounds.ToString();
   UpdateInsetsForWindow(window);
 }
 
@@ -119,39 +118,41 @@ void ChromeKeyboardBoundsObserver::UpdateInsetsForWindow(aura::Window* window) {
   while (content::RenderWidgetHost* widget = widgets->GetNextHost()) {
     content::RenderWidgetHostView* view = widget->GetView();
     if (view && window->Contains(view->GetNativeView())) {
+      if (ShouldEnableInsets(window))
+        UpdateInsetsForHostView(view);
+      else
+        view->SetInsets(gfx::Insets());
+    }
+  }
+}
+
+void ChromeKeyboardBoundsObserver::UpdateInsetsForHostView(
+    content::RenderWidgetHostView* view) {
   gfx::Rect view_bounds = view->GetViewBounds();
   gfx::Rect intersect = gfx::IntersectRects(view_bounds, occluded_bounds_);
-      int overlap = ShouldEnableInsets(window) ? intersect.height() : 0;
-      if (overlap > 0 && overlap < view_bounds.height())
+  int overlap = intersect.height();
+  if (overlap > 0 && overlap < view_bounds.height()) {
+    DVLOG(2) << "SetInsets for: " << view << " Overlap: " << overlap;
     view->SetInsets(gfx::Insets(0, 0, overlap, 0));
-      else
+  } else {
     view->SetInsets(gfx::Insets());
   }
-  }
 }
 
 bool ChromeKeyboardBoundsObserver::ShouldWindowOverscroll(
     aura::Window* window) {
-  // When the WS is running, all available windows are Chrome windows and
-  // should overscroll.
-  if (::features::IsUsingWindowService())
-    return true;
-
-  aura::Window* root_window = window->GetRootWindow();
-  if (!root_window)
-    return true;
+  // The virtual keyboard should not overscroll.
+  if (window->GetToplevelWindow() == keyboard_window_->GetToplevelWindow())
+    return false;
 
-  if (root_window != keyboard_window_->GetRootWindow())
+  // IME windows should not overscroll.
+  extensions::AppWindow* app_window =
+      AppWindowRegistryUtil::GetAppWindowForNativeWindowAnyProfile(
+          window->GetToplevelWindow());
+  if (app_window && app_window->is_ime_window())
     return false;
 
-  // Shell IME window container contains virtual keyboard windows and IME
-  // windows (IME windows are created by the chrome.app.window.create API).
-  // They should never be overscrolled.
-  ash::RootWindowController* root_window_controller =
-      ash::RootWindowController::ForWindow(root_window);
-  return !root_window_controller
-              ->GetContainer(ash::kShellWindowId_ImeWindowParentContainer)
-              ->Contains(window);
+  return true;
 }
 
 bool ChromeKeyboardBoundsObserver::ShouldEnableInsets(aura::Window* window) {

chrome/browser/ui/ash/chrome_keyboard_bounds_observer.h

@@ -12,6 +12,10 @@
 #include "ui/aura/window_observer.h"
 #include "ui/gfx/geometry/rect.h"
 
+namespace content {
+class RenderWidgetHostView;
+}
+
 // Class responsible for updating insets for windows overlapping the virtual
 // keyboard.
 class ChromeKeyboardBoundsObserver
@@ -41,6 +45,7 @@ class ChromeKeyboardBoundsObserver
   void OnWindowDestroyed(aura::Window* window) override;
 
   void UpdateInsetsForWindow(aura::Window* window);
+  void UpdateInsetsForHostView(content::RenderWidgetHostView* view);
   bool ShouldWindowOverscroll(aura::Window* window);
   bool ShouldEnableInsets(aura::Window* window);
 

chrome/browser/ui/ash/keyboard_controller_browsertest.cc

@@ -22,6 +22,8 @@
 #include "ui/base/ime/input_method.h"
 #include "ui/base/ime/input_method_factory.h"
 #include "ui/base/ui_base_features.h"
+#include "ui/display/display.h"
+#include "ui/display/screen.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/public/keyboard_switches.h"
@@ -79,14 +81,10 @@ class KeyboardLoadedWaiter : public ChromeKeyboardControllerClient::Observer {
   DISALLOW_COPY_AND_ASSIGN(KeyboardLoadedWaiter);
 };
 
-aura::Window* GetKeyboardRootWindow() {
-  return ChromeKeyboardControllerClient::Get()
+ui::InputMethod* GetInputMethod() {
+  aura::Window* root_window = ChromeKeyboardControllerClient::Get()
                                   ->GetKeyboardWindow()
                                   ->GetRootWindow();
-}
-
-ui::InputMethod* GetInputMethod() {
-  aura::Window* root_window = GetKeyboardRootWindow();
   return root_window ? root_window->GetHost()->GetInputMethod() : nullptr;
 }
 
@@ -195,7 +193,7 @@ IN_PROC_BROWSER_TEST_F(KeyboardControllerWebContentTest,
   controller->FlushForTesting();
 
   // Drag the top left corner of the keyboard to move it.
-  ui::test::EventGenerator event_generator(GetKeyboardRootWindow());
+  ui::test::EventGenerator event_generator(contents_window->GetRootWindow());
   event_generator.MoveMouseTo(gfx::Point(0, 0));
   event_generator.PressLeftButton();
   event_generator.MoveMouseTo(gfx::Point(50, 50));
@@ -268,10 +266,14 @@ IN_PROC_BROWSER_TEST_F(KeyboardControllerAppWindowTest,
   controller->ShowKeyboard();
   KeyboardVisibleWaiter(true).Wait();
 
-  aura::Window* keyboard_window = GetKeyboardRootWindow();
-  ASSERT_TRUE(keyboard_window);
-  int screen_height = keyboard_window->bounds().height();
-  gfx::Rect test_bounds(0, 0, 0, screen_height - ime_window_visible_height + 1);
+  int screen_height = display::Screen::GetScreen()
+                          ->GetPrimaryDisplay()
+                          .GetSizeInPixel()
+                          .height();
+  int keyboard_height = screen_height - ime_window_visible_height + 1;
+  ASSERT_GT(keyboard_height, 0);
+  gfx::Rect test_bounds = controller->GetKeyboardWindow()->bounds();
+  test_bounds.set_height(keyboard_height);
   controller->GetKeyboardWindow()->SetBounds(test_bounds);
   // Allow actions triggered by window bounds observers to complete.
   base::RunLoop().RunUntilIdle();

chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc

@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/shell.h"
 #include "base/command_line.h"
 #include "base/files/file.h"
 #include "base/run_loop.h"
@@ -17,6 +16,8 @@
 #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/display/display.h"
+#include "ui/display/screen.h"
 #include "ui/keyboard/public/keyboard_switches.h"
 #include "ui/keyboard/resources/keyboard_resource_util.h"
 
@@ -60,6 +61,10 @@ bool WaitUntilHidden() {
   return !ChromeKeyboardControllerClient::Get()->is_keyboard_visible();
 }
 
+gfx::Size GetScreenBounds() {
+  return display::Screen::GetScreen()->GetPrimaryDisplay().GetSizeInPixel();
+}
+
 }  // namespace
 
 class KeyboardEndToEndTest : public InProcessBrowserTest {
@@ -356,7 +361,7 @@ IN_PROC_BROWSER_TEST_F(
     ToggleKeyboardOnNonOverlappingWindowDoesNotAffectViewport) {
   // Set the window bounds so that it does not overlap with the keyboard.
   // The virtual keyboard takes up no more than half the screen height.
-  const auto screen_bounds = ash::Shell::GetPrimaryRootWindow()->bounds();
+  gfx::Size screen_bounds = GetScreenBounds();
   browser()->window()->SetBounds(
       gfx::Rect(0, 0, screen_bounds.width(), screen_bounds.height() / 2));
   aura::test::WaitForAllChangesToComplete();
@@ -380,7 +385,7 @@ IN_PROC_BROWSER_TEST_F(
   // Shift the window down so that it overlaps with the keyboard, but shrink the
   // window size so that when it moves upwards, it will no longer overlap with
   // the keyboard.
-  const auto screen_bounds = ash::Shell::GetPrimaryRootWindow()->bounds();
+  gfx::Size screen_bounds = GetScreenBounds();
   browser()->window()->SetBounds(gfx::Rect(0, screen_bounds.height() / 2,
                                            screen_bounds.width(),
                                            screen_bounds.height() / 2));
@@ -410,7 +415,7 @@ IN_PROC_BROWSER_TEST_F(
   // Shift the window down so that it overlaps with the keyboard, and expand the
   // window size so that when it moves upwards, it will still overlap with
   // the keyboard.
-  const auto screen_bounds = ash::Shell::GetPrimaryRootWindow()->bounds();
+  gfx::Size screen_bounds = GetScreenBounds();
   browser()->window()->SetBounds(gfx::Rect(0, screen_bounds.height() / 3,
                                            screen_bounds.width(),
                                            screen_bounds.height() / 3 * 2));

ui/keyboard/keyboard_controller.cc

@@ -486,7 +486,7 @@ bool KeyboardController::IsKeyboardEnableRequested() const {
 }
 
 bool KeyboardController::IsKeyboardOverscrollEnabled() const {
-  if (!keyboard::IsKeyboardEnabled())
+  if (!IsKeyboardEnableRequested())
     return false;
 
   // Users of the sticky accessibility on-screen keyboard are likely to be using
@@ -674,12 +674,14 @@ void KeyboardController::SetContainerBehaviorInternal(
 }
 
 void KeyboardController::ShowKeyboard(bool lock) {
+  DVLOG(1) << "ShowKeyboard";
   set_keyboard_locked(lock);
   ShowKeyboardInternal(display::Display());
 }
 
 void KeyboardController::ShowKeyboardInDisplay(
     const display::Display& display) {
+  DVLOG(1) << "ShowKeyboardInDisplay: " << display.id();
   set_keyboard_locked(true);
   ShowKeyboardInternal(display);
 }
@@ -811,8 +813,9 @@ void KeyboardController::ShowKeyboardIfWithinTransientBlurThreshold() {
 }
 
 void KeyboardController::OnShowVirtualKeyboardIfEnabled() {
+  DVLOG(1) << "OnShowVirtualKeyboardIfEnabled: " << IsKeyboardEnableRequested();
   // Calling |ShowKeyboardInternal| may move the keyboard to another display.
-  if (keyboard::IsKeyboardEnabled() && !keyboard_locked_)
+  if (IsKeyboardEnableRequested() && !keyboard_locked_)
     ShowKeyboardInternal(display::Display());
 }
 
@@ -827,6 +830,7 @@ void KeyboardController::PopulateKeyboardContent(
     bool show_keyboard) {
   DCHECK(show_keyboard || state_ == KeyboardControllerState::INITIAL);
 
+  DVLOG(1) << "PopulateKeyboardContent: " << StateToStr(state_);
   TRACE_EVENT0("vk", "PopulateKeyboardContent");
 
   if (parent_container_->children().empty()) {
@@ -835,6 +839,7 @@ void KeyboardController::PopulateKeyboardContent(
     // |this| and the callback does not outlive |ui_|.
     // TODO(https://crbug.com/845780): Use a weak ptr here in case this
     // assumption changes.
+    DVLOG(1) << "LoadKeyboardWindow";
     aura::Window* keyboard_window = ui_->LoadKeyboardWindow(
         base::BindOnce(&KeyboardController::NotifyKeyboardWindowLoaded,
                        base::Unretained(this)));

ui/keyboard/keyboard_ui.cc

@@ -18,6 +18,7 @@ KeyboardUI::KeyboardUI() = default;
 KeyboardUI::~KeyboardUI() = default;
 
 void KeyboardUI::ShowKeyboardWindow() {
+  DVLOG(1) << "ShowKeyboardWindow";
   aura::Window* window = GetKeyboardWindow();
   if (window) {
     TRACE_EVENT0("vk", "ShowKeyboardWindow");
@@ -26,6 +27,7 @@ void KeyboardUI::ShowKeyboardWindow() {
 }
 
 void KeyboardUI::HideKeyboardWindow() {
+  DVLOG(1) << "HideKeyboardWindow";
   aura::Window* window = GetKeyboardWindow();
   if (window)
     window->Hide();