Modify toolbar action bar layout for material design

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

Index: chrome/browser/ui/views/toolbar/toolbar_view.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.cc b/chrome/browser/ui/views/toolbar/toolbar_view.cc
index 2218f483d4df33407b264dd4225a8cdd83ee6f67..8757f2d44c984da3cac4b219b5406d75395d39a9 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_view.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_view.cc
@@ -478,6 +478,8 @@ gfx::Size ToolbarView::GetPreferredSize() const {
   gfx::Size size(location_bar_->GetPreferredSize());
   if (is_display_mode_normal()) {
     const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
+    const int right_padding =
+        GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING);

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

Tiny nit: Define this below |browser_actions_width| to match the order they're
used in the next statement (2 places)

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

Done.

     const int browser_actions_width =
         browser_actions_->GetPreferredSize().width();
     const int content_width =
@@ -489,9 +491,8 @@ gfx::Size ToolbarView::GetPreferredSize() const {
              ? element_padding + home_->GetPreferredSize().width()
              : 0) +
         GetLayoutConstant(TOOLBAR_STANDARD_SPACING) +
-        GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING) +
         browser_actions_width +
-        (browser_actions_width > 0 ? element_padding : 0) +
+        (browser_actions_width == 0 ? right_padding : 0) +
         app_menu_button_->GetPreferredSize().width();

[Evan Stade, 2015/12/09 01:05:10]

these two functions copy each other a lot. Can we share the code (perhaps with
the use of fn pointers or base::Callbacks)?

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

I'll refactor in a follow-on CL once this one lands.

     size.Enlarge(content_width, 0);
   }
@@ -502,6 +503,8 @@ gfx::Size ToolbarView::GetMinimumSize() const {
   gfx::Size size(location_bar_->GetMinimumSize());
   if (is_display_mode_normal()) {
     const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
+    const int right_padding =
+        GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING);
     const int browser_actions_width =
         browser_actions_->GetMinimumSize().width();
     const int content_width =
@@ -513,9 +516,8 @@ gfx::Size ToolbarView::GetMinimumSize() const {
              ? element_padding + home_->GetMinimumSize().width()
              : 0) +
         GetLayoutConstant(TOOLBAR_STANDARD_SPACING) +
-        GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING) +
         browser_actions_width +
-        (browser_actions_width > 0 ? element_padding : 0) +
+        (browser_actions_width == 0 ? right_padding : 0) +

[Evan Stade, 2015/12/09 01:05:09]

nit: these two lines could perhaps be

(browser_actions_width > 0 ? browser_actions_width : right_padding) +

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

Done.

         app_menu_button_->GetMinimumSize().width();
     size.Enlarge(content_width, 0);
   }
@@ -535,9 +537,9 @@ void ToolbarView::Layout() {
 
   // We assume all child elements except the location bar are the same height.
   // Set child_y such that buttons appear vertically centered.
-  int child_height =
+  const int child_height =
       std::min(back_->GetPreferredSize().height(), height());
-  int child_y = CenteredChildY(height(), child_height);
+  const int child_y = CenteredChildY(height(), child_height);
 
   // If the window is maximized, we extend the back button to the left so that
   // clicking on the left-most pixel will activate the back button.
@@ -546,8 +548,9 @@ void ToolbarView::Layout() {
   //                will be slightly the wrong size.  We should force a
   //                Layout() in this case.
   //                http://crbug.com/5540
-  bool maximized = browser_->window() && browser_->window()->IsMaximized();
-  int back_width = back_->GetPreferredSize().width();
+  const bool maximized =
+      browser_->window() && browser_->window()->IsMaximized();
+  const int back_width = back_->GetPreferredSize().width();
   const gfx::Insets insets(GetLayoutInsets(TOOLBAR));
   if (maximized) {
     back_->SetBounds(0, child_y, back_width + insets.left(), child_height);
@@ -580,36 +583,34 @@ void ToolbarView::Layout() {
   next_element_x =
       home_->bounds().right() + GetLayoutConstant(TOOLBAR_STANDARD_SPACING);
 
-  int browser_actions_desired_width =
-      browser_actions_->GetPreferredSize().width();
   int app_menu_width = app_menu_button_->GetPreferredSize().width();
-  const int location_bar_right_padding =
+  const int right_padding =
       GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING);
 
   int available_width = std::max(
-      0, width() - insets.right() - app_menu_width -
-             (browser_actions_desired_width > 0 ? element_padding : 0) -
-             location_bar_right_padding - next_element_x);
+      0,
+      width() - insets.right() - app_menu_width -
+      (browser_actions_->GetPreferredSize().width() == 0 ? right_padding : 0) -

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

A short comment explaining this calculation would be useful.

[Evan Stade, 2015/12/09 01:05:09]

perhaps better as GetPreferredSize().IsEmpty()

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

Done and done.

+      next_element_x);
   // Don't allow the omnibox to shrink to the point of non-existence, so
   // subtract its minimum width from the available width to reserve it.
-  int minimum_location_bar_width = location_bar_->GetMinimumSize().width();
-  int browser_actions_width = browser_actions_->GetWidthForMaxWidth(
-      available_width - minimum_location_bar_width);
+  const int browser_actions_width = browser_actions_->GetWidthForMaxWidth(
+      available_width - location_bar_->GetMinimumSize().width());
   available_width -= browser_actions_width;
-  int location_bar_width = available_width;
+  const int location_bar_width = available_width;
 
-  int location_height = location_bar_->GetPreferredSize().height();
-  int location_y = CenteredChildY(height(), location_height);
+  const int location_height = location_bar_->GetPreferredSize().height();
+  const int location_y = CenteredChildY(height(), location_height);
 
   location_bar_->SetBounds(next_element_x, location_y,
                            location_bar_width, location_height);
-  next_element_x = location_bar_->bounds().right() + location_bar_right_padding;
 
+  next_element_x = location_bar_->bounds().right();
   browser_actions_->SetBounds(
       next_element_x, child_y, browser_actions_width, child_height);
   next_element_x = browser_actions_->bounds().right();
-  if (browser_actions_width > 0)
-    next_element_x += element_padding;
+  if (!browser_actions_width)
+    next_element_x += right_padding;
 
   // The browser actions need to do a layout explicitly, because when an
   // extension is loaded/unloaded/changed, BrowserActionContainer removes and