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

ui/views/controls/menu/menu_item_view.cc

 // Copyright (c) 2012 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/i18n/case_conversion.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/accessibility/ax_view_state.h"
@@ skipping 392 matching lines @@
 void MenuItemView::OnPaint(gfx::Canvas* canvas) {
   PaintButton(canvas, PB_NORMAL);
 }
 
 gfx::Size MenuItemView::GetPreferredSize() const {
   const MenuItemDimensions& dimensions(GetDimensions());
   return gfx::Size(dimensions.standard_width + dimensions.children_width,
                    dimensions.height);
 }
 
+int MenuItemView::GetHeightForWidth(int width) const {
+  int height = 0;
+  // If this is a container, check the child view. Otherwise, we use the
+  // default menu height for everything but the icon.
+  if (!IsContainer())

[sky, 2014/09/12 20:09:37]

If !container, can't you just use GetPreferredSize().height()? I'm ok with being
inconsistent for the icon_view_. Layout doesn't ask for the heightforwidth
either, so it's ok not to here too.

[Devlin, 2014/09/12 22:41:37]

As long as we don't mind being inconsistent for icon_view_, yeah.  Done.

+   height = icon_view_ ? icon_view_->GetHeightForWidth(width) : 0;

[sky, 2014/09/12 20:09:37]

nit: spacing.

[Devlin, 2014/09/12 22:41:37]

moot (line gone).

+  else
+    height = child_at(0)->GetHeightForWidth(width);
+
+  if (!icon_view_ && GetRootMenuItem()->has_icons())
+    height = std::max(height, GetMenuConfig().check_height);
+  height += GetBottomMargin() + GetTopMargin();
+
+  // There's a chance our dimensions changed as a result of this. If so,
+  // invalidate them so they get recalculated as needed.
+  if (is_dimensions_valid() && height != GetDimensions().height)

[sky, 2014/09/12 20:09:37]

I don't understand the need for this. Can you elaborate? MenuItemDimensions
should only matter for the !container case, which I think you should early out
of here.

[Devlin, 2014/09/12 22:41:37]

But the Dimensions are used in GetPreferredSize(), so if our preferred size
changed here (as is the case with the extension action overflow container), we
need to invalidate those dimensions.  Otherwise they'll never be recalculated in
GetPreferredSize(), and we use the old dimensions.

[sky, 2014/09/15 15:29:54]

Why does that matter though? GetPreferredSize and GetHeightForWidth need not
return the same thing. There is no guarantee or implication that one influences
the other.

[Devlin, 2014/09/15 15:39:20]

In SubmenuView::GetPreferredSize(), we loop over the MenuItemView children in
two passes - the first to get the width (and find the maximum width for the
menu), and the second to GetHeightForWidth().  Unfortunately, if a menu item has
flexible width (like the extension action overflow container), then the
dimensions initially calculated in the first call to GetPreferredSize() will be
incorrect (since the width isn't set at that time - we're still calculating it),
but they will still be "valid".  As such, we need to invalidate them before
GetPreferredSize() is called again (since the preferred size is cached as part
of GetPreferredSize()) when we draw the menu items.  It seemed simplest to me to
do this here, so that we can confine the logic to MenuItemView (the thing that's
actually changing), but we could also potentially do it SubmenuView() after
calling GetHeightForWidth(), or we could make the first loop
SubmenuView::GetPreferredSize() have more complex checks so that it doesn't call
GetPreferredSize() on the MenuItemViews that have flexible widths.

+    invalidate_dimensions();
+
+  return height;
+}
+
 const MenuItemView::MenuItemDimensions& MenuItemView::GetDimensions() const {
   if (!is_dimensions_valid())
     dimensions_ = CalculateDimensions();
   DCHECK(is_dimensions_valid());
   return dimensions_;
 }
 
 MenuController* MenuItemView::GetMenuController() {
   return GetRootMenuItem()->controller_;
 }
@@ skipping 641 matching lines @@
     } else {
       const Type& type = menu_item->GetType();
       if (type == CHECKBOX || type == RADIO)
         return true;
     }
   }
   return false;
 }
 
 }  // namespace views