Decrease extension button size + add spacing

This makes ToolbarActionView the same size as other toolbar buttons
instead of increasing its internal size with 4dp to accomodate for
padding between items.

This reduces the BrowserActionsContainer height by 4dp (matching its
current visual height) so it should no longer be able to push the
toolbar height. It also fixes some polish where the spacing left and
right of the browser actions were inadvertedly 10dp instead of 8dp.

Bug: chromium:831393, chromium:916241, chromium:916462
Change-Id: I63e5413ce187f173a5e35b40a3ec8f02a68fa09c
Reviewed-on: https://chromium-review.googlesource.com/c/1394870Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619999}

chrome/browser/ui/layout_constants.cc

@@ -99,7 +99,7 @@ gfx::Insets GetLayoutInsets(LayoutInset inset) {
     case TOOLBAR_ACTION_VIEW: {
       // TODO(afakhry): Unify all toolbar button sizes on all platforms.
       // https://crbug.com/822967.
-      return gfx::Insets(touch_ui ? 10 : 2);
+      return gfx::Insets(touch_ui ? 10 : 0);
     }
   }
   NOTREACHED();

chrome/browser/ui/toolbar/toolbar_actions_bar.cc

@@ -166,7 +166,7 @@ gfx::Size ToolbarActionsBar::GetFullSize() const {
     num_rows += (std::max(0, icon_count - 1) / num_icons);
   }
 
-  return gfx::ScaleToFlooredSize(GetViewSize(), num_icons, num_rows);
+  return gfx::Size(IconCountToWidth(num_icons), IconCountToWidth(num_rows));
 }
 
 int ToolbarActionsBar::GetMinimumWidth() const {
@@ -178,11 +178,16 @@ int ToolbarActionsBar::GetMaximumWidth() const {
 }
 
 int ToolbarActionsBar::IconCountToWidth(size_t icons) const {
-  return icons * GetViewSize().width();
+  if (icons == 0)
+    return 0;
+  return icons * GetViewSize().width() +
+         (icons - 1) * GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
 }
 
 size_t ToolbarActionsBar::WidthToIconCount(int pixels) const {
-  return base::ClampToRange(pixels / GetViewSize().width(), 0,
+  const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
+  return base::ClampToRange(
+      (pixels + element_padding) / (GetViewSize().width() + element_padding), 0,
       static_cast<int>(toolbar_actions_.size()));
 }
 
@@ -274,8 +279,10 @@ gfx::Rect ToolbarActionsBar::GetFrameForIndex(
                                   : relative_index;
 
   const auto size = GetViewSize();
-  return gfx::Rect(
-      gfx::Point(index_in_row * size.width(), row_index * size.height()), size);
+  const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
+  return gfx::Rect(gfx::Point(index_in_row * (size.width() + element_padding),
+                              row_index * (size.height() + element_padding)),
+                   size);
 }
 
 std::vector<ToolbarActionViewController*>
@@ -372,7 +379,7 @@ bool ToolbarActionsBar::ShowToolbarActionPopup(const std::string& action_id,
 void ToolbarActionsBar::SetOverflowRowWidth(int width) {
   DCHECK(in_overflow_mode());
   platform_settings_.icons_per_overflow_menu_row =
-      std::max(width / GetViewSize().width(), 1);
+      std::max(WidthToIconCount(width), static_cast<size_t>(1));
 }
 
 void ToolbarActionsBar::OnResizeComplete(int width) {

chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc

@@ -255,7 +255,8 @@ TEST_P(ToolbarActionsBarUnitTest, BasicToolbarActionsBarTest) {
   EXPECT_EQ(3u, toolbar_actions_bar()->GetIconCount());
   const gfx::Size view_size = toolbar_actions_bar()->GetViewSize();
   // Check the widths.
-  int expected_width = 3 * view_size.width();
+  int expected_width =
+      3 * view_size.width() + 2 * GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
   EXPECT_EQ(expected_width, toolbar_actions_bar()->GetFullSize().width());
   // Since all icons are showing, the current width should be the max width.
   int maximum_width = expected_width;
@@ -271,7 +272,8 @@ TEST_P(ToolbarActionsBarUnitTest, BasicToolbarActionsBarTest) {
   EXPECT_EQ(2u, toolbar_actions_bar()->GetIconCount());
 
   // The current width should now be enough for two icons.
-  expected_width = 2 * view_size.width();
+  expected_width =
+      2 * view_size.width() + GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
   EXPECT_EQ(expected_width, toolbar_actions_bar()->GetFullSize().width());
   // The maximum and minimum widths should have remained constant (since we have
   // the same number of actions).
@@ -328,7 +330,7 @@ TEST_P(ToolbarActionsBarUnitTest, BasicToolbarActionsBarTest) {
 
   // If we resize by enough to include a new icon, width and icon count should
   // both increase.
-  width += view_size.width();
+  width += view_size.width() + GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
   toolbar_actions_bar()->OnResizeComplete(width);
   EXPECT_EQ(width, toolbar_actions_bar()->GetFullSize().width());
   EXPECT_EQ(2u, toolbar_actions_bar()->GetIconCount());
@@ -336,7 +338,7 @@ TEST_P(ToolbarActionsBarUnitTest, BasicToolbarActionsBarTest) {
   // If we shrink the bar so that a full icon can't fit, it should resize to
   // hide that icon.
   toolbar_actions_bar()->OnResizeComplete(width - 1);
-  width -= view_size.width();
+  width -= view_size.width() + GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
   EXPECT_EQ(width, toolbar_actions_bar()->GetFullSize().width());
   EXPECT_EQ(1u, toolbar_actions_bar()->GetIconCount());
 }
@@ -442,8 +444,11 @@ TEST_P(ToolbarActionsBarUnitTest, TestHighlightMode) {
 // Test the bounds calculation for different indices.
 TEST_P(ToolbarActionsBarUnitTest, TestActionFrameBounds) {
   const auto size = toolbar_actions_bar()->GetViewSize();
-  const auto icon_rect = [size](int x, int y) {
-    return gfx::Rect(gfx::Point(x * size.width(), y * size.height()), size);
+  const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
+  const auto icon_rect = [size, element_padding](int x, int y) {
+    return gfx::Rect(gfx::Point(x * (element_padding + size.width()),
+                                y * (element_padding + size.height())),
+                     size);
   };
 
   constexpr int kIconsPerOverflowRow = 3;
@@ -456,8 +461,11 @@ TEST_P(ToolbarActionsBarUnitTest, TestActionFrameBounds) {
                           ActionType::BROWSER_ACTION);
   }
   toolbar_model()->SetVisibleIconCount(kNumExtensions);
-  const int icon_width = toolbar_actions_bar()->GetViewSize().width();
-  overflow_bar()->SetOverflowRowWidth(icon_width * kIconsPerOverflowRow);
+  const int overflow_width =
+      kIconsPerOverflowRow *
+          (toolbar_actions_bar()->GetViewSize().width() + element_padding) -
+      element_padding;
+  overflow_bar()->SetOverflowRowWidth(overflow_width);
   EXPECT_EQ(kIconsPerOverflowRow,
             overflow_bar()->platform_settings().icons_per_overflow_menu_row);
 

chrome/browser/ui/views/toolbar/browser_actions_container.cc

@@ -519,20 +519,18 @@ int BrowserActionsContainer::OnDragUpdated(
     const auto size = toolbar_actions_bar_->GetViewSize();
     const int offset_into_icon_area = GetMirroredXInView(event.x()) -
                                       GetResizeAreaWidth() + (size.width() / 2);
-    const int before_icon_unclamped = offset_into_icon_area / size.width();
-
-    // Next, figure out what row we're on. This only matters for overflow mode,
-    // but the calculation is the same for both.
-    row_index = event.y() / size.height();
-
-    // Sanity check - we should never be on a different row in the main
-    // container.
-    DCHECK(ShownInsideMenu() || row_index == 0);
+    const int before_icon_unclamped =
+        toolbar_actions_bar_->WidthToIconCount(offset_into_icon_area);
 
     // We need to figure out how many icons are visible on the relevant row.
     // In the main container, this will just be the visible actions.
     int visible_icons_on_row = VisibleBrowserActionsAfterAnimation();
     if (ShownInsideMenu()) {
+      // Next, figure out what row we're on.
+      const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
+      row_index =
+          (event.y() + element_padding) / (size.height() + element_padding);
+
       const int icons_per_row = platform_settings().icons_per_overflow_menu_row;
       // If this is the final row of the overflow, then this is the remainder of
       // visible icons. Otherwise, it's a full row (kIconsPerRow).
@@ -575,11 +573,8 @@ int BrowserActionsContainer::OnPerformDrop(
   // Make sure we have the same view as we started with.
   DCHECK_EQ(GetIdAt(data.index()), data.id());
 
-  size_t i = drop_position_->row *
-      platform_settings().icons_per_overflow_menu_row +
-      drop_position_->icon_in_row;
-  if (ShownInsideMenu())
-    i += main_container_->VisibleBrowserActionsAfterAnimation();
+  size_t i = GetDropPositionIndex();
+
   // |i| now points to the item to the right of the drop indicator*, which is
   // correct when dragging an icon to the left. When dragging to the right,
   // however, we want the icon being dragged to get the index of the item to
@@ -712,23 +707,18 @@ void BrowserActionsContainer::OnPaint(gfx::Canvas* canvas) {
     // The two-pixel width drop indicator.
     constexpr int kDropIndicatorWidth = 2;
 
-    // Convert back to a pixel offset into the container.  First find the X
-    // coordinate of the drop icon.
-    const auto size = toolbar_actions_bar_->GetViewSize();
-    // TODO(pbos): The drag/drop separator and view placement should share code
-    // after ToolbarActionsBar and BrowserActionsContainer merge.
-    const int drop_icon_x = GetResizeAreaWidth() +
-                            drop_position_->icon_in_row * size.width() -
-                            (kDropIndicatorWidth / 2);
-
-    // Next, clamp so the indicator doesn't touch the adjoining toolbar items.
-    const int drop_indicator_x =
-        base::ClampToRange(drop_icon_x, 1, width() - kDropIndicatorWidth - 1);
-
-    const int row_height = size.height();
-    const int drop_indicator_y = row_height * drop_position_->row;
-    gfx::Rect indicator_bounds = GetMirroredRect(gfx::Rect(
-        drop_indicator_x, drop_indicator_y, kDropIndicatorWidth, row_height));
+    const size_t i = GetDropPositionIndex();
+    const gfx::Rect frame = toolbar_actions_bar_->GetFrameForIndex(i);
+    gfx::Rect indicator_bounds = GetMirroredRect(
+        gfx::Rect(GetResizeAreaWidth() + frame.x() -
+                      GetLayoutConstant(TOOLBAR_ELEMENT_PADDING) / 2 -
+                      kDropIndicatorWidth / 2,
+                  frame.y(), kDropIndicatorWidth, frame.height()));
+    // Clamp the indicator to the view bounds so that heading / trailing markers
+    // don't paint outside the controller. It's OK if they paint over the resize
+    // area or separator (but the in-menu container has neither).
+    indicator_bounds.set_x(base::ClampToRange(
+        indicator_bounds.x(), 0, width() - indicator_bounds.width()));
 
     // Color of the drop indicator.
     // Always get the theme provider of the browser widget, since if this view
@@ -787,7 +777,7 @@ int BrowserActionsContainer::GetWidthForIconCount(size_t num_icons) const {
   if (num_icons == 0)
     return 0;
   return GetResizeAreaWidth() + GetSeparatorAreaWidth() +
-         num_icons * toolbar_actions_bar_->GetViewSize().width();
+         toolbar_actions_bar_->IconCountToWidth(num_icons);
 }
 
 int BrowserActionsContainer::GetWidthWithAllActionsVisible() const {
@@ -795,6 +785,15 @@ int BrowserActionsContainer::GetWidthWithAllActionsVisible() const {
       toolbar_actions_bar_->toolbar_actions_unordered().size());
 }
 
+size_t BrowserActionsContainer::GetDropPositionIndex() const {
+  size_t i =
+      drop_position_->row * platform_settings().icons_per_overflow_menu_row +
+      drop_position_->icon_in_row;
+  if (ShownInsideMenu())
+    i += main_container_->VisibleBrowserActionsAfterAnimation();
+  return i;
+}
+
 int BrowserActionsContainer::GetResizeAreaWidth() const {
   if (!resize_area_)
     return 0;

chrome/browser/ui/views/toolbar/browser_actions_container.h

@@ -267,6 +267,9 @@ class BrowserActionsContainer : public views::View,
   int GetWidthForIconCount(size_t num_icons) const;
   int GetWidthWithAllActionsVisible() const;
 
+  // Get index of the drag-drop position.
+  size_t GetDropPositionIndex() const;
+
   // Returns the preferred width given the limit of |max_width|. (Unlike most
   // views, since we don't want to show part of an icon or a large space after
   // the omnibox, this is probably *not* |max_width|).