Fix opaque frame incognito icon and tabstrip positioning.

This affects both MD and non-MD and is based on the current glass behavior + the
fix for bug 580757.

12345678901234567890123456789012345678901234567890123456789012345678901234567890
* Eliminate kAvatarInnerSpacing.  Replace with the opaque frame equivalent of
  https://codereview.chromium.org/1624803002/ (which unfortunately is more
  complicated due to the split of OpaqueBrowserFrameView and
  OpaqueBrowserFrameViewLayout; I'm inclined to undo that split at some point).
  This deals with the "caption buttons on left" case better than the existing
  code, and avoids excessive space in incognito or MD.
* Position/size the incognito icon the same way in both normal and maximized
  mode for MD.  This matches glass, and is necessary because the MD incognito
  icon is much smaller and so shouldn't be positioned/clipped the same way as in
  non-MD.
* Remove unconditional addition of one pixel before tabstrip leading edge.
  Replace with code to position the tabstrip edges based on
  NonClientBorderThickness() instead of FrameBorderThickness() -- in restored
  windows, the two differ by kClientEdgeThickness (i.e. 1 pixel).  This is more
  correct in maximized mode, where that extra pixel is removed, and more correct
  for the "caption buttons on left" case as well, in which case AFAICT we want
  the mirror behavior.  (I don't know what the existing comment about how the
  two sides differ was supposed to mean; I think it's wrong.)  This results in a
  variety of 1-pixel changes to the tabstrip left and/or right edges,
  necessitating some unit test tweaks.
* Call LayoutIncognitoIcon() unconditionally so we can use it to adjust the
  tabstrip bounds even when there's no incognito icon.  This is all a bit more
  confusing than on glass, where we can just lay out a zero-width incognito icon
  and use it to position the tabstrip in all cases, because of the need to
  support caption buttons on either side (screw you Linux).

BUG=519020
TEST=none

Review URL: https://codereview.chromium.org/1622833002

Cr-Commit-Position: refs/heads/master@{#371443}

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

@@ -431,6 +431,12 @@ gfx::Size OpaqueBrowserFrameView::GetTabstripPreferredSize() const {
   return s;
 }
 
+int OpaqueBrowserFrameView::GetToolbarLeadingCornerClientWidth() const {
+  return browser_view()->GetToolbarBounds().x() - kContentEdgeShadowThickness +
+      GetThemeProvider()->GetImageSkiaNamed(
+          IDR_CONTENT_TOP_LEFT_CORNER)->width();
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // OpaqueBrowserFrameView, protected:
 

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

@@ -86,6 +86,7 @@ class OpaqueBrowserFrameView : public BrowserNonClientFrameView,
   int GetTabStripHeight() const override;
   bool IsToolbarVisible() const override;
   gfx::Size GetTabstripPreferredSize() const override;
+  int GetToolbarLeadingCornerClientWidth() const override;
 
  protected:
   views::ImageButton* minimize_button() const { return minimize_button_; }

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

@@ -11,6 +11,7 @@
 #include "chrome/browser/ui/views/profiles/avatar_menu_button.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/signin/core/common/profile_management_switches.h"
+#include "ui/base/material_design/material_design_controller.h"
 #include "ui/gfx/font.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/label.h"
@@ -42,9 +43,6 @@ const int kIconLeftSpacing = 2;
 // There is a 4 px gap between the icon and the title text.
 const int kIconTitleSpacing = 4;
 
-// Space between the edge of the avatar and the tabstrip.
-const int kAvatarInnerSpacing = 4;
-
 // How far the new avatar button is from the closest caption button.
 const int kNewAvatarButtonOffset = 5;
 
@@ -118,12 +116,8 @@ gfx::Rect OpaqueBrowserFrameViewLayout::GetBoundsForTabStrip(
     int available_width) const {
   int x = leading_button_start_ + GetLayoutInsets(AVATAR_ICON).right();
   available_width -= x + NewTabCaptionSpacing() + trailing_button_start_;
-  gfx::Rect bounds(x, GetTabStripInsetsTop(false), std::max(0, available_width),
+  return gfx::Rect(x, GetTabStripInsetsTop(false), std::max(0, available_width),
                    tabstrip_preferred_size.height());
-
-  if (delegate_->ShouldShowAvatar() && !ShouldIncognitoIconBeOnRight())
-    bounds.Inset(kAvatarInnerSpacing, 0, 0, 0);
-  return bounds;
 }
 
 gfx::Size OpaqueBrowserFrameViewLayout::GetMinimumSize(
@@ -386,6 +380,19 @@ void OpaqueBrowserFrameViewLayout::LayoutNewStyleAvatar(views::View* host) {
 void OpaqueBrowserFrameViewLayout::LayoutIncognitoIcon(views::View* host) {
   const int old_button_size = leading_button_start_ + trailing_button_start_;
 
+  // Any buttons/icon/title were laid out based on the frame border thickness,
+  // but the tabstrip bounds need to be based on the non-client border thickness
+  // on any side where there aren't other buttons forcing a larger inset.
+  const bool md = ui::MaterialDesignController::IsModeMaterial();
+  int min_button_width = NonClientBorderThickness();
+  // In non-MD, the toolbar has a rounded corner that we don't want the tabstrip
+  // to overlap.
+  if (!md && !avatar_button_)
+    min_button_width += delegate_->GetToolbarLeadingCornerClientWidth();
+  leading_button_start_ = std::max(leading_button_start_, min_button_width);
+  // The trailing corner is a mirror of the leading one.
+  trailing_button_start_ = std::max(trailing_button_start_, min_button_width);
+
   if (avatar_button_) {
     const gfx::Insets insets(GetLayoutInsets(AVATAR_ICON));
     const gfx::Size size(delegate_->GetOTRAvatarIcon().size());
@@ -400,8 +407,8 @@ void OpaqueBrowserFrameViewLayout::LayoutIncognitoIcon(views::View* host) {
     }
     const int bottom = GetTabStripInsetsTop(false) +
         delegate_->GetTabStripHeight() - insets.bottom();
-    int y = IsTitleBarCondensed() ?
-        FrameBorderThickness(false) : (bottom - size.height());
+    const int y = (md || !IsTitleBarCondensed()) ?
+        (bottom - size.height()) : FrameBorderThickness(false);
     avatar_button_->SetBounds(x, y, size.width(), bottom - y);
   }
 
@@ -606,15 +613,9 @@ void OpaqueBrowserFrameViewLayout::Layout(views::View* host) {
   LayoutWindowControls(host);
   LayoutTitleBar(host);
 
-  // We now add a single pixel to the leading spacing. We do this because the
-  // avatar and tab strip start one pixel inward compared to where things start
-  // on the trailing side.
-  leading_button_start_++;
-
   if (delegate_->IsRegularOrGuestSession())
     LayoutNewStyleAvatar(host);
-  else
-    LayoutIncognitoIcon(host);
+  LayoutIncognitoIcon(host);
 
   client_view_bounds_ = CalculateClientAreaBounds(
       host->width(), host->height());

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

@@ -54,6 +54,11 @@ class OpaqueBrowserFrameViewLayoutDelegate {
   // it.
   virtual gfx::Size GetTabstripPreferredSize() const = 0;
 
+  // Returns the width of the portion of the toolbar's leading-edge rounded
+  // corner that is within the client area.  This is only necessary pre-Material
+  // Design.
+  virtual int GetToolbarLeadingCornerClientWidth() const = 0;
+
  protected:
   virtual ~OpaqueBrowserFrameViewLayoutDelegate() {}
 };

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

@@ -115,6 +115,10 @@ class TestLayoutDelegate : public OpaqueBrowserFrameViewLayoutDelegate {
     return IsTabStripVisible() ? gfx::Size(78, 29) : gfx::Size(0, 0);
   }
 
+  int GetToolbarLeadingCornerClientWidth() const override {
+    return 0;
+  }
+
  private:
   base::string16 window_title_;
   bool show_avatar_;
@@ -261,7 +265,7 @@ TEST_F(OpaqueBrowserFrameViewLayoutTest, BasicWindowMaximized) {
   EXPECT_EQ("429,0 25x18", restore_button_->bounds().ToString());
   EXPECT_EQ("454,0 46x18", close_button_->bounds().ToString());
 
-  EXPECT_EQ("-5,-3 392x29",
+  EXPECT_EQ("-6,-3 393x29",
             layout_manager_->GetBoundsForTabStrip(
                 delegate_->GetTabstripPreferredSize(), kWidth).ToString());
   EXPECT_EQ("262x61", layout_manager_->GetMinimumSize(kWidth).ToString());
@@ -287,7 +291,7 @@ TEST_F(OpaqueBrowserFrameViewLayoutTest, MaximizedWithYOffset) {
   EXPECT_EQ("429,0 25x20", restore_button_->bounds().ToString());
   EXPECT_EQ("454,0 46x20", close_button_->bounds().ToString());
 
-  EXPECT_EQ("-5,-3 392x29",
+  EXPECT_EQ("-6,-3 393x29",
             layout_manager_->GetBoundsForTabStrip(
                 delegate_->GetTabstripPreferredSize(), kWidth).ToString());
   EXPECT_EQ("262x61", layout_manager_->GetMinimumSize(kWidth).ToString());
@@ -312,7 +316,7 @@ TEST_F(OpaqueBrowserFrameViewLayoutTest, WindowButtonsOnLeft) {
   EXPECT_EQ("0,0 0x0", restore_button_->bounds().ToString());
   EXPECT_EQ("4,1 43x18", close_button_->bounds().ToString());
 
-  EXPECT_EQ("93,13 398x29",
+  EXPECT_EQ("92,13 398x29",
             layout_manager_->GetBoundsForTabStrip(
                 delegate_->GetTabstripPreferredSize(), kWidth).ToString());
   EXPECT_EQ("261x73", layout_manager_->GetMinimumSize(kWidth).ToString());
@@ -333,7 +337,7 @@ TEST_F(OpaqueBrowserFrameViewLayoutTest, WithoutCaptionButtons) {
   EXPECT_EQ("0,0 0x0", restore_button_->bounds().ToString());
   EXPECT_EQ("0,0 0x0", close_button_->bounds().ToString());
 
-  EXPECT_EQ("-5,-3 500x29",
+  EXPECT_EQ("-6,-3 501x29",
             layout_manager_->GetBoundsForTabStrip(
                 delegate_->GetTabstripPreferredSize(), kWidth).ToString());
   EXPECT_EQ("251x61", layout_manager_->GetMinimumSize(kWidth).ToString());
@@ -355,7 +359,7 @@ TEST_F(OpaqueBrowserFrameViewLayoutTest, MaximizedWithoutCaptionButtons) {
   EXPECT_EQ("0,0 0x0", restore_button_->bounds().ToString());
   EXPECT_EQ("0,0 0x0", close_button_->bounds().ToString());
 
-  EXPECT_EQ("-5,-3 500x29",
+  EXPECT_EQ("-6,-3 501x29",
             layout_manager_->GetBoundsForTabStrip(
                 delegate_->GetTabstripPreferredSize(), kWidth).ToString());
   EXPECT_EQ("251x61", layout_manager_->GetMinimumSize(kWidth).ToString());