Eliminate GetLayoutInsets(TAB).

Callers outside Tab only referred to the vertical insets, which are now either 0
or Tab::GetStrokeWidth(), a new function added for this purpose, depending on
the context.

Inside Tab this is subsumed into GetTabEndcapWidth(), which itself has been
split into two parts: a general-purpose routine and a pre-refresh-specific
"paint" width routine.  This may be cleaned up further depending on how I solve
narrow tabs in the end.

BUG=none
TEST=none

Change-Id: Ifca96b76b694f3f922e0252cf5198c7a9bb475e3
Reviewed-on: https://chromium-review.googlesource.com/1112773Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570168}

chrome/browser/ui/layout_constants.cc

@@ -110,13 +110,6 @@ gfx::Insets GetLayoutInsets(LayoutInset inset) {
       return touch_optimized_material ? gfx::Insets(5) : gfx::Insets(4);
     }
 
-    case TAB: {
-      // TODO(pkasting): This should disappear; the horizontal portion should
-      // be computed in tab.cc, and the vertical portion become a standalone
-      // value (that should perhaps be 0 in Refresh).
-      constexpr int kTabHorizontalInset[] = {16, 18, 24, 16, 16};
-      return gfx::Insets(1, kTabHorizontalInset[mode]);
-    }
     case TOOLBAR_BUTTON:
       return gfx::Insets(touch_optimized_material ? 12 : 6);
 

chrome/browser/ui/layout_constants.h

@@ -107,9 +107,6 @@ enum LayoutInset {
   // (e.g. does not highlight on hover).
   LOCATION_BAR_ICON_INTERIOR_PADDING,
 
-  // The padding inside the tab bounds that defines the tab contents region.
-  TAB,
-
   // The padding inside the border of a toolbar button (around the image).
   TOOLBAR_BUTTON,
 

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

@@ -332,8 +332,8 @@ void BrowserNonClientFrameView::PaintToolbarBackground(
   if (tp->HasCustomImage(IDR_THEME_TOOLBAR)) {
     canvas->TileImageInt(*tp->GetImageSkiaNamed(IDR_THEME_TOOLBAR),
                          x + GetThemeBackgroundXInset(),
-                         y - GetTopInset(false) - GetLayoutInsets(TAB).top(), x,
-                         y, w, toolbar_bounds.height());
+                         y - GetTopInset(false) - Tab::GetStrokeHeight(), x, y,
+                         w, toolbar_bounds.height());
   } else {
     canvas->FillRect(toolbar_bounds,
                      tp->GetColor(ThemeProperties::COLOR_TOOLBAR));

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

@@ -513,19 +513,16 @@ int GlassBrowserFrameView::TopAreaHeight(bool restored) const {
   if (frame()->IsFullscreen() && !restored)
     return 0;
 
-  const int top = FrameTopBorderThickness(restored);
-  // The tab top inset is equal to the height of any shadow region above the
-  // tabs, plus a 1 px top stroke.  In maximized mode, we want to push the
-  // shadow region off the top of the screen but leave the top stroke.
-  if (IsMaximized() && !restored)
-    return top - GetLayoutInsets(TAB).top() + 1;
-
-  // Besides the frame border, there's empty space atop the window in restored
-  // mode, to use to drag the window around.
-  constexpr int kNonClientRestoredExtraThickness = 11;
-  constexpr int kRefreshNonClientRestoredExtraThickness = 4;
-  return top + (MD::IsRefreshUi() ? kRefreshNonClientRestoredExtraThickness
-                                  : kNonClientRestoredExtraThickness);
+  int top = FrameTopBorderThickness(restored);
+  if (!IsMaximized() || restored) {
+    // Besides the frame border, there's empty space atop the window in restored
+    // mode, to use to drag the window around.
+    constexpr int kNonClientRestoredExtraThickness = 11;
+    constexpr int kRefreshNonClientRestoredExtraThickness = 4;
+    top += MD::IsRefreshUi() ? kRefreshNonClientRestoredExtraThickness
+                             : kNonClientRestoredExtraThickness;
+  }
+  return top;
 }
 
 int GlassBrowserFrameView::TitlebarMaximizedVisualHeight() const {

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

@@ -161,24 +161,17 @@ int OpaqueBrowserFrameViewLayout::NonClientBorderThickness() const {
 }
 
 int OpaqueBrowserFrameViewLayout::NonClientTopHeight(bool restored) const {
-  if (delegate_->ShouldShowWindowTitle()) {
-    // The + 2 here puts at least 1 px of space on top and bottom of the icon.
-    const int icon_height =
-        TitlebarTopThickness(restored) + delegate_->GetIconSize() + 2;
-    const int caption_button_height = DefaultCaptionButtonY(restored) +
-                                      kCaptionButtonHeight +
-                                      kCaptionButtonBottomPadding;
-    return std::max(icon_height, caption_button_height) +
-        kContentEdgeShadowThickness;
-  }
-
-  int thickness = FrameTopBorderThickness(restored);
-  // The tab top inset is equal to the height of any shadow region above the
-  // tabs, plus a 1 px top stroke.  In maximized mode, we want to push the
-  // shadow region off the top of the screen but leave the top stroke.
-  if (!restored && delegate_->IsTabStripVisible() && IsTitleBarCondensed())
-    thickness -= GetLayoutInsets(TAB).top() - 1;
-  return thickness;
+  if (!delegate_->ShouldShowWindowTitle())
+    return FrameTopBorderThickness(restored);
+
+  // The + 2 here puts at least 1 px of space on top and bottom of the icon.
+  const int icon_height =
+      TitlebarTopThickness(restored) + delegate_->GetIconSize() + 2;
+  const int caption_button_height = DefaultCaptionButtonY(restored) +
+                                    kCaptionButtonHeight +
+                                    kCaptionButtonBottomPadding;
+  return std::max(icon_height, caption_button_height) +
+         kContentEdgeShadowThickness;
 }
 
 int OpaqueBrowserFrameViewLayout::GetTabStripInsetsTop(bool restored) const {

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

@@ -261,9 +261,8 @@ class OpaqueBrowserFrameViewLayoutTest : public views::ViewsTestBase {
     gfx::Rect tabstrip_bounds(
         layout_manager_->GetBoundsForTabStrip(tabstrip_min_size, kWindowWidth));
     EXPECT_EQ(tabstrip_x, tabstrip_bounds.x());
-    int maximized_top_border_height = -GetLayoutInsets(TAB).top() + 1;
     if (maximized) {
-      EXPECT_EQ(maximized_top_border_height, tabstrip_bounds.y());
+      EXPECT_EQ(0, tabstrip_bounds.y());
     } else {
       int tabstrip_nonexcluded_y =
           OpaqueBrowserFrameViewLayout::kFrameBorderThickness +
@@ -318,8 +317,7 @@ class OpaqueBrowserFrameViewLayoutTest : public views::ViewsTestBase {
       restored_border_height +=
           OpaqueBrowserFrameViewLayout::kRefreshNonClientExtraTopThickness;
     }
-    int top_border_height =
-        maximized ? maximized_top_border_height : restored_border_height;
+    int top_border_height = maximized ? 0 : restored_border_height;
     int min_height = top_border_height + browser_view_min_size.height();
     EXPECT_EQ(min_height, min_size.height());
   }

chrome/browser/ui/views/tabs/new_tab_button.cc

@@ -455,7 +455,7 @@ void NewTabButton::PaintFill(bool pressed,
       // For touch, the background matches the active tab background
       // positioning in Tab::PaintTab().
       const int offset_y =
-          is_touch_ui ? -GetLayoutInsets(TAB).top() : non_touch_offset_y;
+          is_touch_ui ? -Tab::GetStrokeHeight() : non_touch_offset_y;
       // The new tab background is mirrored in RTL mode, but the theme
       // background should never be mirrored. Mirror it here to compensate.
       float x_scale = 1.0f;

chrome/browser/ui/views/tabs/tab.cc

@@ -119,17 +119,20 @@ int Center(int size, int item_size) {
   return extra_space / 2;
 }
 
-// Returns the width of the tab endcap in DIP.  More precisely, this is the
+// Returns the width of the tab endcap in DIP.  Pre-refresh, this is the
 // width of the curve making up either the outer or inner edge of the stroke.
-//
-// For non-material-refresh mode, these two curves are horizontally offset by
-// 1 px (regardless of scale), the total width of the endcap from tab outer
-// edge to the inside end of the stroke inner edge is
-// (GetUnscaledEndcapWidth() * scale) + 1.
-float GetTabEndcapWidth() {
-  // TODO(pkasting): This should become a member function and vary with
-  // GetCornerRadius().
-  return GetLayoutInsets(TAB).left() - (MD::IsRefreshUi() ? 0.0f : 0.5f);
+int GetTabEndcapWidth() {
+  constexpr int kEndcapWidth[] = {16, 18, 24, 16, 16};
+  return kEndcapWidth[MD::GetMode()];
+}
+
+// Pre-refresh, endcaps paint slightly differently than they layout.
+float GetTabEndcapWidthForPainting() {
+  DCHECK(!MD::IsRefreshUi());
+
+  // For painting the endcaps, the top corners are actually shifted outwards 0.5
+  // DIP from the grid.
+  return GetTabEndcapWidth() - 0.5f;
 }
 
 void DrawHighlight(gfx::Canvas* canvas,
@@ -188,15 +191,14 @@ gfx::Path OffsetAndIntersectPaths(gfx::Path& left_path,
 // The refresh-specific implementation of GetInteriorPath() (see below).
 gfx::Path GetRefreshInteriorPath(float scale,
                                  const gfx::Rect& bounds,
-                                 float endcap_width,
                                  float horizontal_inset) {
+  const float endcap_width = GetTabEndcapWidth();
   const float radius = (endcap_width / 2) * scale;
 
   const gfx::RectF aligned_bounds =
       ScaleAndAlignBounds(bounds, endcap_width, scale);
   const float left = aligned_bounds.x();
-  const float top =
-      aligned_bounds.y() + (TabStrip::ShouldDrawStrokes() ? 1 : 0);
+  const float top = aligned_bounds.y() + Tab::GetStrokeHeight();
   const float right = aligned_bounds.right();
   const float bottom = aligned_bounds.bottom();
 
@@ -257,17 +259,15 @@ gfx::Path GetRefreshInteriorPath(float scale,
 // inset from the edge.
 gfx::Path GetInteriorPath(float scale,
                           const gfx::Rect& bounds,
-                          float endcap_width,
                           float horizontal_inset = 0) {
-  if (MD::IsRefreshUi()) {
-    return GetRefreshInteriorPath(scale, bounds, endcap_width,
-                                  horizontal_inset);
-  }
+  if (MD::IsRefreshUi())
+    return GetRefreshInteriorPath(scale, bounds, horizontal_inset);
 
   const float right = bounds.width() * scale;
   // The bottom of the tab needs to be pixel-aligned or else when we call
   // ClipPath with anti-aliasing enabled it can cause artifacts.
   const float bottom = std::ceil(bounds.height() * scale);
+  const float endcap_width = GetTabEndcapWidthForPainting();
 
   // Construct the interior path by intersecting paths representing the left
   // and right halves of the tab.  Compared to computing the full path at once,
@@ -304,8 +304,8 @@ gfx::Path GetInteriorPath(float scale,
 gfx::Path GetRefreshBorderPath(const gfx::Rect& bounds,
                                bool extend_to_top,
                                float scale,
-                               float endcap_width,
                                float stroke_thickness) {
+  const float endcap_width = GetTabEndcapWidth();
   const float outer_radius = (endcap_width / 2) * scale - stroke_thickness;
   const float inner_radius = (endcap_width / 2) * scale + stroke_thickness;
 
@@ -366,26 +366,25 @@ gfx::Path GetRefreshBorderPath(const gfx::Rect& bounds,
 }
 
 // Returns a path corresponding to the tab's outer border for a given tab
-// |bounds|, |scale|, and |endcap_width|.  If |unscale_at_end| is true, this
-// path will be normalized to a 1x scale by scaling by 1/scale before returning.
-// If |extend_to_top| is true, the path is extended vertically to the top of the
+// |scale| and |bounds|.  If |unscale_at_end| is true, this path will be
+// normalized to a 1x scale by scaling by 1/scale before returning.  If
+// |extend_to_top| is true, the path is extended vertically to the top of the
 // tab bounds.  The caller uses this for Fitts' Law purposes in
 // maximized/fullscreen mode.
 gfx::Path GetBorderPath(float scale,
                         bool unscale_at_end,
                         bool extend_to_top,
-                        float endcap_width,
                         const gfx::Rect& bounds) {
-  const float stroke_thickness = TabStrip::ShouldDrawStrokes() ? 1 : 0;
+  const float stroke_thickness = Tab::GetStrokeHeight();
 
   gfx::Path path;
   if (MD::IsRefreshUi()) {
-    path = GetRefreshBorderPath(bounds, extend_to_top, scale, endcap_width,
-                                stroke_thickness);
+    path = GetRefreshBorderPath(bounds, extend_to_top, scale, stroke_thickness);
   } else {
     const float top = scale - stroke_thickness;
     const float right = bounds.width() * scale;
     const float bottom = bounds.height() * scale;
+    const float endcap_width = GetTabEndcapWidthForPainting();
 
     path.moveTo(0, bottom);
     path.rLineTo(0, -stroke_thickness);
@@ -459,7 +458,8 @@ Tab::Tab(TabController* controller, gfx::AnimationContainer* container)
 
   // This will cause calls to GetContentsBounds to return only the rectangle
   // inside the tab shape, rather than to its extents.
-  SetBorder(views::CreateEmptyBorder(GetLayoutInsets(TAB)));
+  SetBorder(views::CreateEmptyBorder(
+      gfx::Insets(GetStrokeHeight(), GetTabEndcapWidth())));
 
   title_->SetHorizontalAlignment(gfx::ALIGN_TO_HEAD);
   title_->SetElideBehavior(gfx::FADE_TAIL);
@@ -558,10 +558,9 @@ bool Tab::GetHitTestMask(gfx::Path* mask) const {
   // shadow of the tab, such that the user can click anywhere along the top
   // edge of the screen to select a tab. Ditto for immersive fullscreen.
   const views::Widget* widget = GetWidget();
-  *mask =
-      GetBorderPath(GetWidget()->GetCompositor()->device_scale_factor(), true,
-                    widget && (widget->IsMaximized() || widget->IsFullscreen()),
-                    GetTabEndcapWidth(), bounds());
+  *mask = GetBorderPath(
+      GetWidget()->GetCompositor()->device_scale_factor(), true,
+      widget && (widget->IsMaximized() || widget->IsFullscreen()), bounds());
   return true;
 }
 
@@ -888,8 +887,8 @@ void Tab::PaintChildren(const views::PaintInfo& info) {
   // The paint recording scale for tabs is consistent along the x and y axis.
   const float paint_recording_scale = info.paint_recording_scale_x();
   constexpr int kFaviconPadding = 1;
-  clip_recorder.ClipPathWithAntiAliasing(GetInteriorPath(
-      paint_recording_scale, bounds(), GetTabEndcapWidth(), kFaviconPadding));
+  clip_recorder.ClipPathWithAntiAliasing(
+      GetInteriorPath(paint_recording_scale, bounds(), kFaviconPadding));
   View::PaintChildren(info);
 }
 
@@ -903,7 +902,7 @@ void Tab::OnPaint(gfx::Canvas* canvas) {
   if (!controller_->ShouldPaintTab(
           this,
           base::BindRepeating(&GetBorderPath, canvas->image_scale(), true,
-                              false, GetTabEndcapWidth()),
+                              false),
           &clip))
     return;
 
@@ -1093,7 +1092,7 @@ void Tab::FrameColorsChanged() {
 
 // static
 int Tab::GetMinimumInactiveWidth() {
-  return GetLayoutInsets(TAB).width();
+  return GetTabEndcapWidth() * 2;
 }
 
 // static
@@ -1112,8 +1111,16 @@ int Tab::GetPinnedWidth() {
   return kTabPinnedContentWidth + GetMinimumInactiveWidth();
 }
 
+// static
+int Tab::GetStrokeHeight() {
+  return TabStrip::ShouldDrawStrokes() ? 1 : 0;
+}
+
 // static
 float Tab::GetInverseDiagonalSlope() {
+  // In refresh, tab sides do not have slopes, so no one should call this.
+  DCHECK(!MD::IsRefreshUi());
+
   // This is computed from the border path as follows:
   // * The endcap width is enough for the whole stroke outer curve, i.e. the
   //   side diagonal plus the curves on both its ends.
@@ -1124,13 +1131,14 @@ float Tab::GetInverseDiagonalSlope() {
   //   so the diagonal is ((height - 1.5 - 1.5) * scale - 1 - (scale - 1)) px
   //   high.  Simplifying this gives (height - 4) * scale px, or (height - 4)
   //   DIP.
-  return (GetTabEndcapWidth() - 4) / (GetLayoutConstant(TAB_HEIGHT) - 4);
+  return (GetTabEndcapWidthForPainting() - 4) /
+         (GetLayoutConstant(TAB_HEIGHT) - 4);
 }
 
 // static
 int Tab::GetOverlap() {
   // We want to overlap the endcap portions entirely.
-  return gfx::ToCeiledInt(GetTabEndcapWidth());
+  return GetTabEndcapWidth();
 }
 
 void Tab::RepaintSubsequentTab() {
@@ -1161,7 +1169,7 @@ void Tab::PaintTab(gfx::Canvas* canvas, const gfx::Path& clip) {
   int active_tab_y_offset = 0;
   if (GetThemeProvider()->HasCustomImage(IDR_THEME_TOOLBAR)) {
     active_tab_fill_id = IDR_THEME_TOOLBAR;
-    active_tab_y_offset = -GetLayoutInsets(TAB).top();
+    active_tab_y_offset = -GetStrokeHeight();
   }
 
   if (IsActive()) {
@@ -1210,7 +1218,6 @@ void Tab::PaintTabBackground(gfx::Canvas* canvas,
   // |y_offset| is only set when |fill_id| is being used.
   DCHECK(!y_offset || fill_id);
 
-  const float endcap_width = GetTabEndcapWidth();
   const SkColor active_color = controller_->GetTabBackgroundColor(TAB_ACTIVE);
   const SkColor inactive_color =
       controller_->GetTabBackgroundColor(TAB_INACTIVE);
@@ -1225,18 +1232,18 @@ void Tab::PaintTabBackground(gfx::Canvas* canvas,
   // If |paint_hover_effect|, we don't try to cache since hover effects change
   // on every invalidation and we would need to invalidate the cache based on
   // the hover states.
+  //
   // Finally, in refresh, we don't cache for non-integral scale factors, since
   // tabs draw with slightly different offsets so as to pixel-align the layout
   // rect (see ScaleAndAlignBounds()).
   const float scale = canvas->image_scale();
   if (fill_id || paint_hover_effect ||
       (MD::IsRefreshUi() && (std::trunc(scale) != scale))) {
-    gfx::Path fill_path = GetInteriorPath(scale, bounds(), endcap_width);
+    gfx::Path fill_path = GetInteriorPath(scale, bounds());
     PaintTabBackgroundFill(canvas, fill_path, active, paint_hover_effect,
                            active_color, inactive_color, fill_id, y_offset);
     if (TabStrip::ShouldDrawStrokes()) {
-      gfx::Path stroke_path =
-          GetBorderPath(scale, false, false, endcap_width, bounds());
+      gfx::Path stroke_path = GetBorderPath(scale, false, false, bounds());
       gfx::ScopedCanvas scoped_canvas(clip ? canvas : nullptr);
       if (clip)
         canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);
@@ -1248,9 +1255,8 @@ void Tab::PaintTabBackground(gfx::Canvas* canvas,
         active ? background_active_cache_ : background_inactive_cache_;
     if (!cache.CacheKeyMatches(scale, size(), active_color, inactive_color,
                                stroke_color)) {
-      gfx::Path fill_path = GetInteriorPath(scale, bounds(), endcap_width);
-      gfx::Path stroke_path =
-          GetBorderPath(scale, false, false, endcap_width, bounds());
+      gfx::Path fill_path = GetInteriorPath(scale, bounds());
+      gfx::Path stroke_path = GetBorderPath(scale, false, false, bounds());
       cc::PaintRecorder recorder;
 
       {

chrome/browser/ui/views/tabs/tab.h

@@ -187,6 +187,10 @@ class Tab : public gfx::AnimationDelegate,
   // Returns the width for pinned tabs. Pinned tabs always have this width.
   static int GetPinnedWidth();
 
+  // Returns the height of any area reserved for a stroke at the top and bottom
+  // of the tab, in DIP.
+  static int GetStrokeHeight();
+
   // Returns the inverse of the slope of the diagonal portion of the tab outer
   // border.  (This is a positive value, so it's specifically for the slope of
   // the leading edge.)