Dynamically calculate the number of extension icons to show per row in overflow

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

Index: chrome/browser/ui/views/toolbar/browser_actions_container.cc
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.cc b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
index 5ed94f3e2f00e37eb91ffd966dbf4b2c33d79a12..8bdacfb96f357661e23b84166e61f6d719fdc86c 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -49,22 +49,8 @@ using extensions::Extension;
 
 namespace {
 
-// Horizontal spacing between most items in the container, as well as after the
-// last item or chevron (if visible).
-const int kItemSpacing = ToolbarView::kStandardSpacing;
-
 // Horizontal spacing before the chevron (if visible).
-const int kChevronSpacing = kItemSpacing - 2;
-
-// The maximum number of icons to show per row when in overflow mode (showing
-// icons in the application menu).
-// TODO(devlin): Compute the right number of icons to show, depending on the
-//               menu width.
-#if defined(OS_LINUX)
-const int kIconsPerMenuRow = 8;  // The menu on Linux is wider.
-#else
-const int kIconsPerMenuRow = 7;
-#endif
+const int kChevronSpacing = ToolbarView::kStandardSpacing - 2;
 
 // A version of MenuButton with almost empty insets to fit properly on the
 // toolbar.
@@ -119,6 +105,12 @@ BrowserActionsContainer::DropPosition::DropPosition(
 // BrowserActionsContainer
 
 // static
+int BrowserActionsContainer::icons_per_menu_row = 1;
+
+// static
+const int BrowserActionsContainer::kItemSpacing = ToolbarView::kStandardSpacing;
+
+// static
 bool BrowserActionsContainer::disable_animations_during_testing_ = false;
 
 BrowserActionsContainer::BrowserActionsContainer(
@@ -325,8 +317,8 @@ gfx::Size BrowserActionsContainer::GetPreferredSize() const {
     // When in overflow, y is multiline, so the pixel count is IconHeight()
     // times the number of rows needed.
     return gfx::Size(
-        IconCountToWidth(kIconsPerMenuRow, false),
-        (((icon_count - 1) / kIconsPerMenuRow) + 1) * IconHeight());
+        IconCountToWidth(icons_per_menu_row, false),
+        (((icon_count - 1) / icons_per_menu_row) + 1) * IconHeight());
   }
 
   // We calculate the size of the view by taking the current width and
@@ -386,9 +378,9 @@ void BrowserActionsContainer::Layout() {
          i < browser_action_views_.size(); ++i) {
       BrowserActionView* view = browser_action_views_[i];
       size_t index = i - main_container_->VisibleBrowserActionsAfterAnimation();
-      int row_index = static_cast<int>(index) / kIconsPerMenuRow;
+      int row_index = static_cast<int>(index) / icons_per_menu_row;
       int x = kItemSpacing + (index * IconWidth(true)) -
-          (row_index * IconWidth(true) * kIconsPerMenuRow);
+          (row_index * IconWidth(true) * icons_per_menu_row);
       gfx::Rect rect_bounds(
           x, IconHeight() * row_index, icon_width, IconHeight());
       view->SetBoundsRect(rect_bounds);
@@ -483,9 +475,9 @@ int BrowserActionsContainer::OnDragUpdated(
     // visible icons. Otherwise, it's a full row (kIconsPerRow).
     visible_icons_on_row =
         row_index ==
-            static_cast<size_t>(visible_icons_on_row / kIconsPerMenuRow) ?
-                visible_icons_on_row % kIconsPerMenuRow :
-                kIconsPerMenuRow;
+            static_cast<size_t>(visible_icons_on_row / icons_per_menu_row) ?
+                visible_icons_on_row % icons_per_menu_row :
+                icons_per_menu_row;
   }
 
   // Because the user can drag outside the container bounds, we need to clamp to
@@ -523,7 +515,7 @@ int BrowserActionsContainer::OnPerformDrop(
   DCHECK(model_);
 
   size_t i =
-      drop_position_->row * kIconsPerMenuRow + drop_position_->icon_in_row;
+      drop_position_->row * icons_per_menu_row + drop_position_->icon_in_row;

[Finnur, 2014/09/10 10:48:27]

Danger! Danger! Drop can be broken if the wrench menu is opened, triggering a
change to this static value and then someone tries to drag to the main
container. Would have been nasty to track down. :)

[Devlin, 2014/09/10 16:14:26]

Can it, though?

In the main container, there are no "rows", and, as such, drop_position_->row is
always 0, so the value of icons_per_menu_row doesn't matter at all.  The only
time it matters is for the overflow container (in which case it's correct by
definition, since the overflow container uses this to determine how many icons
it has).  I think this is actually safe.  (I've verified by hand that dragging
still works, just to be sure.)

[Finnur, 2014/09/11 09:12:43]

Good.

   if (in_overflow_mode())
     i += GetFirstVisibleIconIndex();
   // |i| now points to the item to the right of the drop indicator*, which is
@@ -1008,8 +1000,14 @@ int BrowserActionsContainer::IconCountToWidth(int icons,
       (icons == 0) ? 0 : ((icons * IconWidth(true)) - kItemSpacing);
   int chevron_size = chevron_ && display_chevron ?
       (kChevronSpacing + chevron_->GetPreferredSize().width()) : 0;
-  return ToolbarView::kStandardSpacing + icons_size + chevron_size +
-      ToolbarView::kStandardSpacing;
+  // In overflow mode, our padding is to use item spacing on either end (just so
+  // we can see the drop indicator). Otherwise we use the standard toolbar
+  // spacing.
+  // Note: These are actually the same thing, but, on the offchance one
+  // changes, let's get it right.
+  int padding =
+      2 * (in_overflow_mode() ? kItemSpacing : ToolbarView::kStandardSpacing);
+  return icons_size + chevron_size + padding;
 }
 
 size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {