Revert "CrOS Shelf: Change icon centering strategy"

This reverts commit 0a9cd867.

Reason for revert: ShelfViewTest.IconCenteringTest is failing on bots.
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=ash_unittests&tests=ShelfViewTest.IconCenteringTest
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/21645
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/21646


Original change's description:
> CrOS Shelf: Change icon centering strategy
> 
> The main change here is that icons are first centered on the whole
> screen, then, when space becomes tighter, they are centered over the
> available area on the shelf.
> 
> A few changes to make this work properly:
> 
> * Fix an issue in |GetDimensionOfCenteredShelfItems| which was taking
>   into account all items, even the ones within overflow, making the
>   calculations incorrect.
> * Another issue was that the code calculating ideal bounds would
>   assume control buttons (app list, back, overflow) were the same size
>   as app buttons (56) while they're actually a little smaller (40).
>   Giving control buttons their "real" size in the layout code allows
>   for the removal of some overrides at each button class level.
> * Split out a small chunk from |CalculateIdealBounds| which is getting
>   too long. More of that to come in future changes.
> * Make the special spacing between the app list button and the first
>   app icon also there on the other side for more symmetry.
> 
> Also add some extensive test coverage.
> 
> Bug: 891080, 933291
> Change-Id: If4bb69aa6e182cd29c02d467039b5d839d09f493
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1512299
> Commit-Queue: Manu Cornet <manucornet@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#642245}

TBR=xiyuan@chromium.org,manucornet@chromium.org

Change-Id: I4ad5f95b4c6b2db13e05a6e280e987f25981392b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 891080, 933291
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1531922Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642409}

ash/shelf/app_list_button.cc

@@ -188,6 +188,19 @@ const char* AppListButton::GetClassName() const {
   return kViewClassName;
 }
 
+std::unique_ptr<views::InkDropRipple> AppListButton::CreateInkDropRipple()
+    const {
+  const int app_list_button_radius = ShelfConstants::control_border_radius();
+  gfx::Point center = GetCenterPoint();
+  gfx::Rect bounds(center.x() - app_list_button_radius,
+                   center.y() - app_list_button_radius,
+                   2 * app_list_button_radius, 2 * app_list_button_radius);
+  return std::make_unique<views::FloodFillInkDropRipple>(
+      size(), GetLocalBounds().InsetsFrom(bounds),
+      GetInkDropCenterBasedOnLastEvent(), GetInkDropBaseColor(),
+      ink_drop_visible_opacity());
+}
+
 void AppListButton::PaintButtonContents(gfx::Canvas* canvas) {
   gfx::PointF circle_center(GetCenterPoint());
 

ash/shelf/app_list_button.h

@@ -50,6 +50,7 @@ class ASH_EXPORT AppListButton : public ShelfControlButton,
 
  protected:
   // views::Button:
+  std::unique_ptr<views::InkDropRipple> CreateInkDropRipple() const override;
   void PaintButtonContents(gfx::Canvas* canvas) override;
 
  private:

ash/shelf/app_list_button_unittest.cc

@@ -16,7 +16,6 @@
 #include "ash/root_window_controller.h"
 #include "ash/session/session_controller.h"
 #include "ash/shelf/shelf.h"
-#include "ash/shelf/shelf_constants.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shelf/shelf_view_test_api.h"
 #include "ash/shell.h"
@@ -154,7 +153,7 @@ TEST_F(AppListButtonTest, ButtonPositionInTabletMode) {
 
   Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(false);
   test_api.RunMessageLoopUntilAnimationsDone();
-  EXPECT_EQ(ShelfConstants::button_spacing(), app_list_button()->bounds().x());
+  EXPECT_EQ(0, app_list_button()->bounds().x());
 }
 
 class VoiceInteractionAppListButtonTest : public AppListButtonTest {

ash/shelf/overflow_bubble_view.cc

@@ -179,8 +179,6 @@ void OverflowBubbleView::OnScrollEvent(ui::ScrollEvent* event) {
 gfx::Rect OverflowBubbleView::GetBubbleBounds() {
   const gfx::Size content_size = GetPreferredSize();
   const gfx::Rect anchor_rect = GetAnchorRect();
-  const int distance_to_overflow_button =
-      kDistanceToMainShelf + (kShelfSize - kShelfControlSize) / 2;
   gfx::Rect monitor_rect =
       display::Screen::GetScreen()
           ->GetDisplayNearestPoint(anchor_rect.CenterPoint())
@@ -193,7 +191,7 @@ gfx::Rect OverflowBubbleView::GetBubbleBounds() {
         base::i18n::IsRTL()
             ? anchor_rect.x() - kEndPadding
             : anchor_rect.right() - content_size.width() - kEndPadding,
-        anchor_rect.y() - distance_to_overflow_button - content_size.height(),
+        anchor_rect.y() - kDistanceToMainShelf - content_size.height(),
         content_size.width() + 2 * kEndPadding, content_size.height());
     if (bounds.x() < monitor_rect.x())
       bounds.Offset(monitor_rect.x() - bounds.x(), 0);
@@ -205,10 +203,9 @@ gfx::Rect OverflowBubbleView::GetBubbleBounds() {
       0, anchor_rect.bottom() - content_size.height() - kEndPadding,
       content_size.width(), content_size.height() + 2 * kEndPadding);
   if (shelf_->alignment() == SHELF_ALIGNMENT_LEFT)
-    bounds.set_x(anchor_rect.right() + distance_to_overflow_button);
+    bounds.set_x(anchor_rect.right() + kDistanceToMainShelf);
   else
-    bounds.set_x(anchor_rect.x() - distance_to_overflow_button -
-                 content_size.width());
+    bounds.set_x(anchor_rect.x() - kDistanceToMainShelf - content_size.width());
   if (bounds.y() < monitor_rect.y())
     bounds.Offset(0, monitor_rect.y() - bounds.y());
   if (bounds.bottom() > monitor_rect.bottom())

ash/shelf/shelf_control_button.cc

@@ -22,22 +22,21 @@ namespace ash {
 ShelfControlButton::ShelfControlButton(ShelfView* shelf_view)
     : ShelfButton(shelf_view), shelf_(shelf_view->shelf()) {
   set_has_ink_drop_action_on_click(true);
+  SetSize(gfx::Size(kShelfControlSize, kShelfControlSize));
 }
 
 ShelfControlButton::~ShelfControlButton() = default;
 
 gfx::Point ShelfControlButton::GetCenterPoint() const {
-  return gfx::Point(width() / 2.f, height() / 2.f);
+  return gfx::Point(width() / 2.f, width() / 2.f);
 }
 
 std::unique_ptr<views::InkDropRipple> ShelfControlButton::CreateInkDropRipple()
     const {
-  const int button_radius = ShelfConstants::control_border_radius();
-  gfx::Point center = GetCenterPoint();
-  gfx::Rect bounds(center.x() - button_radius, center.y() - button_radius,
-                   2 * button_radius, 2 * button_radius);
   return std::make_unique<views::FloodFillInkDropRipple>(
-      size(), GetLocalBounds().InsetsFrom(bounds),
+      size(),
+      gfx::Insets(ShelfConstants::button_size() / 2 -
+                  ShelfConstants::control_border_radius()),
       GetInkDropCenterBasedOnLastEvent(), GetInkDropBaseColor(),
       ink_drop_visible_opacity());
 }
@@ -52,8 +51,21 @@ const char* ShelfControlButton::GetClassName() const {
   return "ash/ShelfControlButton";
 }
 
+gfx::Rect ShelfControlButton::CalculateButtonBounds() const {
+  ShelfAlignment alignment = shelf_->alignment();
+  gfx::Rect content_bounds = GetContentsBounds();
+  // Align the button to the top of a bottom-aligned shelf, to the right edge
+  // a left-aligned shelf, and to the left edge of a right-aligned shelf.
+  const int inset = (ShelfConstants::shelf_size() - kShelfControlSize) / 2;
+  const int x = alignment == SHELF_ALIGNMENT_LEFT
+                    ? content_bounds.right() - inset - kShelfControlSize
+                    : content_bounds.x() + inset;
+  return gfx::Rect(x, content_bounds.y() + inset, kShelfControlSize,
+                   kShelfControlSize);
+}
+
 void ShelfControlButton::PaintButtonContents(gfx::Canvas* canvas) {
-  PaintBackground(canvas, GetContentsBounds());
+  PaintBackground(canvas, CalculateButtonBounds());
 }
 
 void ShelfControlButton::PaintBackground(gfx::Canvas* canvas,

ash/shelf/shelf_control_button.h

@@ -37,6 +37,9 @@ class ASH_EXPORT ShelfControlButton : public ShelfButton {
   void PaintButtonContents(gfx::Canvas* canvas) override;
 
  private:
+  // Calculates the bounds of the control button based on the shelf alignment.
+  gfx::Rect CalculateButtonBounds() const;
+
   Shelf* shelf_;
 
   DISALLOW_COPY_AND_ASSIGN(ShelfControlButton);

ash/shelf/shelf_view.h

@@ -247,10 +247,9 @@ class ASH_EXPORT ShelfView : public views::View,
   // Returns whether |item| should belong in the pinned section of the shelf.
   bool IsItemPinned(const ShelfItem& item) const;
 
-  // Enumerates the shelf apps and returns the total size they occupy,
-  // accounting for all apps or, if the total size is greater than |max_size|,
-  // the size of however many app can fit without exceeding |max_size|.
-  int GetDimensionOfAppIcons(int max_size) const;
+  // Enumerates the shelf items that are centered in the new UI and returns
+  // the total size they occupy.
+  int GetDimensionOfCenteredShelfItems() const;
 
   // Returns the index of the item after which the separator should be shown,
   // or -1 if no separator is required.
@@ -344,9 +343,7 @@ class ASH_EXPORT ShelfView : public views::View,
 
   // Calculates the ideal bounds. The bounds of each button corresponding to an
   // item in the model is set in |view_model_|.
-  void CalculateIdealBounds() const;
-
-  void LayoutOverflowButton() const;
+  void CalculateIdealBounds(gfx::Rect* overflow_bounds) const;
 
   // Returns the index of the last view whose max primary axis coordinate is
   // less than |max_value|. Returns -1 if nothing fits, or there are no views.

ash/shelf/shelf_view_unittest.cc

@@ -12,7 +12,6 @@
 #include "ash/app_list/test/app_list_test_helper.h"
 #include "ash/app_list/views/app_list_view.h"
 #include "ash/focus_cycler.h"
-#include "ash/ime/ime_controller.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/shelf_prefs.h"
@@ -2282,122 +2281,6 @@ TEST_F(ShelfViewTest, AppListButtonDoesShowContextMenu) {
   EXPECT_TRUE(test_api_->CloseMenu());
 }
 
-void ExpectWithinOnePixel(int a, int b) {
-  EXPECT_TRUE(abs(a - b) <= 1) << "Values " << a << " and " << b
-                               << " should have a difference no greater than 1";
-}
-
-TEST_F(ShelfViewTest, IconCenteringTest) {
-  const display::Display display =
-      display::Screen::GetScreen()->GetPrimaryDisplay();
-  const int screen_width = display.bounds().width();
-  const int screen_center = screen_width / 2;
-
-  // Show the IME panel, to introduce for asymettry with a larger status area.
-  Shell::Get()->ime_controller()->ShowImeMenuOnShelf(true);
-
-  // At the start, we have exactly one app icon for the browser. That should
-  // be centered on the screen.
-  const ShelfAppButton* button1 = GetButtonByID(model_->items()[2].id);
-  ExpectWithinOnePixel(screen_center,
-                       button1->GetBoundsInScreen().CenterPoint().x());
-  // Also check that the distance between the icon edge and the screen edge is
-  // the same on both sides.
-  ExpectWithinOnePixel(button1->GetBoundsInScreen().x(),
-                       screen_width - button1->GetBoundsInScreen().right());
-
-  const int apps_that_can_fit_at_center_of_screen = 8;
-  std::vector<ShelfAppButton*> app_buttons;
-  // Start with just the browser app button.
-  app_buttons.push_back(GetButtonByID(model_->items()[2].id));
-  int n_buttons = 1;
-
-  // Now repeat the same process by adding apps until they can't fit at the
-  // center of the screen.
-  for (int i = 1; i < apps_that_can_fit_at_center_of_screen; ++i) {
-    // Add a new app and add its button to our list.
-    app_buttons.push_back(GetButtonByID(AddApp()));
-    n_buttons = app_buttons.size();
-    if (n_buttons % 2 == 1) {
-      // Odd number of apps. Check that the middle app is exactly at the center
-      // of the screen.
-      ExpectWithinOnePixel(
-          screen_center,
-          app_buttons[n_buttons / 2]->GetBoundsInScreen().CenterPoint().x());
-    }
-    // Also check that the first icon is at the same distance from the left
-    // screen edge as the last icon is from the right screen edge.
-    ExpectWithinOnePixel(
-        app_buttons[0]->GetBoundsInScreen().x(),
-        screen_width - app_buttons[n_buttons - 1]->GetBoundsInScreen().right());
-  }
-
-  // Add one more app. Now the block of apps should be at the center of the
-  // shelf part of the shelf widget (not including the status area) as opposed
-  // to at the center of the whole screen. But we're not overflowing yet.
-  app_buttons.push_back(GetButtonByID(AddApp()));
-  n_buttons = app_buttons.size();
-  EXPECT_FALSE(shelf_view_->GetOverflowButton()->visible());
-  // Icons at either end should also be at the same distance from the app list
-  // button on the left, and the status area on the right.
-  gfx::NativeWindow window = shelf_view_->shelf_widget()->GetNativeWindow();
-  views::View* status_area_view = RootWindowController::ForWindow(window)
-                                      ->GetStatusAreaWidget()
-                                      ->GetContentsView();
-  const int status_area_left = status_area_view->GetBoundsInScreen().x();
-  const int app_list_button_right =
-      shelf_view_->GetAppListButton()->GetBoundsInScreen().right();
-  ExpectWithinOnePixel(
-      app_buttons[0]->GetBoundsInScreen().x() - app_list_button_right,
-      status_area_left -
-          app_buttons[n_buttons - 1]->GetBoundsInScreen().right());
-
-  // Add another app. The overflow button should now appear.
-  app_buttons.push_back(GetButtonByID(AddApp()));
-  n_buttons = app_buttons.size();
-  EXPECT_TRUE(shelf_view_->GetOverflowButton()->visible());
-}
-
-TEST_F(ShelfViewTest, FirstAndLastVisibleIndex) {
-  // At the start, the only things visible on the shelf are the app list button
-  // (index 1) and the browser app button (index 2).
-  EXPECT_EQ(1, shelf_view_->first_visible_index());
-  EXPECT_EQ(2, shelf_view_->last_visible_index());
-  // By enabling tablet mode, the back button (index 0) should become visible.
-  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
-  EXPECT_EQ(0, shelf_view_->first_visible_index());
-  EXPECT_EQ(2, shelf_view_->last_visible_index());
-  // And things should return back to the previous state once tablet mode is off
-  // again.
-  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(false);
-  EXPECT_EQ(1, shelf_view_->first_visible_index());
-  EXPECT_EQ(2, shelf_view_->last_visible_index());
-  // Now let's add some apps until the overflow button shows up, each time
-  // checking the first and last visible indices are what we expect.
-  int last_visible_index = 2;
-  int last_visible_index_before_overflow;
-  ShelfID last_added_item_id;
-  while (true) {
-    last_added_item_id = AddApp();
-    if (shelf_view_->GetOverflowButton()->visible()) {
-      last_visible_index_before_overflow = last_visible_index;
-      break;
-    }
-    last_visible_index++;
-    EXPECT_EQ(1, shelf_view_->first_visible_index());
-    EXPECT_EQ(last_visible_index, shelf_view_->last_visible_index());
-  }
-  // Now remove the last item we just added. That should get rid of the
-  // overflow button, and get back to the previous state.
-  RemoveByID(last_added_item_id);
-  EXPECT_EQ(1, shelf_view_->first_visible_index());
-  EXPECT_EQ(last_visible_index_before_overflow,
-            shelf_view_->last_visible_index());
-  // Adding another app should let the overflow button appear again.
-  AddApp();
-  EXPECT_TRUE(shelf_view_->GetOverflowButton()->visible());
-}
-
 // Test class that tests both context and application menus.
 class ShelfViewMenuTest : public ShelfViewTest,
                           public testing::WithParamInterface<bool> {