Fix three small bugs in BrowserActionsContainer

chrome/browser/ui/views/toolbar/browser_actions_container.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/browser_actions_container.h"
 
 #include "base/compiler_specific.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 287 matching lines @@
 
 gfx::Size BrowserActionsContainer::GetPreferredSize() const {
   if (in_overflow_mode()) {
     int icon_count = GetIconCount();
     // In overflow, we always have a preferred size of a full row (even if we
     // don't use it), and always of at least one row. The parent may decide to
     // show us even when empty, e.g. as a drag target for dragging in icons from
     // the main container.
     int row_count =
         ((std::max(0, icon_count - 1)) / icons_per_overflow_menu_row_) + 1;
-    return gfx::Size(
-        IconCountToWidth(icons_per_overflow_menu_row_, false),
-        row_count * IconHeight());
+    return gfx::Size(IconCountToWidth(icons_per_overflow_menu_row_),
+                     row_count * IconHeight());
   }
 
   // If there are no actions to show, then don't show the container at all.
   if (browser_action_views_.empty())
     return gfx::Size();
 
   // We calculate the size of the view by taking the current width and
   // subtracting resize_amount_ (the latter represents how far the user is
   // resizing the view or, if animating the snapping, how far to animate it).
   // But we also clamp it to a minimum size and the maximum size, so that the
   // container can never shrink too far or take up more space than it needs.
   // In other words: MinimumNonemptyWidth() < width() - resize < ClampTo(MAX).
   int preferred_width = std::min(
       std::max(MinimumNonemptyWidth(), container_width_ - resize_amount_),
-      IconCountToWidth(-1, false));
+      IconCountToWidth(-1));
   return gfx::Size(preferred_width, IconHeight());
 }
 
 int BrowserActionsContainer::GetHeightForWidth(int width) const {
   if (in_overflow_mode())
     icons_per_overflow_menu_row_ = (width - kItemSpacing) / IconWidth(true);
   return GetPreferredSize().height();
 }
 
 gfx::Size BrowserActionsContainer::GetMinimumSize() const {
-  int min_width = std::min(MinimumNonemptyWidth(), IconCountToWidth(-1, false));
+  int min_width = std::min(MinimumNonemptyWidth(), IconCountToWidth(-1));
   return gfx::Size(min_width, IconHeight());
 }
 
 void BrowserActionsContainer::Layout() {
   if (browser_action_views_.empty()) {
     SetVisible(false);
     return;
   }
 
   SetVisible(true);
   if (resize_area_)
     resize_area_->SetBounds(0, 0, kItemSpacing, height());
 
   // If the icons don't all fit, show the chevron (unless suppressed).
   int max_x = GetPreferredSize().width();
-  if (IconCountToWidth(-1, false) > max_x && !suppress_chevron_ && chevron_) {
+  if (IconCountToWidth(-1) > max_x && !suppress_chevron_ && chevron_) {
     chevron_->SetVisible(true);
     gfx::Size chevron_size(chevron_->GetPreferredSize());
-    max_x -=
-        ToolbarView::kStandardSpacing + chevron_size.width() + kChevronSpacing;
+    max_x -= chevron_size.width() + kChevronSpacing;
     chevron_->SetBounds(
         width() - ToolbarView::kStandardSpacing - chevron_size.width(),
         0,
         chevron_size.width(),
         chevron_size.height());
   } else if (chevron_) {
     chevron_->SetVisible(false);
   }
+  // Subtract off the trailing padding.

[Peter Kasting, 2014/10/02 21:49:56]

Should we be doing this before we compare IconCountToWidth() to this value
above?

[Devlin, 2014/10/02 22:11:06]

Nope.  IconCountToWidth() will be the full width of the container, so if we
subtract it off there, then max_x will always be less than IconCountToWidth(-1)
(max_x is intended to be the max_x for an icon, not the max_x for the
container).

+  max_x -= ToolbarView::kStandardSpacing;
 
   // Now draw the icons for the browser actions in the available space.
   int icon_width = IconWidth(false);
   if (in_overflow_mode()) {
     for (size_t i = 0;
          i < main_container_->VisibleBrowserActionsAfterAnimation(); ++i) {
       // Ensure that any browser actions shown in the main view are hidden in
       // the overflow view.
       browser_action_views_[i]->SetVisible(false);
     }
@@ skipping 216 matching lines @@
 void BrowserActionsContainer::OnResize(int resize_amount, bool done_resizing) {
   if (!done_resizing) {
     resize_amount_ = resize_amount;
     OnBrowserActionVisibilityChanged();
     return;
   }
 
   // Up until now we've only been modifying the resize_amount, but now it is
   // time to set the container size to the size we have resized to, and then
   // animate to the nearest icon count size if necessary (which may be 0).
-  int max_width = IconCountToWidth(-1, false);
+  int max_width = IconCountToWidth(-1);
   container_width_ =
       std::min(std::max(0, container_width_ - resize_amount), max_width);
 
   // Save off the desired number of visible icons.  We do this now instead of at
   // the end of the animation so that even if the browser is shut down while
   // animating, the right value will be restored on next run.
   // NOTE: Don't save the icon count in incognito because there may be fewer
   // icons in that mode. The result is that the container in a normal window is
   // always at least as wide as in an incognito window.
   int visible_icons = WidthToIconCount(container_width_);
@@ skipping 22 matching lines @@
                     observers_,
                     OnBrowserActionsContainerAnimationEnded());
 }
 
 content::WebContents* BrowserActionsContainer::GetCurrentWebContents() {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
 extensions::ActiveTabPermissionGranter*
     BrowserActionsContainer::GetActiveTabPermissionGranter() {
-  content::WebContents* web_contents =
-      browser_->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* web_contents = GetCurrentWebContents();
   if (!web_contents)
     return NULL;
   return extensions::TabHelper::FromWebContents(web_contents)->
       active_tab_permission_granter();
 }
 
 ExtensionPopup* BrowserActionsContainer::TestGetPopup() {
   return popup_owner_ ? popup_owner_->view_controller()->popup() : NULL;
 }
 
@@ skipping 247 matching lines @@
   CreateBrowserActionViews();
   Animate(gfx::Tween::LINEAR, GetIconCount());
 }
 
 Browser* BrowserActionsContainer::GetBrowser() {
   return browser_;
 }
 
 void BrowserActionsContainer::LoadImages() {
   ui::ThemeProvider* tp = GetThemeProvider();
-  if (!tp || !chevron_)
-    return;
-
-  chevron_->SetImage(views::Button::STATE_NORMAL,
-                     *tp->GetImageSkiaNamed(IDR_BROWSER_ACTIONS_OVERFLOW));
-
-  const int kImages[] = IMAGE_GRID(IDR_DEVELOPER_MODE_HIGHLIGHT);
-  highlight_painter_.reset(views::Painter::CreateImageGridPainter(kImages));
+  if (tp && chevron_) {
+    chevron_->SetImage(views::Button::STATE_NORMAL,
+                       *tp->GetImageSkiaNamed(IDR_BROWSER_ACTIONS_OVERFLOW));
+  }
+  if (!in_overflow_mode()) {
+    // Highlighting only takes place on the main bar, so no need for it in
+    // overflow.

[Peter Kasting, 2014/10/02 21:49:56]

Nit: Put this comment above the conditional instead of inside.

[Devlin, 2014/10/02 22:11:06]

Done.

+    const int kImages[] = IMAGE_GRID(IDR_DEVELOPER_MODE_HIGHLIGHT);
+    highlight_painter_.reset(views::Painter::CreateImageGridPainter(kImages));
+  }
 }
 
 void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
   SetVisible(!browser_action_views_.empty());
   if (owner_view_) {
     owner_view_->Layout();
     owner_view_->SchedulePaint();
   } else {
     // In overflow mode, we don't have an owner view, but we still have to
     // update ourselves.
     Layout();
     SchedulePaint();
   }
 }
 
 int BrowserActionsContainer::GetPreferredWidth() {
-  size_t visible_actions = GetIconCount();
-  return IconCountToWidth(
-      visible_actions,
-      chevron_ && visible_actions < browser_action_views_.size());
+  return IconCountToWidth(GetIconCount());
 }
 
 void BrowserActionsContainer::SetChevronVisibility() {
   if (chevron_) {
     chevron_->SetVisible(
         VisibleBrowserActionsAfterAnimation() < browser_action_views_.size());
   }
 }
 
-int BrowserActionsContainer::IconCountToWidth(int icons,
-                                              bool display_chevron) const {
+int BrowserActionsContainer::IconCountToWidth(int icons) const {
   if (icons < 0)
     icons = browser_action_views_.size();
-  if ((icons == 0) && !display_chevron)
+  bool display_chevron =
+      chevron_ && icons < static_cast<int>(browser_action_views_.size());

[Peter Kasting, 2014/10/02 21:49:56]

Nit: I think casting icons to size_t is probably better, since we've already
ensured it's non-negative.

[Devlin, 2014/10/02 22:11:06]

Done.

+  if (icons == 0 && !display_chevron)
     return ToolbarView::kStandardSpacing;
-  int icons_size =
-      (icons == 0) ? 0 : ((icons * IconWidth(true)) - kItemSpacing);
-  int chevron_size = chevron_ && display_chevron ?
+  int icons_size = icons == 0 ? 0 : ((icons * IconWidth(true)) - kItemSpacing);

[Peter Kasting, 2014/10/02 21:49:56]

Nit: Up to you, but I think the parens around the condition that the previous
code had are more readable.

[Devlin, 2014/10/02 22:11:06]

Happy to defer to OWNER's preference. :) Done.

+  int chevron_size = display_chevron ?
       (kChevronSpacing + chevron_->GetPreferredSize().width()) : 0;
   // In overflow mode, our padding is to use item spacing on either end (just so
   // we can see the drop indicator). Otherwise we use the standard toolbar
   // spacing.
   // Note: These are actually the same thing, but, on the offchance one
   // changes, let's get it right.
   int padding =
       2 * (in_overflow_mode() ? kItemSpacing : ToolbarView::kStandardSpacing);
   return icons_size + chevron_size + padding;
 }
 
 size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
   // Check for widths large enough to show the entire icon set.
-  if (pixels >= IconCountToWidth(-1, false))
+  if (pixels >= IconCountToWidth(-1))
     return browser_action_views_.size();
 
   // We reserve space for the padding on either side of the toolbar...
   int available_space = pixels - (ToolbarView::kStandardSpacing * 2);
   // ... and, if the chevron is enabled, the chevron.
   if (chevron_)
     available_space -= (chevron_->GetPreferredSize().width() + kChevronSpacing);
 
   // Now we add an extra between-item padding value so the space can be divided
   // evenly by (size of icon with padding).
   return static_cast<size_t>(
       std::max(0, available_space + kItemSpacing) / IconWidth(true));
 }
 
 int BrowserActionsContainer::MinimumNonemptyWidth() const {
   if (!chevron_)
     return ToolbarView::kStandardSpacing;
   return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
       chevron_->GetPreferredSize().width();
 }
 
 void BrowserActionsContainer::Animate(gfx::Tween::Type tween_type,
                                       size_t num_visible_icons) {
-  int target_size = IconCountToWidth(num_visible_icons,
-      num_visible_icons < browser_action_views_.size());
+  int target_size = IconCountToWidth(num_visible_icons);
   if (resize_animation_ && !disable_animations_during_testing_) {
     // Animate! We have to set the animation_target_size_ after calling Reset(),
     // because that could end up calling AnimationEnded which clears the value.
     resize_animation_->Reset();
     resize_animation_->SetTweenType(tween_type);
     animation_target_size_ = target_size;
     resize_animation_->Show();
   } else {
     animation_target_size_ = target_size;
     AnimationEnded(resize_animation_.get());
@@ skipping 37 matching lines @@
   if (initialized_ && profile_->IsOffTheRecord()) {
     main_displayed = in_overflow_mode() ?
         main_container_->VisibleBrowserActionsAfterAnimation() :
         VisibleBrowserActionsAfterAnimation();
   }
 
   // The overflow displays any (displayable) icons not shown by the main bar.
   return in_overflow_mode() ?
       displayable_icon_count - main_displayed : main_displayed;
 }