Clean up ChromeKeyboardBoundsObserver and fix for Mash

https://crrev.com/c/1366879 fixed small window repositioning for Mash
but exposed a bug in ChromeKeyboardBoundsObserver::ShouldEnableInsets.

This fixes that issue and includes some cleanup done while debugging.

It also updates the filter file tpo remove tests that are now passing!

Bug: 906888
Change-Id: I64dcfe8de4a5970d69a76f26646b80be01016a50
Reviewed-on: https://chromium-review.googlesource.com/c/1372319
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615752}

ash/keyboard/ash_keyboard_ui.cc

@@ -175,7 +175,7 @@ void AshKeyboardUI::OnWindowBoundsChanged(aura::Window* window,
                                           const gfx::Rect& old_bounds,
                                           const gfx::Rect& new_bounds,
                                           ui::PropertyChangeReason reason) {
-  DVLOG(1) << "OnWindowBoundsChanged: " << window << ": "
+  DVLOG(1) << "OnWindowBoundsChanged: " << window->GetName() << ": "
            << new_bounds.ToString();
 
   // Normally OnKeyboardVisibleBoundsChanged is triggered from

chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.cc

@@ -13,10 +13,28 @@
 #include "extensions/browser/app_window/app_window.h"
 #include "ui/aura/window.h"
 #include "ui/base/ui_base_features.h"
+#include "ui/display/display.h"
+#include "ui/display/screen.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
+namespace {
+
+// Returns the RenderWidgetHostView contained by |window|.
+content::RenderWidgetHostView* GetHostViewForWindow(aura::Window* window) {
+  std::unique_ptr<content::RenderWidgetHostIterator> hosts(
+      content::RenderWidgetHost::GetRenderWidgetHosts());
+  while (content::RenderWidgetHost* host = hosts->GetNextHost()) {
+    content::RenderWidgetHostView* view = host->GetView();
+    if (view && window->Contains(view->GetNativeView()))
+      return view;
+  }
+  return nullptr;
+}
+
+}  // namespace
+
 ChromeKeyboardBoundsObserver::ChromeKeyboardBoundsObserver(
     aura::Window* keyboard_window)
     : keyboard_window_(keyboard_window) {
@@ -34,6 +52,7 @@ ChromeKeyboardBoundsObserver::~ChromeKeyboardBoundsObserver() {
 
 void ChromeKeyboardBoundsObserver::OnKeyboardOccludedBoundsChanged(
     const gfx::Rect& bounds) {
+  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << bounds.ToString();
   UpdateOccludedBounds(
       ChromeKeyboardControllerClient::Get()->IsKeyboardOverscrollEnabled()
           ? bounds
@@ -45,23 +64,31 @@ void ChromeKeyboardBoundsObserver::UpdateOccludedBounds(
   DVLOG(1) << "UpdateOccludedBounds: " << occluded_bounds.ToString();
   occluded_bounds_ = occluded_bounds;
 
+  std::unique_ptr<content::RenderWidgetHostIterator> hosts(
+      content::RenderWidgetHost::GetRenderWidgetHosts());
+
+  // If |occluded_bounds| is empty (i.e. the keyboard is hidden or floating)
+  // reset the insets for all RenderWidgetHosts and remove observers.
+  if (occluded_bounds.IsEmpty()) {
+    while (content::RenderWidgetHost* host = hosts->GetNextHost()) {
+      content::RenderWidgetHostView* view = host->GetView();
+      if (view)
+        view->SetInsets(gfx::Insets());
+    }
+    RemoveAllObservedWindows();
+    return;
+  }
+
   // Adjust the height of the viewport for visible windows on the primary
   // display. TODO(kevers): Add EnvObserver to properly initialize insets if a
   // window is created while the keyboard is visible.
-  std::unique_ptr<content::RenderWidgetHostIterator> widgets(
-      content::RenderWidgetHost::GetRenderWidgetHosts());
-  while (content::RenderWidgetHost* widget = widgets->GetNextHost()) {
-    content::RenderWidgetHostView* view = widget->GetView();
+  while (content::RenderWidgetHost* host = hosts->GetNextHost()) {
+    content::RenderWidgetHostView* view = host->GetView();
     // Can be null, e.g. if the RenderWidget is being destroyed or
     // the render process crashed.
     if (!view)
       continue;
 
-    if (occluded_bounds.IsEmpty()) {
-      view->SetInsets(gfx::Insets());
-      continue;
-    }
-
     aura::Window* window = view->GetNativeView();
     // Added while we determine if RenderWidgetHostViewChildFrame can be
     // changed to always return a non-null value: https://crbug.com/644726.
@@ -72,12 +99,9 @@ void ChromeKeyboardBoundsObserver::UpdateOccludedBounds(
     if (!ShouldWindowOverscroll(window))
       continue;
 
-    UpdateInsetsForHostView(view);
+    UpdateInsets(window, view);
     AddObservedWindow(window);
   }
-
-  if (occluded_bounds.IsEmpty())
-    RemoveAllObservedWindows();
 }
 
 void ChromeKeyboardBoundsObserver::AddObservedWindow(aura::Window* window) {
@@ -101,7 +125,16 @@ void ChromeKeyboardBoundsObserver::OnWidgetBoundsChanged(
     views::Widget* widget,
     const gfx::Rect& new_bounds) {
   DVLOG(1) << "OnWidgetBoundsChanged: " << new_bounds.ToString();
-  UpdateInsetsForWindow(widget->GetNativeView());
+
+  aura::Window* window = widget->GetNativeView();
+  if (!ShouldWindowOverscroll(window))
+    return;
+
+  content::RenderWidgetHostView* host_view = GetHostViewForWindow(window);
+  if (!host_view)
+    return;  // Transition edge case
+
+  UpdateInsets(window, host_view);
 }
 
 void ChromeKeyboardBoundsObserver::OnWidgetDestroying(views::Widget* widget) {
@@ -110,34 +143,24 @@ void ChromeKeyboardBoundsObserver::OnWidgetDestroying(views::Widget* widget) {
   observed_widgets_.erase(widget);
 }
 
-void ChromeKeyboardBoundsObserver::UpdateInsetsForWindow(aura::Window* window) {
-  if (!ShouldWindowOverscroll(window))
-    return;
-
-  std::unique_ptr<content::RenderWidgetHostIterator> widgets(
-      content::RenderWidgetHost::GetRenderWidgetHosts());
-  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(
+void ChromeKeyboardBoundsObserver::UpdateInsets(
+    aura::Window* window,
     content::RenderWidgetHostView* view) {
   gfx::Rect view_bounds = view->GetViewBounds();
+  if (!ShouldEnableInsets(window)) {
+    DVLOG(2) << "ResetInsets: " << window->GetName()
+             << " Bounds: " << view_bounds.ToString();
+    view->SetInsets(gfx::Insets());
+    return;
+  }
   gfx::Rect intersect = gfx::IntersectRects(view_bounds, occluded_bounds_);
   int overlap = intersect.height();
-  if (overlap > 0 && overlap < view_bounds.height()) {
-    DVLOG(2) << "SetInsets for: " << view << " Overlap: " << overlap;
+  DVLOG(2) << "SetInsets: " << window->GetName()
+           << " Bounds: " << view_bounds.ToString() << " Overlap: " << overlap;
+  if (overlap > 0 && overlap < view_bounds.height())
     view->SetInsets(gfx::Insets(0, 0, overlap, 0));
-  } else {
+  else
     view->SetInsets(gfx::Insets());
-  }
 }
 
 bool ChromeKeyboardBoundsObserver::ShouldWindowOverscroll(
@@ -157,7 +180,11 @@ bool ChromeKeyboardBoundsObserver::ShouldWindowOverscroll(
 }
 
 bool ChromeKeyboardBoundsObserver::ShouldEnableInsets(aura::Window* window) {
-  return keyboard_window_->IsVisible() &&
-         keyboard_window_->GetRootWindow() == window->GetRootWindow() &&
-         ChromeKeyboardControllerClient::Get()->IsKeyboardOverscrollEnabled();
+  if (!keyboard_window_->IsVisible() ||
+      !ChromeKeyboardControllerClient::Get()->IsKeyboardOverscrollEnabled()) {
+    return false;
+  }
+  const auto* screen = display::Screen::GetScreen();
+  return screen->GetDisplayNearestWindow(window).id() ==
+         screen->GetDisplayNearestWindow(keyboard_window_).id();
 }

chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.h

@@ -25,15 +25,15 @@ class ChromeKeyboardBoundsObserver
   explicit ChromeKeyboardBoundsObserver(aura::Window* keyboard_window);
   ~ChromeKeyboardBoundsObserver() override;
 
-  // Provides the bounds occluded by the keyboard any time they change.
-  // (i.e. by the KeyboardController through KeyboardUI::InitInsets).
-  void UpdateOccludedBounds(const gfx::Rect& occluded_bounds);
-
   // keyboard::ChromeKeyboardControllerClient::Observer:
   void OnKeyboardVisibilityChanged(bool visible) override {}
   void OnKeyboardOccludedBoundsChanged(const gfx::Rect& bounds) override;
 
  private:
+  // Provides the bounds occluded by the keyboard any time they change.
+  // (i.e. by the KeyboardController through KeyboardUI::InitInsets).
+  void UpdateOccludedBounds(const gfx::Rect& occluded_bounds);
+
   void AddObservedWindow(aura::Window* window);
   void RemoveAllObservedWindows();
 
@@ -42,8 +42,7 @@ class ChromeKeyboardBoundsObserver
                              const gfx::Rect& new_bounds) override;
   void OnWidgetDestroying(views::Widget* widget) override;
 
-  void UpdateInsetsForWindow(aura::Window* window);
-  void UpdateInsetsForHostView(content::RenderWidgetHostView* view);
+  void UpdateInsets(aura::Window* window, content::RenderWidgetHostView* view);
   bool ShouldWindowOverscroll(aura::Window* window);
   bool ShouldEnableInsets(aura::Window* window);
 

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

 # These tests currently fail with SingleProcessMash enabled.
 # Bug: crbug.com/877118
 
-# Changing the Chrome keyboard window size does not change the Ash keyboard
-# window size. These tests may require a test mojo interface for Mash.
-# https://crbug.com/843332.
--KeyboardControllerAppWindowTest.DisableOverscrollForImeWindow
--KeyboardEndToEndOverscrollTest.ToggleKeyboardOnMaximizedWindowAffectsViewport
-
-# Keyboard window does not move browser windows. https://crbug.com/906888.
--KeyboardEndToEndOverscrollTest.ToggleKeyboardOnShortOverlappingWindowMovesWindowUpwards
--KeyboardEndToEndOverscrollTest.ToggleKeyboardOnTallOverlappingWindowMovesWindowUpwardsAndAffectsViewport
-
 # Mouse move events are not handled correctly in tests.
 # TODO(stevenjb): Determine why this works in practice but fails in the test.
 -KeyboardControllerWebContentTest.CanDragFloatingKeyboardWithMouse