Extension Toolbar redesign, part 1 (overflow)

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/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/extension_view_host.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/extensions/browser_action_drag_data.h"
 #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 #include "chrome/browser/ui/views/extensions/extension_popup.h"
 #include "chrome/browser/ui/views/toolbar/browser_action_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/common/extensions/command.h"
 #include "chrome/common/pref_names.h"
 #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"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/dragdrop/drag_utils.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/nine_image_painter_factory.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
@@ skipping 10 matching lines @@
 
 namespace {
 
 // Horizontal spacing between most items in the container, as well as after the
 // last item or chevron (if visible).
 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 {
  public:
   ChevronMenuButton(views::ButtonListener* listener,
                     const base::string16& text,
                     views::MenuButtonListener* menu_button_listener,
                     bool show_menu_marker)
       : views::MenuButton(listener,
                           text,
@@ skipping 17 matching lines @@
 };
 
 }  // namespace
 
 // static
 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),
       resize_amount_(0),
       animation_target_size_(0),
       drop_indicator_position_(-1),
       task_factory_(this),
       show_menu_task_factory_(this) {
   set_id(VIEW_ID_BROWSER_ACTION_TOOLBAR);
 
   model_ = extensions::ExtensionToolbarModel::Get(browser->profile());
   if (model_)
     model_->AddObserver(this);
 
-  extension_keybinding_registry_.reset(new ExtensionKeybindingRegistryViews(
-      browser->profile(),
-      owner_view->GetFocusManager(),
-      extensions::ExtensionKeybindingRegistry::ALL_EXTENSIONS,
-      this));
+  bool overflow_experiment =
+      extensions::FeatureSwitch::extension_action_redesign()->IsEnabled();
+  DCHECK(!handling_overflow() || overflow_experiment);
 
-  resize_animation_.reset(new gfx::SlideAnimation(this));
-  resize_area_ = new views::ResizeArea(this);
-  AddChildView(resize_area_);
+  if (!handling_overflow()) {
+    extension_keybinding_registry_.reset(new ExtensionKeybindingRegistryViews(
+        browser->profile(),
+        owner_view->GetFocusManager(),
+        extensions::ExtensionKeybindingRegistry::ALL_EXTENSIONS,
+        this));
 
-  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_);
+    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() {
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
                     observers_,
                     OnBrowserActionsContainerDestroyed());
 
   if (overflow_menu_)
     overflow_menu_->set_observer(NULL);
   if (model_)
@@ skipping 80 matching lines @@
 void BrowserActionsContainer::ExecuteExtensionCommand(
     const extensions::Extension* extension,
     const extensions::Command& command) {
   // Global commands are handled by the ExtensionCommandsGlobalRegistry
   // instance.
   DCHECK(!command.global());
   extension_keybinding_registry_->ExecuteCommand(extension->id(),
                                                  command.accelerator());
 }
 
+bool BrowserActionsContainer::ShownInsideMenu() const {
+  return handling_overflow();
+}
+
 void BrowserActionsContainer::AddObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void BrowserActionsContainer::RemoveObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 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() {
   if (browser_action_views_.empty()) {
     SetVisible(false);
     return;
   }
 
   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 -=
         ToolbarView::kStandardSpacing + chevron_size.width() + kChevronSpacing;
     chevron_->SetBounds(
         width() - ToolbarView::kStandardSpacing - chevron_size.width(),
         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);
+      }
     }
   }
 }
 
 bool BrowserActionsContainer::GetDropFormats(
     int* formats,
     std::set<OSExchangeData::CustomFormat>* custom_formats) {
   custom_formats->insert(BrowserActionDragData::GetBrowserActionCustomFormat());
 
   return true;
@@ skipping 21 matching lines @@
     return ui::DragDropTypes::DRAG_MOVE;
   }
   StopShowFolderDropMenuTimer();
 
   // Figure out where to display the indicator.  This is a complex calculation:
 
   // First, we figure out how much space is to the left of the icon area, so we
   // can calculate the true offset into the icon area.
   int width_before_icons = ToolbarView::kStandardSpacing +
       (base::i18n::IsRTL() ?
           (chevron_->GetPreferredSize().width() + kChevronSpacing) : 0);

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

Is this not a seg fault risk because the wrench menu isn't a drag target yet?

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

Correct. I'll fix this.

   int offset_into_icon_area = event.x() - width_before_icons;
 
   // Next, we determine which icon to place the indicator in front of.  We want
   // to place the indicator in front of icon n when the cursor is between the
   // midpoints of icons (n - 1) and n.  To do this we take the offset into the
   // icon area and transform it as follows:
   //
   // Real icon area:
   //   0   a     *  b        c
   //   |   |        |        |
@@ skipping 196 matching lines @@
   return SessionTabHelper::FromWebContents(active_tab)->session_id().id();
 }
 
 void BrowserActionsContainer::OnBrowserActionExecuted(
     BrowserActionButton* button) {
   ShowPopup(button, ExtensionPopup::SHOW, true);
 }
 
 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*
     BrowserActionsContainer::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents =
       browser_->tab_strip_model()->GetActiveWebContents();
   if (!web_contents)
     return NULL;
   return extensions::TabHelper::FromWebContents(web_contents)->
       active_tab_permission_granter();
@@ skipping 187 matching lines @@
 
       if (browser_action_views_.size() > visible_actions) {
         // If we have more icons than we can show, then we must not be changing
         // the container size (since we either removed an icon from the main
         // area and one from the overflow list will have shifted in, or we
         // removed an entry directly from the overflow list).
         OnBrowserActionVisibilityChanged();
       } 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.
     }
   }
 }
 
 void BrowserActionsContainer::BrowserActionMoved(const Extension* extension,
                                                  int index) {
   if (!ShouldDisplayBrowserAction(extension))
     return;
 
   if (profile_->IsOffTheRecord())
@@ skipping 21 matching lines @@
   // we delete and recreate everything here, but that's how it's done in
   // BrowserActionMoved(), too. If we want to optimize it, we could move the
   // existing icons, instead of deleting it all.
   DeleteBrowserActionViews();
   CreateBrowserActionViews();
   SaveDesiredSizeAndAnimate(gfx::Tween::LINEAR, browser_action_views_.size());
 }
 
 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));
 }
 
 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() {
   if (overflow_menu_)
     overflow_menu_->CancelMenu();
 }
 
 void BrowserActionsContainer::StopShowFolderDropMenuTimer() {
   show_menu_task_factory_.InvalidateWeakPtrs();
 }
@@ skipping 23 matching lines @@
 }
 
 int BrowserActionsContainer::IconCountToWidth(int icons,
                                               bool display_chevron) const {
   if (icons < 0)
     icons = browser_action_views_.size();
   if ((icons == 0) && !display_chevron)
     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;
 }
 
 size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
   // Check for widths large enough to show the entire icon set.
   if (pixels >= IconCountToWidth(-1, false))
     return browser_action_views_.size();
 
   // 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>(
       std::max(0, available_space + kItemSpacing) / IconWidth(true));
 }
 
 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(
     gfx::Tween::Type tween_type,
     size_t num_visible_icons) {
   // 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.
   if (!profile_->IsOffTheRecord())
     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();
     resize_animation_->SetTweenType(tween_type);
     animation_target_size_ = target_size;
     resize_animation_->Show();
   } else {
     animation_target_size_ = target_size;
     AnimationEnded(resize_animation_.get());
   }
@@ skipping 36 matching lines @@
                                      views::BubbleBorder::TOP_RIGHT,
                                      show_action);
   popup_->GetWidget()->AddObserver(this);
   popup_button_ = button;
 
   // Only set button as pushed if it was triggered by a user click.
   if (should_grant)
     popup_button_->SetButtonPushed();
   return true;
 }