Add some extra height to bookmark bar if the font wants to be larger

chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
index 884ab3339ad7d9c523d5c869b6a761fe2e16f75a..bed906015d966a71e5a77107025ee5e41aee717b 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
@@ -172,7 +172,7 @@ static const int kAppsShortcutButtonTag = 2;
 
 // Preferred padding between text and edge.
 static const int kButtonPaddingHorizontal = 6;
-static const int kButtonPaddingVertical = 4;
+static const int kButtonPaddingVertical = 5;

[kylix_rd, 2016/08/05 18:32:09]

This value is inconsistent with the other buttons' padding and was causing flaky
behavior of the code which tries to expand the size of the bookmark bar to
accommodate larger fonts.

 
 static const gfx::ElideBehavior kElideBehavior = gfx::FADE_TAIL;
 
@@ -843,16 +843,33 @@ int BookmarkBarView::GetToolbarOverlap() const {
           size_animation_.GetCurrentValue());
 }
 
+int BookmarkBarView::GetPreferredHeight() const {
+  int height = 0;
+  for (int i = 0; i < child_count(); ++i) {
+    const views::View* view = child_at(i);
+    if (view->visible()) {
+      gfx::Size pref = view->GetPreferredSize();
+      // If this is the instructions view, enlarge it's height by the same

[Peter Kasting, 2016/08/05 21:23:00]

Nit: its

[kylix_rd, 2016/08/08 15:33:21]

Done.

+      // padding as the other buttons. This is necessary if the instructions
+      // view is the only visible view.

[Peter Kasting, 2016/08/05 21:23:00]

This comment confuses me a bit.  Are we basically just doing this because of
this?  (If so, feel free to change to this comment:)

We want as much padding around the instructions label as we have around the
labels in the buttons.

+      if (view == instructions_)
+        pref.Enlarge(0, kButtonPaddingVertical * 2);

[Peter Kasting, 2016/08/05 21:23:00]

What if we just set this as an empty border on the instructions view?  Would
that be good enough to avoid this GetPreferredHeight() method?  (Perhaps we also
need a layout manager?  That can autosize the height to the max of the preferred
child heights.)

[kylix_rd, 2016/08/08 15:33:21]

The instructions view doesn't respect the set borders. GetPreferredSize()
ignores them. An empty border could be set on the instructions_ view in Init and
BookmarkBarInstructionsView::GetPreferredSize() modified to respect them.
We need this method to determine whether the preferred height is larger than
chrome::kBookmarkBarHeight. IOW, kBookmarkBarHeight is the minimum height and
should only grow if any child view wants to be larger.

[Peter Kasting, 2016/08/08 19:10:01]

It seems offhand like part of the problem there is that the instructions view
does manual sizing and layout instead of just using a layout manager, which
would presumably pay attention to this value by default.  I know you tweaked it
to respect your new border, but only in GetPreferredSize(), and only the height
(and not width) of the border.  A layout manager seems much more robust.  Is
there a reason using one won't work?  (In this case, I'd think we'd need a
BoxLayout.)
Sure, but that could be done without the loop here (and only the max() call) if
the bookmark bar view was using a layout manager to compute the preferred size. 
Or it could probably be done without overriding this method at all by adding a
dummy child of kBookmarkBarHeight.

+      height = std::max(pref.height(), height);
+    }
+  }
+  return std::max(height, chrome::kBookmarkBarHeight);
+}
+
 gfx::Size BookmarkBarView::GetPreferredSize() const {
   gfx::Size prefsize;
   if (IsDetached()) {
+    int padded_height = GetPreferredHeight();
     prefsize.set_height(
-        chrome::kBookmarkBarHeight +
-        static_cast<int>(
-            (chrome::kNTPBookmarkBarHeight - chrome::kBookmarkBarHeight) *
-            (1 - size_animation_.GetCurrentValue())));
+        padded_height +
+        static_cast<int>((chrome::kNTPBookmarkBarHeight - padded_height) *
+                         (1 - size_animation_.GetCurrentValue())));
   } else {
-    prefsize.set_height(static_cast<int>(chrome::kBookmarkBarHeight *
+    prefsize.set_height(static_cast<int>((GetPreferredHeight()) *

[Peter Kasting, 2016/08/05 21:23:00]

Nit: No need for extra parens

[kylix_rd, 2016/08/08 15:33:21]

Done.

                                          size_animation_.GetCurrentValue()));
   }
   return prefsize;
@@ -879,7 +896,7 @@ gfx::Size BookmarkBarView::GetMinimumSize() const {
   // if they are visible.
   int width = GetHorizontalMargin();
 
-  int height = chrome::kBookmarkBarHeight;
+  int height = GetPreferredHeight();
   if (IsDetached()) {
     double current_state = 1 - size_animation_.GetCurrentValue();
     width += 2 * static_cast<int>(kNewTabHorizontalPadding * current_state);
@@ -925,7 +942,7 @@ void BookmarkBarView::Layout() {
   int top_margin = IsDetached() ? kDetachedTopMargin : 0;
   int y = top_margin;
   int width = View::width() - 2 * GetHorizontalMargin();
-  int height = chrome::kBookmarkBarHeight - kBottomMargin;
+  int height = GetPreferredHeight() - kBottomMargin;
   int separator_margin = kSeparatorMargin;
 
   if (IsDetached()) {
@@ -938,7 +955,7 @@ void BookmarkBarView::Layout() {
     // For the attached appearance, pin the content to the bottom of the bar
     // when animating in/out, as shrinking its height instead looks weird.  This
     // also matches how we layout infobars.
-    y += View::height() - chrome::kBookmarkBarHeight;
+    y += View::height() - GetPreferredHeight();
   }
 
   gfx::Size other_bookmarks_pref = other_bookmarks_button_->visible() ?