Modify toolbar action bar layout for material design

chrome/browser/ui/toolbar/toolbar_actions_bar.cc

Index: chrome/browser/ui/toolbar/toolbar_actions_bar.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
index 442119b4e388975fa7014de0d4f71517952a8327..79e32936ef8421eed55b6846d1e50fa80c73de2b 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
@@ -38,12 +38,24 @@ namespace {
 
 using WeakToolbarActions = std::vector<ToolbarActionViewController*>;
 
-// Matches ToolbarView::kStandardSpacing;
-const int kLeftPadding = 3;
-const int kRightPadding = kLeftPadding;
-const int kItemSpacing = kLeftPadding;
-const int kOverflowLeftPadding = kItemSpacing;
-const int kOverflowRightPadding = kItemSpacing;
+// Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING).

[Peter Kasting, 2015/12/08 21:56:24]

While I'm on the hook for the long-term solution here, I think you should do the
short-term thing Evan was going to do, which is to just move layout_constants.*
to chrome/browser/ui/.  Then you can go ahead and call this function directly
here.

You're welcome to do that as a separate change.

[tdanderson, 2015/12/09 18:22:58]

Evan landed https://codereview.chromium.org/1508203003/ so I will instead call
GetLayoutConstant() here.

+const int kItemSpacing = 3;
+const int kItemSpacingMaterial = 4;
+const int kItemSpacingMaterialHybrid = 8;
+
+int GetItemSpacing() {
+  switch (ui::MaterialDesignController::GetMode()) {
+    case ui::MaterialDesignController::Mode::NON_MATERIAL:
+      return kItemSpacing;
+    case ui::MaterialDesignController::Mode::MATERIAL_NORMAL:
+      return kItemSpacingMaterial;
+    case ui::MaterialDesignController::Mode::MATERIAL_HYBRID:
+      return kItemSpacingMaterialHybrid;
+    default:
+      NOTREACHED() << "Invalid material design mode.";
+      return kItemSpacing;
+  }
+}
 
 enum DimensionType { WIDTH, HEIGHT };
 
@@ -106,10 +118,8 @@ void SortContainer(std::vector<Type1>* to_sort,
 // static
 bool ToolbarActionsBar::disable_animations_for_testing_ = false;
 
-ToolbarActionsBar::PlatformSettings::PlatformSettings(bool in_overflow_mode)
-    : left_padding(in_overflow_mode ? kOverflowLeftPadding : kLeftPadding),
-      right_padding(in_overflow_mode ? kOverflowRightPadding : kRightPadding),
-      item_spacing(kItemSpacing),
+ToolbarActionsBar::PlatformSettings::PlatformSettings()
+    : item_spacing(GetItemSpacing()),
       icons_per_overflow_menu_row(1),
       chevron_enabled(!extensions::FeatureSwitch::extension_action_redesign()->
                           IsEnabled()) {
@@ -122,7 +132,7 @@ ToolbarActionsBar::ToolbarActionsBar(ToolbarActionsBarDelegate* delegate,
       browser_(browser),
       model_(ToolbarActionsModel::Get(browser_->profile())),
       main_bar_(main_bar),
-      platform_settings_(main_bar != nullptr),
+      platform_settings_(),
       popup_owner_(nullptr),
       model_observer_(this),
       suppress_layout_(false),
@@ -146,7 +156,7 @@ ToolbarActionsBar::~ToolbarActionsBar() {
 
 // static
 int ToolbarActionsBar::IconWidth(bool include_padding) {
-  return GetIconDimension(WIDTH) + (include_padding ? kItemSpacing : 0);
+  return GetIconDimension(WIDTH) + (include_padding ? GetItemSpacing() : 0);
 }
 
 // static
@@ -189,8 +199,8 @@ gfx::Size ToolbarActionsBar::GetPreferredSize() const {
 
 int ToolbarActionsBar::GetMinimumWidth() const {
   if (!platform_settings_.chevron_enabled || toolbar_actions_.empty())
-    return kLeftPadding;
-  return kLeftPadding + delegate_->GetChevronWidth() + kRightPadding;
+    return platform_settings_.item_spacing;
+  return 2 * platform_settings_.item_spacing + delegate_->GetChevronWidth();
 }
 
 int ToolbarActionsBar::GetMaximumWidth() const {
@@ -200,17 +210,18 @@ int ToolbarActionsBar::GetMaximumWidth() const {
 int ToolbarActionsBar::IconCountToWidth(int icons) const {
   if (icons < 0)
     icons = toolbar_actions_.size();
-  bool display_chevron =
+  const bool display_chevron =

[Devlin, 2015/12/08 22:13:44]

nit: IMO, having POD like this be const is overkill, and doesn't really ever
help much (it just means that when someone does want it to change, they need to
take it out).  Unless there's reason to do it, I'd just have these stick as
bool, int.

[tdanderson, 2015/12/09 18:22:58]

IMO in general doing this improves readability. When tracing or debugging a
method I like knowing upfront which locals can/will change without needing to
scan through the entire body. It also guards against someone else accidentally
or incorrectly mutating a variable that was originally not meant to change. And
if they do want it to change, IMO the cost of removing 'const' does not outweigh
the benefit of having it there in the first place.

My preference would be to leave the const in here (and in
browser_actions_container.cc). If you still disagree I'm happy to discuss
further / change them back in a follow-up CL.

[Devlin, 2015/12/09 19:22:27]

TL;DR: I'd prefer them removed.

The chance of someone "accidentally" modifying a POD is pretty slim, I think
(the reason it's so beneficial for objects is that if I call Foo(), I may not
know if Foo() has some side-effect - we should know that foo = bar modifies the
value of foo :)).  So in my opinion it's just making extra work for someone else
down the road, and doesn't really add much in the way of readability.  The style
guide also encourages const in three main cases - function arguments by
reference or ptr, methods, and data members; this doesn't fall into those
categories.

[tdanderson, 2015/12/09 19:58:14]

OK, fair enough. I have removed them in:
https://codereview.chromium.org/1505403003/

       platform_settings_.chevron_enabled &&
       static_cast<size_t>(icons) < toolbar_actions_.size();
   if (icons == 0 && !display_chevron)
-    return platform_settings_.left_padding;
-  int icons_size = (icons == 0) ? 0 :
+    return platform_settings_.item_spacing;
+
+  const int icons_size = (icons == 0) ? 0 :
       (icons * IconWidth(true)) - platform_settings_.item_spacing;
-  int chevron_size = display_chevron ? delegate_->GetChevronWidth() : 0;
-  int padding = platform_settings_.left_padding +
-                platform_settings_.right_padding;
-  return icons_size + chevron_size + padding;
+  const int chevron_size = display_chevron ? delegate_->GetChevronWidth() : 0;
+  const int side_padding = platform_settings_.item_spacing * 2;

[Peter Kasting, 2015/12/08 21:56:24]

Nit: I'd probably just inline this below.

[tdanderson, 2015/12/09 18:22:58]

IMO inlining these three values would make the return statement unnecessarily
hard to read, so I prefer to leave this as-is. I'm happy to discuss as a
follow-up if you feel strongly otherwise.

+
+  return icons_size + chevron_size + side_padding;
 }
 
 size_t ToolbarActionsBar::WidthToIconCount(int pixels) const {
@@ -218,10 +229,9 @@ size_t ToolbarActionsBar::WidthToIconCount(int pixels) const {
   if (pixels >= IconCountToWidth(-1))
     return toolbar_actions_.size();
 
-  // We reserve space for the padding on either side of the toolbar...
-  int available_space = pixels -
-      (platform_settings_.left_padding + platform_settings_.right_padding);
-  // ... and, if the chevron is enabled, the chevron.
+  // We reserve space for the padding on either side of the toolbar and,
+  // if enabled, for the chevron.
+  int available_space = pixels - (platform_settings_.item_spacing * 2);
   if (platform_settings_.chevron_enabled)
     available_space -= delegate_->GetChevronWidth();
 
@@ -311,7 +321,7 @@ gfx::Rect ToolbarActionsBar::GetFrameForIndex(
   size_t index_in_row = in_overflow_mode() ?
       relative_index % icons_per_overflow_row : relative_index;
 
-  return gfx::Rect(platform_settings().left_padding +
+  return gfx::Rect(platform_settings().item_spacing +
                        index_in_row * IconWidth(true),
                    row_index * IconHeight(),
                    IconWidth(false),
@@ -436,7 +446,7 @@ bool ToolbarActionsBar::ShowToolbarActionPopup(const std::string& action_id,
 void ToolbarActionsBar::SetOverflowRowWidth(int width) {
   DCHECK(in_overflow_mode());
   platform_settings_.icons_per_overflow_menu_row =
-      std::max((width - kItemSpacing) / IconWidth(true), 1);
+      std::max((width - platform_settings_.item_spacing) / IconWidth(true), 1);
 }
 
 void ToolbarActionsBar::OnResizeComplete(int width) {