Exclude caption button bounds from non-client frame hit testing

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

Index: chrome/browser/ui/views/frame/glass_browser_frame_view.cc
diff --git a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc b/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
index 64f28ac79fffb97eb8023698a873f66fa1ab2cc2..5754c8c080970885c5b830d0b82700826dbc38f7 100644
--- a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
+++ b/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/frame/glass_browser_frame_view.h"
 
+#include <dwmapi.h>
 #include <utility>
 
 #include "base/strings/utf_string_conversions.h"
@@ -28,6 +29,7 @@
 #include "ui/base/theme_provider.h"
 #include "ui/display/win/dpi.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/icon_util.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/scoped_canvas.h"
@@ -70,6 +72,9 @@ const int kNewTabCaptionMaximizedSpacing = 16;
 // TODO(bsep): Windows 10 caption buttons look very different and we would like
 // the profile switcher button to match on that platform.
 const int kProfileSwitcherButtonHeight = 20;
+// There is a small one-pixel strip right above the caption buttons in which the
+// resize border "peeks" through.
+const int kCaptionButtonTopInset = 1;
 
 // Converts the |image| to a Windows icon and returns the corresponding HICON
 // handle. |image| is resized to desired |width| and |height| if needed.
@@ -245,6 +250,29 @@ int GlassBrowserFrameView::NonClientHitTest(const gfx::Point& point) {
   if (frame_component != HTNOWHERE)
     return frame_component;
 
+  // On Windows 8+, the caption buttons are almost butted up to the top right
+  // corner of the window. This code will ensure that the mouse isn't set to a

[Peter Kasting, 2016/07/19 02:48:34]

Tiny nit: will ensure that -> ensures

[kylix_rd, 2016/07/19 14:11:14]

Is this a stylistic subjective preference or just trying to reducing verbiage? I
do tend to eschew brevity for more clarity.

[Peter Kasting, 2016/07/19 22:44:52]

I think "will ensure" is a bit unclear because it implies the code will do
something later rather than now.  So I was mainly just aiming to reduce
potential for misinterpretation.

[kylix_rd, 2016/07/20 16:04:25]

Fair enough. I'll defer to your judgement on this.

+  // size cursor while hovering over the caption buttons, thus giving the
+  // incorrect impression that the user can resize the window.
+  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
+    RECT button_bounds = {0};
+    if (SUCCEEDED(DwmGetWindowAttribute(views::HWNDForWidget(frame()),
+                                        DWMWA_CAPTION_BUTTON_BOUNDS,
+                                        &button_bounds,
+                                        sizeof(button_bounds)))) {
+      gfx::Rect buttons = gfx::ConvertRectToDIP(display::win::GetDPIScale(),
+                                                gfx::Rect(button_bounds));
+      // Even though the tiny area above the caption buttons is very difficult
+      // to hit, this ensures behavioral consistency with other top-level
+      // windows. It is worth noting that this area is the same size (1px)
+      // regardless of the current scale factor. For that reason the rect is
+      // inset after it is adjusted for the device scale.

[Peter Kasting, 2016/07/19 02:48:34]

Wait, isn't this sort of backwards?  If the region is always 1 px, then it seems
like we'd need to inset before converting to DIP to match that.  That would
potentially have the problem that the 1 px region might get "lost" due to the
lossy conversion to DIP... which suggests that we'd benefit from having the
current position and the desired rect be a PointF and a RectF, respectively...
ugh.  I hate that views works in DIP everywhere instead of px.

If all of the above is correct, then perhaps for now the code here is correct,
but we should have a comment like "The resize region around the caption buttons
is always 1 px.  Since views works in DIP, the closest we can get to this while
still ensuring there's always a resizable region is to inset after scaling. 
This may still display a resize cursor erroneously when the mouse isn't actually
in the outermost pixel, which seems better than not displaying a resize cursor
at all."

Or is that not right either?

[kylix_rd, 2016/07/19 14:11:14]

I'd thought about all of that. Yes, I would rather be working in pixels, but the
point being passed in is in DIPs. What if we scale the point back to pixels
instead for this test? Would that be more or less accurate? <shrug>

[Peter Kasting, 2016/07/19 22:44:52]

I think the issue is that since the mouse coord passed in is not a PointF, we
can't know whether it's actually on the outermost 1 px or not.  To fix this,
we'd need to pass in a PointF (or, even more wildly, pass in an integer in px
instead of DIP).  That sort of fix goes back to the whole "views does everything
in integer DIPs and it shouldn't" issue with high DPI, which isn't addressable
in the scope of this patch.  So probably the best thing for the code is to do it
the way you've done it, but explain in the comment why this is incorrect but the
best we can do without larger views API changes.

[kylix_rd, 2016/07/20 16:04:25]

I think we're in agreement here. This is the best approach given the API we're
working with.

+      buttons.Inset(0, kCaptionButtonTopInset, 0, 0);
+      if (buttons.Contains(point))
+        return HTNOWHERE;
+    }
+  }
+
   int top_border_thickness = FrameTopBorderThickness(false);
   // We want the resize corner behavior to apply to the kResizeCornerWidth
   // pixels at each end of the top and bottom edges.  Because |point|'s x