Add GetCaptionButtonHeightInDIPs() method

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

Index: chrome/browser/ui/views/frame/minimize_button_metrics_win.cc
diff --git a/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc b/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc
index 095f2574c83c9ed91b29b7ec8972c8022cbe5d8d..1124386f9b2115a33f4235267c4b0fc7fd05bc78 100644
--- a/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc
+++ b/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc
@@ -24,6 +24,12 @@ const int kWin8ButtonBoundsPositionOffset = 10;
 const int kWin10ButtonBoundsPositionOffset = 6;
 const int kInvalidOffset = static_cast<int>(0x80000000);
 
+// These constant were determined manually by testing on Windows 7, 8 and 10.
+const int kWin7ButtonHeight = 20;
+const int kWin8ButtonHeight = 20;
+const int kWin10ButtonHeight = 28;
+const int kInvalidHeight = static_cast<int>(0x80000000);
+
 using base::win::GetVersion;
 using display::win::ScreenWin;
 
@@ -35,6 +41,14 @@ int GetDefaultButtonBoundsOffset() {
   return kWin7ButtonBoundsPositionOffset;
 }
 
+int GetDefaultButtonHeight() {
+  if (GetVersion() >= base::win::VERSION_WIN10)
+    return kWin10ButtonHeight;
+  if (GetVersion() >= base::win::VERSION_WIN8)
+    return kWin8ButtonHeight;
+  return kWin7ButtonHeight;
+}
+
 }  // namespace
 
 // static
@@ -43,6 +57,9 @@ int MinimizeButtonMetrics::last_cached_minimize_button_x_delta_ = 0;
 // static
 int MinimizeButtonMetrics::button_bounds_position_offset_ = kInvalidOffset;
 
+// static
+int MinimizeButtonMetrics::caption_button_height_ = kInvalidHeight;
+
 MinimizeButtonMetrics::MinimizeButtonMetrics()
     : hwnd_(nullptr),
       cached_minimize_button_x_delta_(last_cached_minimize_button_x_delta_),
@@ -123,7 +140,7 @@ int MinimizeButtonMetrics::GetMinimizeButtonOffsetForWindow() const {
     TITLEBARINFOEX titlebar_info = {0};
     titlebar_info.cbSize = sizeof(TITLEBARINFOEX);
     SendMessage(hwnd_, WM_GETTITLEBARINFOEX, 0,
-                reinterpret_cast<WPARAM>(&titlebar_info));
+                reinterpret_cast<LPARAM>(&titlebar_info));
 
     // Under DWM WM_GETTITLEBARINFOEX won't return the right thing until after
     // WM_NCACTIVATE (maybe it returns classic values?). In an attempt to
@@ -189,3 +206,29 @@ int MinimizeButtonMetrics::GetAndCacheMinimizeButtonOffsetX() const {
   last_cached_minimize_button_x_delta_ = cached_minimize_button_x_delta_;
   return minimize_button_offset;
 }
+
+int MinimizeButtonMetrics::GetCaptionButtonHeight() const {
+  // It's hard to get the height of the caption buttons exactly right for all
+  // resolutions/DPI settings. http://crbug.com/668278 tracks pixel-precise
+  // positioning for this. (According to robliao@ and bsep@ the solution is
+  // probably GetSystemMetricsForDpi.) GetSystemMetrics provides a good
+  // approximation - it seems to be no more than 1px off in practice.

[Peter Kasting, 2017/04/26 01:04:23]

This comment confuses me a bit because it's not clear if what we're saying is
simply that views' current DIP-based layout doesn't capture heights well when
they aren't integral DIPs, or that this function is supposed to return a
px-based value but returns the wrong one, or what.

This is complicated by the fact that I think your actual implementation here
returns a px-based value (that's what MSDN documents GetSystemMetrics as doing),
and I think you need to return a DIP-based value.

[emx, 2017/04/26 14:19:26]

OK, Mihai and I looked into this together and I've updated the comment.
GetSystemMetrics(SM_CYSIZE) always returns 28, regardless of the DPI settings of
the primary monitor (or any other monitor). I think that means it's
"device-independent" pixels, despite what the documentation says. 28 is what we
need to later pass to View::SetBounds() to make the avatar button look the same
height as the caption buttons. (My understanding is that a View does the scaling
internally somewhere.) So basically the height of the caption buttons - in
whatever units Views::SetBounds() takes - is unaffected by DPI scaling, which is
why the 4 methods that scale it give the wrong result.

+
+  // Note that the following methods do not return the results the caller
+  // expects with DPI scaling settings other than 100%:
+  //   DwmGetWindowAttribute(hwnd_, DWMWA_CAPTION_BUTTON_BOUNDS, ...)
+  //   SendMessage(hwnd_, WM_GETTITLEBARINFOEX, ...)
+  //   display::win::ScreenWin::GetSystemMetricsInDIP(SM_CYSIZE)
+  //     these all return (height * scale factor) e.g. 56 at 200%

[Peter Kasting, 2017/04/26 01:04:23]

This doesn't seem right, reading the code.  GetSystemMetricsInDIP actually does
what you've documented GetSystemMetricsForHwnd as doing -- returns (height /
primary monitor DSF).  Presumably GetSystemMetrics returns the metric in pixels
based on the primary monitor's DSF to begin with, which makes this result in
coherent DIP-based value, rounded to the nearest DIP (which is what you want
here, I think).

Other frame code uses this function for similar purposes to yours -- see e.g.
CustomFrameView::IconSize().  It really seems like the right one to use.  I'm
wondering if somehow the original test you did assumed it multiplied by the
scale factor when it didn't, and tried to correct for that unnecessarily, or
something?  I don't know what happened, but can you check again here?

[emx, 2017/04/26 14:19:26]

Yes, you're right, I got GetSystemMetricsForHwnd and GetSystemMetricsInDIP the
wrong way around. GetSystemMetricsForHwnd returns 56 at 200% scaling, like
DwmGetWindowAttribute and SendMessage(WM_GETTITLEBARINFOEX). Updated the
comment.
I've tested it again and 28 is the only value that gives the (approximately)
right result at all DPI scaling settings. The above comment is Mihai's and my
theory as to why, but even if the explanation is wrong, the test results remain
the same.

+  //   display::win::ScreenWin::GetSystemMetricsForHwnd(hwnd_, SM_CYSIZE)
+  //     returns (height / by primary monitor scale factor) e.g. 22 for 125%
+
+  if (caption_button_height_ == kInvalidHeight) {
+    int button_height_from_sysmetrics = GetSystemMetrics(SM_CYSIZE);
+    if (button_height_from_sysmetrics == 0)
+      return GetDefaultButtonHeight();
+    caption_button_height_ = button_height_from_sysmetrics;
+  }
+
+  return caption_button_height_;
+}