Revert of Make extensions that desire to act pop out if in overflow (patchset...

Revert of Make extensions that desire to act pop out if in overflow (patchset #6 id:200001 of https://codereview.chromium.org/675023002/)

Reason for revert:
Suspect this patch of causing errors on linux valgrind for LocationBarControllerUnitTest.NavigationClearsState.

http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%285%29/builds/31337

Sample valgrind output:
Suppression (error hash=#606630BA25518095#):
For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/memory-sheriff#TOC-Suppressing-memory-reports
{
<insert_a_suppression_name_here>
Memcheck:Unaddressable
fun:_ZN16ExtensionService21NotifyExtensionLoadedEPKN10extensions9ExtensionE
fun:_ZN16ExtensionService12AddExtensionEPKN10extensions9ExtensionE
fun:_ZN10extensions12_GLOBAL__N_129LocationBarControllerUnitTest12AddExtensionEbRKSs
fun:_ZN10extensions12_GLOBAL__N_156LocationBarControllerUnitTest_NavigationClearsState_Test8TestBodyEv
}

Original issue's description:
> Make extensions that desire to act pop out if in overflow
>
> If an extension desires to act, it should pop itself out of
> the overflow menu.  There should also be a visual queue for
> extensions that are already visible, but what exactly that
> should be is still being discussed.
>
> BUG=417441
>
> Committed: https://crrev.com/d604171517135387ca3b4c33d7f1774c8d2d38b0
> Cr-Commit-Position: refs/heads/master@{#302511}

TBR=finnur@chromium.org,pkasting@chromium.org,rdevlin.cronin@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=417441

Review URL: https://codereview.chromium.org/700453003

Cr-Commit-Position: refs/heads/master@{#302563}

chrome/browser/extensions/extension_toolbar_model.cc

@@ -121,8 +121,9 @@ void ExtensionToolbarModel::MoveExtensionIcon(const std::string& id,
   UpdatePrefs();
 }
 
-void ExtensionToolbarModel::SetVisibleIconCount(size_t count) {
-  visible_icon_count_ = (count == toolbar_items_.size()) ? -1 : count;
+void ExtensionToolbarModel::SetVisibleIconCount(int count) {
+  visible_icon_count_ =
+      count == static_cast<int>(toolbar_items_.size()) ? -1 : count;
 
   // Only set the prefs if we're not in highlight mode and the profile is not
   // incognito. Highlight mode is designed to be a transitory state, and should
@@ -151,18 +152,11 @@ void ExtensionToolbarModel::OnExtensionActionUpdated(
       ExtensionRegistry::Get(profile_)->enabled_extensions().GetByID(
           extension_action->extension_id());
   // Notify observers if the extension exists and is in the model.
-  ExtensionList::const_iterator iter =
-      std::find(toolbar_items_.begin(), toolbar_items_.end(), extension);
-  if (iter != toolbar_items_.end()) {
-    FOR_EACH_OBSERVER(
-        Observer, observers_, ToolbarExtensionUpdated(extension));
-    // If the action was in the overflow menu, we have to alert observers that
-    // the toolbar needs to be reordered (to show the action).
-    if (static_cast<size_t>(iter - toolbar_items_.begin()) >=
-            visible_icon_count()) {
-      FOR_EACH_OBSERVER(
-          Observer, observers_, OnToolbarReorderNecessary(web_contents));
-    }
+  if (extension &&
+      std::find(toolbar_items_.begin(),
+                toolbar_items_.end(),
+                extension) != toolbar_items_.end()) {
+    FOR_EACH_OBSERVER(Observer, observers_, ToolbarExtensionUpdated(extension));
   }
 }
 
@@ -506,7 +500,9 @@ void ExtensionToolbarModel::IncognitoPopulate() {
       ExtensionToolbarModel::Get(profile_->GetOriginalProfile());
 
   // Find the absolute value of the original model's count.
-  int original_visible = original_model->visible_icon_count();
+  int original_visible = original_model->GetVisibleIconCount();
+  if (original_visible == -1)
+    original_visible = original_model->toolbar_items_.size();
 
   // In incognito mode, we show only those extensions that are
   // incognito-enabled. Further, any actions that were overflowed in regular
@@ -603,49 +599,6 @@ void ExtensionToolbarModel::OnExtensionToolbarPrefChange() {
   }
 }
 
-size_t ExtensionToolbarModel::GetVisibleIconCountForTab(
-    content::WebContents* web_contents) const {
-  if (all_icons_visible())
-    return visible_icon_count();  // Already displaying all actions.
-
-  ExtensionActionAPI* extension_action_api = ExtensionActionAPI::Get(profile_);
-  size_t total_icons = visible_icon_count_;
-  for (size_t i = total_icons; i < toolbar_items_.size(); ++i) {
-    if (extension_action_api->ExtensionWantsToRun(toolbar_items_[i].get(),
-                                                  web_contents))
-      ++total_icons;
-  }
-  return total_icons;
-}
-
-ExtensionList ExtensionToolbarModel::GetItemOrderForTab(
-    content::WebContents* web_contents) const {
-  // If we're highlighting, the items are always the same.
-  if (is_highlighting_)
-    return highlighted_items_;
-
-  // Start by initializing the array to be the same as toolbar items (this isn't
-  // any more expensive than initializing it to be of the same size with all
-  // nulls, and saves us time at the end).
-  ExtensionList result = toolbar_items_;
-  if (toolbar_items_.empty())
-    return result;
-
-  ExtensionList overflowed_actions_wanting_to_run;
-  ExtensionActionAPI* extension_action_api = ExtensionActionAPI::Get(profile_);
-  size_t boundary = visible_icon_count();
-  // Rotate any actions that want to run to the boundary between visible and
-  // overflowed actions.
-  for (ExtensionList::iterator iter = result.begin() + boundary;
-       iter != result.end(); ++iter) {
-    if (extension_action_api->ExtensionWantsToRun(iter->get(), web_contents)) {
-      std::rotate(result.begin() + boundary, iter, iter + 1);
-      ++boundary;
-    }
-  }
-  return result;
-}
-
 bool ExtensionToolbarModel::ShowExtensionActionPopup(
     const Extension* extension,
     Browser* browser,

chrome/browser/extensions/extension_toolbar_model.h

@@ -41,17 +41,16 @@ class ExtensionToolbarModel : public content::NotificationObserver,
   // delegate.
   class Observer {
    public:
-    // TODO(devlin): Rename these methods to be OnFoo.
-    // Signals that an |extension| has been added to the toolbar at |index|.
+    // An extension has been added to the toolbar and should go at |index|.
     virtual void ToolbarExtensionAdded(const Extension* extension,
                                        int index) = 0;
 
-    // Signals that the given |extension| has been removed from the toolbar.
+    // The given |extension| should be removed from the toolbar.
     virtual void ToolbarExtensionRemoved(const Extension* extension) = 0;
 
-    // Signals that the given |extension| has been moved to |index|. |index| is
-    // the desired *final* index of the extension (that is, in the adjusted
-    // order, extension should be at |index|).
+    // The given |extension| has been moved to |index|. |index| is the desired
+    // *final* index of the extension (that is, in the adjusted order, extension
+    // should be at |index|).
     virtual void ToolbarExtensionMoved(const Extension* extension,
                                        int index) = 0;
 
@@ -59,18 +58,18 @@ class ExtensionToolbarModel : public content::NotificationObserver,
     // updated.
     virtual void ToolbarExtensionUpdated(const Extension* extension) = 0;
 
-    // Signals the |extension| to show the popup now in the active window.
+    // Signal the |extension| to show the popup now in the active window.
     // If |grant_active_tab| is true, then active tab permissions should be
     // given to the extension (only do this if this is through a user action).
     // Returns true if a popup was slated to be shown.
     virtual bool ShowExtensionActionPopup(const Extension* extension,
                                           bool grant_active_tab) = 0;
 
-    // Signals when the container needs to be redrawn because of a size change,
+    // Signal when the container needs to be redrawn because of a size change,
     // and when the model has finished loading.
     virtual void ToolbarVisibleCountChanged() = 0;
 
-    // Signals that the model has entered or exited highlighting mode, or that
+    // Signal that the model has entered or exited highlighting mode, or that
     // the extensions being highlighted have (probably*) changed. Highlighting
     // mode indicates that only a subset of the extensions are actively
     // displayed, and those extensions should be highlighted for extra emphasis.
@@ -79,12 +78,6 @@ class ExtensionToolbarModel : public content::NotificationObserver,
     //   with the new set (and just assume the new set is different).
     virtual void ToolbarHighlightModeChanged(bool is_highlighting) = 0;
 
-    // Signals that the toolbar needs to be reordered for the given
-    // |web_contents|. This is caused by an overflowed action wanting to run,
-    // and needing to "pop itself out".
-    virtual void OnToolbarReorderNecessary(
-        content::WebContents* web_contents) = 0;
-
     // Returns the browser associated with the Observer.
     virtual Browser* GetBrowser() = 0;
 
@@ -95,7 +88,7 @@ class ExtensionToolbarModel : public content::NotificationObserver,
   // Convenience function to get the ExtensionToolbarModel for a Profile.
   static ExtensionToolbarModel* Get(Profile* profile);
 
-  // Adds or removes an observer.
+  // Add or remove an observer.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
@@ -105,14 +98,10 @@ class ExtensionToolbarModel : public content::NotificationObserver,
   // Sets the number of extension icons that should be visible.
   // If count == size(), this will set the visible icon count to -1, meaning
   // "show all actions".
-  void SetVisibleIconCount(size_t count);
-
-  size_t visible_icon_count() const {
-    return visible_icon_count_ == -1 ?
-        toolbar_items().size() : static_cast<size_t>(visible_icon_count_);
-  }
+  void SetVisibleIconCount(int count);
 
-  bool all_icons_visible() const { return visible_icon_count_ == -1; }
+  // As above, a return value of -1 represents "show all actions".
+  int GetVisibleIconCount() const { return visible_icon_count_; }
 
   bool extensions_initialized() const { return extensions_initialized_; }
 
@@ -124,16 +113,6 @@ class ExtensionToolbarModel : public content::NotificationObserver,
 
   void OnExtensionToolbarPrefChange();
 
-  // Returns the item order for a given tab. This can be different from the
-  // base item order if the action wants to run on the given page, and needs to
-  // be popped out of overflow.
-  ExtensionList GetItemOrderForTab(content::WebContents* web_contents) const;
-
-  // Returns the visible icon count for a given tab. This can be different from
-  // the base item order if the action wants to run on the given page and needs
-  // to be popped out of overflow.
-  size_t GetVisibleIconCountForTab(content::WebContents* web_contents) const;
-
   // Finds the Observer associated with |browser| and tells it to display a
   // popup for the given |extension|. If |grant_active_tab| is true, this
   // grants active tab permissions to the |extension|; only do this because of
@@ -147,7 +126,7 @@ class ExtensionToolbarModel : public content::NotificationObserver,
   // the overflow bucket).
   void EnsureVisibility(const ExtensionIdList& extension_ids);
 
-  // Highlights the extensions specified by |extension_ids|. This will cause
+  // Highlight the extensions specified by |extension_ids|. This will cause
   // the ToolbarModel to only display those extensions.
   // Highlighting mode is only entered if there is at least one extension to
   // be shown.
@@ -250,9 +229,7 @@ class ExtensionToolbarModel : public content::NotificationObserver,
   ExtensionIdList last_known_positions_;
 
   // The number of icons visible (the rest should be hidden in the overflow
-  // chevron). A value of -1 indicates that all icons should be visible.
-  // TODO(devlin): Make a new variable to indicate that all icons should be
-  // visible, instead of overloading this one.
+  // chevron).
   int visible_icon_count_;
 
   content::NotificationRegistrar registrar_;

chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm

@@ -245,10 +245,6 @@ class ExtensionServiceObserverBridge
 
   void ToolbarVisibleCountChanged() override {}
 
-  void OnToolbarReorderNecessary(content::WebContents* web_contents) override {
-    // TODO(devlin): Implement on mac.
-  }
-
   void ToolbarHighlightModeChanged(bool is_highlighting) override {}
 
   Browser* GetBrowser() override { return browser_; }
@@ -359,7 +355,9 @@ class ExtensionServiceObserverBridge
 }
 
 - (void)resizeContainerAndAnimate:(BOOL)animate {
-  int iconCount = toolbarModel_->visible_icon_count();
+  int iconCount = toolbarModel_->GetVisibleIconCount();
+  if (iconCount < 0)  // If no buttons are hidden.
+    iconCount = [self buttonCount];
 
   [containerView_ resizeToWidth:[self containerWidthWithButtonCount:iconCount]
                         animate:animate];
@@ -379,8 +377,9 @@ class ExtensionServiceObserverBridge
   if (!toolbarModel_)
     return 0;
 
-  int savedButtonCount = toolbarModel_->visible_icon_count();
-  if (static_cast<NSUInteger>(savedButtonCount) > [self buttonCount])
+  int savedButtonCount = toolbarModel_->GetVisibleIconCount();
+  if (savedButtonCount < 0 ||  // all icons are visible
+      static_cast<NSUInteger>(savedButtonCount) > [self buttonCount])
     savedButtonCount = [self buttonCount];
   return [self containerWidthWithButtonCount:savedButtonCount];
 }

chrome/browser/ui/toolbar/wrench_menu_model.cc

@@ -716,7 +716,7 @@ void WrenchMenuModel::CreateExtensionToolbarOverflowMenu() {
   extensions::ExtensionToolbarModel* toolbar_model =
       extensions::ExtensionToolbarModel::Get(browser_->profile());
   // A count of -1 means all actions are visible.
-  if (!toolbar_model->all_icons_visible())
+  if (toolbar_model->GetVisibleIconCount() != -1)
     AddSeparator(ui::UPPER_SEPARATOR);
 #endif  // defined(TOOLKIT_VIEWS)
 }

chrome/browser/ui/views/toolbar/browser_actions_container.h

@@ -285,7 +285,6 @@ class BrowserActionsContainer
                                 bool grant_active_tab) override;
   void ToolbarVisibleCountChanged() override;
   void ToolbarHighlightModeChanged(bool is_highlighting) override;
-  void OnToolbarReorderNecessary(content::WebContents* web_contents) override;
   Browser* GetBrowser() override;
 
   void LoadImages();
@@ -320,15 +319,12 @@ class BrowserActionsContainer
   // case the container wouldn't be shown at all.
   int MinimumNonemptyWidth() const;
 
-  // Animates to the target size (unless testing, in which case we go straight
-  // to the target size).
+  // Animate to the target size (unless testing, in which case we go straight to
+  // the target size).
   void Animate(gfx::Tween::Type type, size_t num_visible_icons);
 
-  // Reorders the views to match the toolbar model for the active tab.
-  void ReorderViews();
-
   // Returns the number of icons that this container should draw. This differs
-  // from the model's visible_icon_count if this container is for the overflow.
+  // from the model's GetVisibleIconCount if this container is for the overflow.
   size_t GetIconCount() const;
 
   // Whether this container is in overflow mode (as opposed to in 'main'
@@ -377,14 +373,6 @@ class BrowserActionsContainer
   // Don't show the chevron while animating.
   bool suppress_chevron_;
 
-  // True if we should suppress animation; we typically do this e.g. when
-  // switching tabs changes the state of the icons.
-  bool suppress_animation_;
-
-  // True if we should suppress layout, such as when we are creating or
-  // adjusting a lot of views.
-  bool suppress_layout_;
-
   // This is used while the user is resizing (and when the animations are in
   // progress) to know how wide the delta is between the current state and what
   // we should draw.

chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc

@@ -116,7 +116,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsBarBrowserTest, DragBrowserActions) {
   EXPECT_EQ(extension_b()->id(), browser_actions_bar()->GetExtensionId(2));
   EXPECT_EQ(3u, container->VisibleBrowserActions());
   EXPECT_FALSE(container->chevron()->visible());
-  EXPECT_TRUE(model->all_icons_visible());
+  EXPECT_EQ(-1, model->GetVisibleIconCount());
 
   // TODO(devlin): Ideally, we'd also have tests for dragging from the legacy
   // overflow menu (i.e., chevron) to the main bar, but this requires either
@@ -438,8 +438,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsContainerOverflowTest,
   overflow_bar()->Layout();
 
   // All actions are showing, and are in the installation order.
-  EXPECT_TRUE(model()->all_icons_visible());
-  EXPECT_EQ(3u, model()->visible_icon_count());
+  EXPECT_EQ(-1, model()->GetVisibleIconCount());
   ASSERT_EQ(3u, main_bar()->num_toolbar_actions());
   EXPECT_EQ(extension_a()->id(), main_bar()->GetIdAt(0u));
   EXPECT_EQ(extension_b()->id(), main_bar()->GetIdAt(1u));