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 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 {
  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* 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),
       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);
 

[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));
+  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_);
+    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_);
+    // 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() {
   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 is_slave();
+}
+
 void BrowserActionsContainer::AddObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void BrowserActionsContainer::RemoveObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 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() {
   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);
+    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);
+      }
     }
   }
 }
 
 bool BrowserActionsContainer::GetDropFormats(
     int* formats,
     std::set<OSExchangeData::CustomFormat>* custom_formats) {
   custom_formats->insert(BrowserActionDragData::GetBrowserActionCustomFormat());
 
   return true;
@@ skipping 238 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;

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

Obviously not this patch's code, but this return looks kind of... useless.

[Finnur, 2014/06/24 17:00:49]

No, we want only one action to get deleted (it is inside the for loop).

But I'll add a comment.

     }
   }
 }
 
 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();
-  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() {
   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;
 }