Fix for 504133 - wandering identity switcher button

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 18fcec12dd2d4d340a626bbb433d640eaf94026a..1725d6ac93c6a68e56f21561152ade80a6905dd2 100644
--- a/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc
+++ b/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc
@@ -6,6 +6,7 @@
 
 #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"
@@ -13,8 +14,50 @@
 
 namespace {
 
+const int kWin7ButtonBoundsPositionOffset = 1;
+const int kWin8ButtonBoundsPositionOffset = 10;
+const int kWin10ButtonBoundsPositionOffset = 6;
+const int kInvalidOffset = (int)0x80000000;
+
+using base::win::GetVersion;
 using display::win::ScreenWin;
 
+static int gButtonBoundsPositionOffset = kInvalidOffset;
+
+int GetDefaultButtonBoundsOffset() {

[Ilya Kulshin, 2016/05/09 19:53:23]

Add a comment describing how to determine the offset if a new OS version comes
out. It would also be good to have something to remind us to update this code
when we get a new version. Maybe a DCHECK? Or is that too aggressive?

[kylix_rd, 2016/05/09 21:54:23]

I think that adding a DCHECK may be too aggressive. All things being equal,
these defaults should never actually be used since the true offset is determined
programatically. These values are only used in the short time prior to the
window receiving WM_ACTIVATE and WM_GETTITLEBARINFOEX can be used.

+  return (GetVersion() >= base::win::VERSION_WIN10)

[Ilya Kulshin, 2016/05/09 19:53:23]

I think this code would be more readable as a series of if-else statements.

[kylix_rd, 2016/05/09 21:54:23]

Done.

+             ? kWin10ButtonBoundsPositionOffset
+             : (GetVersion() >= base::win::VERSION_WIN8)
+                   ? kWin8ButtonBoundsPositionOffset
+                   : kWin7ButtonBoundsPositionOffset;
+}
+
+// This function attempts to calculate the odd and varying difference
+// between the results of DwmGetWindowAttribute with the
+// DWMWA_CAPTION_BUTTON_BOUNDS flag and the information from the
+// WM_GETTITLEBARINFOEX message. It will return an empirically determined
+// offset until the window has been activated and the message returns
+// valid rectangles.
+int GetButtonBoundsPositionOffset(HWND hwnd,
+                                  const RECT& button_bounds,
+                                  const RECT& window_bounds,
+                                  bool was_activated) {

[Ilya Kulshin, 2016/05/09 19:53:23]

Consider making these into member functions so you don'thave to pass around
was_activated and hwnd

[kylix_rd, 2016/05/09 21:54:23]

Done.

+  if (gButtonBoundsPositionOffset == kInvalidOffset) {
+    if (!was_activated)
+      return GetDefaultButtonBoundsOffset();
+    TITLEBARINFOEX info = {0};
+    info.cbSize = sizeof(info);
+    SendMessage(hwnd, WM_GETTITLEBARINFOEX, 0, reinterpret_cast<LPARAM>(&info));
+    if (info.rgrect[2].right == info.rgrect[2].left ||
+        (info.rgstate[2] & (STATE_SYSTEM_INVISIBLE | STATE_SYSTEM_OFFSCREEN |
+                            STATE_SYSTEM_UNAVAILABLE)))
+      return GetDefaultButtonBoundsOffset();
+    gButtonBoundsPositionOffset =
+        info.rgrect[2].left - (button_bounds.left + window_bounds.left);
+  }
+  return gButtonBoundsPositionOffset;
+}
+
 int GetMinimizeButtonOffsetForWindow(HWND hwnd, bool was_activated) {
   bool dwm_button_pos = false;
   POINT minimize_button_corner = {0};
@@ -24,10 +67,17 @@ int GetMinimizeButtonOffsetForWindow(HWND hwnd, bool was_activated) {
     if (button_bounds.left != button_bounds.right) {
       // This converts the button coordinate into screen coordinates
       // thus, ensuring that the identity switcher is placed in the
-      // same location as before.
+      // same location as before. An additional constant is added because
+      // there is a difference between the caption button bounds and
+      // the values obtained through WM_GETTITLEBARINFOEX. This difference
+      // varies between OS versions, and no metric describing this difference
+      // has been located.
       RECT window_bounds = {0};
       if (GetWindowRect(hwnd, &window_bounds)) {
-        minimize_button_corner = {button_bounds.left + window_bounds.left, 0};
+        int offset = GetButtonBoundsPositionOffset(
+            hwnd, button_bounds, window_bounds, was_activated);
+        minimize_button_corner = {
+            button_bounds.left + window_bounds.left + offset, 0};
         dwm_button_pos = true;
       }
     }
@@ -48,13 +98,11 @@ int GetMinimizeButtonOffsetForWindow(HWND hwnd, bool was_activated) {
     // 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 (!was_activated ||
-        titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right ||
+    if (titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right ||
         (titlebar_info.rgstate[2] &
          (STATE_SYSTEM_INVISIBLE | STATE_SYSTEM_OFFSCREEN |
-          STATE_SYSTEM_UNAVAILABLE))) {
+          STATE_SYSTEM_UNAVAILABLE)))
       return 0;
-    }
     minimize_button_corner = {titlebar_info.rgrect[2].left, 0};
   }
 
@@ -94,7 +142,8 @@ void MinimizeButtonMetrics::OnHWNDActivated() {
 }
 
 int MinimizeButtonMetrics::GetMinimizeButtonOffsetX() const {
-  if (!ui::win::IsAeroGlassEnabled() || cached_minimize_button_x_delta_ == 0) {
+  if (was_activated_ || !ui::win::IsAeroGlassEnabled() ||

[kylix_rd, 2016/05/09 18:24:52]

This was the cause of the regression. The code was over-zealously caching the
button offset. Once the window has been activated once, it should continue to
calculate the actual location of the Identity switcher button and not return the
cached value.

+      cached_minimize_button_x_delta_ == 0) {
     const int minimize_button_offset = GetAndCacheMinimizeButtonOffsetX();
     if (minimize_button_offset > 0)
       return minimize_button_offset;