Make extensions that desire to act pop out if in overflow

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 080617f30596321e2b2e1cb491090b3e50b5cf2a..fdc26771345ed005ce60812f01e4337db1360466 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -60,7 +60,8 @@ ScopedVector<ToolbarActionViewController> GetToolbarActions(
   // Extension actions come first.
   extensions::ExtensionActionManager* action_manager =
       extensions::ExtensionActionManager::Get(browser->profile());
-  const extensions::ExtensionList& toolbar_items = model->toolbar_items();
+  const extensions::ExtensionList& toolbar_items = model->GetItemOrderForTab(
+      browser->tab_strip_model()->GetActiveWebContents());
   for (const scoped_refptr<const Extension>& extension : toolbar_items) {
     actions.push_back(new ExtensionActionViewController(
         extension.get(),
@@ -125,6 +126,8 @@ BrowserActionsContainer::BrowserActionsContainer(
       resize_area_(NULL),
       chevron_(NULL),
       suppress_chevron_(false),
+      suppress_animation_(false),
+      suppress_layout_(false),
       resize_amount_(0),
       animation_target_size_(0) {
   set_id(VIEW_ID_BROWSER_ACTION_TOOLBAR);
@@ -194,8 +197,23 @@ BrowserActionView* BrowserActionsContainer::GetViewForExtension(
 }
 
 void BrowserActionsContainer::RefreshBrowserActionViews() {
+  if (browser_action_views_.empty())
+    return;  // Nothing to do.
+
+  // When we do a bulk-refresh of views (such as when we switch tabs), we don't
+  // animate the difference. We only animate when it's a change driven by the
+  // action.
+  suppress_animation_ = true;

[Peter Kasting, 2014/10/30 22:06:00]

Nit: Use base::AutoReset to be safe against recursion and reduce the chance of
future maintainers accidentally bypassing your "set the variable back"
statements:

  ...
  base::AutoReset<bool> animation_resetter(&suppress_animation_, true);
  {
    base::AutoReset<bool> layout_resetter(&suppress_layout_, true);
    for (BrowserActionView* view : browser_action_views_)
      view->UpdateState();
  }
  ReorderViews();
}

[Devlin, 2014/10/31 17:44:09]

Done.

+  // Don't layout until the end.
+  suppress_layout_ = true;
+
   for (BrowserActionView* view : browser_action_views_)
     view->UpdateState();
+
+  suppress_layout_ = false;
+  ReorderViews();  // Also triggers a layout.
+
+  suppress_animation_ = false;
 }
 
 void BrowserActionsContainer::CreateBrowserActionViews() {
@@ -203,6 +221,9 @@ void BrowserActionsContainer::CreateBrowserActionViews() {
   if (!model_)
     return;
 
+  // We don't Layout while creating views. Instead, Layout() once at the end.
+  suppress_layout_ = true;

[Peter Kasting, 2014/10/30 22:06:00]

Nit: AutoReset here too, perhaps

[Devlin, 2014/10/31 17:44:09]

Done.

+
   ScopedVector<ToolbarActionViewController> actions =
       GetToolbarActions(model_, browser_);
   for (ToolbarActionViewController* controller : actions) {
@@ -212,6 +233,11 @@ void BrowserActionsContainer::CreateBrowserActionViews() {
     AddChildView(view);
   }
   actions.weak_clear();
+
+  suppress_layout_ = false;
+
+  Layout();
+  SchedulePaint();
 }
 
 void BrowserActionsContainer::DeleteBrowserActionViews() {
@@ -362,6 +388,9 @@ void BrowserActionsContainer::Layout() {
     return;
   }
 
+  if (suppress_layout_)

[Peter Kasting, 2014/10/30 22:05:59]

Nit: I think this should happen at the top of the function.  Otherwise, calling
Layout() with |suppress_layout_| set will have a functional effect when we go
from some views to no views, but not when we go from no views to some views.

[Devlin, 2014/10/31 17:44:09]

Done.

+    return;
+
   SetVisible(true);
   if (resize_area_)
     resize_area_->SetBounds(0, 0, kItemSpacing, height());
@@ -637,7 +666,7 @@ void BrowserActionsContainer::AnimationEnded(const gfx::Animation* animation) {
                     OnBrowserActionsContainerAnimationEnded());
 }
 
-content::WebContents* BrowserActionsContainer::GetCurrentWebContents() {
+content::WebContents* BrowserActionsContainer::GetCurrentWebContents() const {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
@@ -880,8 +909,15 @@ bool BrowserActionsContainer::ShowExtensionActionPopup(
 }
 
 void BrowserActionsContainer::ToolbarVisibleCountChanged() {
-  if (GetPreferredWidth() != container_width_)
+  if (GetPreferredWidth() != container_width_) {
     Animate(gfx::Tween::EASE_OUT, GetIconCount());
+  } else if (animation_target_size_ != 0) {
+    // It's possible that we're right where we're supposed to be in terms of
+    // icon count, but that we're also currently resizing. If this is the case,
+    // end the current animation with the current width.
+    animation_target_size_ = container_width_;
+    resize_animation_->Reset();
+  }
 }
 
 void BrowserActionsContainer::ToolbarHighlightModeChanged(
@@ -895,6 +931,12 @@ void BrowserActionsContainer::ToolbarHighlightModeChanged(
   Animate(gfx::Tween::LINEAR, GetIconCount());
 }
 
+void BrowserActionsContainer::ToolbarReorderNecessary(
+    content::WebContents* web_contents) {
+  if (GetCurrentWebContents() == web_contents)
+    ReorderViews();
+}
+
 Browser* BrowserActionsContainer::GetBrowser() {
   return browser_;
 }
@@ -980,7 +1022,8 @@ int BrowserActionsContainer::MinimumNonemptyWidth() const {
 void BrowserActionsContainer::Animate(gfx::Tween::Type tween_type,
                                       size_t num_visible_icons) {
   int target_size = IconCountToWidth(num_visible_icons);
-  if (resize_animation_ && !disable_animations_during_testing_) {
+  if (resize_animation_ && !disable_animations_during_testing_ &&
+      !suppress_animation_) {
     // 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();
@@ -993,12 +1036,40 @@ void BrowserActionsContainer::Animate(gfx::Tween::Type tween_type,
   }
 }
 
+void BrowserActionsContainer::ReorderViews() {
+  extensions::ExtensionList new_order =
+      model_->GetItemOrderForTab(GetCurrentWebContents());
+  if (new_order.empty())
+    return;  // Nothing to do.
+
+  // Run through the views and compare them to the desired order. If something
+  // is out of place, find the correct spot for it.
+  for (size_t i = 0; i < new_order.size() - 1; ++i) {
+    if (new_order[i]->id() !=
+            browser_action_views_[i]->view_controller()->GetId()) {
+      // Find where the correct view is (it's guaranteed to be after our current
+      // index, since everything up to this point is correct).

[Peter Kasting, 2014/10/30 22:06:00]

Is it conceivable that the two sets of views are not in-sync?  If so, the code
below would run off the end of the arrays in question.

I assume not, but it certainly made me take a closer look at the loop.

[Devlin, 2014/10/31 17:44:09]

Hmm...I don't think so.  Views are added to the container synchronously once
they are added to the model, so we should be good.

That said, it doesn't hurt to DCHECK it.

+      size_t j = i + 1;
+      while (new_order[i]->id() !=
+                 browser_action_views_[j]->view_controller()->GetId())
+        ++j;
+      std::swap(browser_action_views_[i], browser_action_views_[j]);

[Peter Kasting, 2014/10/30 22:06:00]

I wonder if std::rotate() would make more sense than std::swap().  rotate()
could be more performant, or it could make no sense at all, depending on what
kinds of model changes you have to account for.

In general, if you're only needing to account for extensions popping themselves
out of overflow, you could conceivably change the API so that you call once for
each single such extension, or maybe call with a vector of the extension IDs
that are popping out of overflow, or some such.  Then that could be implemented
here using rotate().  The "something changed" API you have designed in this CL
is more general, but it's also less efficient if you don't need that generality.

[Devlin, 2014/10/31 17:44:09]

The reason not to do that is because ReorderViews() is also called when we
switch tabs, in which case neither the model itself nor its underlying items
change, so we can't call it with the specific extension(s) that update. 
Theoretically, we could have the model call back into this with the necessary
changes whenever tabs change, but that seems a little messier than this way.

So if we don't have a known set of extension differences to move forward, I
think this way might be easiest (and it's still pretty simple, given that
extensions are < 100, and on average around 2).

WDYT?

[Peter Kasting, 2014/10/31 19:01:07]

Fine with me.

+    }
+  }
+
+  // Our visible browser actions may have changed - re-Layout() and check the
+  // size.
+  ToolbarVisibleCountChanged();
+  OnBrowserActionVisibilityChanged();
+}
+
 size_t BrowserActionsContainer::GetIconCount() const {
   if (!model_)
     return 0u;
 
   // Find the absolute value for the model's visible count.
-  int model_visible_size = model_->GetVisibleIconCount();
+  int model_visible_size =
+      model_->GetVisibleIconCountForTab(GetCurrentWebContents());
   size_t absolute_model_visible_size = model_visible_size == -1 ?
       model_->toolbar_items().size() : model_visible_size;
 
@@ -1006,7 +1077,7 @@ size_t BrowserActionsContainer::GetIconCount() const {
   // Good time for some sanity checks: We should never try to display more
   // icons than we have, and we should always have a view per item in the model.
   // (The only exception is if this is in initialization.)
-  if (initialized_) {
+  if (initialized_ && !suppress_layout_) {
     size_t num_extension_actions = 0u;
     for (BrowserActionView* view : browser_action_views_) {
       // No component action should ever have a valid extension id, so we can