[Extensions Toolbar] Protect against crazy bounds

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

 // Copyright 2013 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 "chrome/browser/ui/views/toolbar/toolbar_view.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/i18n/number_formatting.h"
@@ skipping 586 matching lines @@
     home_->SetBounds(next_element_x, child_y,
                      home_->GetPreferredSize().width(), child_height);
   } else {
     home_->SetVisible(false);
     home_->SetBounds(next_element_x, child_y, 0, child_height);
   }
   next_element_x = home_->bounds().right() +
                    theme_provider->GetDisplayProperty(
                        ThemeProperties::PROPERTY_TOOLBAR_VIEW_STANDARD_SPACING);
 
-  int browser_actions_width = browser_actions_->GetPreferredSize().width();
+  int browser_actions_desired_width =
+      browser_actions_->GetPreferredSize().width();
   int app_menu_width = app_menu_->GetPreferredSize().width();
   const int right_edge_spacing = theme_provider->GetDisplayProperty(
       ThemeProperties::PROPERTY_TOOLBAR_VIEW_RIGHT_EDGE_SPACING);
   const int location_bar_right_padding = theme_provider->GetDisplayProperty(
       ThemeProperties::PROPERTY_TOOLBAR_VIEW_LOCATION_BAR_RIGHT_PADDING);
+
+  // Don't allow the omnibox to shrink to the point of non-existence.
+  int kMinimumLocationBarWidth = 100;

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

(1) You should probably BUG=58915 in your CL description
(2) Use GetMinimumSize() on various elements (e.g. location_bar_) rather than
hardcoding values here

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

1) Wow, that's an old one.  Good find.
2) Done.

   int available_width = std::max(
-      0, width() - right_edge_spacing - app_menu_width - browser_actions_width -
-             (browser_actions_width > 0 ? element_padding : 0) -
-             location_bar_right_padding - next_element_x);
+      0, width() - right_edge_spacing - app_menu_width -
+             (browser_actions_desired_width > 0 ? element_padding : 0) -
+             location_bar_right_padding - next_element_x -
+             kMinimumLocationBarWidth);
+  int browser_actions_width =
+      std::min(available_width, browser_actions_desired_width);
+  available_width -= browser_actions_width;

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

This gives the browser actions area as much space as it wants up to the limit of
making the location bar really small.  Maybe if people haven't manually sized
this area we should instead limit the space this takes up to e.g. half the
toolbar width or something?  After all, even if you have a minimum width for the
location bar to avoid glitchy-looking UI, it's still not going to be much fun to
use.

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

Hmm... I'd like to avoid this for now.  For one thing, it's more consistent with
current behavior, and I'd be worried that it'd be hard to find the right balance
for a minimum without a lot of experimentation, and would probably require a
little UI review, which would slow this down a lot.

More importantly, though, most of our users have relatively few extensions (over
90% have 3 or fewer), so the number of users for whom this would have an effect
is pretty small, and is also (probably?) more likely to be the power users that
already know they can resize it.  I'd like to avoid any "implicit" resizing,
unless absolutely necessary for usability (as in the case of the omnibox
shrinking to nothingness).

+  int location_bar_width = kMinimumLocationBarWidth + available_width;

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

This statement seems wrong.  Why are we adding in kMinimumLocationBarWidth?

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

Above, we subtract out the minimum location bar width from the available width,
so the "leftover" width is the available width + the minimum location bar width.
 I've added a comment to explain this a bit more.

 
   int location_height = location_bar_->GetPreferredSize().height();
   int location_y = (height() - location_height + 1) / 2;
   location_bar_->SetBounds(next_element_x, location_y,
-                           std::max(available_width, 0), location_height);
+                           location_bar_width, location_height);
   next_element_x = location_bar_->bounds().right() + location_bar_right_padding;
 
   browser_actions_->SetBounds(
       next_element_x, child_y, browser_actions_width, child_height);
   next_element_x = browser_actions_->bounds().right();
   if (browser_actions_width > 0)
     next_element_x += element_padding;
 
   // The browser actions need to do a layout explicitly, because when an
   // extension is loaded/unloaded/changed, BrowserActionContainer removes and
@@ skipping 208 matching lines @@
   SchedulePaint();
 }
 
 int ToolbarView::content_shadow_height() const {
   ui::ThemeProvider* theme_provider = GetThemeProvider();
   return theme_provider->GetDisplayProperty(
       browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH
           ? ThemeProperties::PROPERTY_TOOLBAR_VIEW_CONTENT_SHADOW_HEIGHT_ASH
           : ThemeProperties::PROPERTY_TOOLBAR_VIEW_CONTENT_SHADOW_HEIGHT);
 }