[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..a37a49d8b7528688efc9c31d95391dc4ea6e5606 100644
--- a/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
+++ b/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
@@ -21,7 +21,8 @@ ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
                                                    WrenchMenu* wrench_menu)
     : browser_(browser),
       wrench_menu_(wrench_menu),
-      container_(NULL),
+      container_(nullptr),
+      max_height_(0),
       browser_actions_container_observer_(this),
       weak_factory_(this) {
   BrowserActionsContainer* main =
@@ -29,7 +30,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 +41,14 @@ ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
     browser_actions_container_observer_.Add(container_);
     browser_actions_container_observer_.Add(main);
   }
+
+  // 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: As of Sept. 2015, we can show 104 actions, so less than 0.0002% of
+  // our users will be affected.
+  max_height_ = ToolbarActionsBar::IconHeight() * 1;

[Peter Kasting, 2015/09/17 00:18:04]

I'm confused.  Does this mean one row of icons?  How do we fit 104 icons on one
row?

[Devlin, 2015/09/17 17:00:27]

D'oh.  Was testing to make sure it worked (and didn't want to install 104
extensions) ;)

s/1/13.

+  ClipHeightTo(0, max_height_);
 }
 
 ExtensionToolbarMenuView::~ExtensionToolbarMenuView() {
@@ -51,24 +60,30 @@ 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 if necessary.

[Peter Kasting, 2015/09/17 00:18:04]

I wonder if we should fix this in ScrollView.

[Devlin, 2015/09/17 17:00:27]

That would make me concerned about breaking anything else that uses a
ScrollView...

+  if (container_->GetPreferredSize().height() > max_height_)
+    s.set_width(s.width() + GetScrollBarWidth());

[Peter Kasting, 2015/09/17 00:18:04]

Nit: s.Enlarge(GetScrollbarWidth(), 0));

[Devlin, 2015/09/17 17:00:27]

Done.

+  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);

[Peter Kasting, 2015/09/17 00:18:04]

(Does this include the scrollbar width in the calculation at the right times?)

[Devlin, 2015/09/17 17:00:27]

Yes.

 }
 
 void ExtensionToolbarMenuView::Layout() {
-  gfx::Size sz = GetPreferredSize();
-  SetBounds(start_padding() + 1, 0, sz.width(), sz.height());
-  container_->SetBounds(0, 0, sz.width(), sz.height());
+  SetPosition(gfx::Point(start_padding() + 1, 0));

[Peter Kasting, 2015/09/17 00:18:04]

Why +1?

[Devlin, 2015/09/17 17:00:27]

It was because the start_padding() was returning the range of the padding,
rather than the width of the padding (hence the -1 in that method).  Removed
both.

+  SizeToPreferredSize();
+  views::ScrollView::Layout();
 }
 
 void ExtensionToolbarMenuView::OnBrowserActionDragDone() {