Make extensions that desire to act pop out if in overflow

chrome/browser/extensions/extension_toolbar_model.h

Index: chrome/browser/extensions/extension_toolbar_model.h
diff --git a/chrome/browser/extensions/extension_toolbar_model.h b/chrome/browser/extensions/extension_toolbar_model.h
index 950366e7768daa6bb3cbb5b9f9d845ef54b30d6e..088171bd0818fc382540ef6bf184cb2b5d39134e 100644
--- a/chrome/browser/extensions/extension_toolbar_model.h
+++ b/chrome/browser/extensions/extension_toolbar_model.h
@@ -78,6 +78,12 @@ class ExtensionToolbarModel : public content::NotificationObserver,
     //   with the new set (and just assume the new set is different).
     virtual void ToolbarHighlightModeChanged(bool is_highlighting) = 0;
 
+    // Signal that the toolbar needs to be reordered for the given

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

Nit: "Signals" (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
, "comments should be descriptive, not imperative")

Same comment applies to other existing functions in this file.

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

Yeah, this and the name itself were to match existing style in this file. 
Updating comments.

+    // |web_contents|. This is caused by an overflowed action wanting to run,
+    // and needing to "pop itself out".
+    virtual void ToolbarReorderNecessary(

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

Nit: This is a strange name for a function.  It sounds as if it should return a
bool, but instead it returns void.  Maybe "ReorderToolbarIfNecessary", or
"OnToolbarReorderNecessary"?  (Some of the other functions here have problematic
names too... I might add "On" to a lot of them that are event handlers.)

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

/sigh... I know - it's bothered me most times I add a new function here and
reach for OnFoo, and realize it's totally different from the rest.  But in order
to keep this patch small and targeted, can we rename the new function and add a
TODO for the rest?

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

Doesn't bother me.  Bonus points if you go write a separate renaming patch to do
the renaming immediately, but I'm fine either way.

+        content::WebContents* web_contents) = 0;
+
     // Returns the browser associated with the Observer.
     virtual Browser* GetBrowser() = 0;
 
@@ -103,6 +109,12 @@ class ExtensionToolbarModel : public content::NotificationObserver,
   // As above, a return value of -1 represents "show all actions".
   int GetVisibleIconCount() const { return visible_icon_count_; }
 
+  // Returns the number of visible icons as an absolute value.

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

Nit: I think this name/comment are misleading.  They suggest you're going to do
a mathematical abs().  I would use some other word than "Absolute" in the
function name.  Maybe "Actual", or "True"?

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

Yeah, I struggled with the name.  The problem with "actual" and "true" is that
it implies the other is "fake" (and why would we ever want a fake value?).

I think the real problem here is that GetVisibleIconCount() is trying to do two
things - return the number of icons and return whether or not all icons are
visible.  Let's just make a function for that.  Solves naming, and improves
readability in some cases.

SGTY?

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

Good resolution.  I'm happy with it.

+  size_t GetAbsoluteVisibleIconCount() const {
+    return visible_icon_count_ == -1 ?
+        toolbar_items().size() : static_cast<size_t>(visible_icon_count_);
+  }
+
   bool extensions_initialized() const { return extensions_initialized_; }
 
   const ExtensionList& toolbar_items() const {
@@ -113,6 +125,17 @@ 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.
+  // A result of -1 indicates "all icons".
+  int 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