Make extensions that desire to act pop out if in overflow

chrome/browser/extensions/extension_toolbar_model.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_TOOLBAR_MODEL_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_TOOLBAR_MODEL_H_
 
 #include "base/compiler_specific.h"
 #include "base/observer_list.h"
 #include "base/prefs/pref_change_registrar.h"
@@ skipping 60 matching lines @@
 
     // 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.
     // * probably, because if we are in highlight mode and receive a call to
     //   highlight a new set of extensions, we do not compare the current set
     //   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;
 
    protected:
     virtual ~Observer() {}
   };
 
   // Convenience function to get the ExtensionToolbarModel for a Profile.
   static ExtensionToolbarModel* Get(Profile* profile);
 
   // Add or remove an observer.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
   // Moves the given |extension|'s icon to the given |index|.
   void MoveExtensionIcon(const std::string& id, size_t index);
 
   // 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(int count);
 
   // 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 {
     return is_highlighting_ ? highlighted_items_ : toolbar_items_;
   }
 
   bool is_highlighting() const { return is_highlighting_; }
 
   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
   // a direct user action.
   bool ShowExtensionActionPopup(const Extension* extension,
                                 Browser* browser,
                                 bool grant_active_tab);
 
   // Ensures that the extensions in the |extension_ids| list are visible on the
   // toolbar. This might mean they need to be moved to the front (if they are in
@@ skipping 120 matching lines @@
   base::Closure pref_change_callback_;
 
   base::WeakPtrFactory<ExtensionToolbarModel> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionToolbarModel);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_TOOLBAR_MODEL_H_