Make extensions that desire to act pop out if in overflow

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/tab_helper.h"
@@ skipping 42 matching lines @@
 // TODO(devlin): We should move this to the model, once it supports component
 // actions.
 ScopedVector<ToolbarActionViewController> GetToolbarActions(
     extensions::ExtensionToolbarModel* model,
     Browser* browser) {
   ScopedVector<ToolbarActionViewController> actions;
 
   // 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(),
         browser,
         action_manager->GetExtensionAction(*extension)));
   }
 
   // Component actions come second.
   ScopedVector<ToolbarActionViewController> component_actions =
       ComponentToolbarActionsFactory::GetInstance()->
@@ skipping 44 matching lines @@
     : initialized_(false),
       profile_(browser->profile()),
       browser_(browser),
       main_container_(main_container),
       popup_owner_(NULL),
       model_(extensions::ExtensionToolbarModel::Get(browser->profile())),
       container_width_(0),
       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);
   if (model_)  // |model_| can be NULL in views unittests.
     model_->AddObserver(this);
 
   bool overflow_experiment =
       extensions::FeatureSwitch::extension_action_redesign()->IsEnabled();
   DCHECK(!in_overflow_mode() || overflow_experiment);
 
@@ skipping 49 matching lines @@
 BrowserActionView* BrowserActionsContainer::GetViewForExtension(
     const Extension* extension) {
   for (BrowserActionView* view : browser_action_views_) {
     if (view->view_controller()->GetId() == extension->id())
       return view;
   }
   return NULL;
 }
 
 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() {
   DCHECK(browser_action_views_.empty());
   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) {
     BrowserActionView* view =
         new BrowserActionView(make_scoped_ptr(controller), browser_, this);
     browser_action_views_.push_back(view);
     AddChildView(view);
   }
   actions.weak_clear();
+
+  suppress_layout_ = false;
+
+  Layout();
+  SchedulePaint();
 }
 
 void BrowserActionsContainer::DeleteBrowserActionViews() {
   HideActivePopup();
   STLDeleteElements(&browser_action_views_);
 }
 
 size_t BrowserActionsContainer::VisibleBrowserActions() const {
   size_t visible_actions = 0;
   for (const BrowserActionView* view : browser_action_views_) {
@@ skipping 130 matching lines @@
   int min_width = std::min(MinimumNonemptyWidth(), IconCountToWidth(-1));
   return gfx::Size(min_width, IconHeight());
 }
 
 void BrowserActionsContainer::Layout() {
   if (browser_action_views_.empty()) {
     SetVisible(false);
     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());
 
   // If the icons don't all fit, show the chevron (unless suppressed).
   int max_x = GetPreferredSize().width();
   if (IconCountToWidth(-1) > max_x && !suppress_chevron_ && chevron_) {
     chevron_->SetVisible(true);
     gfx::Size chevron_size(chevron_->GetPreferredSize());
     max_x -= chevron_size.width() + kChevronSpacing;
@@ skipping 255 matching lines @@
   resize_amount_ = 0;
   suppress_chevron_ = false;
   SetChevronVisibility();
   OnBrowserActionVisibilityChanged();
 
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
                     observers_,
                     OnBrowserActionsContainerAnimationEnded());
 }
 
-content::WebContents* BrowserActionsContainer::GetCurrentWebContents() {
+content::WebContents* BrowserActionsContainer::GetCurrentWebContents() const {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
 extensions::ActiveTabPermissionGranter*
     BrowserActionsContainer::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents = GetCurrentWebContents();
   if (!web_contents)
     return NULL;
   return extensions::TabHelper::FromWebContents(web_contents)->
       active_tab_permission_granter();
@@ skipping 222 matching lines @@
     bool grant_active_tab) {
   // Don't override another popup, and only show in the active window.
   if (popup_owner_ || !browser_->window()->IsActive())
     return false;
 
   BrowserActionView* view = GetViewForExtension(extension);
   return view && view->view_controller()->ExecuteAction(grant_active_tab);
 }
 
 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(
     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();
   Animate(gfx::Tween::LINEAR, GetIconCount());
 }
 
+void BrowserActionsContainer::ToolbarReorderNecessary(
+    content::WebContents* web_contents) {
+  if (GetCurrentWebContents() == web_contents)
+    ReorderViews();
+}
+
 Browser* BrowserActionsContainer::GetBrowser() {
   return browser_;
 }
 
 void BrowserActionsContainer::LoadImages() {
   if (in_overflow_mode())
     return;  // Overflow mode has neither a chevron nor highlighting.
 
   ui::ThemeProvider* tp = GetThemeProvider();
   if (tp && chevron_) {
@@ skipping 65 matching lines @@
 int BrowserActionsContainer::MinimumNonemptyWidth() const {
   if (!chevron_)
     return ToolbarView::kStandardSpacing;
   return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
       chevron_->GetPreferredSize().width();
 }
 
 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();
     resize_animation_->SetTweenType(tween_type);
     animation_target_size_ = target_size;
     resize_animation_->Show();
   } else {
     animation_target_size_ = target_size;
     AnimationEnded(resize_animation_.get());
   }
 }
 
+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;
 
 #if !defined(NDEBUG)

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

Nit: While here: use DCHECK_IS_ON instead of looking at NDEBUG, so release
trybots will run these sanity checks.

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

Good idea.

   // 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
       // use this to check the extension amount.
       // TODO(devlin): Fix this to just check model size when the model also
       // includes component actions.
       if (crx_file::id_util::IdIsValid(view->view_controller()->GetId()))
         ++num_extension_actions;
     }
     DCHECK_LE(absolute_model_visible_size, num_extension_actions);
     DCHECK_EQ(model_->toolbar_items().size(), num_extension_actions);
   }
 #endif
 
   // The overflow displays any icons not shown by the main bar.
   return in_overflow_mode() ?
       model_->toolbar_items().size() - absolute_model_visible_size :
       absolute_model_visible_size;
 }