Make hiding an extension action cause it to go to the 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 300 matching lines @@
     BrowserActionsContainerObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void BrowserActionsContainer::RemoveObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 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.
   if (icon_count == 0)
     return gfx::Size();
 
   if (in_overflow_mode()) {
     // When in overflow, y is multiline, so the pixel count is IconHeight()
     // times the number of rows needed.
     return gfx::Size(
@@ skipping 284 matching lines @@
     OnBrowserActionVisibilityChanged();
     return;
   }
 
   // Up until now we've only been modifying the resize_amount, but now it is
   // time to set the container size to the size we have resized to, and then
   // animate to the nearest icon count size if necessary (which may be 0).
   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(
     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 150 matching lines @@
     DCHECK(browser_action_views_[i]->extension() != extension) <<
            "Asked to add a browser action view for an extension that already "
            "exists.";
   }
 #endif
   CloseOverflowMenu();
 
   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);
   BrowserActionView* view =
       new BrowserActionView(extension,
                             GetExtensionAction(*extension, profile_),
                             browser_,
                             this);
   browser_action_views_.insert(browser_action_views_.begin() + index, view);
   AddChildViewAt(view, index);
 
   // If we are still initializing the container, don't bother animating.
   if (!model_->extensions_initialized())
     return;
 
   // Enlarge the container if it was already at maximum size and we're not in
   // the middle of upgrading.
   if ((model_->GetVisibleIconCount() < 0) &&
       !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();
   }
 }
 
 void BrowserActionsContainer::ToolbarExtensionRemoved(
     const Extension* extension) {
   CloseOverflowMenu();
 
@@ skipping 14 matching lines @@
         // If we have more icons than we can show, then we must not be changing
         // the container size (since we either removed an icon from the main
         // area and one from the overflow list will have shifted in, or we
         // removed an entry directly from the overflow list).
         OnBrowserActionVisibilityChanged();
       } else {
         // Either we went from overflow to no-overflow, or we shrunk the no-
         // 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.
     }
   }
 }
 
 void BrowserActionsContainer::ToolbarExtensionMoved(const Extension* extension,
                                                     int index) {
   if (!ShouldDisplayBrowserAction(extension))
     return;
@@ skipping 30 matching lines @@
   if (view)
     view->UpdateState();
 }
 
 bool BrowserActionsContainer::ShowExtensionActionPopup(
     const Extension* extension) {
   return ShowPopupForExtension(extension, false, false);
 }
 
 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(
     bool is_highlighting) {
   // The visual highlighting is done in OnPaint(). It's a bit of a pain that
   // we delete and recreate everything here, but given everything else going on
   // (the lack of highlight, n more extensions appearing, etc), it's not worth
   // 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() {
   ui::ThemeProvider* tp = GetThemeProvider();
   if (!tp || !chevron_)
     return;
 
   chevron_->SetImage(views::Button::STATE_NORMAL,
                      *tp->GetImageSkiaNamed(IDR_BROWSER_ACTIONS_OVERFLOW));
 
   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();
   }
 }
 
 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());
   }
   container_width_ =
       IconCountToWidth(visible_actions, chevron_ && chevron_->visible());
 }
 
 void BrowserActionsContainer::CloseOverflowMenu() {
   if (overflow_menu_)
@@ skipping 56 matching lines @@
       std::max(0, available_space + kItemSpacing) / IconWidth(true));
 }
 
 int BrowserActionsContainer::MinimumNonemptyWidth() const {
   if (!chevron_)
     return ToolbarView::kStandardSpacing;
   return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
       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_) {
     // Animate! We have to set the animation_target_size_ after calling Reset(),
     // because that could end up calling AnimationEnded which clears the value.
     resize_animation_->Reset();
     resize_animation_->SetTweenType(tween_type);
     animation_target_size_ = target_size;
     resize_animation_->Show();
   } else {
@@ skipping 32 matching lines @@
 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;
 }
+
+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;
+}