Modify toolbar action bar layout for material design

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

 // Copyright 2013 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.
 
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/i18n/number_formatting.h"
@@ skipping 460 matching lines @@
   return GetWidget()->GetAccelerator(command_id, accelerator);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ToolbarView, views::View overrides:
 
 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 =
         GetLayoutInsets(TOOLBAR).width() +
         back_->GetPreferredSize().width() + element_padding +
         forward_->GetPreferredSize().width() + element_padding +
         reload_->GetPreferredSize().width() +
         (show_home_button_.GetValue()
              ? 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);
   }
   return SizeForContentSize(size);
 }
 
 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 =
         GetLayoutInsets(TOOLBAR).width() +
         back_->GetMinimumSize().width() + element_padding +
         forward_->GetMinimumSize().width() + element_padding +
         reload_->GetMinimumSize().width() +
         (show_home_button_.GetValue()
              ? 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);
   }
   return SizeForContentSize(size);
 }
 
 void ToolbarView::Layout() {
   // If we have not been initialized yet just do nothing.
   if (back_ == NULL)
     return;
 
   if (!is_display_mode_normal()) {
     location_bar_->SetBounds(0, PopupTopSpacing(), width(),
                              location_bar_->GetPreferredSize().height());
     return;
   }
 
   // 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.
   // TODO(abarth):  If the window becomes maximized but is not resized,
   //                then Layout() might not be called and the back button
   //                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);
     back_->SetLeadingMargin(insets.left());
   } else {
     back_->SetBounds(insets.left(), child_y, back_width, child_height);
     back_->SetLeadingMargin(0);
   }
   const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
   int next_element_x = back_->bounds().right() + element_padding;
@@ skipping 12 matching lines @@
     home_->SetVisible(true);
     home_->SetBounds(next_element_x, child_y,
                      home_->GetPreferredSize().width(), child_height);
   } else {
     home_->SetVisible(false);
     home_->SetBounds(next_element_x, child_y, 0, child_height);
   }
   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
   // re-adds everything, regardless of whether it has a page action. For a
   // page action, browser action bounds do not change, as a result of which
   // SetBounds does not do a layout at all.
   // TODO(sidchat): Rework the above behavior so that explicit layout is not
   //                required.
   browser_actions_->Layout();
 
@@ skipping 195 matching lines @@
 void ToolbarView::OnShowHomeButtonChanged() {
   Layout();
   SchedulePaint();
 }
 
 int ToolbarView::content_shadow_height() const {
   return GetLayoutConstant(
       (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) ?
           TOOLBAR_CONTENT_SHADOW_HEIGHT_ASH : TOOLBAR_CONTENT_SHADOW_HEIGHT);
 }