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..7d2533c6904216e46f98c852a3ab0ceb24bef52c 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -125,7 +125,8 @@ 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),
@@ -196,6 +197,8 @@ void BrowserActionsContainer::Init() {
     container_width_ = GetPreferredWidth();
     SetChevronVisibility();
   }
+
+  initialized_ = true;
 }
 
 BrowserActionView* BrowserActionsContainer::GetViewForExtension(
@@ -269,14 +272,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 +434,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 +547,28 @@ 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) {
+    // 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;
 }
 
@@ -625,7 +650,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);
 }
 
@@ -671,7 +695,8 @@ extensions::ActiveTabPermissionGranter*
 }
 
 size_t BrowserActionsContainer::GetFirstVisibleIconIndex() const {
-  return in_overflow_mode() ? model_->GetVisibleIconCount() : 0;
+  return in_overflow_mode() ?
+      main_container_->VisibleBrowserActionsAfterAnimation() : 0;
 }
 
 ExtensionPopup* BrowserActionsContainer::TestGetPopup() {
@@ -679,11 +704,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 +973,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();
   }
 }
 
@@ -963,8 +989,10 @@ int BrowserActionsContainer::GetPreferredWidth() {
 }
 
 void BrowserActionsContainer::SetChevronVisibility() {
-  if (chevron_)
-    chevron_->SetVisible(GetIconCount() < browser_action_views_.size());
+  if (chevron_) {
+    chevron_->SetVisible(
+        VisibleBrowserActionsAfterAnimation() < browser_action_views_.size());
+  }
 }
 
 void BrowserActionsContainer::CloseOverflowMenu() {
@@ -1017,11 +1045,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>(
@@ -1062,22 +1091,37 @@ bool BrowserActionsContainer::ShouldDisplayBrowserAction(
 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;
 }