Exclude caption button bounds from non-client frame hit testing

chrome/browser/ui/views/frame/glass_browser_frame_view.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/glass_browser_frame_view.h"
 
 #include <utility>
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/windows_version.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/profiles/profile_indicator_icon.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "components/signin/core/common/profile_management_switches.h"
+#include "dwmapi.h"

[Peter Kasting, 2016/07/18 19:12:15]

This should be in <> and moved up above the <utility> #include.

[kylix_rd, 2016/07/18 20:35:10]

Done.

 #include "grit/theme_resources.h"
 #include "skia/ext/image_operations.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle_win.h"
 #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"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/resources/grit/views_resources.h"
 #include "ui/views/win/hwnd_util.h"
 #include "ui/views/window/client_view.h"
 
@@ skipping 22 matching lines @@
 const int kNewTabCaptionRestoredSpacing = 5;
 // In maximized mode, where the New Tab button and the caption buttons are at
 // similar vertical coordinates, we need to reserve a larger, 16 px gap to avoid
 // looking too cluttered.
 const int kNewTabCaptionMaximizedSpacing = 16;
 // Height of the profile switcher button. Same as the height of the Windows 7/8
 // caption buttons.
 // 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
+// sizable frame "peeks" through. This will ensure that the behavior is

[Peter Kasting, 2016/07/18 19:12:16]

Nit: sizable frame -> resize border

I would omit the second sentence and do any necessary explanation thereof at the
call site where this is used.

[kylix_rd, 2016/07/18 20:35:10]

Done.

+// consistent with most Windows applications.
+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.
 base::win::ScopedHICON CreateHICONFromSkBitmapSizedTo(
     const gfx::ImageSkia& image,
     int width,
     int height) {
   return IconUtil::CreateHICONFromSkBitmap(
       width == image.width() && height == image.height()
           ? *image.bitmap()
@@ skipping 155 matching lines @@
   gfx::Rect sys_menu_region(client_border_thickness,
                             display::win::GetSystemMetricsInDIP(SM_CYSIZEFRAME),
                             display::win::GetSystemMetricsInDIP(SM_CXSMICON),
                             display::win::GetSystemMetricsInDIP(SM_CYSMICON));
   if (sys_menu_region.Contains(point))
     return (frame_component == HTCLIENT) ? HTCLIENT : HTSYSMENU;
 
   if (frame_component != HTNOWHERE)
     return frame_component;
 
+  // On Windows 8+, the caption buttons are almost butted up to the top right
+  // of the window. This code will ensure that the mouse isn't set to a size

[Peter Kasting, 2016/07/18 19:12:15]

Nit: of -> corner of

[kylix_rd, 2016/07/18 20:35:10]

Done.

+  // 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};
+    const HWND hwnd = views::HWNDForWidget(frame());

[Peter Kasting, 2016/07/18 19:12:15]

Nit: I'd probably just inline this into the next statement

[kylix_rd, 2016/07/18 20:35:10]

Done.

+

[Peter Kasting, 2016/07/18 19:12:16]

Nit: No blank here

[kylix_rd, 2016/07/18 20:35:10]

Done.

+    if (SUCCEEDED(DwmGetWindowAttribute(hwnd, DWMWA_CAPTION_BUTTON_BOUNDS,
+                                        &button_bounds,
+                                        sizeof(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.

[Peter Kasting, 2016/07/18 19:12:15]

Nit: This comment really belongs on the Inset() call.  We should probably give
more detail about how this works, e.g. does Windows normally include the resize
border portion within the caption button bounds, and is it 1 px regardless of
scale factor?  The latter question also relates to my next comment.

[kylix_rd, 2016/07/18 20:35:10]

Done.

+      gfx::Rect buttons = gfx::ConvertRectToDIP(display::win::GetDPIScale(),
+                                                gfx::Rect(button_bounds));
+      buttons.Inset(0, kCaptionButtonTopInset, 0, 0);

[Peter Kasting, 2016/07/18 19:12:16]

Is it correct to inset after scaling?  It's hard for me to work out the effects
of different choices on different scale factors.

[kylix_rd, 2016/07/18 20:35:10]

From my (admittedly empirical) investigation, it does indeed seem to be a
constant 1px regardless of overall DSF. On my Surface Book set to 300% scaling,
the area above the buttons which hit-test as a sizable region seems to still be
1px.

+      if (buttons.Contains(point))
+        return (HTNOWHERE);

[Peter Kasting, 2016/07/18 19:12:16]

Should this be HTCAPTION?

Nit: No parens

[kylix_rd, 2016/07/18 20:35:10]

I want to ensure that subsequent processing lands at DefWindowProc which will do
the OS-level hit-testing. Making this HTCAPTION may cause odd follow-on effects,
such as disabling the caption buttons and make clicking on them move the window
rather than their intended function.

+    }
+  }
+
   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
   // coordinate is based on the DWM-inset portion of the window (so, it's 0 at
   // the first pixel inside the left DWM margin), we need to subtract the DWM
   // margin thickness, which we calculate as the total frame border thickness
   // minus the nonclient border thickness.
   const int dwm_margin = FrameBorderThickness() - client_border_thickness;
   int window_component = GetHTComponentForFrame(
       point, top_border_thickness, client_border_thickness,
@@ skipping 419 matching lines @@
   static bool initialized = false;
   if (!initialized) {
     for (int i = 0; i < kThrobberIconCount; ++i) {
       throbber_icons_[i] =
           ui::LoadThemeIconFromResourcesDataDLL(IDI_THROBBER_01 + i);
       DCHECK(throbber_icons_[i]);
     }
     initialized = true;
   }
 }