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

Determine how many extension icons to show on each row of the overflow menu based on the width of other menu items.

Screenshot: http://imgur.com/mPQAbxs
BUG=412503

Review URL: https://codereview.chromium.org/553233002

Cr-Commit-Position: refs/heads/master@{#294944}

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

@@ -50,22 +50,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_overflow_menu_row_ = 1;
+
+// static
+const int BrowserActionsContainer::kItemSpacing = ToolbarView::kStandardSpacing;
+
 // static
 bool BrowserActionsContainer::disable_animations_during_testing_ = false;
 
@@ -344,8 +336,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_overflow_menu_row_, false),
+        (((icon_count - 1) / icons_per_overflow_menu_row_) + 1) * IconHeight());
   }
 
   // We calculate the size of the view by taking the current width and
@@ -360,6 +352,12 @@ gfx::Size BrowserActionsContainer::GetPreferredSize() const {
   return gfx::Size(preferred_width, IconHeight());
 }
 
+int BrowserActionsContainer::GetHeightForWidth(int width) const {
+  if (in_overflow_mode())
+    icons_per_overflow_menu_row_ = (width - kItemSpacing) / IconWidth(true);
+  return GetPreferredSize().height();
+}
+
 gfx::Size BrowserActionsContainer::GetMinimumSize() const {
   int min_width = std::min(MinimumNonemptyWidth(), IconCountToWidth(-1, false));
   return gfx::Size(min_width, IconHeight());
@@ -405,9 +403,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_overflow_menu_row_;
       int x = kItemSpacing + (index * IconWidth(true)) -
-          (row_index * IconWidth(true) * kIconsPerMenuRow);
+          (row_index * IconWidth(true) * icons_per_overflow_menu_row_);
       gfx::Rect rect_bounds(
           x, IconHeight() * row_index, icon_width, IconHeight());
       view->SetBoundsRect(rect_bounds);
@@ -502,9 +500,10 @@ 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_overflow_menu_row_) ?
+                visible_icons_on_row % icons_per_overflow_menu_row_ :
+                icons_per_overflow_menu_row_;
   }
 
   // Because the user can drag outside the container bounds, we need to clamp to
@@ -541,8 +540,8 @@ int BrowserActionsContainer::OnPerformDrop(
             data.id());
   DCHECK(model_);
 
-  size_t i =
-      drop_position_->row * kIconsPerMenuRow + drop_position_->icon_in_row;
+  size_t i = drop_position_->row * icons_per_overflow_menu_row_ +
+             drop_position_->icon_in_row;
   if (in_overflow_mode())
     i += GetFirstVisibleIconIndex();
   // |i| now points to the item to the right of the drop indicator*, which is
@@ -1045,8 +1044,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 {

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

@@ -131,6 +131,10 @@ class BrowserActionsContainer
       public BrowserActionView::Delegate,
       public extensions::ExtensionKeybindingRegistry::Delegate {
  public:
+  // Horizontal spacing between most items in the container, as well as after
+  // the last item or chevron (if visible).
+  static const int kItemSpacing;
+
   // Constructs a BrowserActionContainer for a particular |browser| object, and
   // specifies which view is the |owner_view|. For documentation of
   // |main_container|, see class comments.
@@ -197,6 +201,7 @@ class BrowserActionsContainer
 
   // Overridden from views::View:
   virtual gfx::Size GetPreferredSize() const OVERRIDE;
+  virtual int GetHeightForWidth(int width) const OVERRIDE;
   virtual gfx::Size GetMinimumSize() const OVERRIDE;
   virtual void Layout() OVERRIDE;
   virtual bool GetDropFormats(int* formats,
@@ -254,6 +259,12 @@ class BrowserActionsContainer
   // unit tests.
   void TestSetIconVisibilityCount(size_t icons);
 
+  // Returns the width of an icon, optionally with its padding.
+  static int IconWidth(bool include_padding);
+
+  // Returns the height of an icon.
+  static int IconHeight();
+
   // During testing we can disable animations by setting this flag to true,
   // so that the bar resizes instantly, instead of having to poll it while it
   // animates to open/closed status.
@@ -267,19 +278,11 @@ class BrowserActionsContainer
   virtual void OnThemeChanged() OVERRIDE;
 
  private:
-  friend class BrowserActionView;  // So it can access IconWidth().
-
   // A struct representing the position at which an action will be dropped.
   struct DropPosition;
 
   typedef std::vector<BrowserActionView*> BrowserActionViews;
 
-  // Returns the width of an icon, optionally with its padding.
-  static int IconWidth(bool include_padding);
-
-  // Returns the height of an icon.
-  static int IconHeight();
-
   // extensions::ExtensionToolbarModel::Observer implementation.
   virtual void ToolbarExtensionAdded(const extensions::Extension* extension,
                                      int index) OVERRIDE;
@@ -429,6 +432,10 @@ class BrowserActionsContainer
 
   ObserverList<BrowserActionsContainerObserver> observers_;
 
+  // The maximum number of icons to show per row when in overflow mode (showing
+  // icons in the application menu).
+  static int icons_per_overflow_menu_row_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserActionsContainer);
 };
 

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

@@ -12,6 +12,21 @@
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/browser/ui/views/toolbar/wrench_menu.h"
 #include "ui/views/controls/menu/menu_item_view.h"
+#include "ui/views/controls/menu/submenu_view.h"
+
+namespace {
+
+// Returns the padding before the BrowserActionsContainer in the menu.
+int start_padding() {
+  // We pad enough on the left so that the first icon starts at the same point
+  // as the labels. We need to subtract 1 because we want the pixel *before*
+  // the label, and we subtract kItemSpacing because there needs to be padding
+  // so we can see the drop indicator.
+  return views::MenuItemView::label_start() - 1 -
+      BrowserActionsContainer::kItemSpacing;
+}
+
+}  // namespace
 
 ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
                                                    WrenchMenu* wrench_menu)
@@ -45,10 +60,20 @@ gfx::Size ExtensionToolbarMenuView::GetPreferredSize() const {
   return container_->GetPreferredSize();
 }
 
+int ExtensionToolbarMenuView::GetHeightForWidth(int width) const {
+  const views::MenuConfig& menu_config =
+      static_cast<const views::MenuItemView*>(parent())->GetMenuConfig();
+  int end_padding = menu_config.arrow_to_edge_padding -
+                    BrowserActionsContainer::kItemSpacing;
+  width -= start_padding() + end_padding;
+
+  int height = container_->GetHeightForWidth(width);
+  return height;
+}
+
 void ExtensionToolbarMenuView::Layout() {
-  // All buttons are given the same width.
   gfx::Size sz = GetPreferredSize();
-  SetBounds(views::MenuItemView::label_start(), 0, sz.width(), sz.height());
+  SetBounds(start_padding() + 1, 0, sz.width(), sz.height());
   container_->SetBounds(0, 0, sz.width(), sz.height());
 }
 

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

@@ -27,6 +27,7 @@ class ExtensionToolbarMenuView : public views::View,
 
   // views::View:
   virtual gfx::Size GetPreferredSize() const OVERRIDE;
+  virtual int GetHeightForWidth(int width) const OVERRIDE;
   virtual void Layout() OVERRIDE;
 
  private:

ui/views/controls/menu/menu_item_view.cc

@@ -410,6 +410,19 @@ gfx::Size MenuItemView::GetPreferredSize() const {
                    dimensions.height);
 }
 
+int MenuItemView::GetHeightForWidth(int width) const {
+  // If this isn't a container, we can just use the preferred size's height.
+  if (!IsContainer())
+    return GetPreferredSize().height();
+
+  int height = child_at(0)->GetHeightForWidth(width);
+  if (!icon_view_ && GetRootMenuItem()->has_icons())
+    height = std::max(height, GetMenuConfig().check_height);
+  height += GetBottomMargin() + GetTopMargin();
+
+  return height;
+}
+
 const MenuItemView::MenuItemDimensions& MenuItemView::GetDimensions() const {
   if (!is_dimensions_valid())
     dimensions_ = CalculateDimensions();

ui/views/controls/menu/menu_item_view.h

@@ -265,6 +265,11 @@ class VIEWS_EXPORT MenuItemView : public View {
   // Returns the preferred size of this item.
   virtual gfx::Size GetPreferredSize() const OVERRIDE;
 
+  // Gets the preferred height for the given |width|. This is only different
+  // from GetPreferredSize().width() if the item has a child view with flexible
+  // dimensions.
+  virtual int GetHeightForWidth(int width) const OVERRIDE;
+
   // Return the preferred dimensions of the item in pixel.
   const MenuItemDimensions& GetDimensions() const;
 

ui/views/controls/menu/menu_item_view_unittest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/views/controls/menu/menu_item_view.h"
+
+#include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/controls/menu/submenu_view.h"
+#include "ui/views/view.h"
+
+namespace {
+
+// A simple View class that will match its height to the available width.
+class SquareView : public views::View {
+ public:
+  SquareView() {}
+  virtual ~SquareView() {}
+
+ private:
+  virtual gfx::Size GetPreferredSize() const OVERRIDE {
+    return gfx::Size(1, 1);
+  }
+  virtual int GetHeightForWidth(int width) const OVERRIDE {
+    return width;
+  }
+};
+
+// A MenuItemView implementation with a public destructor (so we can clean up
+// in tests).
+class TestMenuItemView : public views::MenuItemView {
+ public:
+  TestMenuItemView() : views::MenuItemView(NULL) {}
+  virtual ~TestMenuItemView() {}
+};
+
+}  // namespace
+
+TEST(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
+  TestMenuItemView root_menu;
+  root_menu.set_owned_by_client();
+
+  // Append a normal MenuItemView.
+  views::MenuItemView* label_view =
+      root_menu.AppendMenuItemWithLabel(1, base::ASCIIToUTF16("item 1"));
+
+  // Append a second MenuItemView that has a child SquareView.
+  views::MenuItemView* flexible_view =
+      root_menu.AppendMenuItemWithLabel(2, base::string16());
+  flexible_view->AddChildView(new SquareView());
+  // Set margins to 0 so that we know width should match height.
+  flexible_view->SetMargins(0, 0);
+
+  views::SubmenuView* submenu = root_menu.GetSubmenu();
+
+  // The first item should be the label view.
+  ASSERT_EQ(label_view, submenu->GetMenuItemAt(0));
+  gfx::Size label_size = label_view->GetPreferredSize();
+
+  // The second item should be the flexible view.
+  ASSERT_EQ(flexible_view, submenu->GetMenuItemAt(1));
+  gfx::Size flexible_size = flexible_view->GetPreferredSize();
+
+  // The flexible view's "preferred size" should be 1x1...
+  EXPECT_EQ(flexible_size, gfx::Size(1, 1));
+
+  // ...but it should use whatever space is available to make a square.
+  int flex_height = flexible_view->GetHeightForWidth(label_size.width());
+  EXPECT_EQ(label_size.width(), flex_height);
+
+  // The submenu should be tall enough to allow for both menu items at the given
+  // width.
+  EXPECT_EQ(label_size.height() + flex_height,
+            submenu->GetPreferredSize().height());
+}

ui/views/controls/menu/submenu_view.cc

@@ -114,9 +114,9 @@ void SubmenuView::Layout() {
   for (int i = 0; i < child_count(); ++i) {
     View* child = child_at(i);
     if (child->visible()) {
-      gfx::Size child_pref_size = child->GetPreferredSize();
-      child->SetBounds(x, y, menu_item_width, child_pref_size.height());
-      y += child_pref_size.height();
+      int child_height = child->GetHeightForWidth(menu_item_width);
+      child->SetBounds(x, y, menu_item_width, child_height);
+      y += child_height;
     }
   }
 }
@@ -130,7 +130,11 @@ gfx::Size SubmenuView::GetPreferredSize() const {
   int max_complex_width = 0;
   // The max. width of items which contain a label and maybe an accelerator.
   int max_simple_width = 0;
-  int height = 0;
+
+  // We perform the size calculation in two passes. In the first pass, we
+  // calculate the width of the menu. In the second, we calculate the height
+  // using that width. This allows views that have flexible widths to adjust
+  // accordingly.
   for (int i = 0; i < child_count(); ++i) {
     const View* child = child_at(i);
     if (!child->visible())
@@ -145,25 +149,31 @@ gfx::Size SubmenuView::GetPreferredSize() const {
           std::max(max_minor_text_width_, dimensions.minor_text_width);
       max_complex_width = std::max(max_complex_width,
           dimensions.standard_width + dimensions.children_width);
-      height += dimensions.height;
     } else {
-      gfx::Size child_pref_size =
-          child->visible() ? child->GetPreferredSize() : gfx::Size();
-      max_complex_width = std::max(max_complex_width, child_pref_size.width());
-      height += child_pref_size.height();
+      max_complex_width = std::max(max_complex_width,
+                                   child->GetPreferredSize().width());
     }
   }
   if (max_minor_text_width_ > 0) {
     max_minor_text_width_ +=
         GetMenuItem()->GetMenuConfig().label_to_minor_text_padding;
   }
+  // Finish calculating our optimum width.
   gfx::Insets insets = GetInsets();
-  return gfx::Size(
-      std::max(max_complex_width,
-               std::max(max_simple_width + max_minor_text_width_ +
-                        insets.width(),
-               minimum_preferred_width_ - 2 * insets.width())),
-      height + insets.height());
+  int width = std::max(max_complex_width,
+                       std::max(max_simple_width + max_minor_text_width_ +
+                                    insets.width(),
+                                minimum_preferred_width_ - 2 * insets.width()));
+
+  // Then, the height for that width.
+  int height = 0;
+  int menu_item_width = width - insets.width();
+  for (int i = 0; i < child_count(); ++i) {
+    const View* child = child_at(i);
+    height += child->visible() ? child->GetHeightForWidth(menu_item_width) : 0;
+  }
+
+  return gfx::Size(width, height + insets.height());
 }
 
 void SubmenuView::GetAccessibleState(ui::AXViewState* state) {

ui/views/views.gyp

@@ -507,6 +507,7 @@
       'controls/combobox/combobox_unittest.cc',
       'controls/label_unittest.cc',
       'controls/menu/menu_controller_unittest.cc',
+      'controls/menu/menu_item_view_unittest.cc',
       'controls/menu/menu_model_adapter_unittest.cc',
       'controls/menu/menu_runner_cocoa_unittest.mm',
       'controls/native/native_view_host_aura_unittest.cc',