Add GetCaptionButtonHeightInDIPs() method

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/frame/minimize_button_metrics_win.h"
 
 #include "base/logging.h"
 #include "base/i18n/rtl.h"
 #include "base/win/windows_version.h"
 #include "dwmapi.h"
 #include "ui/base/win/shell.h"
 #include "ui/display/win/dpi.h"
 #include "ui/display/win/screen_win.h"
 #include "ui/gfx/geometry/point.h"
 
 namespace {
 
 // These constants were determined by manually adding various offsets
 // until the identity switcher was placed at the same location as before.
 // When a new or updated OS version is released, a new constant may need
 // to be added to this list and GetDefaultButtonBoundsOffset() is updated.
 const int kWin7ButtonBoundsPositionOffset = 1;
 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;
 
 int GetDefaultButtonBoundsOffset() {
   if (GetVersion() >= base::win::VERSION_WIN10)
     return kWin10ButtonBoundsPositionOffset;
   if (GetVersion() >= base::win::VERSION_WIN8)
     return kWin8ButtonBoundsPositionOffset;
   return kWin7ButtonBoundsPositionOffset;
 }
 
+int GetDefaultButtonHeight() {
+  if (GetVersion() >= base::win::VERSION_WIN10)
+    return kWin10ButtonHeight;
+  if (GetVersion() >= base::win::VERSION_WIN8)
+    return kWin8ButtonHeight;
+  return kWin7ButtonHeight;
+}
+
 }  // namespace
 
 // static
 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_),
       was_activated_(false) {
 }
 
 MinimizeButtonMetrics::~MinimizeButtonMetrics() {
 }
 
 void MinimizeButtonMetrics::Init(HWND hwnd) {
@@ skipping 60 matching lines @@
   if (!dwm_button_pos) {
     // Fallback to using the message for the titlebar info only if the above
     // code fails. It can fail if DWM is disabled globally or only for the
     // given HWND. The WM_GETTITLEBARINFOEX message can fail if we are not
     // active/visible. By fail we get a location of 0; the return status
     // code is always the same and similarly the state never seems to change
     // (titlebar_info.rgstate).
     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
     // return a consistant value we cache the last value across instances and
     // use it until we get the activate.
     if (titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right ||
         (titlebar_info.rgstate[2] &
          (STATE_SYSTEM_INVISIBLE | STATE_SYSTEM_OFFSCREEN |
           STATE_SYSTEM_UNAVAILABLE)))
       return 0;
@@ skipping 45 matching lines @@
     cached_minimize_button_x_delta_ = minimize_button_offset;
   } else {
     RECT client_rect = {0};
     GetClientRect(hwnd_, &client_rect);
     cached_minimize_button_x_delta_ =
         client_rect.right - minimize_button_offset;
   }
   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_;
+}