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..80fd29d775a6971d3b9f933fe6d51fdafafb2dbd 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);

[Peter Kasting, 2017/04/24 23:00:17]

Nit: This is a bizarre choice.  Why not just -1?  Better yet, why not 0, and
just check for that directly below and eliminate this entirely?

[emx, 2017/04/25 11:57:40]

I agree that it's a bizarre choice, but I wanted to keep it consistent with
kInvalidOffset. (That one probably can't be 0 since 0 is a valid offset.) Should
I change both to -1?

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

It's theoretically valid, but the only real constraint is that the offset not
match one of the ones we really use.  Honestly, IMO using signal values here is
bad anyway, and it'd be better to use signal bools ("value is set") or
base::Optional or similar.
Maybe?  Although I wouldn't aim for any consistency, as it doesn't make anything
easier to read or understand IMO.

+
 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::button_bounds_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,28 @@ int MinimizeButtonMetrics::GetAndCacheMinimizeButtonOffsetX() const {
   last_cached_minimize_button_x_delta_ = cached_minimize_button_x_delta_;
   return minimize_button_offset;
 }
+
+int MinimizeButtonMetrics::GetMinimizeButtonHeight() const {
+  // It's hard to get the height of the caption buttons exactly right for all
+  // resolutions/DPI settings. GetSystemMetricsForDpi is probaly the real
+  // solution [http://crbug.com/668278], but GetSystemMetrics provides a good

[Peter Kasting, 2017/04/24 23:00:17]

Bug 668278 is a tracking bug, it's not about adding a GetSystemMetricsForDpi
method, and shouldn't be about that.

I don't know what such a method is supposed to do anyway.  Explain more, and/or
file a specific bug on this.

[emx, 2017/04/25 11:57:40]

robliao@ and bsep@ told me that GetSystemMetricsForDpi is likely to be the
solution to this. I've revised the comment. I don't think I'm in a position to
file another bug on this as I don't know any more about this than what they told
me.

+  // approximation for now.
+
+  // Note that the following methods do not return the correct result with DPI
+  // scaling settings other than 100%:

[Peter Kasting, 2017/04/24 23:00:17]

In what way are they incorrect?

[emx, 2017/04/25 11:57:40]

More accurately, they're not what the caller expects. I don't know the correct
terminology for this, but in this case the caller expects the "unscaled" size,
i.e. 28 regardless of DPI settings (DIPs?). The first 3 methods return the
"scaled" size, e.g. 56 at 200% scaling. The last one returns 28 / primary
monitor scaling. I've edited the comment.

+  //   DwmGetWindowAttribute(hwnd_, DWMWA_CAPTION_BUTTON_BOUNDS, ...)
+  //   SendMessage(hwnd_, WM_GETTITLEBARINFOEX, ...)
+  //   display::win::ScreenWin::GetSystemMetricsInDIP(SM_CYSIZE)
+  //   display::win::ScreenWin::GetSystemMetricsForHwnd(hwnd_, SM_CYSIZE)
+
+  if (button_bounds_height_ == kInvalidHeight) {
+    int button_height_from_sysmetrics = GetSystemMetrics(SM_CYSIZE);
+    if (button_height_from_sysmetrics == 0) {

[Peter Kasting, 2017/04/24 23:00:17]

When can this happen?  This needs to be explained.  I'd like to remove
GetDefaultButtonHeight() entirely.

[emx, 2017/04/25 11:57:40]

I don't know when it can happen. MSDN does not explain this, but the function
does provide for the possibility of failure
[https://msdn.microsoft.com/en-us/library/windows/desktop/ms724385(v=vs.85).aspx],
so I don't think it's safe to assume it will never fail.

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

Most other places in code that we call GetSystemMetrics(), we don't check for a
zero return.  I think it's OK to avoid specially handling failure here.  The
only way this should fail, I think, is if you try to read a metric you don't
have permission to read, which should never happen in Chrome.

+      DLOG(ERROR) << "GetSystemMetrics(SM_CYSIZE) error " << ::GetLastError();

[Peter Kasting, 2017/04/24 23:00:17]

Avoid logging.

[emx, 2017/04/25 11:57:40]

Logging removed.

+      return GetDefaultButtonHeight();
+    }
+    button_bounds_height_ = button_height_from_sysmetrics;
+  }
+
+  return button_bounds_height_;
+}