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..9805bb56de3a3b0555ff9b375c3cc4a5c4dfb4ae 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 slave mode (inside the app menu, aka. hamburger menu).
+const int kPaddingForBadge = 9;
+
+// The maximum number of icons to show per row when in slave 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* master)
     : profile_(browser->profile()),
       browser_(browser),
       owner_view_(owner_view),
+      master_(master),
       popup_(NULL),
       popup_button_(NULL),
       model_(NULL),
       container_width_(0),
+      resize_area_(NULL),
       chevron_(NULL),
       overflow_menu_(NULL),
       suppress_chevron_(false),
@@ -117,22 +128,29 @@ BrowserActionsContainer::BrowserActionsContainer(Browser* browser,
   if (model_)
     model_->AddObserver(this);
 

[Devlin, 2014/06/23 17:55:29]

For a sanity check, can we do something like:
DCHECK(!is_slave() || FeatureSwitch::extension_action_redesign()->IsEnabled())?

(If so, maybe just make a bool for the switch since it'll be used on line 144.)

-  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_);
+  if (!is_slave()) {
+    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_);
+
+    // Master mode doesn't need a chevron overflow when overflow is shown inside
+    // the hamburger menu.
+    if (!extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
+      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 +251,10 @@ void BrowserActionsContainer::ExecuteExtensionCommand(
                                                  command.accelerator());
 }
 
+bool BrowserActionsContainer::ShownInsideMenu() const {
+  return is_slave();
+}
+
 void BrowserActionsContainer::AddObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.AddObserver(observer);
@@ -244,23 +266,36 @@ void BrowserActionsContainer::RemoveObserver(
 }
 
 gfx::Size BrowserActionsContainer::GetPreferredSize() const {
-  // 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));
-  // Height will be ignored by the ToolbarView.
-  return gfx::Size(preferred_width, 0);
+  size_t our_icon_count = browser_action_views_.size() -
+      (is_slave() ? master_->VisibleBrowserActions() : 0);

[Devlin, 2014/06/23 17:55:28]

I'm a little nervous about how master and slave each have a full set of browser
action icons.  At best, it seems like extra work, and at worst, it seems very
prone to error.  Unfortunately, unless we link the master and slave, it'd be
difficult to use any other approach.  What do you think?

[Finnur, 2014/06/25 16:18:11]

Yeah, I know. But I think there are complexities either way we do it.

+
+  // If there are no actions to show, or master is already showing them all,
+  // then no further work is required.
+  if (browser_action_views_.empty() || our_icon_count == 0)
+    return gfx::Size();
+
+  if (is_slave()) {
+    return gfx::Size(
+        IconCountToWidth(kIconsPerMenuRow, false),
+        (std::max(((our_icon_count - 1) / kIconsPerMenuRow), 0U) + 1) *

[Devlin, 2014/06/23 17:55:29]

I think you'll get a compile error here on pickier systems - need 0lu.
That said, do you actually need the std::max?  If our_icon_count == 0, we return
on line 275, so our_icon_count - 1 will be >= 0, and >= 0 / > 0 is >= 0 always,
right?

Also, it may be worth putting a small comment with the y-calculation.

[Finnur, 2014/06/25 16:18:11]

Good point.

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

I don't see a change for this...?

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

Somehow forgot to upload it. Should be done now.

+            IconHeight());
+  } else {

[Devlin, 2014/06/23 17:55:28]

nit: no need for the else after a return.

+    // 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));
+    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 +305,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 -=
@@ -285,19 +321,39 @@ void BrowserActionsContainer::Layout() {
         chevron_size.width(),
         chevron_size.height());
   } else {
-    chevron_->SetVisible(false);
+    if (chevron_)

[Devlin, 2014/06/23 17:55:29]

nit: else if (chevron_) {

+      chevron_->SetVisible(false);
   }
 
   // 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 (is_slave()) {
+    size_t our_icon_count =
+        browser_action_views_.size() - master_->VisibleBrowserActions();
+    for (size_t i = 0; i < our_icon_count; ++i) {
+      BrowserActionView* view =
+          browser_action_views_[i + master_->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)),
+          icon_width,
+          IconHeight() + kPaddingForBadge);
+      view->SetBoundsRect(rect_bounds);
       view->SetVisible(true);
-    } 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 +612,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,7 +823,8 @@ 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());
       }
@@ -811,20 +870,29 @@ void BrowserActionsContainer::HighlightModeChanged(bool is_highlighting) {
 
 void BrowserActionsContainer::LoadImages() {
   ui::ThemeProvider* tp = GetThemeProvider();
-  chevron_->SetImage(views::Button::STATE_NORMAL,
-                     *tp->GetImageSkiaNamed(IDR_BROWSER_ACTIONS_OVERFLOW));
+  if (tp && chevron_) {

[Devlin, 2014/06/23 17:55:29]

maybe instead:
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));
+    const int kImages[] = IMAGE_GRID(IDR_DEVELOPER_MODE_HIGHLIGHT);
+    highlight_painter_.reset(views::Painter::CreateImageGridPainter(kImages));
+  }
 }
 
 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 = is_slave() ? 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 +936,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 +950,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 +959,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 +978,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();