Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin.

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

Index: chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
diff --git a/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc b/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
index 372482c2247b4a136866de75e7ca76388c701819..69be7cdd549cfa745c5130c727f577b969bae7dc 100644
--- a/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
+++ b/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
@@ -13,19 +13,21 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/views/frame/browser_frame.h"
-#include "chrome/browser/ui/views/frame/browser_frame_common_win.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/browser_window_property_manager_win.h"
 #include "chrome/browser/ui/views/frame/system_menu_insertion_delegate_win.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/common/chrome_constants.h"
 #include "ui/base/theme_provider.h"
+#include "ui/gfx/geometry/point.h"
 #include "ui/gfx/win/dpi.h"
 #include "ui/views/controls/menu/native_menu_win.h"
 
 namespace {
 
-const int kClientEdgeThickness = 3;
+// The amount of additional non-client area to draw beyond what we have Windows
+// draw, in DIPs. Only used in Windows versions pre-10.

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

Nit: "Only used pre-Win 10."

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

Done.

+const int kDWMFrameBorderExtensionDips = 3;
 
 }  // namespace
 
@@ -95,12 +97,15 @@ bool BrowserDesktopWindowTreeHostWin::GetClientAreaInsets(
   }
 
   int border_thickness = GetSystemMetrics(SM_CXSIZEFRAME);
-  // In fullscreen mode, we have no frame. In restored mode, we draw our own
-  // client edge over part of the default frame.
-  if (GetWidget()->IsFullscreen())
+  if (GetWidget()->IsFullscreen()) {
+    // In fullscreen mode there is no frame.
     border_thickness = 0;
-  else if (!IsMaximized() && base::win::GetVersion() < base::win::VERSION_WIN10)
-    border_thickness -= kClientEdgeThickness;
+  } else if (!IsMaximized() &&
+             base::win::GetVersion() < base::win::VERSION_WIN10) {
+    // Reduce the Windows non-client border size because we extended the border
+    // into our client area in ::UpdateDWMFrame().

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

Nit: No :: (this is not a global method)

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

Done.

+    border_thickness -= kDWMFrameBorderExtensionDips;
+  }
   insets->Set(0, border_thickness, border_thickness, border_thickness);
   return true;
 }
@@ -226,8 +231,14 @@ bool BrowserDesktopWindowTreeHostWin::ShouldUseNativeFrame() const {
   // context of the BrowserView destructor.
   if (!browser_view_->browser())
     return false;
-  return chrome::ShouldUseNativeFrame(browser_view_,
-                                      GetWidget()->GetThemeProvider());
+  // We don't theme popup or app windows, so regardless of whether or not a
+  // theme is active for normal browser windows, we don't want to use the custom
+  // frame for popups/apps.
+  if (!browser_view_->IsBrowserTypeNormal())
+    return true;
+  // Otherwise, we use the native frame when we're told we should by the theme
+  // provider (e.g. no custom theme is active).
+  return GetWidget()->GetThemeProvider()->ShouldUseNativeFrame();
 }
 
 void BrowserDesktopWindowTreeHostWin::FrameTypeChanged() {
@@ -259,43 +270,41 @@ void BrowserDesktopWindowTreeHostWin::UpdateDWMFrame() {
 MARGINS BrowserDesktopWindowTreeHostWin::GetDWMFrameMargins() const {
   MARGINS margins = { 0 };
 
-  // If the opaque frame is visible, we use the default (zero) margins.
-  // Otherwise, we need to figure out how to extend the glass in.
-  if (GetWidget()->ShouldUseNativeFrame()) {
-    // In fullscreen mode, we don't extend glass into the client area at all,
-    // because the GDI-drawn text in the web content composited over it will
-    // become semi-transparent over any glass area.
-    if (!IsMaximized() && !GetWidget()->IsFullscreen()) {
-      margins.cyTopHeight = kClientEdgeThickness + 1;
-      // On Windows 10, we don't draw our own window border, so don't extend the
-      // nonclient area in for it. The top is extended so that the MARGINS isn't
-      // treated as an empty (ignored) extension.
-      if (base::win::GetVersion() >= base::win::VERSION_WIN10) {
-        margins.cxLeftWidth = 0;
-        margins.cxRightWidth = 0;
-        margins.cyBottomHeight = 0;
-      } else {
-        margins.cxLeftWidth = kClientEdgeThickness + 1;
-        margins.cxRightWidth = kClientEdgeThickness + 1;
-        margins.cyBottomHeight = kClientEdgeThickness + 1;
-      }
-    }
-    // In maximized mode, we only have a titlebar strip of glass, no side/bottom
-    // borders.
-    if (!browser_view_->IsFullscreen()) {
-      gfx::Rect tabstrip_bounds(
-          browser_frame_->GetBoundsForTabStrip(browser_view_->tabstrip()));
-      tabstrip_bounds = gfx::win::DIPToScreenRect(tabstrip_bounds);
-      margins.cyTopHeight = tabstrip_bounds.bottom();
-
-      // On pre-Win 10, we need to offset the DWM frame into the toolbar so that
-      // the blackness doesn't show up on our rounded toolbar corners.  In Win
-      // 10 and above there are no rounded corners, so this is unnecessary.
-      const int kDWMFrameTopOffset = 3;
-      if (base::win::GetVersion() < base::win::VERSION_WIN10)
-        margins.cyTopHeight += kDWMFrameTopOffset;
+  // If the opaque frame is visible or we're fullscreen, we don't extend the
+  // glass in at all because it won't be visible.
+  if (!GetWidget()->ShouldUseNativeFrame() || GetWidget()->IsFullscreen())

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

Could GetWidget()->IsFullscreen() and browser_view_->IsFullscreen() disagree?
The old code checked both (for different pieces).

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

Wow I HOPE they don't disagree...

I double-checked this and they end up delegating to the same place
(FullscreenHandler::fullscreen()). And I can't think of a reason anything that
could be fullscreen would even want to delegate somewhere else, because that
class is used for all top-level windows.

+    return margins;
+
+  if (!IsMaximized()) {
+    if (base::win::GetVersion() < base::win::VERSION_WIN10) {
+      gfx::Point dip_margin(kDWMFrameBorderExtensionDips,
+                            kDWMFrameBorderExtensionDips);
+      gfx::Point pixel_margin = gfx::win::DIPToScreenPoint(dip_margin);

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

This is a behavior change, albeit one that looks correct to me.  Can you
demonstrate (e.g. in a screenshot) the effect of this change?

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

Here are some screenshots of this change: http://imgur.com/a/8Wd7L Best I can
tell there's no visual difference.

There is one small visual difference shown in this pair of screenshots:
http://imgur.com/a/uQ9yt There's a bug where in hidpi mode if our resolution is
odd the client area will "pull away" from the frame and leave 1 pixel of
blackness due to rounding. But after this change we're now keeping the amount of
frame relative to the scale, so the pixel of blackness is replaced by a pixel of
frame color. I think it looks better.

+      margins.cxLeftWidth = pixel_margin.x();

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

These lines no longer add 1 to the value like they used to.  Why not?

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

I was confused by why were adding 1 to this value so I played around with it. As
long as this border is sufficiently large to cover the areas we don't draw its
exact value doesn't matter. It's hard to discover the reason the original
committer was adding 1, but I assume based on my testing that 3 pixels ended up
not being sufficient in the hidpi case so it was padded 1 extra pixel. It seems
more correct to just say the value is in DIPs and scale it correctly. Hence this
change.

+      margins.cxRightWidth = pixel_margin.x();
+      margins.cyBottomHeight = pixel_margin.y();
+
+      // Even though we're extending the frame to the bottom of the tabstrip
+      // below we need to offset the DWM frame into the toolbar so that the
+      // blackness doesn't show up on our rounded toolbar corners.

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

Hmm.  This comment feels a bit misleading.  This is really about whether the
toolbar has rounded corners, which I'm not convinced is the same as whether
we're pre-Win10.  Did I make the glass frame draw square toolbar corners in MD? 
If so, we would want to put this inside an "if (!md)" conditional.

Maybe the comment should be something like:

"Since the toolbar has rounded corners that show the glass frame behind, the
extension of the glass area to the bottom of the tabstrip below isn't
sufficient.  We need to go down further to the bottom of the rounded corner
region."

Also, this implies that we may not want the same value for this constant as for
the other three (as indeed the old code ended up using 3 for this and 4 for the
others).

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

Ok I like your comment better, replaced. I can gate this behind "md" if you
think it's very important, but it wasn't gated before and it won't make a
visible difference.

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

If we find blocks that are only relevant in a non-MD world, we should definitely
add "if (!md)" gates to them; that way after we ship MD, when we go back and rip
out all non-MD code, we'll know to rip this stuff out too.

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

Good point. Done.

+      margins.cyTopHeight = pixel_margin.y();
+    } else {
+      // On Windows 10 we don't need extra border thickness because we draw
+      // right up to the edge of the client area and there are no rounded
+      // corners anywhere that we need to make sure are covered.
+      margins.cxLeftWidth = 0;

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

These lines are unnecessary since |margins| has already been zero-initted.

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

Done. I kept the comment though.

+      margins.cxRightWidth = 0;
+      margins.cyTopHeight = 0;
+      margins.cyBottomHeight = 0;
     }
   }
+
+  // Extend top for the tabstrip background.
+  gfx::Rect tabstrip_bounds(
+      browser_frame_->GetBoundsForTabStrip(browser_view_->tabstrip()));
+  tabstrip_bounds = gfx::win::DIPToScreenRect(tabstrip_bounds);
+  margins.cyTopHeight += tabstrip_bounds.bottom();
+
   return margins;
 }