1473764 - Query spatial boundaries for double- and higher-order clicks from Windows in a DPI-aware manner

Status: NEW

(Core :: Widget: Win32, enhancement)

Description

[h-h]

In `nsWindow.cpp` for Windows, besides `GetDoubleClickTime()` there are spatial boundaries which 2 clicks must meet to be grouped together. `GetSystemMetrics()` is not DPI-aware, so we must use `GetSystemMetricsForDpi()`, if available, to query the values `SM_CXDOUBLECLK` and `SM_CYDOUBLECLK`.

Comment 1

[h-h]

Attachment: Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

Also discussed in review of this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=550528

Review commit: https://reviewboard.mozilla.org/r/255174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255174/

Comment 2

[h-h]

I left comment(s) at: https://reviewboard.mozilla.org/r/255174/diff/1#index_header .

Comment 3

[h-h]

The HTML attachments from bug 550528 prove, that it's still working as before:
- https://bug550528.bmoattachments.org/attachment.cgi?id=8989991
- Alternative: https://bug550528.bmoattachments.org/attachment.cgi?id=430677

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

https://reviewboard.mozilla.org/r/255174/#review262098

Looks good to me but I'm not familiar with HiDPI nor per-monitor DPI code. Perhaps, jimm?

::: commit-message-6c0fa:3
(Diff revision 1)
> +Also discussed in review of this bug:
> +https://bugzilla.mozilla.org/show_bug.cgi?id=550528

Please explain *why* this change is necessary and the points of this change which is hard to read from the code instead of pointing URL because when other developers investigate the reason of this change, it should be explained by the commit message.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

jimm:

Are you familiar with HiDPI and per-monitor DPI code? Or Jonathan Kew or emk-san is better reviewer for this?

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

https://reviewboard.mozilla.org/r/255174/#review263502

This looks correct to me. Lets fix the commit and resubmit for an r+.

::: commit-message-6c0fa:3
(Diff revision 1)
> +Also discussed in review of this bug:
> +https://bugzilla.mozilla.org/show_bug.cgi?id=550528

yeah lets clean this up.

Comment 7

[h-h]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255174/diff/1-2/

Comment 8

[h-h]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255174/diff/2-3/

Comment 9

[h-h]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

I changed the commit message.

Comment 10

[h-h]

Bump.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

https://reviewboard.mozilla.org/r/255174/#review266790

::: widget/windows/WinUtils.cpp:1047
(Diff revision 3)
>    return info.mOutWnd;
>  }
>  
>  /* static */
> +gfx::IntSize
> +WinUtils::GetSpatialDoubleClickBoundaries(float aDpi) {

nit - other methods push the paren to ther next line.

::: widget/windows/WinUtils.cpp:1053
(Diff revision 3)
> +  gfx::IntSize boundaries;
> +
> +  if (sGetSystemMetricsForDpi) {
> +    int32_t dpi = NSToIntRound(aDpi);
> +    boundaries = gfx::IntSize(
> +      sGetSystemMetricsForDpi(SM_CXDOUBLECLK, dpi),

I wonder how often this new api fails? Don't think we need a check but lets add a debug assert on a zero result to be safe.

Comment 12

[h-h]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255174/diff/3-4/

Comment 13

[h-h]

Hey Jim,

I fixed your issues. It it okay this way?

Comment 14

[h-h]

Comment on attachment 8990161 [details]
Bug 1473764 - DPI-aware spatial boundaries for double- and higher-order clicks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255174/diff/4-5/

Comment 15

[Jim Mathies [:jimm]]

(In reply to h-h from comment #13)
> Hey Jim,
> 
> I fixed your issues. It it okay this way?

Sure, you should post the latest patch and ask for a review.