Recalculate positioning and height of bookmark bar icons.

The calculations for bookmark bar icons should only impact the start y location of the icons and not the height.

Regression mentioned in comment #5 in the bug is due to removing kBookmarkBarBottomMargin in the previous CL for the bug.
The underlying reason for this is that GetPreferredHeight() was being used for both the bookmark height as well as the button heights.

Bug: 865555
Change-Id: Ief5328a2a3a8c23ebfbaa9a393f1c3af4c7a6920
Reviewed-on: https://chromium-review.googlesource.com/1170226
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584470}

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

@@ -858,7 +858,12 @@ int BookmarkBarView::GetToolbarOverlap() const {
 }
 
 int BookmarkBarView::GetPreferredHeight() const {
-  int height = GetLayoutConstant(BOOKMARK_BAR_HEIGHT);
+  return std::max(GetTallestButtonHeight(),
+                  GetLayoutConstant(BOOKMARK_BAR_HEIGHT));
+}
+
+int BookmarkBarView::GetTallestButtonHeight() const {
+  int height = 0;
   for (int i = 0; i < child_count(); ++i) {
     const views::View* view = child_at(i);
     if (view->visible())
@@ -948,12 +953,15 @@ void BookmarkBarView::Layout() {
   int x = kBookmarkBarHorizontalMargin;
   int width = View::width() - 2 * kBookmarkBarHorizontalMargin;
 
-  int height = GetPreferredHeight();
+  int height = GetTallestButtonHeight();
+
   int y = (GetContentsBounds().height() - height) / 2;
 
   if (browser_view_) {
-    height -= browser_view_->GetAdditionalBookmarkBarBottomMargin();
-    y += browser_view_->GetAdditionalBookmarkBarTopMargin();
+    y += browser_view_->GetBookmarkBarContentVerticalOffset();
+
+    // Ensure y is within bounds of the bookmark bar view.
+    y = std::max(0, y);
   }
 
   gfx::Size other_bookmarks_pref = other_bookmarks_button_->visible() ?

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

@@ -396,8 +396,13 @@ class BookmarkBarView : public views::AccessiblePaneView,
   // or -1 if |button| is not a bookmark button from this bar.
   int GetIndexForButton(views::View* button);
 
+  // Gets the preferred height of the bookmark bar which is the max of either
+  // the bookmark bar constant or the tallest icon.
   int GetPreferredHeight() const;
 
+  // Gets the height of the tallest visible button on the bookmark bar.
+  int GetTallestButtonHeight() const;
+
   // Needed to react to kShowAppsShortcutInBookmarkBar changes.
   PrefChangeRegistrar profile_pref_registrar_;
 

chrome/browser/ui/views/chrome_layout_provider.cc

@@ -71,7 +71,7 @@ gfx::Insets ChromeLayoutProvider::GetInsetsMetric(int metric) const {
     case INSETS_BOOKMARKS_BAR_BUTTON:
       if (ui::MaterialDesignController::IsTouchOptimizedUiEnabled())
         return gfx::Insets(8, 12);
-      return GetInsetsMetric(views::InsetsMetric::INSETS_LABEL_BUTTON);
+      return gfx::Insets(6);
     case INSETS_TOAST:
       return gfx::Insets(0, kHarmonyLayoutUnit);
     default:

chrome/browser/ui/views/frame/browser_view.cc

@@ -1770,33 +1770,38 @@ FullscreenControlHost* BrowserView::GetFullscreenControlHost() {
   return fullscreen_control_host_.get();
 }
 
-int BrowserView::GetAdditionalBookmarkBarBottomMargin() {
-  // If the bookmark bar is attached, the bottom inset should be the same as the
-  // height between the bottom of the location bar to the bottom of the toolbar,
-  // taking into account the overlap between the bookmark bar and toolbar.
-  if (toolbar_ && bookmark_bar_view_.get() &&
-      !bookmark_bar_view_->IsDetached()) {
-    return (toolbar_->height() - GetLocationBarView()->height() -
-            bookmark_bar_view_->GetToolbarOverlap()) /
-           2;
-  }
+int BrowserView::GetBookmarkBarContentVerticalOffset() const {
+  if (!bookmark_bar_view_.get()) {
     return 0;
-}
+  }
 
-int BrowserView::GetAdditionalBookmarkBarTopMargin() {
-  // If the infobar is being displayed and the bookmark is detached, there
-  // should be an additional inset at the top to center the bookmarks a bit
-  // below the shadow of the infobar.
-  if (IsInfoBarVisible() && bookmark_bar_view_.get() &&
-      bookmark_bar_view_->IsDetached()) {
-    // 2 is roughly the number of visible pixels of the shadow over the bookmark
-    // bar, so if we shift the top of the bookmark buttons down by this value,
-    // it will appear centered.
+  // If the info bar is visible and the bookmark bar is detached, the info bar
+  // will be rendered above the bookmark bar. When this is the case, the shadow
+  // of the info bar will overlap onto the bookmark bar. The icons on the
+  // bookmark bar should be moved down so the icons appeared centered on the non
+  // shaded parts of the bookmark bar.
+  if (IsInfoBarVisible() && bookmark_bar_view_->IsDetached()) {
+    // 2 is roughly the number of visible pixels of the infobar shadow over the
+    // bookmark bar, so if we shift the top of the bookmark buttons down by this
+    // value, it will appear centered.
     return 2;
   }
+
+  // If the bookmark bar is attached, there will appear to be extra space above
+  // the bookmark bar icons due to the space below the location bar on the
+  // toolbar. The bookmark bar icons need to be moved up to compensate.
+  if (!bookmark_bar_view_->IsDetached()) {
+    return -GetBottomInsetOfLocationBarWithinToolbar() / 2;
+  }
   return 0;
 }
 
+int BrowserView::GetBottomInsetOfLocationBarWithinToolbar() const {
+  return (toolbar_->height() - GetLocationBarView()->height() -
+          bookmark_bar_view_->GetToolbarOverlap()) /
+         2;
+}
+
 views::View* BrowserView::GetInitiallyFocusedView() {
   return nullptr;
 }

chrome/browser/ui/views/frame/browser_view.h

@@ -519,16 +519,10 @@ class BrowserView : public BrowserWindow,
   // not yet exist.
   FullscreenControlHost* GetFullscreenControlHost();
 
-  // Gets the additional bottom margin for the bookmark bar. Returns non-zero
-  // when the bookmark bar is attached. This ensures the buttons appear centered
-  // between the bottom of the omnibox and the bottom of the bookmark bar.
-  int GetAdditionalBookmarkBarBottomMargin();
-
-  // Gets the additional top margin for the bookmark bar. Returns non-zero when
-  // the bookmark bar is detached and an infobar is showing. This ensures the
-  // bookmark buttons appear centered within the area that is not covered by the
-  // infobar's shadow.
-  int GetAdditionalBookmarkBarTopMargin();
+  // Gets the amount to vertically shift the placement of the icons on the
+  // bookmark bar so the icons appear centered relative to the views above and
+  // below them.
+  int GetBookmarkBarContentVerticalOffset() const;
 
  private:
   // Do not friend BrowserViewLayout. Use the BrowserViewLayoutDelegate
@@ -644,6 +638,11 @@ class BrowserView : public BrowserWindow,
       version_info::Channel,
       Profile* profile) const;
 
+  // Returns the amount of space between the bottom of the location bar to the
+  // bottom of the toolbar. This does not include the part of the toolbar that
+  // overlaps with the bookmark bar.
+  int GetBottomInsetOfLocationBarWithinToolbar() const;
+
   // The BrowserFrame that hosts this view.
   BrowserFrame* frame_ = nullptr;
 

chrome/browser/ui/views/material_refresh_layout_provider.cc

@@ -24,7 +24,7 @@ gfx::Insets MaterialRefreshLayoutProvider::GetInsetsMetric(int metric) const {
       // circular for favicon-only bookmarks.
       if (ui::MaterialDesignController::IsTouchOptimizedUiEnabled())
         return gfx::Insets(8, 10);
-      return gfx::Insets(5, 6);
+      break;
   }
   return ChromeLayoutProvider::GetInsetsMetric(metric);
 }