Extension Toolbar redesign, part 1 (overflow)

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

Index: chrome/browser/ui/views/toolbar/browser_actions_container.cc
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.cc b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
index 64a2be4c3d981a827be51f4f56bdbd2684851ed8..5bfb0236158ddb47c419f1feb7b7d06c5d87cf99 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -27,6 +27,7 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/pref_names.h"
 #include "extensions/browser/runtime_data.h"
+#include "extensions/common/feature_switch.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "grit/ui_resources.h"
@@ -57,6 +58,14 @@ const int kItemSpacing = ToolbarView::kStandardSpacing;
 // Horizontal spacing before the chevron (if visible).
 const int kChevronSpacing = kItemSpacing - 2;
 
+// Padding to make sure the badge appears in the right location vertically when
+// in overflow mode (inside the app menu, aka. hamburger menu).
+const int kPaddingForBadge = 9;
+
+// The maximum number of icons to show per row when in overflow mode (showing
+// icons in the application menu).
+const int kIconsPerMenuRow = 7;
+
 // A version of MenuButton with almost empty insets to fit properly on the
 // toolbar.
 class ChevronMenuButton : public views::MenuButton {
@@ -94,15 +103,17 @@ bool BrowserActionsContainer::disable_animations_during_testing_ = false;
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionsContainer
 
-BrowserActionsContainer::BrowserActionsContainer(Browser* browser,
-                                                 View* owner_view)
+BrowserActionsContainer::BrowserActionsContainer(
+    Browser* browser, View* owner_view, BrowserActionsContainer* main_container)
     : profile_(browser->profile()),
       browser_(browser),
       owner_view_(owner_view),
+      main_container_(main_container),
       popup_(NULL),
       popup_button_(NULL),
       model_(NULL),
       container_width_(0),
+      resize_area_(NULL),
       chevron_(NULL),
       overflow_menu_(NULL),
       suppress_chevron_(false),
@@ -117,22 +128,33 @@ BrowserActionsContainer::BrowserActionsContainer(Browser* browser,
   if (model_)
     model_->AddObserver(this);
 
-  extension_keybinding_registry_.reset(new ExtensionKeybindingRegistryViews(
-      browser->profile(),
-      owner_view->GetFocusManager(),
-      extensions::ExtensionKeybindingRegistry::ALL_EXTENSIONS,
-      this));
-
-  resize_animation_.reset(new gfx::SlideAnimation(this));
-  resize_area_ = new views::ResizeArea(this);
-  AddChildView(resize_area_);
-
-  chevron_ = new ChevronMenuButton(NULL, base::string16(), this, false);
-  chevron_->EnableCanvasFlippingForRTLUI(true);
-  chevron_->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_ACCNAME_EXTENSIONS_CHEVRON));
-  chevron_->SetVisible(false);
-  AddChildView(chevron_);
+  bool overflow_experiment =
+      extensions::FeatureSwitch::extension_action_redesign()->IsEnabled();
+  DCHECK(!handling_overflow() || overflow_experiment);
+
+  if (!handling_overflow()) {
+    extension_keybinding_registry_.reset(new ExtensionKeybindingRegistryViews(
+        browser->profile(),
+        owner_view->GetFocusManager(),
+        extensions::ExtensionKeybindingRegistry::ALL_EXTENSIONS,
+        this));
+
+    resize_animation_.reset(new gfx::SlideAnimation(this));
+    resize_area_ = new views::ResizeArea(this);
+    AddChildView(resize_area_);
+
+    // 'Main' mode doesn't need a chevron overflow when overflow is shown inside
+    // the hamburger menu.
+    if (!overflow_experiment) {
+      chevron_ = new ChevronMenuButton(NULL, base::string16(), this, false);
+      chevron_->SetBorder(views::Border::NullBorder());
+      chevron_->EnableCanvasFlippingForRTLUI(true);
+      chevron_->SetAccessibleName(
+          l10n_util::GetStringUTF16(IDS_ACCNAME_EXTENSIONS_CHEVRON));
+      chevron_->SetVisible(false);
+      AddChildView(chevron_);
+    }
+  }
 }
 
 BrowserActionsContainer::~BrowserActionsContainer() {
@@ -233,6 +255,10 @@ void BrowserActionsContainer::ExecuteExtensionCommand(
                                                  command.accelerator());
 }
 
+bool BrowserActionsContainer::ShownInsideMenu() const {
+  return handling_overflow();
+}
+
 void BrowserActionsContainer::AddObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.AddObserver(observer);
@@ -244,23 +270,36 @@ void BrowserActionsContainer::RemoveObserver(
 }
 
 gfx::Size BrowserActionsContainer::GetPreferredSize() const {
+  size_t our_icon_count = browser_action_views_.size() -
+      (handling_overflow() ? main_container_->VisibleBrowserActions() : 0);
+
+  // If there are no actions to show, or we are in overflow mode and the main
+  // container is already showing them all, then no further work is required.
+  if (browser_action_views_.empty() || our_icon_count == 0)

[Devlin, 2014/06/26 00:04:07]

I don't think it should be possible for browser_action_views_ to be empty and
*not* have |our_icon_count| be zero... prefer just
if (our_icon_count == 0) {
...

+    return gfx::Size();
+
+  if (handling_overflow()) {
+    return gfx::Size(
+        IconCountToWidth(kIconsPerMenuRow, false),
+        (std::max(((our_icon_count - 1) / kIconsPerMenuRow), 0U) + 1) *
+            IconHeight());
+  }
+
   // 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).
+  // 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));
-  // Height will be ignored by the ToolbarView.
-  return gfx::Size(preferred_width, 0);
+  return gfx::Size(preferred_width, IconHeight());
 }
 
 gfx::Size BrowserActionsContainer::GetMinimumSize() const {
   int min_width = std::min(MinimumNonemptyWidth(), IconCountToWidth(-1, false));
-  // Height will be ignored by the ToolbarView.
-  return gfx::Size(min_width, 0);
+  return gfx::Size(min_width, IconHeight());
 }
 
 void BrowserActionsContainer::Layout() {
@@ -270,11 +309,12 @@ void BrowserActionsContainer::Layout() {
   }
 
   SetVisible(true);
-  resize_area_->SetBounds(0, 0, kItemSpacing, height());
+  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_) {
+  if ((IconCountToWidth(-1, false) > max_x) && !suppress_chevron_ && chevron_) {
     chevron_->SetVisible(true);
     gfx::Size chevron_size(chevron_->GetPreferredSize());
     max_x -=
@@ -284,20 +324,39 @@ void BrowserActionsContainer::Layout() {
         0,
         chevron_size.width(),
         chevron_size.height());
-  } else {
-    chevron_->SetVisible(false);
+  } else if (chevron_) {
+      chevron_->SetVisible(false);

[Devlin, 2014/06/26 00:04:07]

nit: indentation

   }
 
   // Now draw the icons for the browser actions in the available space.
   int icon_width = IconWidth(false);
-  for (size_t i = 0; i < browser_action_views_.size(); ++i) {
-    BrowserActionView* view = browser_action_views_[i];
-    int x = ToolbarView::kStandardSpacing + (i * IconWidth(true));
-    if (x + icon_width <= max_x) {
-      view->SetBounds(x, 0, icon_width, height());
+  if (handling_overflow()) {
+    size_t our_icon_count =
+        browser_action_views_.size() - main_container_->VisibleBrowserActions();

[Devlin, 2014/06/26 00:04:07]

We rely on VisibleBrowserActions() here, even though it will be "set" for the
main container in this same method (since it just returns which browser actions
are visible, and that's modified here on line 356ish.  Is there a risk of a race
condition, in which we render incorrectly because we use an out-dated number of
visible actions?  (If so, would it only be for one frame?  I'm not familiar
enough with our graphics stack...)

[Finnur, 2014/06/26 12:00:44]

This code path only hits in the overflow container and it only checks the main
container (never the other way around). The overflow container is created when
the menu opens, at which point the main container has already been created. I
guess it is possible to be out-dated due to animations that are in progress
while the menu is firing up... It doesn't hurt to use my new function that takes
animations into account. I'll do that.

+    for (size_t i = 0; i < our_icon_count; ++i) {

[Devlin, 2014/06/26 00:04:07]

I personally think it's a bit clear that this is handling overflow as:
for (size_t i = main_container_->VisibleBrowserActions();
     i < browser_action_views_.size();
     ++i) {
  BrowserActionView* view = browser_action_views_[i];
  ...
}

But I don't feel strongly about it.

[Finnur, 2014/06/26 12:00:44]

Done.

+      BrowserActionView* view =
+          browser_action_views_[i + main_container_->VisibleBrowserActions()];
+      int x = ToolbarView::kStandardSpacing + i * IconWidth(true) -
+          ((i / kIconsPerMenuRow) * (IconWidth(true) * kIconsPerMenuRow));
+      gfx::Rect rect_bounds(
+          x,
+          IconHeight() *
+              (std::max((static_cast<int>(i) / kIconsPerMenuRow), 0)),

[Devlin, 2014/06/26 00:04:07]

I think this std::max() isn't needed, either?

[Finnur, 2014/06/26 12:00:44]

What is this, a crusade now? ;)

But yeah, I think you are right. Removed it.

+          icon_width,
+          IconHeight() + kPaddingForBadge);
+      view->SetBoundsRect(rect_bounds);
       view->SetVisible(true);

[Devlin, 2014/06/26 00:04:07]

We never SetVisible(false) for the overflow container - do we need to?

[Finnur, 2014/06/26 12:00:44]

The |view| here is the browser action. For the main container, we hide the
|view|s that overflow, but the overflow container is unbounded (it just grows
more menu lines as needed). We might need a scroll solution in the menu
eventually, but it is out of scope for now.

[Devlin, 2014/06/26 16:29:13]

Yeah, I was wondering if, since both BACs have a full set of browser actions,
the overflow container would have to ensure that any items that no longer
displayed (because they moved to the main bar) are, in fact, hidden (rather than
simply overlayed by another).  Basically, just something like:
for (int i = 0; i < main_container_->VisibleBrowserActions(); ++i) {
  // Ensure that any browser actions shown in the main view are hidden in
  // the overflow view.
  (*browser_action_views_[i])->SetVisible(false);
}

But I'll defer to your expertise for whether or not this is needed.

[Finnur, 2014/06/27 14:23:38]

Done.

-    } else {
-      view->SetVisible(false);
+    }
+  } else {
+    for (size_t i = 0; i < browser_action_views_.size(); ++i) {
+      BrowserActionView* view = browser_action_views_[i];
+      int x = ToolbarView::kStandardSpacing + (i * IconWidth(true));
+      if (x + icon_width <= max_x) {
+        view->SetBounds(x, 0, icon_width, IconHeight());
+        view->SetVisible(true);
+      } else {
+        view->SetVisible(false);
+      }
     }
   }
 }
@@ -556,8 +615,10 @@ void BrowserActionsContainer::OnBrowserActionExecuted(
 
 void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
   SetVisible(!browser_action_views_.empty());
-  owner_view_->Layout();
-  owner_view_->SchedulePaint();
+  if (owner_view_) {
+    owner_view_->Layout();
+    owner_view_->SchedulePaint();
+  }
 }
 
 extensions::ActiveTabPermissionGranter*
@@ -765,11 +826,12 @@ void BrowserActionsContainer::BrowserActionRemoved(const Extension* extension) {
       } else {
         // Either we went from overflow to no-overflow, or we shrunk the no-
         // overflow container by 1.  Either way the size changed, so animate.
-        chevron_->SetVisible(false);
+        if (chevron_)
+          chevron_->SetVisible(false);
         SaveDesiredSizeAndAnimate(gfx::Tween::EASE_OUT,
                                   browser_action_views_.size());
       }
-      return;
+      return;  // We have found the action to remove, bail out.
     }
   }
 }
@@ -811,6 +873,9 @@ void BrowserActionsContainer::HighlightModeChanged(bool is_highlighting) {
 
 void BrowserActionsContainer::LoadImages() {
   ui::ThemeProvider* tp = GetThemeProvider();
+  if (!tp || !chevron_)
+    return;
+
   chevron_->SetImage(views::Button::STATE_NORMAL,
                      *tp->GetImageSkiaNamed(IDR_BROWSER_ACTIONS_OVERFLOW));
 
@@ -819,12 +884,19 @@ void BrowserActionsContainer::LoadImages() {
 }
 
 void BrowserActionsContainer::SetContainerWidth() {
-  int visible_actions = model_->GetVisibleIconCount();
+  // The slave only draws the overflow (what isn't visible in the other
+  // container).
+  int visible_actions = handling_overflow() ? model_->toolbar_items().size() -
+                                              model_->GetVisibleIconCount()
+                                            : model_->GetVisibleIconCount();
   if (visible_actions < 0)  // All icons should be visible.
     visible_actions = model_->toolbar_items().size();
-  chevron_->SetVisible(
-    static_cast<size_t>(visible_actions) < model_->toolbar_items().size());
-  container_width_ = IconCountToWidth(visible_actions, chevron_->visible());
+  if (chevron_) {
+    chevron_->SetVisible(
+      static_cast<size_t>(visible_actions) < model_->toolbar_items().size());
+  }
+  container_width_ =
+      IconCountToWidth(visible_actions, chevron_ ? chevron_->visible() : false);
 }
 
 void BrowserActionsContainer::CloseOverflowMenu() {
@@ -868,7 +940,7 @@ int BrowserActionsContainer::IconCountToWidth(int icons,
     return ToolbarView::kStandardSpacing;
   int icons_size =
       (icons == 0) ? 0 : ((icons * IconWidth(true)) - kItemSpacing);
-  int chevron_size = display_chevron ?
+  int chevron_size = chevron_ && display_chevron ?
       (kChevronSpacing + chevron_->GetPreferredSize().width()) : 0;
   return ToolbarView::kStandardSpacing + icons_size + chevron_size +
       ToolbarView::kStandardSpacing;
@@ -882,8 +954,8 @@ size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
   // We need to reserve space for the resize area, chevron, and the spacing on
   // either side of the chevron.
   int available_space = pixels - ToolbarView::kStandardSpacing -
-      chevron_->GetPreferredSize().width() - kChevronSpacing -
-      ToolbarView::kStandardSpacing;
+      (chevron_ ? chevron_->GetPreferredSize().width() : 0) -
+      kChevronSpacing - ToolbarView::kStandardSpacing;
   // 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>(
@@ -891,8 +963,10 @@ size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
 }
 
 int BrowserActionsContainer::MinimumNonemptyWidth() const {
-  return ToolbarView::kStandardSpacing + kChevronSpacing +
-      chevron_->GetPreferredSize().width() + ToolbarView::kStandardSpacing;
+  return ToolbarView::kStandardSpacing +
+         (chevron_ ? (kChevronSpacing + chevron_->GetPreferredSize().width() +
+                      ToolbarView::kStandardSpacing)
+                   : 0);
 }
 
 void BrowserActionsContainer::SaveDesiredSizeAndAnimate(
@@ -908,7 +982,7 @@ void BrowserActionsContainer::SaveDesiredSizeAndAnimate(
     model_->SetVisibleIconCount(num_visible_icons);
   int target_size = IconCountToWidth(num_visible_icons,
       num_visible_icons < browser_action_views_.size());
-  if (!disable_animations_during_testing_) {
+  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();