Pop extensions out of the action overflow menu

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 5ed94f3e2f00e37eb91ffd966dbf4b2c33d79a12..2c03a7e1497e62ed44230c093bc88a2def0a88ff 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -269,14 +269,21 @@ void BrowserActionsContainer::ExecuteExtensionCommand(
                                                  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());
@@ -424,7 +431,7 @@ bool BrowserActionsContainer::CanDrop(const OSExchangeData& data) {
 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;
@@ -537,13 +544,25 @@ int BrowserActionsContainer::OnPerformDrop(
   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) {
+    // Let the main container update the model.
+    if (in_overflow_mode())
+      main_container_->NotifyActionMovedToOverflow();
+    else if (!profile_->IsOffTheRecord())  // This is the main container.

[Finnur, 2014/09/11 09:23:48]

What happens if you drop an icon onto the main container in incognito? Does the
size increase by one?

[Devlin, 2014/09/11 17:46:05]

tl;dr: All fixed. :)

As it turns out, this particular code was fine - but there was a bug in the
GetIconCount() code for incognito in general, because it did all its
calculations based off the model.  Additionally, the behavior was a little
unpredictable to begin with.

Per offline chat, I tweaked the behavior of incognito window bars to make any
actions which were initially hidden, remain hidden.  Now the behavior is
correct.
- A new incognito window will have only visible actions for those extensions
which are visible in the main window (and have incognito privileges).
- Changing incognito visible action count doesn't modify regular visible action
count.
- Popping in/out in incognito works as expected.

+      model_->SetVisibleIconCount(model_->GetVisibleIconCount() + 1);
+
+    // 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;
 }
 
@@ -625,7 +644,6 @@ void BrowserActionsContainer::OnResize(int resize_amount, bool done_resizing) {
   int visible_icons = WidthToIconCount(container_width_);
   if (!profile_->IsOffTheRecord())
     model_->SetVisibleIconCount(visible_icons);
-
   Animate(gfx::Tween::EASE_OUT, visible_icons);
 }
 
@@ -679,11 +697,7 @@ ExtensionPopup* BrowserActionsContainer::TestGetPopup() {
 }
 
 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) {
@@ -952,6 +966,11 @@ void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
   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();
   }
 }
 
@@ -1017,11 +1036,12 @@ size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
   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>(