Make hiding an extension action cause it to go to the 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 92fbd3e716e193445e8d0cf209c64e5fb8bb7197..de9c60508ce18c6ea05c14e4a22d106997860565 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -318,8 +318,7 @@ void BrowserActionsContainer::RemoveObserver(
 }
 
 gfx::Size BrowserActionsContainer::GetPreferredSize() const {
-  size_t icon_count = browser_action_views_.size() -
-      (in_overflow_mode() ? main_container_->VisibleBrowserActions() : 0);
+  size_t icon_count = GetIconCount();
 
   // If there are no actions to show, or we are in overflow mode and the main
   // container is already showing them all, then no further work is required.
@@ -624,8 +623,18 @@ void BrowserActionsContainer::OnResize(int resize_amount, bool done_resizing) {
   int max_width = IconCountToWidth(-1, false);
   container_width_ =
       std::min(std::max(0, container_width_ - resize_amount), max_width);
-  SaveDesiredSizeAndAnimate(gfx::Tween::EASE_OUT,
-                            WidthToIconCount(container_width_));
+
+  // 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(
@@ -796,8 +805,6 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
   if (!ShouldDisplayBrowserAction(extension))
     return;
 
-  size_t visible_actions = VisibleBrowserActionsAfterAnimation();
-
   // Add the new browser action to the vector and the view hierarchy.
   if (profile_->IsOffTheRecord())
     index = model_->OriginalIndexToIncognito(index);
@@ -819,7 +826,7 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
       !extensions::ExtensionSystem::Get(profile_)->runtime_data()->
           IsBeingUpgraded(extension)) {
     suppress_chevron_ = true;
-    SaveDesiredSizeAndAnimate(gfx::Tween::LINEAR, visible_actions + 1);
+    Animate(gfx::Tween::LINEAR, browser_action_views_.size());
   } else {
     // Just redraw the (possibly modified) visible icon set.
     OnBrowserActionVisibilityChanged();
@@ -854,8 +861,7 @@ void BrowserActionsContainer::ToolbarExtensionRemoved(
         // overflow container by 1.  Either way the size changed, so animate.
         if (chevron_)
           chevron_->SetVisible(false);
-        SaveDesiredSizeAndAnimate(gfx::Tween::EASE_OUT,
-                                  browser_action_views_.size());
+        Animate(gfx::Tween::EASE_OUT, browser_action_views_.size());

[Finnur, 2014/08/21 10:47:51]

I can't say for sure if this is correct. I suspect it is not (and same goes for
ToolbarExtensionAdded). Remember that extensions can be added and removed in the
background and we need to think about what happens if, e.g. an extension is
killed by blacklisting just moments before the user shuts down the browser. Will
the container have time to save what the right value should be before shutting
down? If not, won't the container on next startup have (in this case) room for
one more extension than intended and one fewer in the ExtensionAdded function?

[Devlin, 2014/08/21 13:56:30]

Sorry; I didn't explain very well.

I'm fairly certain that the only time we actually *want* to save the desired
size from the view itself is when the view has been resized.  All these other
times, we actually just pass in the same size as the model already has. 
Specifically:
ToolbarExtensionAdded():
  We add an extension to |browser_action_views_|, increasing the size of the
vector by 1.  If we're not showing all, we're done.  If we are showing all, pass
in the new size (by definition, all) to SaveDesiredSizeAndAnimate().  This means
that we are setting the ToolbarSize to be all visible (which it already is).
ToolbarExtensionRemoved():
  Works very similarly - remove the view (reducing vector size by 1).  If
showing all views, update the size (but this will still show all views).
ToolbarHighlightModeChanged():
  Set the new size to reflect the new current number of visible icons.

In all of these cases, we actually just pass in the "current number of shown
icons" to SaveDesiredSizeAndAnimate(), which would then update the Model.  Of
course, updating from visible size -1 to size -1 doesn't do anything (or from
e.g., 3 to 3 in highlight mode).

More abstractly, ToolbarExtensionAdded/Removed and HighlightModeChanged are all
directly called by the toolbar model, and the model itself has already updated
the visible size.  Calling back into Model to set the visible size is odd at
best, and potentially buggy at worst (if somehow overflow mode highlighted, we
would have a bug with the overflow mode setting the size in the model).

I think the reason we previously did this is because we used to have the
different pref for height, which was actually set by the view itself (and the
model didn't store the pref), meaning that this would be necessary.  Since the
model now controls it, we don't need to in these cases.

Okay, lengthy explanation over with.  Does it make sense?  Are there points I'm
missing?

[Finnur, 2014/08/21 14:08:30]

No, I would agree that there's no need to save the same value over and over
again. 
Do you mean |width|?

[Devlin, 2014/08/21 15:24:59]

Ah yes, sorry, width. :) (that should teach me to try and make sense early in
the morning :P)

       }
       return;  // We have found the action to remove, bail out.
     }
@@ -906,7 +912,10 @@ bool BrowserActionsContainer::ShowExtensionActionPopup(
 }
 
 void BrowserActionsContainer::ToolbarVisibleCountChanged() {
+  int old_container_width = container_width_;
   SetContainerWidth();
+  if (old_container_width != container_width_)
+    Animate(gfx::Tween::EASE_OUT, GetIconCount());

[Finnur, 2014/08/21 10:47:51]

I am unclear on this one too...

[Devlin, 2014/08/21 13:56:30]

Here, we just say "Animate to the number of icons that should be visible
according to the model."  Thinking about this carefully, it will actually be
wrong in incognito mode - which means that GetIconCount should probably discount
any non-incognito extensions while in incognito mode.

 }
 
 void BrowserActionsContainer::ToolbarHighlightModeChanged(
@@ -917,7 +926,7 @@ void BrowserActionsContainer::ToolbarHighlightModeChanged(
   // the extra complexity to create and insert only the new extensions.
   DeleteBrowserActionViews();
   CreateBrowserActionViews();
-  SaveDesiredSizeAndAnimate(gfx::Tween::LINEAR, browser_action_views_.size());
+  Animate(gfx::Tween::LINEAR, browser_action_views_.size());

[Finnur, 2014/08/21 10:47:50]

This is probably a good change, since if the browser crashes while the
highlighting is up then we won't have saved the visible count while
highlighting.

 }
 
 void BrowserActionsContainer::LoadImages() {
@@ -941,13 +950,7 @@ void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
 }
 
 void BrowserActionsContainer::SetContainerWidth() {
-  // The slave only draws the overflow (what isn't visible in the other
-  // container).
-  int visible_actions = in_overflow_mode() ?
-      model_->toolbar_items().size() - model_->GetVisibleIconCount() :
-      model_->GetVisibleIconCount();
-  if (visible_actions < 0)  // All icons should be visible.
-    visible_actions = model_->toolbar_items().size();
+  int visible_actions = GetIconCount();
   if (chevron_) {
     chevron_->SetVisible(
       static_cast<size_t>(visible_actions) < model_->toolbar_items().size());
@@ -1024,17 +1027,8 @@ int BrowserActionsContainer::MinimumNonemptyWidth() const {
       chevron_->GetPreferredSize().width();
 }
 
-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);

[Devlin, 2014/08/21 00:06:29]

Had to move this to OnResize because otherwise the overflow container was trying
to set the model size.  But I don't think we should do this any other time
(besides OnResize), right?  Animate() is also called from ExtensionAdded/Removed
and highlight mode changed, but in all those cases, the model triggers the
change, so it's already up-to-date.

Sound right?

+void BrowserActionsContainer::Animate(gfx::Tween::Type tween_type,
+                                      size_t num_visible_icons) {
   int target_size = IconCountToWidth(num_visible_icons,
       num_visible_icons < browser_action_views_.size());
   if (resize_animation_ && !disable_animations_during_testing_) {
@@ -1087,3 +1081,11 @@ BrowserActionView* BrowserActionsContainer::GetViewForExtension(
 
   return NULL;
 }
+
+size_t BrowserActionsContainer::GetIconCount() const {
+  int visible_icons = model_->GetVisibleIconCount();
+  if (visible_icons == -1)
+    visible_icons = model_->toolbar_items().size();
+  return in_overflow_mode() ?
+      model_->toolbar_items().size() - visible_icons : visible_icons;
+}