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

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

 // Copyright (c) 2012 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/bookmarks/bookmark_bar_view.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <limits>
@@ skipping 154 matching lines @@
 static const int kSeparatorStartX = 2;
 
 // Left-padding for the instructional text.
 static const int kInstructionsPadding = 6;
 
 // Tag for the 'Apps Shortcut' button.
 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;
 
 namespace {
 
 // To enable/disable BookmarkBar animations during testing. In production
 // animations are enabled by default.
 bool animations_enabled = true;
 
 gfx::ImageSkia* GetImageSkiaNamed(int id) {
@@ skipping 650 matching lines @@
   if (infobar_visible_)
     return detached_overlap;
 
   // When detached with no infobar, animate the overlap between the attached and
   // detached states.
   return detached_overlap + static_cast<int>(
       (attached_overlap - detached_overlap) *
           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;
 }
 
 bool BookmarkBarView::CanProcessEventsWithinSubtree() const {
   // If the bookmark bar is attached and the omnibox popup is open (on top of
   // the bar), prevent events from targeting the bookmark bar or any of its
   // descendants. This will prevent hovers/clicks just above the omnibox popup
   // from activating the top few pixels of items on the bookmark bar.
   if (!IsDetached() && browser_view_ &&
       browser_view_->GetLocationBar()->GetOmniboxView()->model()->
           popup_model()->IsOpen()) {
     return false;
   }
   return true;
 }
 
 gfx::Size BookmarkBarView::GetMinimumSize() const {
   // The minimum width of the bookmark bar should at least contain the overflow
   // button, by which one can access all the Bookmark Bar items, and the "Other
   // Bookmarks" folder, along with appropriate margins and button padding.
   // It should also contain the Managed and/or Supervised Bookmarks folders,
   // 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);
     height += static_cast<int>(
         (chrome::kNTPBookmarkBarHeight - chrome::kBookmarkBarHeight) *
             current_state);
   }
 
   if (managed_bookmarks_button_->visible()) {
     gfx::Size size = managed_bookmarks_button_->GetPreferredSize();
@@ skipping 25 matching lines @@
 
 void BookmarkBarView::Layout() {
   // Skip layout during destruction, when no model exists.
   if (!model_)
     return;
 
   int x = GetHorizontalMargin();
   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()) {
     double current_state = 1 - size_animation_.GetCurrentValue();
     x += static_cast<int>(kNewTabHorizontalPadding * current_state);
     y += (View::height() - chrome::kBookmarkBarHeight) / 2;
     width -= static_cast<int>(kNewTabHorizontalPadding * current_state);
     separator_margin -= static_cast<int>(kSeparatorMargin * current_state);
   } else {
     // 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() ?
       other_bookmarks_button_->GetPreferredSize() : gfx::Size();
   gfx::Size overflow_pref = overflow_button_->GetPreferredSize();
   gfx::Size bookmarks_separator_pref =
       bookmarks_separator_view_->GetPreferredSize();
   gfx::Size apps_page_shortcut_pref = apps_page_shortcut_->visible() ?
       apps_page_shortcut_->GetPreferredSize() : gfx::Size();
 
@@ skipping 1222 matching lines @@
     return;
   apps_page_shortcut_->SetVisible(visible);
   UpdateBookmarksSeparatorVisibility();
   LayoutAndPaint();
 }
 
 void BookmarkBarView::OnShowManagedBookmarksPrefChanged() {
   if (UpdateOtherAndManagedButtonsVisibility())
     LayoutAndPaint();
 }