Fix for 504133 - wandering identity switcher button

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"
 
 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};
   RECT button_bounds = {0};
   if (SUCCEEDED(DwmGetWindowAttribute(hwnd, DWMWA_CAPTION_BUTTON_BOUNDS,
                                       &button_bounds, sizeof(button_bounds)))) {
     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;
       }
     }
   }
   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));
 
     // 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 (!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};
   }
 
   // WM_GETTITLEBARINFOEX returns rects in screen coordinates in pixels.
   // DWMNA_CAPTION_BUTTON_BOUNDS is in window (not client) coordinates,
   // but it has been converted to screen coordinates above. We need to
   // convert the minimize button corner offset to DIP before returning it.
   MapWindowPoints(HWND_DESKTOP, hwnd, &minimize_button_corner, 1);
   gfx::Point pixel_point = {minimize_button_corner.x, 0};
   gfx::Point dip_point = ScreenWin::ClientToDIPPoint(hwnd, pixel_point);
@@ skipping 19 matching lines @@
   hwnd_ = hwnd;
 }
 
 void MinimizeButtonMetrics::OnHWNDActivated() {
   was_activated_ = true;
   // NOTE: we don't cache here as it seems only after the activate is the
   // value correct.
 }
 
 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;
   }
 
   // If we fail to get the minimize button offset via the WM_GETTITLEBARINFOEX
   // message or DwmGetWindowAttribute then calculate and return this via the
   // cached_minimize_button_x_delta_ member value. Please see
   // CacheMinimizeButtonDelta() for more details.
   DCHECK(cached_minimize_button_x_delta_);
@@ skipping 16 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;
 }