Dynamically calculate the number of extension icons to show per row in overflow

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

Index: chrome/browser/ui/views/toolbar/browser_actions_container.h
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.h b/chrome/browser/ui/views/toolbar/browser_actions_container.h
index f3702c5aa560c5c1824198a68836a2d4c9d4e201..9d9c6ed3bd1fbe679bcec58f1c693164adee82a0 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.h
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.h
@@ -248,6 +248,21 @@ class BrowserActionsContainer
   // unit tests.
   void TestSetIconVisibilityCount(size_t icons);
 
+  // Returns the width of an icon, optionally with its padding.
+  static int IconWidth(bool include_padding);
+
+  // Returns the height of an icon.
+  static int IconHeight();
+
+  // The maximum number of icons to show per row when in overflow mode (showing
+  // icons in the application menu).
+  // This is set by the active menu before display.
+  static int icons_per_menu_row;

[Finnur, 2014/09/10 10:48:28]

So, two things I'm concerned about here:

1) Most worryingly is that this variable guides both types of containers and I'm
worried someone might use it to calculate something for the main container
_after_ it has been modified to fit an overflow container. In fact, I've already
uncovered what I think is one such bug (see comment on DragDrop).

I think I would like to see this renamed so that it is clear that this variable
should only be used when in overflow. Maybe icons_per_overflow_menu_row? That
way we can have code like:
in_overflow() ? icons_per_overflow_menu_row : 1
(or a helper function that does that, if we do it more than often).

2) It feels very icky to expose a global static like that. I wonder if we can
communicate this value through the ExtensionToolbarModel, instead?
ExtensionToolbarMenuView can get at that through the browser_->profile() pointer
it has and this class already has a direct pointer to the model.

[Devlin, 2014/09/10 16:14:26]

1) It's not really used to guide both types of containers, since there are no
menu rows in the main container.  The only places it's used are in overflow menu
code, or where it "just does the right thing" (like when row is always 0 in your
other comment).  As such, doing something like "in_overflow() ?
icons_per_menu_row : 1" isn't helpful, since we don't assume it to be 1 anywhere
(if you notice, it used to be 7/8 in the old code).

1.5) I'm not really opposed to saying icons_per_overflow_menu_row, but I'm
slightly in favor of keeping the current name, just because "overflow_menu_row"
feels redundant.  After all, the only time we show icons in a menu is in
overflow.

2) I'm kinda torn on whether or not to have it be a static like this.  It's
modeled after many of the constants in menu code, which are static yet modified
based on the menu shown. [1]  The logic is that there's only ever one menu
shown, so it can be shared variable.  I'd be willing to put it as a member of
BrowserActionsContainer, but that seems a little odd since it's only used in
overflow, and it would mean we always have to recalculate it.  I don't really
like the idea of having it on ExtensionToolbarModel, because this really is pure
UI code (and platform-dependent UI code, at that).  It seems like a conceptual
layering violation to put the burden of it on the Model.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[Finnur, 2014/09/11 09:12:43]

How about we compromise on this by doing step 1.5 (naming the variable
icons_per_overflow_menu_row, so that it is obvious what it should not be used
for)? :)

[Devlin, 2014/09/11 17:53:50]

Done. :)

+
+  // Horizontal spacing between most items in the container, as well as after
+  // the last item or chevron (if visible).
+  static const int kItemSpacing;

[Finnur, 2014/09/10 10:48:28]

This is only used internally. Doesn't need to be public.

[Devlin, 2014/09/10 16:14:26]

This is also now used in ExtensionToolbarMenuView in order to properly calculate
padding.

[Finnur, 2014/09/11 09:12:43]

Huh. I see that now.

+
   // During testing we can disable animations by setting this flag to true,
   // so that the bar resizes instantly, instead of having to poll it while it
   // animates to open/closed status.
@@ -261,18 +276,11 @@ class BrowserActionsContainer
   virtual void OnThemeChanged() OVERRIDE;
 
  private:
-  friend class BrowserActionView;  // So it can access IconWidth().
-
   // A struct representing the position at which an action will be dropped.
   struct DropPosition;
 
   typedef std::vector<BrowserActionView*> BrowserActionViews;
 
-  // Returns the width of an icon, optionally with its padding.
-  static int IconWidth(bool include_padding);
-
-  // Returns the height of an icon.
-  static int IconHeight();
 
   // extensions::ExtensionToolbarModel::Observer implementation.
   virtual void ToolbarExtensionAdded(const extensions::Extension* extension,