cros: Clean up ash::WindowPositioner and GetPopupPosition

* Convert GetPopupPosition to take a gfx::Size(), because it does not
  use the position of the passed Rect.
* Convert member variables that always match constant values to just
  use constants
* Convert non-mutating functions to static

I noticed these while looking at WindowPositioner for mustash.

TBR=msw@chromium.org

Bug: 764009
Test: ash_unittests, chrome unit_tests WindowSizer*
Change-Id: Ie9c16a0a41fc46d8b6ca8918e1cfa757f80e8b70
Reviewed-on: https://chromium-review.googlesource.com/811684
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522297}

ash/wm/window_positioner.cc

@@ -21,8 +21,6 @@
 
 namespace ash {
 
-const int WindowPositioner::kMinimumWindowOffset = 32;
-
 // The number of pixels which are kept free top, left and right when a window
 // gets positioned to its default location.
 // static
@@ -280,9 +278,8 @@ void WindowPositioner::GetBoundsAndShowStateForNewWindow(
       // the top level window and use its restore bounds
       // instead. Offset the bounds to prevent the windows from
       // overlapping exactly when restored.
-      *bounds_in_out =
-          top_window_state->GetRestoreBoundsInScreen() +
-          gfx::Vector2d(kMinimumWindowOffset, kMinimumWindowOffset);
+      *bounds_in_out = top_window_state->GetRestoreBoundsInScreen() +
+                       gfx::Vector2d(kFirstPopupOffset, kFirstPopupOffset);
     }
     if (is_saved_bounds || has_restore_bounds) {
       gfx::Rect work_area = display::Screen::GetScreen()
@@ -394,13 +391,7 @@ void WindowPositioner::RearrangeVisibleWindowOnShow(
     added_window->SetBounds(added_bounds);
 }
 
-WindowPositioner::WindowPositioner()
-    : pop_position_offset_increment_x(0),
-      pop_position_offset_increment_y(0),
-      popup_position_offset_from_screen_corner_x(0),
-      popup_position_offset_from_screen_corner_y(0),
-      last_popup_position_x_(0),
-      last_popup_position_y_(0) {}
+WindowPositioner::WindowPositioner() = default;
 
 WindowPositioner::~WindowPositioner() = default;
 
@@ -423,18 +414,15 @@ gfx::Rect WindowPositioner::GetDefaultWindowBounds(
                    default_width, default_height);
 }
 
-gfx::Rect WindowPositioner::GetPopupPosition(const gfx::Rect& old_pos) {
-  int grid = kMinimumWindowOffset;
-  popup_position_offset_from_screen_corner_x = grid;
-  popup_position_offset_from_screen_corner_y = grid;
-  if (!pop_position_offset_increment_x) {
-    // When the popup position increment is 0, the last popup position
-    // was not yet initialized.
-    last_popup_position_x_ = popup_position_offset_from_screen_corner_x;
-    last_popup_position_y_ = popup_position_offset_from_screen_corner_y;
+gfx::Rect WindowPositioner::GetPopupPosition(const gfx::Size& popup_size) {
+  if (!has_last_popup_position_) {
+    // Start with the initial offset.
+    // NOTE: This should probably move into NormalPopupPosition() but is here to
+    // match historical behavior.
+    last_popup_position_x_ = kFirstPopupOffset;
+    last_popup_position_y_ = kFirstPopupOffset;
+    has_last_popup_position_ = true;
   }
-  pop_position_offset_increment_x = grid;
-  pop_position_offset_increment_y = grid;
   // We handle the Multi monitor support by retrieving the active window's
   // work area.
   aura::Window* window = wm::GetActiveWindow();
@@ -444,17 +432,16 @@ gfx::Rect WindowPositioner::GetPopupPosition(const gfx::Rect& old_pos) {
                 ->GetDisplayNearestWindow(window)
                 .work_area()
           : display::Screen::GetScreen()->GetPrimaryDisplay().work_area();
-  // Only try to reposition the popup when it is not spanning the entire
-  // screen.
-  if ((old_pos.width() + popup_position_offset_from_screen_corner_x >=
-       work_area.width()) ||
-      (old_pos.height() + popup_position_offset_from_screen_corner_y >=
-       work_area.height()))
-    return AlignPopupPosition(old_pos, work_area, grid);
-  const gfx::Rect result = SmartPopupPosition(old_pos, work_area, grid);
+  // If the popup would span the entire display, position it at top-left.
+  if ((popup_size.width() + kFirstPopupOffset >= work_area.width()) ||
+      (popup_size.height() + kFirstPopupOffset >= work_area.height())) {
+    return AlignPopupPosition(gfx::Rect(popup_size), work_area);
+  }
+  const gfx::Rect result = SmartPopupPosition(popup_size, work_area);
   if (!result.IsEmpty())
-    return AlignPopupPosition(result, work_area, grid);
-  return NormalPopupPosition(old_pos, work_area);
+    return AlignPopupPosition(result, work_area);
+
+  return NormalPopupPosition(popup_size, work_area);
 }
 
 // static
@@ -462,10 +449,10 @@ void WindowPositioner::SetMaximizeFirstWindow(bool maximize) {
   maximize_first_window = maximize;
 }
 
-gfx::Rect WindowPositioner::NormalPopupPosition(const gfx::Rect& old_pos,
+gfx::Rect WindowPositioner::NormalPopupPosition(const gfx::Size& popup_size,
                                                 const gfx::Rect& work_area) {
-  int w = old_pos.width();
-  int h = old_pos.height();
+  int w = popup_size.width();
+  int h = popup_size.height();
   // Note: The 'last_popup_position' is checked and kept relative to the
   // screen size. The offsetting will be done in the last step when the
   // target rectangle gets returned.
@@ -473,30 +460,29 @@ gfx::Rect WindowPositioner::NormalPopupPosition(const gfx::Rect& old_pos,
   if (last_popup_position_y_ + h > work_area.height() ||
       last_popup_position_x_ + w > work_area.width()) {
     // Popup does not fit on screen. Reset to next diagonal row.
-    last_popup_position_x_ -= last_popup_position_y_ -
-                              popup_position_offset_from_screen_corner_x -
-                              pop_position_offset_increment_x;
-    last_popup_position_y_ = popup_position_offset_from_screen_corner_y;
+    last_popup_position_x_ -=
+        last_popup_position_y_ - kFirstPopupOffset - kNextPopupOffset;
+    last_popup_position_y_ = kFirstPopupOffset;
     reset = true;
   }
   if (last_popup_position_x_ + w > work_area.width()) {
     // Start again over.
-    last_popup_position_x_ = popup_position_offset_from_screen_corner_x;
-    last_popup_position_y_ = popup_position_offset_from_screen_corner_y;
+    last_popup_position_x_ = kFirstPopupOffset;
+    last_popup_position_y_ = kFirstPopupOffset;
     reset = true;
   }
   int x = last_popup_position_x_;
   int y = last_popup_position_y_;
   if (!reset) {
-    last_popup_position_x_ += pop_position_offset_increment_x;
-    last_popup_position_y_ += pop_position_offset_increment_y;
+    last_popup_position_x_ += kNextPopupOffset;
+    last_popup_position_y_ += kNextPopupOffset;
   }
   return gfx::Rect(x + work_area.x(), y + work_area.y(), w, h);
 }
 
-gfx::Rect WindowPositioner::SmartPopupPosition(const gfx::Rect& old_pos,
-                                               const gfx::Rect& work_area,
-                                               int grid) {
+// static
+gfx::Rect WindowPositioner::SmartPopupPosition(const gfx::Size& popup_size,
+                                               const gfx::Rect& work_area) {
   const aura::Window::Windows windows =
       Shell::Get()->mru_window_tracker()->BuildWindowListIgnoreModal();
 
@@ -519,8 +505,8 @@ gfx::Rect WindowPositioner::SmartPopupPosition(const gfx::Rect& old_pos,
   if (regions.empty())
     return gfx::Rect(0, 0, 0, 0);
 
-  int w = old_pos.width();
-  int h = old_pos.height();
+  int w = popup_size.width();
+  int h = popup_size.height();
   int x_end = work_area.width() / 2;
   int x, x_increment;
   // We parse for a proper location on the screen. We do this in two runs:
@@ -535,10 +521,10 @@ gfx::Rect WindowPositioner::SmartPopupPosition(const gfx::Rect& old_pos,
   for (int run = 0; run < 2; run++) {
     if (run == 0) {  // First run: Start left, parse right till mid screen.
       x = 0;
-      x_increment = pop_position_offset_increment_x;
+      x_increment = kNextPopupOffset;
     } else {  // Second run: Start right, parse left till mid screen.
       x = work_area.width() - w;
-      x_increment = -pop_position_offset_increment_x;
+      x_increment = -kNextPopupOffset;
     }
     // Note: The passing (x,y,w,h) window is always relative to the work area's
     // origin.
@@ -561,22 +547,19 @@ gfx::Rect WindowPositioner::SmartPopupPosition(const gfx::Rect& old_pos,
   return gfx::Rect(0, 0, 0, 0);
 }
 
+// static
 gfx::Rect WindowPositioner::AlignPopupPosition(const gfx::Rect& pos,
-                                               const gfx::Rect& work_area,
-                                               int grid) {
-  if (grid <= 1)
-    return pos;
-
-  int x = pos.x() - (pos.x() - work_area.x()) % grid;
-  int y = pos.y() - (pos.y() - work_area.y()) % grid;
+                                               const gfx::Rect& work_area) {
+  int x = pos.x() - (pos.x() - work_area.x()) % kPopupGridSize;
+  int y = pos.y() - (pos.y() - work_area.y()) % kPopupGridSize;
   int w = pos.width();
   int h = pos.height();
 
   // If the alignment was pushing the window out of the screen, we ignore the
   // alignment for that call.
-  if (abs(pos.right() - work_area.right()) < grid)
+  if (abs(pos.right() - work_area.right()) < kPopupGridSize)
     x = work_area.right() - w;
-  if (abs(pos.bottom() - work_area.bottom()) < grid)
+  if (abs(pos.bottom() - work_area.bottom()) < kPopupGridSize)
     y = work_area.bottom() - h;
   return gfx::Rect(x, y, w, h);
 }

ash/wm/window_positioner.h

@@ -19,6 +19,7 @@ class Display;
 
 namespace gfx {
 class Rect;
+class Size;
 }
 
 namespace ash {
@@ -73,12 +74,11 @@ class ASH_EXPORT WindowPositioner {
   WindowPositioner();
   ~WindowPositioner();
 
-  // Find a suitable screen position for a popup window and return it. The
-  // passed input position is only used to retrieve the width and height.
+  // Find a suitable screen position for a popup window and return it.
   // The position is determined on the left / right / top / bottom first. If
   // no smart space is found, the position will follow the standard what other
   // operating systems do (default cascading style).
-  gfx::Rect GetPopupPosition(const gfx::Rect& old_pos);
+  gfx::Rect GetPopupPosition(const gfx::Size& popup_size);
 
   // Accessor to set a flag indicating whether the first window in ASH should
   // be maximized.
@@ -89,34 +89,32 @@ class ASH_EXPORT WindowPositioner {
 
   // Find a smart way to position the popup window. If there is no space this
   // function will return an empty rectangle.
-  gfx::Rect SmartPopupPosition(const gfx::Rect& old_pos,
-                               const gfx::Rect& work_area,
-                               int grid);
+  static gfx::Rect SmartPopupPosition(const gfx::Size& popup_size,
+                                      const gfx::Rect& work_area);
 
   // Find the next available cascading popup position (on the given screen).
-  gfx::Rect NormalPopupPosition(const gfx::Rect& old_pos,
+  gfx::Rect NormalPopupPosition(const gfx::Size& popup_size,
                                 const gfx::Rect& work_area);
 
   // Align the location to the grid / snap to the right / bottom corner.
-  gfx::Rect AlignPopupPosition(const gfx::Rect& pos,
-                               const gfx::Rect& work_area,
-                               int grid);
+  static gfx::Rect AlignPopupPosition(const gfx::Rect& pos,
+                                      const gfx::Rect& work_area);
 
-  // Constant exposed for unittest.
-  static const int kMinimumWindowOffset;
+  // The grid size in DIPs used to align popup windows.
+  static constexpr int kPopupGridSize = 32;
 
   // The offset in X and Y for the next popup which opens.
-  int pop_position_offset_increment_x;
-  int pop_position_offset_increment_y;
+  static constexpr int kNextPopupOffset = 32;
 
   // The position on the screen for the first popup which gets shown if no
   // empty space can be found.
-  int popup_position_offset_from_screen_corner_x;
-  int popup_position_offset_from_screen_corner_y;
+  static constexpr int kFirstPopupOffset = 32;
 
   // The last used position.
-  int last_popup_position_x_;
-  int last_popup_position_y_;
+  // TODO(jamescook): These seem more like *next* popup position.
+  int last_popup_position_x_ = 0;
+  int last_popup_position_y_ = 0;
+  bool has_last_popup_position_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(WindowPositioner);
 };

chrome/browser/ui/ash/window_positioner_unittest.cc

@@ -53,7 +53,7 @@ class WindowPositionerTest : public AshTestBase {
 };
 
 WindowPositionerTest::WindowPositionerTest()
-    : grid_size_(WindowPositioner::kMinimumWindowOffset) {}
+    : grid_size_(WindowPositioner::kPopupGridSize) {}
 
 void WindowPositionerTest::SetUp() {
   AshTestBase::SetUp();
@@ -103,75 +103,75 @@ TEST_F(WindowPositionerTest, cascading) {
   window()->SetBounds(work_area);
   window()->Show();
 
-  gfx::Rect popup_position(0, 0, 200, 200);
+  gfx::Size popup_size(200, 200);
   // Check that it gets cascaded.
-  gfx::Rect cascade_1 = window_positioner()->GetPopupPosition(popup_position);
+  gfx::Rect cascade_1 = window_positioner()->GetPopupPosition(popup_size);
   EXPECT_EQ(gfx::Rect(work_area.x() + grid_size_, work_area.y() + grid_size_,
-                      popup_position.width(), popup_position.height()),
+                      popup_size.width(), popup_size.height()),
             cascade_1);
 
-  gfx::Rect cascade_2 = window_positioner()->GetPopupPosition(popup_position);
+  gfx::Rect cascade_2 = window_positioner()->GetPopupPosition(popup_size);
   EXPECT_EQ(
       gfx::Rect(work_area.x() + 2 * grid_size_, work_area.y() + 2 * grid_size_,
-                popup_position.width(), popup_position.height()),
+                popup_size.width(), popup_size.height()),
       cascade_2);
 
   // Check that if there is even only a pixel missing it will cascade.
   window()->SetBounds(
-      gfx::Rect(work_area.x() + popup_position.width() - 1,
-                work_area.y() + popup_position.height() - 1,
-                work_area.width() - 2 * (popup_position.width() - 1),
-                work_area.height() - 2 * (popup_position.height() - 1)));
+      gfx::Rect(work_area.x() + popup_size.width() - 1,
+                work_area.y() + popup_size.height() - 1,
+                work_area.width() - 2 * (popup_size.width() - 1),
+                work_area.height() - 2 * (popup_size.height() - 1)));
 
-  gfx::Rect cascade_3 = window_positioner()->GetPopupPosition(popup_position);
+  gfx::Rect cascade_3 = window_positioner()->GetPopupPosition(popup_size);
   EXPECT_EQ(
       gfx::Rect(work_area.x() + 3 * grid_size_, work_area.y() + 3 * grid_size_,
-                popup_position.width(), popup_position.height()),
+                popup_size.width(), popup_size.height()),
       cascade_3);
 
   // Check that we overflow into the next line when we do not fit anymore in Y.
-  gfx::Rect popup_position_4(
-      0, 0, 200, work_area.height() - (cascade_3.y() - work_area.y()));
-  gfx::Rect cascade_4 = window_positioner()->GetPopupPosition(popup_position_4);
+  gfx::Size popup_size_4(200,
+                         work_area.height() - (cascade_3.y() - work_area.y()));
+  gfx::Rect cascade_4 = window_positioner()->GetPopupPosition(popup_size_4);
   EXPECT_EQ(
       gfx::Rect(work_area.x() + 2 * grid_size_, work_area.y() + grid_size_,
-                popup_position_4.width(), popup_position_4.height()),
+                popup_size_4.width(), popup_size_4.height()),
       cascade_4);
 
   // Check that we overflow back to the first possible location if we overflow
   // to the end.
-  gfx::Rect popup_position_5(
-      0, 0, work_area.width() + 1 - (cascade_4.x() - work_area.x()),
+  gfx::Size popup_size_5(
+      work_area.width() + 1 - (cascade_4.x() - work_area.x()),
       work_area.height() - (2 * grid_size_ - work_area.y()));
-  gfx::Rect cascade_5 = window_positioner()->GetPopupPosition(popup_position_5);
+  gfx::Rect cascade_5 = window_positioner()->GetPopupPosition(popup_size_5);
   EXPECT_EQ(gfx::Rect(work_area.x() + grid_size_, work_area.y() + grid_size_,
-                      popup_position_5.width(), popup_position_5.height()),
+                      popup_size_5.width(), popup_size_5.height()),
             cascade_5);
 }
 
 TEST_F(WindowPositionerTest, filling) {
   const gfx::Rect work_area =
       display::Screen::GetScreen()->GetPrimaryDisplay().work_area();
-  gfx::Rect popup_position(0, 0, 256, 128);
+  gfx::Size popup_size(256, 128);
   // Leave space on the left and the right and see if we fill top to bottom.
   window()->SetBounds(gfx::Rect(
-      work_area.x() + popup_position.width(), work_area.y(),
-      work_area.width() - 2 * popup_position.width(), work_area.height()));
+      work_area.x() + popup_size.width(), work_area.y(),
+      work_area.width() - 2 * popup_size.width(), work_area.height()));
   window()->Show();
   // Check that we are positioned in the top left corner.
-  gfx::Rect top_left = window_positioner()->GetPopupPosition(popup_position);
-  EXPECT_EQ(gfx::Rect(work_area.x(), work_area.y(), popup_position.width(),
-                      popup_position.height()),
+  gfx::Rect top_left = window_positioner()->GetPopupPosition(popup_size);
+  EXPECT_EQ(gfx::Rect(work_area.x(), work_area.y(), popup_size.width(),
+                      popup_size.height()),
             top_left);
 
   // Now block the found location.
   popup()->SetBounds(top_left);
   popup()->Show();
-  gfx::Rect mid_left = window_positioner()->GetPopupPosition(popup_position);
+  gfx::Rect mid_left = window_positioner()->GetPopupPosition(popup_size);
   EXPECT_EQ(gfx::Rect(work_area.x(),
                       AlignToGridRoundDown(work_area.y() + top_left.height(),
                                            grid_size_),
-                      popup_position.width(), popup_position.height()),
+                      popup_size.width(), popup_size.height()),
             mid_left);
 
   // Block now everything so that we can only put the popup on the bottom
@@ -179,36 +179,33 @@ TEST_F(WindowPositionerTest, filling) {
   // Note: We need to keep one "grid spacing free" if the window does not
   // fit into the grid (which is true for 200 height).`
   popup()->SetBounds(
-      gfx::Rect(work_area.x(), work_area.y(), popup_position.width(),
-                work_area.height() - popup_position.height() - grid_size_ + 1));
-  gfx::Rect bottom_left = window_positioner()->GetPopupPosition(popup_position);
-  EXPECT_EQ(
-      gfx::Rect(work_area.x(), work_area.bottom() - popup_position.height(),
-                popup_position.width(), popup_position.height()),
-      bottom_left);
+      gfx::Rect(work_area.x(), work_area.y(), popup_size.width(),
+                work_area.height() - popup_size.height() - grid_size_ + 1));
+  gfx::Rect bottom_left = window_positioner()->GetPopupPosition(popup_size);
+  EXPECT_EQ(gfx::Rect(work_area.x(), work_area.bottom() - popup_size.height(),
+                      popup_size.width(), popup_size.height()),
+            bottom_left);
 
   // Block now enough to force the right side.
-  popup()->SetBounds(
-      gfx::Rect(work_area.x(), work_area.y(), popup_position.width(),
-                work_area.height() - popup_position.height() + 1));
-  gfx::Rect top_right = window_positioner()->GetPopupPosition(popup_position);
-  EXPECT_EQ(
-      gfx::Rect(AlignToGridRoundDown(work_area.right() - popup_position.width(),
-                                     grid_size_),
-                work_area.y(), popup_position.width(), popup_position.height()),
-      top_right);
+  popup()->SetBounds(gfx::Rect(work_area.x(), work_area.y(), popup_size.width(),
+                               work_area.height() - popup_size.height() + 1));
+  gfx::Rect top_right = window_positioner()->GetPopupPosition(popup_size);
+  EXPECT_EQ(gfx::Rect(AlignToGridRoundDown(
+                          work_area.right() - popup_size.width(), grid_size_),
+                      work_area.y(), popup_size.width(), popup_size.height()),
+            top_right);
 }
 
 TEST_F(WindowPositionerTest, biggerThenBorder) {
   const gfx::Rect work_area =
       display::Screen::GetScreen()->GetPrimaryDisplay().work_area();
 
-  gfx::Rect pop_position(0, 0, work_area.width(), work_area.height());
+  gfx::Size popup_size(work_area.width(), work_area.height());
 
   // Check that the popup is placed full screen.
-  gfx::Rect full = window_positioner()->GetPopupPosition(pop_position);
-  EXPECT_EQ(gfx::Rect(work_area.x(), work_area.y(), pop_position.width(),
-                      pop_position.height()),
+  gfx::Rect full = window_positioner()->GetPopupPosition(popup_size);
+  EXPECT_EQ(gfx::Rect(work_area.x(), work_area.y(), popup_size.width(),
+                      popup_size.height()),
             full);
 }
 

chrome/browser/ui/window_sizer/window_sizer_ash.cc

@@ -46,7 +46,8 @@ bool WindowSizer::GetBrowserBoundsAsh(gfx::Rect* bounds,
     // In case of a popup with an 'unspecified' location in ash, we are
     // looking for a good screen location. We are interpreting (0,0) as an
     // unspecified location.
-    *bounds = ash::Shell::Get()->window_positioner()->GetPopupPosition(*bounds);
+    *bounds = ash::Shell::Get()->window_positioner()->GetPopupPosition(
+        bounds->size());
     determined = true;
   }