Fix misc PIP multi-display bugs and refactor tests.

This change fixes some errors in conversions between Screen and root
window coordinates. It also makes the PIP window layout tests run for
multiple display configurations.

Test: unit test
Bug: b/124689893
Change-Id: I764c48d951e36c2b4bcf1d4334bf0b36a4966e64
Reviewed-on: https://chromium-review.googlesource.com/c/1477463
Commit-Queue: Eliot Courtney <edcourtney@chromium.org>
Reviewed-by: Mitsuru Oshima (Slow) <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636336}

ash/wm/pip/pip_positioner.cc

@@ -16,6 +16,7 @@
 #include "ui/aura/window.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/keyboard/keyboard_controller.h"
+#include "ui/wm/core/coordinate_conversion.h"
 
 namespace ash {
 
@@ -87,6 +88,7 @@ std::vector<gfx::Rect> CollectCollisionRects(const display::Display& display) {
         continue;
       // Use the target bounds in case an animation is in progress.
       rects.push_back(window->GetTargetBounds());
+      ::wm::ConvertRectToScreen(root_window, &rects.back());
       rects.back().Inset(-kPipWorkAreaInsetsDp, -kPipWorkAreaInsetsDp);
     }
   }
@@ -98,6 +100,7 @@ std::vector<gfx::Rect> CollectCollisionRects(const display::Display& display) {
       keyboard_controller->GetRootWindow() == root_window &&
       !keyboard_controller->visual_bounds_in_screen().IsEmpty()) {
     rects.push_back(keyboard_controller->visual_bounds_in_screen());
+    ::wm::ConvertRectToScreen(root_window, &rects.back());
     rects.back().Inset(-kPipWorkAreaInsetsDp, -kPipWorkAreaInsetsDp);
   }
 
@@ -186,13 +189,15 @@ gfx::Point ComputeBestCandidatePoint(const gfx::Point& center,
 
 gfx::Rect PipPositioner::GetMovementArea(const display::Display& display) {
   gfx::Rect work_area = display.work_area();
-  auto* keyboard_controller = keyboard::KeyboardController::Get();
 
   // Include keyboard if it's not floating.
+  auto* keyboard_controller = keyboard::KeyboardController::Get();
   if (keyboard_controller->IsEnabled() &&
       keyboard_controller->GetActiveContainerType() !=
           keyboard::mojom::ContainerType::kFloating) {
     gfx::Rect keyboard_bounds = keyboard_controller->visual_bounds_in_screen();
+    ::wm::ConvertRectToScreen(Shell::GetRootWindowForDisplayId(display.id()),
+                              &keyboard_bounds);
     work_area.Subtract(keyboard_bounds);
   }
 
@@ -201,16 +206,16 @@ gfx::Rect PipPositioner::GetMovementArea(const display::Display& display) {
 }
 
 gfx::Rect PipPositioner::GetBoundsForDrag(const display::Display& display,
-                                          const gfx::Rect& bounds) {
-  gfx::Rect drag_bounds = bounds;
+                                          const gfx::Rect& bounds_in_screen) {
+  gfx::Rect drag_bounds = bounds_in_screen;
   drag_bounds.AdjustToFit(GetMovementArea(display));
   drag_bounds = AvoidObstacles(display, drag_bounds);
   return drag_bounds;
 }
 
 gfx::Rect PipPositioner::GetRestingPosition(const display::Display& display,
-                                            const gfx::Rect& bounds) {
-  gfx::Rect resting_bounds = bounds;
+                                            const gfx::Rect& bounds_in_screen) {
+  gfx::Rect resting_bounds = bounds_in_screen;
   gfx::Rect area = GetMovementArea(display);
   resting_bounds.AdjustToFit(area);
 
@@ -219,21 +224,24 @@ gfx::Rect PipPositioner::GetRestingPosition(const display::Display& display,
   return AvoidObstacles(display, resting_bounds);
 }
 
-gfx::Rect PipPositioner::GetDismissedPosition(const display::Display& display,
-                                              const gfx::Rect& bounds) {
+gfx::Rect PipPositioner::GetDismissedPosition(
+    const display::Display& display,
+    const gfx::Rect& bounds_in_screen) {
   gfx::Rect work_area = GetMovementArea(display);
-  const int gravity = GetGravityToClosestEdge(bounds, work_area);
+  const int gravity = GetGravityToClosestEdge(bounds_in_screen, work_area);
   // Allow the bounds to move at most |kPipDismissMovementProportion| of the
   // length of the bounds in the direction of movement.
-  gfx::Rect bounds_movement_area = bounds;
-  bounds_movement_area.Inset(-bounds.width() * kPipDismissMovementProportion,
-                             -bounds.height() * kPipDismissMovementProportion);
-  gfx::Rect dismissed_bounds =
-      GetAdjustedBoundsByGravity(bounds, bounds_movement_area, gravity);
+  gfx::Rect bounds_movement_area = bounds_in_screen;
+  bounds_movement_area.Inset(
+      -bounds_in_screen.width() * kPipDismissMovementProportion,
+      -bounds_in_screen.height() * kPipDismissMovementProportion);
+  gfx::Rect dismissed_bounds = GetAdjustedBoundsByGravity(
+      bounds_in_screen, bounds_movement_area, gravity);
 
   // If the PIP window isn't close enough to the edge of the screen, don't slide
   // it out.
-  return work_area.Intersects(dismissed_bounds) ? bounds : dismissed_bounds;
+  return work_area.Intersects(dismissed_bounds) ? bounds_in_screen
+                                                : dismissed_bounds;
 }
 
 gfx::Rect PipPositioner::GetPositionAfterMovementAreaChange(
@@ -241,14 +249,15 @@ gfx::Rect PipPositioner::GetPositionAfterMovementAreaChange(
   // Restore to previous bounds if we have them. This lets us move the PIP
   // window back to its original bounds after transient movement area changes,
   // like the keyboard popping up and pushing the PIP window up.
-  const gfx::Rect bounds = window_state->HasRestoreBounds()
-                               ? window_state->GetRestoreBoundsInScreen()
-                               : window_state->window()->GetBoundsInScreen();
-  return GetRestingPosition(window_state->GetDisplay(), bounds);
+  const gfx::Rect bounds_in_screen =
+      window_state->HasRestoreBounds()
+          ? window_state->GetRestoreBoundsInScreen()
+          : window_state->window()->GetBoundsInScreen();
+  return GetRestingPosition(window_state->GetDisplay(), bounds_in_screen);
 }
 
 gfx::Rect PipPositioner::AvoidObstacles(const display::Display& display,
-                                        const gfx::Rect& bounds) {
+                                        const gfx::Rect& bounds_in_screen) {
   gfx::Rect work_area = GetMovementArea(display);
   auto rects = CollectCollisionRects(display);
   // The worst case for this should be: floating keyboard + one system tray +
@@ -257,32 +266,34 @@ gfx::Rect PipPositioner::AvoidObstacles(const display::Display& display,
                                 "should be optimized if there are a lot of "
                                 "windows. Please see crrev.com/c/1221427 for a "
                                 "description of an N^2 algorithm.";
-  return AvoidObstaclesInternal(work_area, rects, bounds);
+  return AvoidObstaclesInternal(work_area, rects, bounds_in_screen);
 }
 
 gfx::Rect PipPositioner::AvoidObstaclesInternal(
     const gfx::Rect& work_area,
     const std::vector<gfx::Rect>& rects,
-    const gfx::Rect& bounds) {
+    const gfx::Rect& bounds_in_screen) {
   gfx::Rect inset_work_area = work_area;
 
   // For even sized bounds, there is no 'center' integer point, so we need
   // to adjust the obstacles and work area to account for this.
-  inset_work_area.Inset(bounds.width() / 2, bounds.height() / 2,
-                        (bounds.width() - 1) / 2, (bounds.height() - 1) / 2);
+  inset_work_area.Inset(
+      bounds_in_screen.width() / 2, bounds_in_screen.height() / 2,
+      (bounds_in_screen.width() - 1) / 2, (bounds_in_screen.height() - 1) / 2);
   std::vector<gfx::Rect> inset_rects(rects);
   for (auto& rect : inset_rects) {
     // Reduce the collision resolution problem from rectangles-rectangle
     // resolution to rectangles-point resolution, by expanding each obstacle
-    // by |bounds| size.
-    rect.Inset(-(bounds.width() - 1) / 2, -(bounds.height() - 1) / 2,
-               -bounds.width() / 2, -bounds.height() / 2);
+    // by |bounds_in_screen| size.
+    rect.Inset(-(bounds_in_screen.width() - 1) / 2,
+               -(bounds_in_screen.height() - 1) / 2,
+               -bounds_in_screen.width() / 2, -bounds_in_screen.height() / 2);
   }
 
   gfx::Point moved_center = ComputeBestCandidatePoint(
-      bounds.CenterPoint(), inset_work_area, inset_rects);
-  gfx::Rect moved_bounds = bounds;
-  moved_bounds.Offset(moved_center - bounds.CenterPoint());
+      bounds_in_screen.CenterPoint(), inset_work_area, inset_rects);
+  gfx::Rect moved_bounds = bounds_in_screen;
+  moved_bounds.Offset(moved_center - bounds_in_screen.CenterPoint());
   return moved_bounds;
 }
 

ash/wm/pip/pip_positioner.h

@@ -19,8 +19,8 @@ namespace wm {
 class WindowState;
 }  // namespace wm
 
-class PipPositionerTest;
-
+// Computes resting and dragging positions for PIP windows. Note that this
+// class uses only Screen coordinates.
 class ASH_EXPORT PipPositioner {
  public:
   static const int kPipDismissTimeMs = 300;
@@ -36,20 +36,19 @@ class ASH_EXPORT PipPositioner {
   // this will be at a screen edge, not in the middle of the screen.
   // TODO(edcourtney): This should consider drag velocity for fling as well.
   static gfx::Rect GetRestingPosition(const display::Display& display,
-                                      const gfx::Rect& bounds);
+                                      const gfx::Rect& bounds_in_screen);
 
   // Adjusts bounds during a drag of a PIP window. For example, this will
   // ensure that the PIP window cannot leave the PIP movement area.
-  // |bounds| is in screen coordinates.
   static gfx::Rect GetBoundsForDrag(const display::Display& display,
-                                    const gfx::Rect& bounds);
+                                    const gfx::Rect& bounds_in_screen);
 
   // Based on the current PIP window position, finds a final location of where
   // the PIP window should be animated to to show a dismissal off the side
   // of the screen. Note that this may return somewhere not off-screen if
   // animating the PIP window off-screen would travel too far.
   static gfx::Rect GetDismissedPosition(const display::Display& display,
-                                        const gfx::Rect& bounds);
+                                        const gfx::Rect& bounds_in_screen);
 
   // Gets the position the PIP window should be moved to after a movement area
   // change. For example, if the shelf is changed from auto-hidden to always
@@ -58,12 +57,13 @@ class ASH_EXPORT PipPositioner {
       wm::WindowState* window_state);
 
  private:
-  friend class PipPositionerTest;
+  friend class PipPositionerDisplayTest;
+  friend class PipPositionerLogicTest;
 
   // Moves |bounds| such that it does not intersect with system ui areas, such
   // as the unified system tray or the floating keyboard.
   static gfx::Rect AvoidObstacles(const display::Display& display,
-                                  const gfx::Rect& bounds);
+                                  const gfx::Rect& bounds_in_screen);
 
   // Internal method for collision resolution. Returns a gfx::Rect with the
   // same size as |bounds|. That rectangle will not intersect any of the
@@ -72,7 +72,7 @@ class ASH_EXPORT PipPositioner {
   // closest such rectangle to |bounds|.
   static gfx::Rect AvoidObstaclesInternal(const gfx::Rect& work_area,
                                           const std::vector<gfx::Rect>& rects,
-                                          const gfx::Rect& bounds);
+                                          const gfx::Rect& bounds_in_screen);
 
   DISALLOW_COPY_AND_ASSIGN(PipPositioner);
 };

ash/wm/pip/pip_unittest.cc

@@ -13,30 +13,53 @@
 #include "ash/shell.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/wm/pip/pip_positioner.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/wm_event.h"
 #include "base/command_line.h"
 #include "ui/aura/window.h"
 #include "ui/events/test/event_generator.h"
+#include "ui/keyboard/keyboard_controller.h"
+#include "ui/keyboard/public/keyboard_switches.h"
+#include "ui/keyboard/test/keyboard_test_util.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
 namespace ash {
 
-using PipTest = AshTestBase;
+namespace {
 
-std::unique_ptr<views::Widget> CreateWidget() {
+std::unique_ptr<views::Widget> CreateWidget(aura::Window* context) {
   std::unique_ptr<views::Widget> widget(new views::Widget);
   views::Widget::InitParams params;
   params.delegate = new views::WidgetDelegateView();
   params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-  params.context = Shell::GetPrimaryRootWindow();
+  params.context = context;
   widget->Init(params);
   return widget;
 }
 
+}  // namespace
+
+class PipTest : public AshTestBase {
+ public:
+  PipTest() = default;
+  ~PipTest() override = default;
+
+  void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        keyboard::switches::kEnableVirtualKeyboard);
+    AshTestBase::SetUp();
+  }
+
+  void TearDown() override { AshTestBase::TearDown(); }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PipTest);
+};
+
 TEST_F(PipTest, ShowInactive) {
-  auto widget = CreateWidget();
+  auto widget = CreateWidget(Shell::GetPrimaryRootWindow());
   const wm::WMEvent pip_event(wm::WM_EVENT_PIP);
   auto* window_state = wm::GetWindowState(widget->GetNativeWindow());
   window_state->OnWMEvent(&pip_event);
@@ -62,8 +85,8 @@ TEST_F(PipTest, ShowInactive) {
 }
 
 TEST_F(PipTest, ShortcutNavigation) {
-  auto widget = CreateWidget();
-  auto pip_widget = CreateWidget();
+  auto widget = CreateWidget(Shell::GetPrimaryRootWindow());
+  auto pip_widget = CreateWidget(Shell::GetPrimaryRootWindow());
   widget->Show();
   pip_widget->Show();
   const wm::WMEvent pip_event(wm::WM_EVENT_PIP);
@@ -109,4 +132,125 @@ TEST_F(PipTest, ShortcutNavigation) {
   EXPECT_TRUE(pip_widget->IsActive());
 }
 
+TEST_F(PipTest, PipInitialPositionAvoidsObstacles) {
+  UpdateDisplay("400x400");
+  std::unique_ptr<aura::Window> window(
+      CreateTestWindowInShellWithBounds(gfx::Rect(100, 300, 100, 100)));
+  wm::WindowState* window_state = wm::GetWindowState(window.get());
+  const wm::WMEvent enter_pip(wm::WM_EVENT_PIP);
+  window_state->OnWMEvent(&enter_pip);
+  window->Show();
+
+  Shell::Get()->EnableKeyboard();
+  auto* keyboard_controller = keyboard::KeyboardController::Get();
+  keyboard_controller->ShowKeyboard(/*lock=*/true);
+  ASSERT_TRUE(keyboard::WaitUntilShown());
+  aura::Window* keyboard_window = keyboard_controller->GetKeyboardWindow();
+  keyboard_window->SetBounds(gfx::Rect(0, 300, 400, 100));
+
+  // Expect the PIP position is shifted below the keyboard.
+  EXPECT_TRUE(window_state->IsPip());
+  EXPECT_TRUE(window->layer()->visible());
+  EXPECT_EQ(gfx::Rect(100, 192, 100, 100), window->layer()->GetTargetBounds());
+}
+
+TEST_F(PipTest, TargetBoundsAffectedByWorkAreaChange) {
+  UpdateDisplay("400x400");
+  Shell::Get()->EnableKeyboard();
+
+  // Place a keyboard window at the initial position of a PIP window.
+  auto* keyboard_controller = keyboard::KeyboardController::Get();
+  keyboard_controller->ShowKeyboard(/*lock=*/true);
+  ASSERT_TRUE(keyboard::WaitUntilShown());
+  aura::Window* keyboard_window = keyboard_controller->GetKeyboardWindow();
+  keyboard_window->SetBounds(gfx::Rect(0, 300, 400, 100));
+
+  std::unique_ptr<aura::Window> window(
+      CreateTestWindowInShellWithBounds(gfx::Rect(100, 300, 100, 100)));
+  wm::WindowState* window_state = wm::GetWindowState(window.get());
+  const wm::WMEvent enter_pip(wm::WM_EVENT_PIP);
+  window_state->OnWMEvent(&enter_pip);
+  window->Show();
+
+  // Ensure the initial PIP position is shifted below the keyboard.
+  EXPECT_TRUE(window_state->IsPip());
+  EXPECT_TRUE(window->layer()->visible());
+  EXPECT_EQ(gfx::Rect(100, 192, 100, 100), window->bounds());
+}
+
+TEST_F(PipTest, PipRestoresToPreviousBoundsOnMovementAreaChangeIfTheyExist) {
+  UpdateDisplay("400x400");
+  std::unique_ptr<aura::Window> window(
+      CreateTestWindowInShellWithBounds(gfx::Rect(200, 200, 100, 100)));
+  wm::WindowState* window_state = wm::GetWindowState(window.get());
+  const wm::WMEvent enter_pip(wm::WM_EVENT_PIP);
+  window_state->OnWMEvent(&enter_pip);
+  window->Show();
+
+  // Position the PIP window on the side of the screen where it will be next
+  // to an edge and therefore in a resting position for the whole test.
+  const gfx::Rect bounds = gfx::Rect(292, 200, 100, 100);
+  // Set restore position to where the window currently is.
+  window->SetBounds(bounds);
+  window_state->SetRestoreBoundsInParent(bounds);
+  EXPECT_TRUE(window_state->HasRestoreBounds());
+
+  // Update the work area so that the PIP window should be pushed upward.
+  UpdateDisplay("400x200");
+  Shell::Get()->SetDisplayWorkAreaInsets(Shell::GetPrimaryRootWindow(),
+                                         gfx::Insets());
+
+  // Set PIP to the updated constrained bounds.
+  const gfx::Rect constrained_bounds =
+      PipPositioner::GetPositionAfterMovementAreaChange(window_state);
+  EXPECT_EQ(gfx::Rect(292, 92, 100, 100), constrained_bounds);
+  window->SetBoundsInScreen(constrained_bounds, window_state->GetDisplay());
+
+  // Restore the original work area.
+  UpdateDisplay("400x400");
+
+  // Expect that the PIP window is put back to where it was before.
+  EXPECT_EQ(gfx::Rect(292, 200, 100, 100),
+            PipPositioner::GetPositionAfterMovementAreaChange(window_state));
+}
+
+TEST_F(
+    PipTest,
+    PipRestoresToPreviousBoundsOnMovementAreaChangeIfTheyExistOnExternalDisplay) {
+  UpdateDisplay("400x400,400x400");
+  auto* root_window = Shell::GetAllRootWindows()[1];
+
+  // Position the PIP window on the side of the screen where it will be next
+  // to an edge and therefore in a resting position for the whole test.
+  auto widget = CreateWidget(root_window);
+  auto* window = widget->GetNativeWindow();
+  wm::WindowState* window_state = wm::GetWindowState(window);
+  const wm::WMEvent enter_pip(wm::WM_EVENT_PIP);
+  window_state->OnWMEvent(&enter_pip);
+  window->Show();
+  window->SetBounds(gfx::Rect(8, 292, 100, 100));
+
+  // Set restore position to where the window currently is.
+  window_state->SetRestoreBoundsInParent(window->bounds());
+  EXPECT_TRUE(window_state->HasRestoreBounds());
+
+  // Update the work area so that the PIP window should be pushed upward.
+  UpdateDisplay("400x400,400x200");
+  Shell::Get()->SetDisplayWorkAreaInsets(root_window, gfx::Insets());
+
+  // Set PIP to the updated constrained bounds.
+  // const gfx::Rect constrained_bounds =
+  // PipPositioner::GetPositionAfterMovementAreaChange(window_state);
+  EXPECT_EQ(gfx::Rect(408, 92, 100, 100), window->GetBoundsInScreen());
+  // window->SetBoundsInScreen(constrained_bounds, window_state->GetDisplay());
+
+  // Restore the original work area.
+  UpdateDisplay("400x400,400x400");
+  Shell::Get()->SetDisplayWorkAreaInsets(root_window, gfx::Insets());
+
+  // Expect that the PIP window is put back to where it was before.
+  EXPECT_EQ(gfx::Rect(408, 292, 100, 100), window->GetBoundsInScreen());
+  // PipPositioner::GetPositionAfterMovementAreaChange(window_state));
+}
+
 }  // namespace ash

ash/wm/pip/pip_window_resizer.cc

@@ -115,6 +115,9 @@ void PipWindowResizer::Drag(const gfx::Point& location_in_parent,
   }
 
   gfx::Rect new_bounds = CalculateBoundsForDrag(location_in_parent);
+  // We do everything in Screen coordinates, so convert here.
+  ::wm::ConvertRectToScreen(GetTarget()->parent(), &new_bounds);
+
   display::Display display = window_state()->GetDisplay();
   gfx::Rect area = PipPositioner::GetMovementArea(display);
 
@@ -133,14 +136,16 @@ void PipWindowResizer::Drag(const gfx::Point& location_in_parent,
   // window is no longer poking outside of the movement area, disable any
   // further swipe-to-dismiss gesture for this drag. Use the initial bounds
   // to decide the locked axis position.
+  gfx::Rect initial_bounds_in_screen = details().initial_bounds_in_parent;
+  ::wm::ConvertRectToScreen(GetTarget()->parent(), &initial_bounds_in_screen);
   if (may_dismiss_horizontally_) {
     if (IsPastLeftOrRightEdge(new_bounds, area))
-      new_bounds.set_y(details().initial_bounds_in_parent.y());
+      new_bounds.set_y(initial_bounds_in_screen.y());
     else if (!IsAtLeftOrRightEdge(new_bounds, area))
       may_dismiss_horizontally_ = false;
   } else if (may_dismiss_vertically_) {
     if (IsPastTopOrBottomEdge(new_bounds, area))
-      new_bounds.set_x(details().initial_bounds_in_parent.x());
+      new_bounds.set_x(initial_bounds_in_screen.x());
     else if (!IsAtTopOrBottomEdge(new_bounds, area))
       may_dismiss_vertically_ = false;
   }
@@ -149,9 +154,7 @@ void PipWindowResizer::Drag(const gfx::Point& location_in_parent,
   if (!may_dismiss_horizontally_ && !may_dismiss_vertically_) {
     // Reset opacity if it's not a dismiss gesture.
     GetTarget()->layer()->SetOpacity(1.f);
-    ::wm::ConvertRectToScreen(GetTarget()->parent(), &new_bounds);
     new_bounds = PipPositioner::GetBoundsForDrag(display, new_bounds);
-    ::wm::ConvertRectFromScreen(GetTarget()->parent(), &new_bounds);
   } else {
     gfx::Rect dismiss_bounds = new_bounds;
     dismiss_bounds.Intersect(area);
@@ -172,6 +175,8 @@ void PipWindowResizer::Drag(const gfx::Point& location_in_parent,
     may_dismiss_vertically_ = false;
   }
 
+  // Convert back to root window coordinates for setting bounds.
+  ::wm::ConvertRectFromScreen(GetTarget()->parent(), &new_bounds);
   if (new_bounds != GetTarget()->bounds()) {
     moved_or_resized_ = true;
     GetTarget()->SetBounds(new_bounds);

ash/wm/window_state.cc

@@ -741,7 +741,8 @@ void WindowState::UpdatePipState(mojom::WindowStateType old_window_state_type) {
 void WindowState::UpdatePipBounds() {
   gfx::Rect new_bounds =
       PipPositioner::GetPositionAfterMovementAreaChange(this);
-  if (window()->GetBoundsInScreen() != new_bounds) {
+  ::wm::ConvertRectFromScreen(window()->GetRootWindow(), &new_bounds);
+  if (window()->bounds() != new_bounds) {
     wm::SetBoundsEvent event(wm::WM_EVENT_SET_BOUNDS, new_bounds,
                              /*animate=*/true);
     OnWMEvent(&event);