Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin.

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

Index: chrome/browser/ui/views/frame/glass_browser_frame_view.cc
diff --git a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc b/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
index 04fbcb0afa9ef120d41d37c765e81781507e11cc..97984dab0463a93f70bf9b69f7afb6af6d458862 100644
--- a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
+++ b/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
@@ -42,16 +42,16 @@ HICON GlassBrowserFrameView::throbber_icons_[
     GlassBrowserFrameView::kThrobberIconCount];
 
 namespace {
-// Size of client edge drawn inside the outer frame borders.
-const int kNonClientBorderThicknessPreWin10 = 3;
-const int kNonClientBorderThicknessWin10 = 1;
-// Besides the frame border, there's empty space atop the window in restored
-// mode, to use to drag the window around.
+// Thickness of the border in the client area that separates it from the
+// non-client area. Not to be confused with kClientEdgeThickness, which is the
+// thickness of the border between the web content and our frame border.
+const int kClientBorderThicknessPreWin10 = 3;
+const int kClientBorderThicknessWin10 = 1;

[Peter Kasting, 2016/04/12 01:49:31]

Nit: Maybe we should have a TODO to get rid of the Win10 constant?

[Bret, 2016/04/12 23:19:38]

I tried this but it's actually a substantial visual difference between the
Windows versions, not a weird compatibility hack.

[Peter Kasting, 2016/04/13 01:00:22]

Sorry, I think I wasn't clear.  I don't mean use the non-Win10 value on Win10; I
mean use "0" on Win10.

That will be a somewhat involved change, I believe.

[Bret, 2016/04/13 22:19:47]

I'm a bit confused because that would be a visual change that I think looks fine
how it is. Are you saying we want the web content to be flush with the Windows
border?

[Peter Kasting, 2016/04/15 00:20:51]

Eventually, yes.  We don't want the current 1 DP border of our own around the
content, just the 1 px Windows border.  This is also what Edge does AFAICT.

[Bret, 2016/04/15 23:07:08]

Oh, okay. I think it would actually be pretty easy. I'll make a note to do it
later, maybe for md only?

[Peter Kasting, 2016/04/19 21:00:00]

Sure.

+// Extra empty space to draw in restored mode to make the titlebar thicker.

[Peter Kasting, 2016/04/12 01:49:31]

This seems misleading.  I'm pretty sure we only use this for tabbed windows,
i.e. when there is no titlebar, and its purpose is indeed to provide a drag
handle.  The old comment seemed clearer and more accurate.

[Bret, 2016/04/12 23:19:38]

Okay you're right. Reverted.

 const int kNonClientRestoredExtraThickness = 11;
-// In the window corners, the resize areas don't actually expand bigger, but the
-// 16 px at the end of the top and bottom edges triggers diagonal resizing.
+// The size of the corners of the titlebar that trigger diagonal resizing.

[Peter Kasting, 2016/04/12 01:49:31]

Again, the old comment seemed clearer and more accurate here, since this implies
the corners are actually 16 px square, instead of being L-shaped.

[Bret, 2016/04/12 23:19:38]

Hmm okay. I found the original comment confusing so I've edited it a bit, but it
should be substantially the same.

 const int kResizeCornerWidth = 16;
-// How far the new avatar button is from the left of the minimize button.
+// How far the avatar button is from the left of the minimize button.
 const int kNewAvatarButtonOffset = 5;
 // The content edge images have a shadow built into them.
 const int kContentEdgeShadowThickness = 2;
@@ -63,6 +63,10 @@ const int kNewTabCaptionRestoredSpacing = 5;
 // similar vertical coordinates, we need to reserve a larger, 16 px gap to avoid
 // looking too cluttered.
 const int kNewTabCaptionMaximizedSpacing = 16;
+// Height of the avatar button. Same as the height of the Windows 7/8 caption
+// buttons but doesn't change for 10 where the caption buttons are much bigger.

[Peter Kasting, 2016/04/12 01:49:31]

This seems like a behavior change?  The old code used the system metrics instead
of hardcoding this, and in general that's what we want -- we want the profile
switcher button, in principle, to look like another caption button.

[Bret, 2016/04/12 23:19:38]

This one is an odd duck. The old code was using SM_CYMENUSIZE which isn't the
caption button height. Reading the API documentation I can't actually tell what
it is exactly (it's at least some kind of button height), it just happens to
return 19 for all Windows versions we support. The caption button height is
SM_CYSIZE ostensibly, but that doesn't return 19 (iirc it returns 22), so it's
wrong. It seems like the way you're supposed to get the caption button sizing is
to handle WM_GETTITLEBARINFOEX and save the information... but on Windows 10 we
don't actually want it to be the same height as the caption buttons because that
would destroy our layout. So I decided the only sensible thing to do was not
even pretend we're using a system metric anymore.

When I traced the actual calculation it was resulting in 19 pixels maximized and
20 pixels restored, so this isn't a behavior change.

[Peter Kasting, 2016/04/13 01:00:22]

My point was that I think we _do_ actually want the Win10 profile switcher
button to look like another caption button.  I think right now its appearance is
more like a Win8 caption button?  (Don't have Win 8, can't check.)  Yes, this
will affect layout; no, we shouldn't try to fix that in this patch.  But I don't
think forcing to 19/20 px is what we actually want; I think the current Win10
behavior is just wrong.

Should check with the UI designers though.

[Bret, 2016/04/13 22:19:47]

In Win7 it does a decent job of pretending to be a caption button, but in Win8 I
think it looks fairly different, since it has a unique background color and
isn't the same width as any of the real caption buttons. Right now we are
forcing 19/20 pixels, it just doesn't look like it from the code. I'd rather
just be clear about the heights we're using until we decide to do something
cleverer.

[Peter Kasting, 2016/04/15 00:20:51]

Yeah, we never really tested anything on Win 8 desktop :(
OK, but this is still a minor behavior change from the old code, I think.

The old code always used the same height, and changed the Y coordinate to be
above the window top in maximized mode. The new code uses a more consistent Y
coordinate and changes the height instead.  This has ramifications for the
precise vertical positioning of the content inside the button.  I think the old
code, while it seems strange, is closer to what Windows actually does when
positioning caption buttons, and I'd stick with that.

[Bret, 2016/04/15 23:07:08]

Okay you're right. I'll keep the same position logic, and add a TODO for making
this look like a Win10 caption button.

+const int kAvatarButtonHeightRestored = 20;
+const int kAvatarButtonHeightMaximized = 19;
 
 // Converts the |image| to a Windows icon and returns the corresponding HICON
 // handle. |image| is resized to desired |width| and |height| if needed.
@@ -104,13 +108,13 @@ gfx::Rect GlassBrowserFrameView::GetBoundsForTabStrip(
   // In maximized RTL windows, don't let the tabstrip overlap the caption area,
   // or the alpha-blending it does will make things like the new avatar button
   // look glitchy.
-  const int offset =
-    (ui::MaterialDesignController::IsModeMaterial() || !base::i18n::IsRTL() ||
-     !frame()->IsMaximized()) ?
-        GetLayoutInsets(AVATAR_ICON).right() : 0;
+  const int offset = (ui::MaterialDesignController::IsModeMaterial() ||
+                      !RTLSpecialLayout() || !frame()->IsMaximized())
+                         ? GetLayoutInsets(AVATAR_ICON).right()
+                         : 0;
   const int x = incognito_bounds_.right() + offset;
-  int end_x = width() - NonClientBorderThickness(false);
-  if (!base::i18n::IsRTL()) {
+  int end_x = width() - ClientBorderThickness(false);
+  if (!RTLSpecialLayout()) {
     end_x = std::min(frame()->GetMinimizeButtonOffset(), end_x) -
         (frame()->IsMaximized() ?
             kNewTabCaptionMaximizedSpacing : kNewTabCaptionRestoredSpacing);
@@ -130,7 +134,7 @@ gfx::Rect GlassBrowserFrameView::GetBoundsForTabStrip(
       }
     }
   }
-  return gfx::Rect(x, NonClientTopBorderHeight(false), std::max(0, end_x - x),
+  return gfx::Rect(x, NonClientTopHeight(false), std::max(0, end_x - x),
                    tabstrip->GetPreferredSize().height());
 }
 
@@ -227,27 +231,29 @@ int GlassBrowserFrameView::NonClientHitTest(const gfx::Point& point) {
 
   // See if we're in the sysmenu region.  We still have to check the tabstrip
   // first so that clicks in a tab don't get treated as sysmenu clicks.
-  int nonclient_border_thickness = NonClientBorderThickness(false);
-  if (gfx::Rect(nonclient_border_thickness,
+  int client_border_thickness = ClientBorderThickness(false);
+  if (gfx::Rect(client_border_thickness,
                 gfx::win::GetSystemMetricsInDIP(SM_CYSIZEFRAME),
                 gfx::win::GetSystemMetricsInDIP(SM_CXSMICON),
-                gfx::win::GetSystemMetricsInDIP(SM_CYSMICON)).Contains(point))
+                gfx::win::GetSystemMetricsInDIP(SM_CYSMICON))
+          .Contains(point))
     return (frame_component == HTCLIENT) ? HTCLIENT : HTSYSMENU;
 
   if (frame_component != HTNOWHERE)
     return frame_component;
 
-  int frame_top_border_height = FrameTopBorderHeight(false);
+  int top_border_thickness = NonClientTopBorderThickness(false);
   // We want the resize corner behavior to apply to the kResizeCornerWidth
   // pixels at each end of the top and bottom edges.  Because |point|'s x
   // coordinate is based on the DWM-inset portion of the window (so, it's 0 at
   // the first pixel inside the left DWM margin), we need to subtract the DWM
   // margin thickness, which we calculate as the total frame border thickness
   // minus the nonclient border thickness.
-  const int dwm_margin = FrameBorderThickness() - nonclient_border_thickness;
-  int window_component = GetHTComponentForFrame(point, frame_top_border_height,
-      nonclient_border_thickness, frame_top_border_height,
-      kResizeCornerWidth - dwm_margin, frame()->widget_delegate()->CanResize());
+  const int dwm_margin = NonClientBorderThickness() - client_border_thickness;
+  int window_component = GetHTComponentForFrame(
+      point, top_border_thickness, client_border_thickness,
+      top_border_thickness, kResizeCornerWidth - dwm_margin,
+      frame()->widget_delegate()->CanResize());
   // Fall back to the caption if no other component matches.
   return (window_component == HTNOWHERE) ? HTCAPTION : window_component;
 }
@@ -266,7 +272,7 @@ void GlassBrowserFrameView::OnPaint(gfx::Canvas* canvas) {
 
 void GlassBrowserFrameView::Layout() {
   if (browser_view()->IsRegularOrGuestSession())
-    LayoutNewStyleAvatar();
+    LayoutAvatar();
   LayoutIncognitoIcon();
   LayoutClientView();
 }
@@ -298,33 +304,33 @@ bool GlassBrowserFrameView::DoesIntersectRect(const views::View* target,
          !frame()->client_view()->bounds().Intersects(rect);
 }
 
-int GlassBrowserFrameView::FrameBorderThickness() const {
+int GlassBrowserFrameView::NonClientBorderThickness() const {
   return (frame()->IsMaximized() || frame()->IsFullscreen()) ?
       0 : gfx::win::GetSystemMetricsInDIP(SM_CXSIZEFRAME);
 }
 
-int GlassBrowserFrameView::FrameTopBorderHeight(bool restored) const {
-  // We'd like to use FrameBorderThickness() here, but the maximized Aero glass
-  // frame has a 0 frame border around most edges and a CYSIZEFRAME-thick border
-  // at the top (see AeroGlassFrame::OnGetMinMaxInfo()).
+int GlassBrowserFrameView::NonClientTopBorderThickness(bool restored) const {
+  // Distinct from NonClientBorderThickness() because we need a border at the
+  // top when maximized to avoid having UI elements drift off the top of the
+  // screen.

[Peter Kasting, 2016/04/12 01:49:31]

Nit: Maybe "Distinct from NonClientBorderThickness() because the system gives
maximized windows an invisible CYSIZEFRAME-thick nonclient region above the top
of the screen, which we need to keep UI elements below."

(At least I assume it's the system doing this, and not our own code elsewhere. 
I didn't see anything relevant in browser_desktop_window_tree_host_win.cc.)

[Bret, 2016/04/12 23:19:38]

Confusingly this is actually our fault. Updated the comment to be more specific.

   return (frame()->IsFullscreen() && !restored) ?
       0 : gfx::win::GetSystemMetricsInDIP(SM_CYSIZEFRAME);
 }
 
-int GlassBrowserFrameView::NonClientBorderThickness(bool restored) const {
+int GlassBrowserFrameView::ClientBorderThickness(bool restored) const {
   if ((frame()->IsMaximized() || frame()->IsFullscreen()) && !restored)
     return 0;
 
   return (base::win::GetVersion() < base::win::VERSION_WIN10)
-             ? kNonClientBorderThicknessPreWin10
-             : kNonClientBorderThicknessWin10;
+             ? kClientBorderThicknessPreWin10
+             : kClientBorderThicknessWin10;
 }
 
-int GlassBrowserFrameView::NonClientTopBorderHeight(bool restored) const {
+int GlassBrowserFrameView::NonClientTopHeight(bool restored) const {
   if (frame()->IsFullscreen() && !restored)
     return 0;
 
-  const int top = FrameTopBorderHeight(restored);
+  const int top = NonClientTopBorderThickness(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.
@@ -343,6 +349,18 @@ bool GlassBrowserFrameView::IsToolbarVisible() const {
       !browser_view()->toolbar()->GetPreferredSize().IsEmpty();
 }
 
+bool GlassBrowserFrameView::RTLSpecialLayout() const {
+  return base::i18n::IsRTL();
+}
+
+int GlassBrowserFrameView::WindowTopY() const {
+  // When maximized this is flush with the top of the screen because we've
+  // extended the frame NonClientTopBorderThickness() beyond the visible area.
+  // When restored this is 1 pixel below the top of the window, leaving a
+  // border. This is the same positioning as the Windows caption buttons.

[Peter Kasting, 2016/04/12 01:49:31]

Nit: Simpler: "Windows draws its caption buttons at the top of the screen for
maximized windows, and 1 px from the top of the window otherwise."

This suggests maybe this function should be named CaptionButtonY().

[Bret, 2016/04/12 23:19:38]

Yeah you're right that's clearer. Done.

+  return frame()->IsMaximized() ? NonClientTopBorderThickness(false) : 1;
+}
+
 void GlassBrowserFrameView::PaintToolbarBackground(gfx::Canvas* canvas) const {
   gfx::Rect toolbar_bounds(browser_view()->GetToolbarBounds());
   if (toolbar_bounds.IsEmpty())
@@ -449,7 +467,7 @@ void GlassBrowserFrameView::PaintClientEdge(gfx::Canvas* canvas) const {
       client_bounds.y() + (md ? toolbar_bounds.y() : toolbar_bounds.bottom());
   const int w = client_bounds.width();
   const int right = client_bounds.right();
-  const int bottom = std::max(y, height() - NonClientBorderThickness(false));
+  const int bottom = std::max(y, height() - ClientBorderThickness(false));
   const int height = bottom - y;
 
   // Draw the client edge images.  For non-MD, we fill the toolbar color
@@ -492,7 +510,7 @@ void GlassBrowserFrameView::FillClientEdgeRects(int x,
   canvas->FillRect(side, color);
 }
 
-void GlassBrowserFrameView::LayoutNewStyleAvatar() {
+void GlassBrowserFrameView::LayoutAvatar() {
   DCHECK(browser_view()->IsRegularOrGuestSession());
   if (!profile_switcher_.view())
     return;
@@ -501,35 +519,24 @@ void GlassBrowserFrameView::LayoutNewStyleAvatar() {
 
   int button_x = frame()->GetMinimizeButtonOffset() -
       kNewAvatarButtonOffset - label_size.width();
-  if (base::i18n::IsRTL())
+  if (RTLSpecialLayout())
     button_x = width() - frame()->GetMinimizeButtonOffset() +
         kNewAvatarButtonOffset;
 
-  // The caption button position and size is confusing.  In maximized mode, the
-  // caption buttons are SM_CYMENUSIZE pixels high and are placed
-  // FrameTopBorderHeight() pixels from the top of the window; all those top
-  // border pixels are offscreen, so this result in caption buttons flush with
-  // the top of the screen.  In restored mode, the caption buttons are first
-  // placed just below a 2 px border at the top of the window (which is the
-  // first two pixels' worth of FrameTopBorderHeight()), then extended upwards
-  // one extra pixel to overlap part of this border.
-  //
-  // To match both of these, we size the button as if it's always the extra one
-  // pixel in height, then we place it at the correct position in restored mode,
-  // or one pixel above the top of the screen in maximized mode.
-  int button_y = frame()->IsMaximized() ? (FrameTopBorderHeight(false) - 1) : 1;
-  profile_switcher_.view()->SetBounds(
-      button_x, button_y, label_size.width(),
-      gfx::win::GetSystemMetricsInDIP(SM_CYMENUSIZE) + 1);
+  const int button_y = WindowTopY();
+  const int button_h = frame()->IsMaximized() ? kAvatarButtonHeightMaximized
+                                              : kAvatarButtonHeightRestored;
+  profile_switcher_.view()->SetBounds(button_x, button_y, label_size.width(),
+                                      button_h);
 }
 
 void GlassBrowserFrameView::LayoutIncognitoIcon() {
   const bool md = ui::MaterialDesignController::IsModeMaterial();
   const gfx::Insets insets(GetLayoutInsets(AVATAR_ICON));
   const gfx::Size size(GetOTRAvatarIcon().size());
-  int x = NonClientBorderThickness(false);
+  int x = ClientBorderThickness(false);
   // In RTL, the icon needs to start after the caption buttons.
-  if (base::i18n::IsRTL()) {
+  if (RTLSpecialLayout()) {
     x = width() - frame()->GetMinimizeButtonOffset() +
         (profile_switcher_.view()
              ? (profile_switcher_.view()->width() + kNewAvatarButtonOffset)
@@ -544,8 +551,9 @@ void GlassBrowserFrameView::LayoutIncognitoIcon() {
   }
   const int bottom = GetTopInset(false) + browser_view()->GetTabStripHeight() -
       insets.bottom();
-  const int y = (md || !frame()->IsMaximized()) ?
-      (bottom - size.height()) : FrameTopBorderHeight(false);
+  const int y = (md || !frame()->IsMaximized())
+                    ? (bottom - size.height())
+                    : NonClientTopBorderThickness(false);
   incognito_bounds_.SetRect(x + (avatar_button() ? insets.left() : 0), y,
                             avatar_button() ? size.width() : 0, bottom - y);
   if (avatar_button())
@@ -560,8 +568,8 @@ gfx::Insets GlassBrowserFrameView::GetClientAreaInsets(bool restored) const {
   if (!browser_view()->IsTabStripVisible())
     return gfx::Insets();
 
-  const int top_height = NonClientTopBorderHeight(restored);
-  const int border_thickness = NonClientBorderThickness(restored);
+  const int top_height = NonClientTopHeight(restored);
+  const int border_thickness = ClientBorderThickness(restored);
   return gfx::Insets(top_height,
                      border_thickness,
                      border_thickness,