[GTK] Improve frame top area padding

When using native GTK frame buttons, we only want the padding returned by
NavButtonProvider::GetTopAreaSpacing() to apply to the side(s) that actually
have frame buttons on them, otherwise the padding on a tab that is adjacent to
the frame edge will depend on which GTK theme is used, rather than being
constant across all themes.

BUG=None
R=pkasting

Change-Id: Ibf5bb40c0d51d6387ca2b21abc54b1adad8f69b0
Reviewed-on: https://chromium-review.googlesource.com/c/1287261
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611939}

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

@@ -36,7 +36,7 @@ void DesktopLinuxBrowserFrameView::Layout() {
 
 void DesktopLinuxBrowserFrameView::MaybeUpdateCachedFrameButtonImages() {
   DrawFrameButtonParams params{
-      GetTopAreaHeight() - layout()->TitlebarTopThickness(!IsMaximized()),
+      GetTopAreaHeight() - layout()->FrameTopThickness(!IsMaximized()),
       IsMaximized(), ShouldPaintAsActive()};
   if (cache_ == params)
     return;

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

@@ -14,12 +14,21 @@ int DesktopLinuxBrowserFrameViewLayout::CaptionButtonY(
     chrome::FrameButtonDisplayType button_id,
     bool restored) const {
   gfx::Insets insets = nav_button_provider_->GetNavButtonMargin(button_id);
-  return insets.top() + TitlebarTopThickness();
+  return insets.top() + FrameTopThickness(!delegate_->IsMaximized());
 }
 
-int DesktopLinuxBrowserFrameViewLayout::TopAreaPadding() const {
-  return nav_button_provider_->GetTopAreaSpacing().left() +
-         TitlebarTopThickness();
+OpaqueBrowserFrameViewLayout::TopAreaPadding
+DesktopLinuxBrowserFrameViewLayout::GetTopAreaPadding(
+    bool has_leading_buttons,
+    bool has_trailing_buttons) const {
+  gfx::Insets insets =
+      nav_button_provider_->GetTopAreaSpacing() +
+      gfx::Insets(0, FrameSideThickness(!delegate_->IsMaximized()));
+  const int leading = base::i18n::IsRTL() ? insets.right() : insets.left();
+  const int trailing = base::i18n::IsRTL() ? insets.left() : insets.right();
+  const int padding = FrameBorderThickness(false);
+  return {has_leading_buttons ? leading : padding,
+          has_trailing_buttons ? trailing : padding};
 }
 
 int DesktopLinuxBrowserFrameViewLayout::GetWindowCaptionSpacing(
@@ -41,8 +50,3 @@ bool DesktopLinuxBrowserFrameViewLayout::ShouldDrawImageMirrored(
     ButtonAlignment alignment) const {
   return false;
 }
-
-int DesktopLinuxBrowserFrameViewLayout::TitlebarTopThickness() const {
-  return OpaqueBrowserFrameViewLayout::TitlebarTopThickness(
-      !delegate_->IsMaximized());
-}

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

@@ -14,10 +14,12 @@ class DesktopLinuxBrowserFrameViewLayout : public OpaqueBrowserFrameViewLayout {
   DesktopLinuxBrowserFrameViewLayout(
       views::NavButtonProvider* nav_button_provider);
 
+ protected:
   // OpaqueBrowserFrameViewLayout:
   int CaptionButtonY(chrome::FrameButtonDisplayType button_id,
                      bool restored) const override;
-  int TopAreaPadding() const override;
+  TopAreaPadding GetTopAreaPadding(bool has_leading_buttons,
+                                   bool has_trailing_buttons) const override;
   int GetWindowCaptionSpacing(views::FrameButton button_id,
                               bool leading_spacing,
                               bool is_leading_button) const override;
@@ -25,8 +27,6 @@ class DesktopLinuxBrowserFrameViewLayout : public OpaqueBrowserFrameViewLayout {
                                ButtonAlignment alignment) const override;
 
  private:
-  int TitlebarTopThickness() const;
-
   views::NavButtonProvider* nav_button_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(DesktopLinuxBrowserFrameViewLayout);

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

@@ -176,9 +176,14 @@ class DesktopLinuxBrowserFrameViewLayoutTest : public ChromeViewsTestBase {
     }
   }
 
-  int TitlebarTopThickness() const {
+  int FrameTopThickness() const {
     return static_cast<OpaqueBrowserFrameViewLayout*>(layout_manager_)
-        ->TitlebarTopThickness(false);
+        ->FrameTopThickness(false);
+  }
+
+  int FrameSideThickness() const {
+    return static_cast<OpaqueBrowserFrameViewLayout*>(layout_manager_)
+        ->FrameSideThickness(false);
   }
 
   views::Widget* widget_ = nullptr;
@@ -208,21 +213,22 @@ TEST_F(DesktopLinuxBrowserFrameViewLayoutTest, NativeNavButtons) {
 
   root_view_->Layout();
 
-  const int frame_thickness = TitlebarTopThickness();
-  int x = frame_thickness;
+  const int frame_top_thickness = FrameTopThickness();
+
+  int x = FrameSideThickness();
 
   // Close button.
   EXPECT_EQ(kCloseButtonSize, close_button_->size());
   x += kTopAreaSpacing.left() + kCloseButtonMargin.left();
   EXPECT_EQ(x, close_button_->x());
-  EXPECT_EQ(kCloseButtonMargin.top() + frame_thickness, close_button_->y());
+  EXPECT_EQ(kCloseButtonMargin.top() + frame_top_thickness, close_button_->y());
 
   // Maximize button.
   EXPECT_EQ(kMaximizeButtonSize, maximize_button_->size());
   x += kCloseButtonSize.width() + kCloseButtonMargin.right() +
        kInterNavButtonSpacing + kMaximizeButtonMargin.left();
   EXPECT_EQ(x, maximize_button_->x());
-  EXPECT_EQ(kMaximizeButtonMargin.top() + frame_thickness,
+  EXPECT_EQ(kMaximizeButtonMargin.top() + frame_top_thickness,
             maximize_button_->y());
 
   // Minimize button.
@@ -230,6 +236,6 @@ TEST_F(DesktopLinuxBrowserFrameViewLayoutTest, NativeNavButtons) {
   x += kMaximizeButtonSize.width() + kMaximizeButtonMargin.right() +
        kInterNavButtonSpacing + kMinimizeButtonMargin.left();
   EXPECT_EQ(x, minimize_button_->x());
-  EXPECT_EQ(kMinimizeButtonMargin.top() + frame_thickness,
+  EXPECT_EQ(kMinimizeButtonMargin.top() + frame_top_thickness,
             minimize_button_->y());
 }

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

@@ -43,8 +43,11 @@ const int OpaqueBrowserFrameViewLayout::kContentEdgeShadowThickness = 2;
 // each side regardless of the system window border size.
 const int OpaqueBrowserFrameViewLayout::kFrameBorderThickness = 4;
 
-// The titlebar has a 2 px 3D edge along the top.
-const int OpaqueBrowserFrameViewLayout::kTitlebarTopEdgeThickness = 2;
+// The frame has a 2 px 3D edge along the top.
+const int OpaqueBrowserFrameViewLayout::kTopFrameEdgeThickness = 2;
+
+// The frame has a 1 px 3D edge along the top.
+const int OpaqueBrowserFrameViewLayout::kSideFrameEdgeThickness = 1;
 
 // The icon is inset 1 px from the left frame border.
 const int OpaqueBrowserFrameViewLayout::kIconLeftSpacing = 1;
@@ -64,8 +67,8 @@ OpaqueBrowserFrameViewLayout::OpaqueBrowserFrameViewLayout()
     : available_space_leading_x_(0),
       available_space_trailing_x_(0),
       minimum_size_for_buttons_(0),
-      has_leading_buttons_(false),
-      has_trailing_buttons_(false),
+      placed_leading_button_(false),
+      placed_trailing_button_(false),
       extra_caption_y_(kCaptionButtonSpacing),
       forced_window_caption_spacing_(-1),
       minimize_button_(nullptr),
@@ -142,8 +145,8 @@ int OpaqueBrowserFrameViewLayout::NonClientTopHeight(bool restored) const {
   // Adding 2px of vertical padding puts at least 1 px of space on the top and
   // bottom of the element.
   constexpr int kVerticalPadding = 2;
-  const int icon_height = TitlebarTopThickness(restored) +
-                          delegate_->GetIconSize() + kVerticalPadding;
+  const int icon_height =
+      FrameTopThickness(restored) + delegate_->GetIconSize() + kVerticalPadding;
   const int caption_button_height = DefaultCaptionButtonY(restored) +
                                     kCaptionButtonHeight +
                                     kCaptionButtonBottomPadding;
@@ -165,12 +168,12 @@ int OpaqueBrowserFrameViewLayout::GetTabStripInsetsTop(bool restored) const {
              : (top + GetNonClientRestoredExtraThickness());
 }
 
-int OpaqueBrowserFrameViewLayout::TitlebarTopThickness(bool restored) const {
-  if (!delegate_->UseCustomFrame())
-    return 0;
-  return restored || !delegate_->IsFrameCondensed()
-             ? kTitlebarTopEdgeThickness
-             : FrameBorderThickness(false);
+int OpaqueBrowserFrameViewLayout::FrameTopThickness(bool restored) const {
+  return IsFrameEdgeVisible(restored) ? kTopFrameEdgeThickness : 0;
+}
+
+int OpaqueBrowserFrameViewLayout::FrameSideThickness(bool restored) const {
+  return IsFrameEdgeVisible(restored) ? kSideFrameEdgeThickness : 0;
 }
 
 int OpaqueBrowserFrameViewLayout::DefaultCaptionButtonY(bool restored) const {
@@ -189,10 +192,6 @@ int OpaqueBrowserFrameViewLayout::CaptionButtonY(
   return DefaultCaptionButtonY(restored);
 }
 
-int OpaqueBrowserFrameViewLayout::TopAreaPadding() const {
-  return FrameBorderThickness(false);
-}
-
 gfx::Rect OpaqueBrowserFrameViewLayout::IconBounds() const {
   return window_icon_bounds_;
 }
@@ -260,10 +259,23 @@ int OpaqueBrowserFrameViewLayout::GetNonClientRestoredExtraThickness() const {
 ///////////////////////////////////////////////////////////////////////////////
 // OpaqueBrowserFrameViewLayout, protected:
 
+OpaqueBrowserFrameViewLayout::TopAreaPadding
+OpaqueBrowserFrameViewLayout::GetTopAreaPadding(
+    bool has_leading_buttons,
+    bool has_trailing_buttons) const {
+  const int padding = FrameBorderThickness(false);
+  return {padding, padding};
+}
+
+bool OpaqueBrowserFrameViewLayout::IsFrameEdgeVisible(bool restored) const {
+  return delegate_->UseCustomFrame() &&
+         (restored || !delegate_->IsFrameCondensed());
+}
+
 bool OpaqueBrowserFrameViewLayout::ShouldDrawImageMirrored(
     views::ImageButton* button,
     ButtonAlignment alignment) const {
-  return alignment == ALIGN_LEADING && !has_leading_buttons_ &&
+  return alignment == ALIGN_LEADING && !placed_leading_button_ &&
          button == close_button_;
 }
 
@@ -311,7 +323,7 @@ void OpaqueBrowserFrameViewLayout::LayoutTitleBar(views::View* host) {
     // slightly uncentered with restored windows, so when the window is
     // restored, instead of calculating the remaining space from below the
     // frame border, we calculate from below the 3D edge.
-    const int unavailable_px_at_top = TitlebarTopThickness(false);
+    const int unavailable_px_at_top = FrameTopThickness(false);
     // When the icon is shorter than the minimum space we reserve for the
     // caption button, we vertically center it.  We want to bias rounding to
     // put extra space below the icon, since we'll use the same Y coordinate for
@@ -353,7 +365,7 @@ void OpaqueBrowserFrameViewLayout::LayoutTitleBar(views::View* host) {
   }
 
   if (use_hidden_icon_location) {
-    if (has_leading_buttons_) {
+    if (placed_leading_button_) {
       // There are window button icons on the left. Don't size the hidden window
       // icon that people can double click on to close the window.
       window_icon_bounds_ = gfx::Rect();
@@ -438,19 +450,20 @@ void OpaqueBrowserFrameViewLayout::SetBoundsForButton(
   // a little notch to fit in the rounded frame.
   button->SetDrawImageMirrored(ShouldDrawImageMirrored(button, alignment));
 
-  int extra_width = TopAreaPadding();
+  TopAreaPadding top_area_padding = GetTopAreaPadding();
 
   switch (alignment) {
     case ALIGN_LEADING: {
+      int extra_width = top_area_padding.leading;
       int button_start_spacing =
-          GetWindowCaptionSpacing(button_id, true, !has_leading_buttons_);
+          GetWindowCaptionSpacing(button_id, true, !placed_leading_button_);
 
       available_space_leading_x_ += button_start_spacing;
       minimum_size_for_buttons_ += button_start_spacing;
 
       bool top_spacing_clickable = is_frame_condensed;
       bool start_spacing_clickable =
-          is_frame_condensed && !has_leading_buttons_;
+          is_frame_condensed && !placed_leading_button_;
       button->SetBounds(
           available_space_leading_x_ - (start_spacing_clickable
                                             ? button_start_spacing + extra_width
@@ -462,22 +475,23 @@ void OpaqueBrowserFrameViewLayout::SetBoundsForButton(
           button_size.height() + (top_spacing_clickable ? caption_y : 0));
 
       int button_end_spacing =
-          GetWindowCaptionSpacing(button_id, false, !has_leading_buttons_);
+          GetWindowCaptionSpacing(button_id, false, !placed_leading_button_);
       available_space_leading_x_ += button_size.width() + button_end_spacing;
       minimum_size_for_buttons_ += button_size.width() + button_end_spacing;
-      has_leading_buttons_ = true;
+      placed_leading_button_ = true;
       break;
     }
     case ALIGN_TRAILING: {
+      int extra_width = top_area_padding.trailing;
       int button_start_spacing =
-          GetWindowCaptionSpacing(button_id, true, !has_trailing_buttons_);
+          GetWindowCaptionSpacing(button_id, true, !placed_trailing_button_);
 
       available_space_trailing_x_ -= button_start_spacing;
       minimum_size_for_buttons_ += button_start_spacing;
 
       bool top_spacing_clickable = is_frame_condensed;
       bool start_spacing_clickable =
-          is_frame_condensed && !has_trailing_buttons_;
+          is_frame_condensed && !placed_trailing_button_;
       button->SetBounds(
           available_space_trailing_x_ - button_size.width(),
           top_spacing_clickable ? 0 : caption_y,
@@ -487,10 +501,10 @@ void OpaqueBrowserFrameViewLayout::SetBoundsForButton(
           button_size.height() + (top_spacing_clickable ? caption_y : 0));
 
       int button_end_spacing =
-          GetWindowCaptionSpacing(button_id, false, !has_trailing_buttons_);
+          GetWindowCaptionSpacing(button_id, false, !placed_trailing_button_);
       available_space_trailing_x_ -= button_size.width() + button_end_spacing;
       minimum_size_for_buttons_ += button_size.width() + button_end_spacing;
-      has_trailing_buttons_ = true;
+      placed_trailing_button_ = true;
       break;
     }
   }
@@ -550,19 +564,25 @@ void OpaqueBrowserFrameViewLayout::SetView(int id, views::View* view) {
   }
 }
 
+OpaqueBrowserFrameViewLayout::TopAreaPadding
+OpaqueBrowserFrameViewLayout::GetTopAreaPadding() const {
+  return GetTopAreaPadding(!leading_buttons_.empty(),
+                           !trailing_buttons_.empty());
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // OpaqueBrowserFrameViewLayout, views::LayoutManager:
 
 void OpaqueBrowserFrameViewLayout::Layout(views::View* host) {
   TRACE_EVENT0("views.frame", "OpaqueBrowserFrameViewLayout::Layout");
   // Reset all our data so that everything is invisible.
-  int top_area_padding = TopAreaPadding();
-  available_space_leading_x_ = top_area_padding;
-  available_space_trailing_x_ = host->width() - top_area_padding;
+  TopAreaPadding top_area_padding = GetTopAreaPadding();
+  available_space_leading_x_ = top_area_padding.leading;
+  available_space_trailing_x_ = host->width() - top_area_padding.trailing;
   minimum_size_for_buttons_ =
       available_space_leading_x_ + host->width() - available_space_trailing_x_;
-  has_leading_buttons_ = false;
-  has_trailing_buttons_ = false;
+  placed_leading_button_ = false;
+  placed_trailing_button_ = false;
 
   LayoutWindowControls(host);
   LayoutTitleBar(host);

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

@@ -33,7 +33,8 @@ class OpaqueBrowserFrameViewLayout : public views::LayoutManager {
   // Constants public for testing only.
   static constexpr int kNonClientExtraTopThickness = 1;
   static const int kFrameBorderThickness;
-  static const int kTitlebarTopEdgeThickness;
+  static const int kTopFrameEdgeThickness;
+  static const int kSideFrameEdgeThickness;
   static const int kIconLeftSpacing;
   static const int kIconTitleSpacing;
   static const int kCaptionSpacing;
@@ -86,14 +87,15 @@ class OpaqueBrowserFrameViewLayout : public views::LayoutManager {
   virtual int CaptionButtonY(chrome::FrameButtonDisplayType button_id,
                              bool restored) const;
 
-  // Returns the initial spacing between the edge of the browser window and the
-  // first button.
-  virtual int TopAreaPadding() const;
-
-  // Returns the thickness of the 3D edge along the top of the titlebar.  If
+  // Returns the thickness of the top 3D edge of the window frame.  If
   // |restored| is true, acts as if the window is restored regardless of the
   // real mode.
-  int TitlebarTopThickness(bool restored) const;
+  int FrameTopThickness(bool restored) const;
+
+  // Returns the thickness of the left and right 3D edges of the window frame.
+  // If |restored| is true, acts as if the window is restored regardless of the
+  // real mode.
+  int FrameSideThickness(bool restored) const;
 
   // Returns the bounds of the titlebar icon (or where the icon would be if
   // there was one).
@@ -141,10 +143,18 @@ class OpaqueBrowserFrameViewLayout : public views::LayoutManager {
     ALIGN_TRAILING
   };
 
+  struct TopAreaPadding {
+    int leading;
+    int trailing;
+  };
+
   // views::LayoutManager:
   void Layout(views::View* host) override;
 
-  bool has_trailing_buttons() const { return has_trailing_buttons_; }
+  // Returns the spacing between the edge of the browser window and the first
+  // frame buttons.
+  virtual TopAreaPadding GetTopAreaPadding(bool has_leading_buttons,
+                                           bool has_trailing_buttons) const;
 
   virtual bool ShouldDrawImageMirrored(views::ImageButton* button,
                                        ButtonAlignment alignment) const;
@@ -181,6 +191,15 @@ class OpaqueBrowserFrameViewLayout : public views::LayoutManager {
   // Internal implementation of ViewAdded() and ViewRemoved().
   void SetView(int id, views::View* view);
 
+  // Returns the spacing between the edge of the browser window and the first
+  // frame buttons.
+  TopAreaPadding GetTopAreaPadding() const;
+
+  // Returns true if a 3D edge should be drawn around the window frame.  If
+  // |restored| is true, acts as if the window is restored regardless of the
+  // real mode.
+  bool IsFrameEdgeVisible(bool restored) const;
+
   // views::LayoutManager:
   gfx::Size GetPreferredSize(const views::View* host) const override;
   void ViewAdded(views::View* host, views::View* view) override;
@@ -192,10 +211,10 @@ class OpaqueBrowserFrameViewLayout : public views::LayoutManager {
   // The layout of the window icon, if visible.
   gfx::Rect window_icon_bounds_;
 
-  // Whether any of the window control buttons were packed on the
-  // leading or trailing sides.
-  bool has_leading_buttons_;
-  bool has_trailing_buttons_;
+  // Whether any of the window control buttons were packed on the leading or
+  // trailing sides.  This state is only valid while Layout() is being run.
+  bool placed_leading_button_;
+  bool placed_trailing_button_;
 
   // Extra offset from the top of the frame to the top of the window control
   // buttons. Configurable based on platform and whether we are under test.

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

@@ -176,7 +176,7 @@ class OpaqueBrowserFrameViewLayoutTest : public ChromeViewsTestBase {
     const int unavailable_px_at_top =
         delegate_->IsMaximized()
             ? 0
-            : OpaqueBrowserFrameViewLayout::kTitlebarTopEdgeThickness;
+            : OpaqueBrowserFrameViewLayout::kTopFrameEdgeThickness;
     return (unavailable_px_at_top + CaptionY() + kCaptionButtonHeight +
             OpaqueBrowserFrameViewLayout::kCaptionButtonBottomPadding -
             delegate_->GetIconSize()) /