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"

[Ilya Kulshin, 2016/05/03 23:40:52]

Keep this as the first include. In general, Chromium follows the Google C++
style guide for include order.

[kylix_rd, 2016/05/04 00:05:46]

Ok, thanks.

+#include "dwmapi.h"
 #include "ui/base/win/shell.h"
 #include "ui/display/win/dpi.h"
+#include "ui/display/win/screen_win.h"
 
 namespace {
 
-int GetMinimizeButtonOffsetForWindow(HWND hwnd) {
+using display::win::ScreenWin;
+
+int GetMinimizeButtonOffsetForWindow(HWND hwnd, bool was_activated) {
+  bool dwm_button_pos = false;
+  POINT minimize_button_corner = { 0 };

[sky, 2016/05/03 23:40:47]

POINT->gfx::Point

[kylix_rd, 2016/05/04 14:25:10]

This is a POINT because it is used down in MapWindowPoints.

[sky, 2016/05/04 17:42:03]

Got it.

+  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) {
+      // 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)))

[sky, 2016/05/03 23:40:47]

Why do you need a reinterpret_cast here?

[Ilya Kulshin, 2016/05/03 23:40:52]

Do you actually need the reinterpret_cast here? Also opening brace should be on
same line.

[kylix_rd, 2016/05/04 00:05:46]

My C++ skills are clearly lacking here... You're probably correct.
 
Of course.

+      {

[sky, 2016/05/03 23:40:47]

nit: { on previous line

+        minimize_button_corner =
+          { button_bounds.left + window_bounds.left, 0 };
+        dwm_button_pos = true;
+      }
+    }
+  }
+  if (!dwm_button_pos) {
   // 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).

[Ilya Kulshin, 2016/05/03 23:40:52]

Indent comment to be aligned with the code, or move outside the if block. Hint:
"git cl format" is your friend (most of the time).

-  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,

[Ilya Kulshin, 2016/05/03 23:40:52]

Do we still need this fallback? In what cases does this give us a useful unswer
that DwmGetWindowAttribute does not?

[kylix_rd, 2016/05/04 00:05:46]

When DWM composing is disable globally or even on a per-window basis,
DwmGetWindowAttribute returns an empty rect (all 0s)

+      reinterpret_cast<WPARAM>(&titlebar_info));
 
-  if (titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right ||
+    // 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 ||
       (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();
 }
 
 }  // 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() ||
-      cached_minimize_button_x_delta_ == 0) {
+  if (!ui::win::IsAeroGlassEnabled() || 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
+  // 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_);
 
   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_);
   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;
 }