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 256 matching lines @@
   DCHECK(!command.global());
   extension_keybinding_registry_->ExecuteCommand(extension->id(),
                                                  command.accelerator());
 }
 
 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);
 
+  // Check if the size of the toolbar needs to be adjusted; this happens when
+  // dragging from overflow to main or vice versa. We can check if this happened
+  // by seeing if the view is visible - if it's not, then the drag originated
+  // from the other container.
+  int size_modifier = 0;
+  if (!browser_action_views_[data.index()]->visible()) {
+    if (i > data.index()) {

[Finnur, 2014/09/10 09:48:01]

I understand why dragging an icon to the left subtracts 1 from the index |i|,
but it isn't immediately apparent why dragging left/right would modify the
|size_modifier|. I think you are referring to container -> overflow as right and
overflow -> container as left. It would probably be clearer if you stated that
instead of left and right. It would also make the comment correct for RTL.

But lets take a step back. This size_modifier code is a bit hard to grok. And it
seems to me like |size_modifier| is redundant. We only modify it when dragging
between containers and it seems to me like it can in all cases be set to
in_overflow_mode() ? -1 : 1

Right? If so, I think we can delete a block of code (before MoveExtensionIcon)
if we simply do:

bool drag_between_containers = !browser_action_views_[data.index()]->visible());
model_->MoveExtensionIcon(
    browser_action_views_[data.index()]->extension(), i);

if (drag_between_containers) {
  // Do what you do inside if(size_modifier) but use:
  // |in_overflow_mode() ? -1 : 1| instead of size_modifier when 
  // setting the visible count.
}

[Devlin, 2014/09/10 15:58:30]

Wow.  Yeah, that's easier.  Shoulda seen that... thanks.

+      // Dragging to the right, so hiding one.
+      size_modifier = -1;
+    } else if (i < data.index()) {
+      // Dragging to the left, so showing one.
+      size_modifier = 1;
+    } else {  // i == data.index()
+      // Edge case: dragging to the same index, but in the other menu.
+      // If this was *dropped* in the overflow container, then we're hiding one,
+      // otherwise we're showing one.
+      size_modifier = in_overflow_mode() ? -1 : 1;
+    }
+  }
+
   model_->MoveExtensionIcon(
       browser_action_views_[data.index()]->extension(), i);
 
+  // Adjust the size of the container, if needed.
+  if (size_modifier) {
+    // Let the main container update the model.
+    if (in_overflow_mode()) {
+      main_container_->NotifyActionMovedToOverflow();
+    } else if (!profile_->IsOffTheRecord()) {
+      model_->SetVisibleIconCount(
+          model_->GetVisibleIconCount() + size_modifier);
+    }
+
+    // The size changed, so we need to animate.
+    Animate(gfx::Tween::EASE_OUT, GetIconCount());
+  }
+
   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 10 matching lines @@
                     observers_,
                     OnBrowserActionsContainerAnimationEnded());
 }
 
 void BrowserActionsContainer::NotifyMenuDeleted(
     BrowserActionOverflowMenuController* controller) {
   DCHECK_EQ(overflow_menu_, controller);
   overflow_menu_ = NULL;
 }
 
+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);
+}
+
 content::WebContents* BrowserActionsContainer::GetCurrentWebContents() {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
 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();
 }
 
 size_t BrowserActionsContainer::GetFirstVisibleIconIndex() const {
   return in_overflow_mode() ? model_->GetVisibleIconCount() : 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 313 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 39 matching lines @@
   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.
   return in_overflow_mode() ?
       displayable_icon_count - main_displayed : main_displayed;
 }