[Extensions Toolbar] Protect against crazy bounds

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

Index: chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
diff --git a/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc b/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
index 5e70b294d75e57c794d90b8950b01b6535067724..4d89f936aa2bd1aa4dccc935638127f427d87a92 100644
--- a/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
+++ b/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
@@ -17,6 +17,15 @@
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/submenu_view.h"
 
+namespace {
+// In *very* extreme cases, it's possible that there are so many overflowed
+// actions, we won't be able to show them all. Cap the height so that the
+// overflow won't make the menu larger than the height of the screen.
+// Note: With this height, we can show 104 actions. Less than 0.0002% of our
+// users will be affected.
+const int kMaxOverflowContainerHeight = 416;

[Peter Kasting, 2015/09/15 00:13:38]

Nit: Prefer to set the desired number of overflow actions or rows, and then
compute the height based on that.  This way if we ever change the size of the
action icons the code will keep working (and you won't need the comment about
how this is 104 actions).

Also, prefer to move all this to the function that uses it, especially when your
explanation describes something the code should do ("cap the height") rather
than just describing what the constant is.

[Devlin, 2015/09/15 18:22:39]

Done.  I don't want to calculate it based on number of actions, because then it
doesn't really limit the height, if the number of icons per row is adjusted. 
But doing a number of rows sounds good.

+}
+
 ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
                                                    WrenchMenu* wrench_menu)
     : browser_(browser),
@@ -29,7 +38,7 @@ ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
           ->toolbar()->browser_actions();
   container_ = new BrowserActionsContainer(browser_, main);
   container_->Init();
-  AddChildView(container_);
+  SetContents(container_);
   // We Layout() the container here so that we know the number of actions
   // that will be visible in ShouldShow().
   container_->Layout();
@@ -40,6 +49,8 @@ ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
     browser_actions_container_observer_.Add(container_);
     browser_actions_container_observer_.Add(main);
   }
+
+  ClipHeightTo(0, kMaxOverflowContainerHeight);
 }
 
 ExtensionToolbarMenuView::~ExtensionToolbarMenuView() {
@@ -51,24 +62,29 @@ bool ExtensionToolbarMenuView::ShouldShow() {
 }
 
 gfx::Size ExtensionToolbarMenuView::GetPreferredSize() const {
-  return container_->GetPreferredSize();
+  gfx::Size s = views::ScrollView::GetPreferredSize();
+  // views::ScrollView::GetPreferredSize() includes the contents' size, but
+  // not the scrollbar width. Add it in.
+  s.set_width(s.width() + GetScrollBarWidth());

[Peter Kasting, 2015/09/15 00:13:38]

I assume GetScrollBarWidth() returns 0 when no scrollbar is showing?

[Devlin, 2015/09/15 18:22:39]

I would have thought so.  Apparently not.  Addressed.

+  return s;
 }
 
 int ExtensionToolbarMenuView::GetHeightForWidth(int width) const {
+  // The width passed in here includes the full width of the menu, so we need
+  // to omit the necessary padding.
   const views::MenuConfig& menu_config =
       static_cast<const views::MenuItemView*>(parent())->GetMenuConfig();
   int end_padding = menu_config.arrow_to_edge_padding -
       container_->toolbar_actions_bar()->platform_settings().item_spacing;
   width -= start_padding() + end_padding;
 
-  int height = container_->GetHeightForWidth(width);
-  return height;
+  return views::ScrollView::GetHeightForWidth(width);
 }
 
 void ExtensionToolbarMenuView::Layout() {
   gfx::Size sz = GetPreferredSize();
   SetBounds(start_padding() + 1, 0, sz.width(), sz.height());

[Peter Kasting, 2015/09/15 00:13:38]

Nit: Maybe

  SetPosition(gfx::Point(start_padding() + 1, 0));
  SizeToPreferredSize();

[Devlin, 2015/09/15 18:22:39]

Sure, done.

-  container_->SetBounds(0, 0, sz.width(), sz.height());
+  views::ScrollView::Layout();
 }
 
 void ExtensionToolbarMenuView::OnBrowserActionDragDone() {