Pop extensions out of the action overflow menu

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 107 matching lines @@
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionsContainer
 
 // static
 bool BrowserActionsContainer::disable_animations_during_testing_ = false;
 
 BrowserActionsContainer::BrowserActionsContainer(
     Browser* browser,
     View* owner_view,
     BrowserActionsContainer* main_container)
-    : profile_(browser->profile()),
+    : initialized_(false),
+      profile_(browser->profile()),
       browser_(browser),
       owner_view_(owner_view),
       main_container_(main_container),
       popup_owner_(NULL),
       model_(NULL),
       container_width_(0),
       resize_area_(NULL),
       chevron_(NULL),
       overflow_menu_(NULL),
       suppress_chevron_(false),
@@ skipping 50 matching lines @@
 
 void BrowserActionsContainer::Init() {
   LoadImages();
 
   // We wait to set the container width until now so that the chevron images
   // will be loaded.  The width calculation needs to know the chevron size.
   if (model_ && model_->extensions_initialized()) {
     container_width_ = GetPreferredWidth();
     SetChevronVisibility();
   }
+
+  initialized_ = true;
 }
 
 BrowserActionView* BrowserActionsContainer::GetViewForExtension(
     const Extension* extension) {
   for (BrowserActionViews::iterator view = browser_action_views_.begin();
        view != browser_action_views_.end(); ++view) {
     if ((*view)->extension() == extension)
       return *view;
   }
   return NULL;
@@ skipping 53 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());
 }
 
+void BrowserActionsContainer::NotifyActionMovedToOverflow() {
+  // When an action is moved to overflow, we shrink the size of the container
+  // by 1.
+  if (!profile_->IsOffTheRecord())
+    model_->SetVisibleIconCount(model_->GetVisibleIconCount() - 1);
+  Animate(gfx::Tween::EASE_OUT,
+          VisibleBrowserActionsAfterAnimation() - 1);
+}
+
 bool BrowserActionsContainer::ShownInsideMenu() const {
   return in_overflow_mode();
 }
 
 void BrowserActionsContainer::OnBrowserActionViewDragDone() {
-  // We notify here as well as in OnPerformDrop because the dragged view is
-  // removed in OnPerformDrop, so it will never get its OnDragDone() call.
-  // TODO(devlin): we should see about fixing that.
+  ToolbarVisibleCountChanged();
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
                     observers_,
                     OnBrowserActionDragDone());
 }
 
 views::View* BrowserActionsContainer::GetOverflowReferenceView() {
   // We should only need an overflow reference when using the traditional
   // chevron overflow.
   DCHECK(chevron_);
   return chevron_;
@@ skipping 127 matching lines @@
   return BrowserActionDragData::AreDropTypesRequired();
 }
 
 bool BrowserActionsContainer::CanDrop(const OSExchangeData& data) {
   return BrowserActionDragData::CanDrop(data, profile_);
 }
 
 int BrowserActionsContainer::OnDragUpdated(
     const ui::DropTargetEvent& event) {
   // First check if we are above the chevron (overflow) menu.
-  if (GetEventHandlerForPoint(event.location()) == chevron_) {
+  if (chevron_ && GetEventHandlerForPoint(event.location()) == chevron_) {
     if (!show_menu_task_factory_.HasWeakPtrs() && !overflow_menu_)
       StartShowFolderDropMenuTimer();
     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. The easiest way to do
@@ skipping 92 matching lines @@
   // correct when dragging an icon to the left. When dragging to the right,
   // however, we want the icon being dragged to get the index of the item to
   // the left of the drop indicator, so we subtract one.
   // * Well, it can also point to the end, but not when dragging to the left. :)
   if (i > data.index())
     --i;
 
   if (profile_->IsOffTheRecord())
     i = model_->IncognitoIndexToOriginal(i);
 
+  // If this was a drag between containers, we will have to adjust the number of
+  // visible icons.
+  bool drag_between_containers =
+      !browser_action_views_[data.index()]->visible();
   model_->MoveExtensionIcon(
       browser_action_views_[data.index()]->extension(), i);
 
+  if (drag_between_containers) {
+    // Add one for the dropped icon.
+    size_t new_icon_count = VisibleBrowserActionsAfterAnimation() + 1;
+
+    // Let the main container update the model.
+    if (in_overflow_mode())
+      main_container_->NotifyActionMovedToOverflow();
+    else if (!profile_->IsOffTheRecord())  // This is the main container.
+      model_->SetVisibleIconCount(model_->GetVisibleIconCount() + 1);
+
+    // The size changed, so we need to animate.
+    Animate(gfx::Tween::EASE_OUT, new_icon_count);
+  }
+
   OnDragExited();  // Perform clean up after dragging.
-  FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
-                    observers_,
-                    OnBrowserActionDragDone());
   return ui::DragDropTypes::DRAG_MOVE;
 }
 
 void BrowserActionsContainer::GetAccessibleState(
     ui::AXViewState* state) {
   state->role = ui::AX_ROLE_GROUP;
   state->name = l10n_util::GetStringUTF16(IDS_ACCNAME_EXTENSIONS);
 }
 
 void BrowserActionsContainer::OnMenuButtonClicked(views::View* source,
@@ skipping 61 matching lines @@
 
   // 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_);
   if (!profile_->IsOffTheRecord())
     model_->SetVisibleIconCount(visible_icons);
-
   Animate(gfx::Tween::EASE_OUT, visible_icons);
 }
 
 void BrowserActionsContainer::AnimationProgressed(
     const gfx::Animation* animation) {
   DCHECK_EQ(resize_animation_.get(), animation);
   resize_amount_ = static_cast<int>(resize_animation_->GetCurrentValue() *
       (container_width_ - animation_target_size_));
   OnBrowserActionVisibilityChanged();
 }
@@ skipping 25 matching lines @@
     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();
 }
 
 size_t BrowserActionsContainer::GetFirstVisibleIconIndex() const {
-  return in_overflow_mode() ? model_->GetVisibleIconCount() : 0;
+  return in_overflow_mode() ?
+      main_container_->VisibleBrowserActionsAfterAnimation() : 0;
 }
 
 ExtensionPopup* BrowserActionsContainer::TestGetPopup() {
   return popup_owner_ ? popup_owner_->view_controller()->popup() : NULL;
 }
 
 void BrowserActionsContainer::TestSetIconVisibilityCount(size_t icons) {
-  model_->SetVisibleIconCount(icons);
-  chevron_->SetVisible(icons < browser_action_views_.size());
-  container_width_ = IconCountToWidth(icons, chevron_->visible());
-  Layout();
-  SchedulePaint();
+  model_->SetVisibleIconCountForTest(icons);
 }
 
 void BrowserActionsContainer::OnPaint(gfx::Canvas* canvas) {
   // If the views haven't been initialized yet, wait for the next call to
   // paint (one will be triggered by entering highlight mode).
   if (model_->is_highlighting() && !browser_action_views_.empty()) {
     views::Painter::PaintPainterAt(
         canvas, highlight_painter_.get(), GetLocalBounds());
   }
 
@@ skipping 248 matching lines @@
 
   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());
 }
 
 void BrowserActionsContainer::SetChevronVisibility() {
-  if (chevron_)
-    chevron_->SetVisible(GetIconCount() < browser_action_views_.size());
+  if (chevron_) {
+    chevron_->SetVisible(
+        VisibleBrowserActionsAfterAnimation() < browser_action_views_.size());
+  }
 }
 
 void BrowserActionsContainer::CloseOverflowMenu() {
   if (overflow_menu_)
     overflow_menu_->CancelMenu();
 }
 
 void BrowserActionsContainer::StopShowFolderDropMenuTimer() {
   show_menu_task_factory_.InvalidateWeakPtrs();
 }
@@ skipping 32 matching lines @@
       (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_ ? chevron_->GetPreferredSize().width() : 0) -
-      kChevronSpacing - ToolbarView::kStandardSpacing;
+  // 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 +
@@ skipping 20 matching lines @@
 bool BrowserActionsContainer::ShouldDisplayBrowserAction(
     const Extension* extension) const {
   // Only display incognito-enabled extensions while in incognito mode.
   return !profile_->IsOffTheRecord() ||
       extensions::util::IsIncognitoEnabled(extension->id(), profile_);
 }
 
 size_t BrowserActionsContainer::GetIconCount() const {
   if (!model_)
     return 0u;
-  // Find the number of icons which could be displayed.
-  size_t displayable_icon_count = 0u;
+
   const extensions::ExtensionList& extensions = model_->toolbar_items();
-  for (extensions::ExtensionList::const_iterator iter = extensions.begin();
-       iter != extensions.end(); ++iter) {
-    displayable_icon_count += ShouldDisplayBrowserAction(iter->get()) ? 1u : 0u;
-  }
+
   // Find the absolute value for the model's visible count.
   int model_size = model_->GetVisibleIconCount();

[Finnur, 2014/09/12 10:30:24]

Maybe it is just too early in the morning for me, but I find this variable name
(and the one below) a little confusing. :)

This isn't really model_size, it is how many are visible, right?

Would visible_size and absolute_visible_size work for you? I think it would make
the code on line 1111 easier to understand.

[Devlin, 2014/09/12 14:01:30]

Well, it's actually how many are visible, according to the model (which, due to
incognito mode, could be different from how many are *actually* visible).  So
it's both. :)  I'll change the name when I get into the office.

[Finnur, 2014/09/12 15:29:52]

LGTM, with name change.

[Devlin, 2014/09/12 15:54:53]

Done.

   size_t absolute_model_size =
       model_size == -1 ? extensions.size() : model_size;
 
-  // The main container will try to show |model_size| icons, but reduce if there
-  // aren't enough displayable icons to do so.
-  size_t main_displayed = std::min(displayable_icon_count, absolute_model_size);
-  // The overflow will display the extras, if any.
+  // Find the number of icons which could be displayed.
+  size_t displayable_icon_count = 0u;
+  size_t main_displayed = 0u;
+  for (size_t i = 0; i < extensions.size(); ++i) {
+    // Should there be an icon for this extension at all?
+    if (ShouldDisplayBrowserAction(extensions[i].get())) {
+      ++displayable_icon_count;
+      // Should we display it on the main bar? If this is an incognito window,
+      // icons have the same overflow status they do in a regular window.
+      main_displayed += i < absolute_model_size ? 1u : 0u;
+    }
+  }
+
+  // If this is an existing (initialized) container from an incognito profile,
+  // we can't trust the model (because the incognito bars don't adjust model
+  // settings). Instead, we go off what we currently have displayed.
+  if (initialized_ && profile_->IsOffTheRecord()) {

[Finnur, 2014/09/12 10:30:24]

Out of curiosity, under what circumstances is |initialized_| false when we get
here? Also, when it is false, doesn't this function return some incorrect value?
Should we early return 0u if it is false, like we do with !model_?

[Devlin, 2014/09/12 14:01:30]

Initialized is false when we call this method from GetPreferredWidth() called
from Init() - and it's important to do that.  It actually returns the proper
value, because this is how we initialize the container.  So the behavior for
incognito mode windows is "When you open a new incognito window, display only
browser actions that were displayed in a regular window. After the window is
open and set up (init'd), trust what's there because it might have adjusted
without the model knowing."

+    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;
 }