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 "chrome/browser/ui/views/frame/minimize_button_metrics_win.h"
+#include "dwmapi.h"
 #include "ui/base/win/shell.h"
 #include "ui/display/win/dpi.h"
+#include "ui/display/win/screen_win.h"

[kylix_rd, 2016/05/03 22:13:12]

Pre-submit checks complained about the include order... it seems that the order
was intentional since the first include was this file's own include.

 
 namespace {
 
-int GetMinimizeButtonOffsetForWindow(HWND hwnd) {
+using display::win::ScreenWin;
+
+int GetMinimizeButtonOffsetForWindow(HWND hwnd, bool was_activated = true) {

[kylix_rd, 2016/05/03 22:13:11]

TODO: remove default parameter.

+  bool dwm_button_pos = false;
+  POINT minimize_button_corner = { 0 };
+  RECT button_bounds = { 0 };
+  if (SUCCEEDED(DwmGetWindowAttribute(hwnd, DWMWA_CAPTION_BUTTON_BOUNDS,
+       reinterpret_cast<PVOID>(&button_bounds), sizeof(button_bounds)))) {
+    if (button_bounds.left != button_bounds.right) {

[kylix_rd, 2016/05/03 22:13:12]

If these values are the same (typically 0), then DWM composition is disabled for
this window or is globally disabled.

+      // This converts the button coordinate into screen coordinates
+      // thus, ensuring that the identity switcher is placed in the
+      // same location as before.
+      RECT window_bounds = { 0 };
+      if (GetWindowRect(hwnd, reinterpret_cast<LPRECT>(&window_bounds)))
+      {
+        minimize_button_corner =
+          { button_bounds.left + window_bounds.left, 0 };
+        dwm_button_pos = true;
+      }
+    }
+  }
+  if (!dwm_button_pos) {

[kylix_rd, 2016/05/03 22:13:11]

If for whatever reason, any of the above fails, fall back to the old message
technique.

   // 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));
+    TITLEBARINFOEX titlebar_info = { 0 };
+    titlebar_info.cbSize = sizeof(TITLEBARINFOEX);
+    SendMessage(hwnd, WM_GETTITLEBARINFOEX, 0,
+      reinterpret_cast<WPARAM>(&titlebar_info));
 
-  if (titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right ||
+    if (!was_activated ||
+       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;
+        STATE_SYSTEM_OFFSCREEN |
+        STATE_SYSTEM_UNAVAILABLE))) {
+      return 0;
+    }
+    minimize_button_corner = { titlebar_info.rgrect[2].left, 0 };
   }
 
   // WM_GETTITLEBARINFOEX returns rects in screen coordinates in pixels.
-  // We need to convert the minimize button corner offset to DIP before
-  // returning it.
-  POINT minimize_button_corner = { titlebar_info.rgrect[2].left, 0 };
+  // 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);
-  return minimize_button_corner.x / display::win::GetDPIScale();
+  gfx::Point pixel_point = { minimize_button_corner.x, 0 };
+  gfx::Point dip_point = ScreenWin::ClientToDIPPoint(hwnd, pixel_point);
+  return dip_point.x();

[kylix_rd, 2016/05/03 22:13:12]

From inspecting the code in display::win::ScreenWin, ClientToDIPPoint seems to
convert the pixel-based point to DIP based points using the scaling factor of
the screen on which the given HWND lives. If that's not the case, then please
advise.

 }
 
 }  // namespace
 
 // static
 int MinimizeButtonMetrics::last_cached_minimize_button_x_delta_ = 0;
 
 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) {
   DCHECK(!hwnd_);
   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.
+  // NOTE: we don't cache here as it seems only after the activate is the
+  // value correct.
 }
 
 int MinimizeButtonMetrics::GetMinimizeButtonOffsetX() const {
   // 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_ || !ui::win::IsAeroGlassEnabled() ||
+  // 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.

[kylix_rd, 2016/05/03 22:13:12]

TODO: update comment

+  if (/*was_activated_ || */!ui::win::IsAeroGlassEnabled() ||

[kylix_rd, 2016/05/03 22:13:12]

TODO: cleanup

Always attempt to get the button offset since DwmGetWindowAttribute doesn't
suffer from the strange issue highlighted in this comment.

       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 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_);
 
   if (base::i18n::IsRTL())
     return cached_minimize_button_x_delta_;
 
   RECT client_rect = {0};
   GetClientRect(hwnd_, &client_rect);
   return client_rect.right - cached_minimize_button_x_delta_;
 }
 
 int MinimizeButtonMetrics::GetAndCacheMinimizeButtonOffsetX() const {
-  const int minimize_button_offset = GetMinimizeButtonOffsetForWindow(hwnd_);
+  const int minimize_button_offset =
+    GetMinimizeButtonOffsetForWindow(hwnd_, was_activated_);

[kylix_rd, 2016/05/03 22:13:12]

TODO: cleaner way to handle this?

   if (minimize_button_offset <= 0)
     return 0;
 
   if (base::i18n::IsRTL()) {
     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;
 }