[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..909fb6b78fa1bda978933030b7d0e13acf02d591 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.

[Peter Kasting, 2015/09/17 19:59:53]

Now that I read this comment I'm a bit worried.  If the window is near the
bottom of the screen or the screen is short, how do we know 13 rows won't still
go off the bottom?  Likewise if the screen is some huge portrait mode setup, we
might have space for more rows?  If the core desire is to stay on the screen it
seems like you'd need to check the screen bounds.  If the core desires is just
"don't make the menu excessively tall" irrespective of screen size, maybe the
comment should be updated.

[Devlin, 2015/09/17 21:38:38]

Views will actually make the menu go upwards if there's not enough room going
downwards, so this is only a concern on a screen that has too few pixels overall
- which should be *very* unlikely with this pixel size.
Comment updated.

+  // Note: As of Sept. 2015, we can show 104 actions (8 per row * 13 rows), so
+  // less than 0.0002% of our users will be affected.

[Peter Kasting, 2015/09/17 19:59:53]

Nit: I might just drop this part of the comment and leave that in the bug or the
CL description if future spelunkers really care; I don't think the code needs
this justification, and I don't like putting time-sensitive comments in the
code, because a future reader won't know whether the comment is still accurate
or not anyway.

[Devlin, 2015/09/17 21:38:38]

Done.

+  max_height_ = ToolbarActionsBar::IconHeight() * 13;

[Peter Kasting, 2015/09/17 19:59:53]

Tiny nit: Consider adding a named kMaxOverflowRows constant defined right here.

[Devlin, 2015/09/17 21:38:38]

Done.

+  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.
+  if (container_->GetPreferredSize().height() > max_height_)
+    s.Enlarge(GetScrollBarWidth(), 0);
+  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());
-  container_->SetBounds(0, 0, sz.width(), sz.height());
+  SetPosition(gfx::Point(start_padding(), 0));
+  SizeToPreferredSize();
+  views::ScrollView::Layout();
 }
 
 void ExtensionToolbarMenuView::OnBrowserActionDragDone() {
@@ -89,8 +104,7 @@ void ExtensionToolbarMenuView::CloseWrenchMenu() {
 
 int ExtensionToolbarMenuView::start_padding() const {
   // 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
+  // as the labels. We subtract kItemSpacing because there needs to be padding
   // so we can see the drop indicator.
   return views::MenuItemView::label_start() - 1 -

[Peter Kasting, 2015/09/17 19:59:53]

You didn't actually drop the -1 here.

[Devlin, 2015/09/17 21:38:38]

Could have sworn I did... thanks.

       container_->toolbar_actions_bar()->platform_settings().item_spacing;