Call EnableNonClientDpiScaling

Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
performing the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787
Committed: https://crrev.com/b297db871b54cd58c32c96897a7a59af295ba796
Cr-Commit-Position: refs/heads/master@{#438674}

[robliao, 2016-12-14 01:08:57]

Description was changed from

==========
Call EnableNonClientDpiScaling For HWNDs for Windows 10.0.14393.0

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
forming the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787
==========

to

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
forming the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787
==========

[robliao, 2016-12-14 01:09:51]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 01:10:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-12-14 01:11:25]

Description was changed from

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
forming the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787
==========

to

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
performing the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787
==========

[robliao, 2016-12-14 01:28:12]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[robliao, 2016-12-14 01:28:29]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-14 01:28:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-12-14 01:28:53]

robliao@chromium.org changed reviewers:
+ sky@chromium.org

[robliao, 2016-12-14 01:28:54]

sky: Please review this change. Thanks!

[sky, 2016-12-14 02:42:41]

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl.cc
File ui/gfx/win/window_impl.cc (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:297: WindowImpl* window =
reinterpret_cast<WindowImpl*>(GetWindowUserData(hwnd));
This isn't necessary for the if case, maybe move declaration above if and move
this to else case?

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1816: return 0;
Should this return TRUE (old code did).

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.h:582: // Whether EnableNonClientDpiScaling
was called successfully with this window.
Can you describe what that means (folks looking at this class won't necessarily
know what this means).

[commit-bot: I haz the power, 2016-12-14 03:30:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 03:30:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[robliao, 2016-12-14 18:50:19]

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl.cc
File ui/gfx/win/window_impl.cc (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:297: WindowImpl* window =
reinterpret_cast<WindowImpl*>(GetWindowUserData(hwnd));
On 2016/12/14 02:42:41, sky wrote:
> This isn't necessary for the if case, maybe move declaration above if and move
> this to else case?

While that is true, this seems more readable and GetWindowUserData is pretty
cheap (it's just grabbing a pointer off of the underlying HWND and only for
extra for WM_NCCREATE which is sent only once in an HWND's lifetime).

Happy to change it if you feel strongly about it, but that would mean we would
have to leave window uninitialized before the if-else check:

WindowImpl* window; // Uninitialized
if (message == WM_NCCREATE) {
  ...
  window = reinterpret_cast<>(cs->lpCreateParams)
  ...
} else {
  window = reinterpret_cast<>(GetWindowUserData(hwnd))
}

or check for WM_NCCREATE twice

const WindowImpl* window = (message == WM_NCCREATE) ? (Get WindowImpl from
l_param) : GetWindowUserData(...)
if (message == WM_NCCREATE) {
  SetWindowLongData(hwnd, window);
  ...
}

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1816: return 0;
On 2016/12/14 02:42:41, sky wrote:
> Should this return TRUE (old code did).

It should return FALSE, to doubly reenforce that we're not handling the message
here. (Normally, if you return FALSE during WM_NCCREATE, this should fail the
CreateWindow[Ex] call. This is useful to signal to the CreateWindow[Ex] caller
that something fatal happened and we cannot proceed in creating the HWND).

While we do special initialization with WM_NCCREATE, we're not "handling" it
(e.g. intending to override default behavior). As a result, we should let
DefWindowProc take its course.

The SetMsgHandled(FALSE) at the top signals to WindowImpl that DefWindowProc
should receive this message. This causes the caller to ignore this functions
return value.

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.h:582: // Whether EnableNonClientDpiScaling
was called successfully with this window.
On 2016/12/14 02:42:41, sky wrote:
> Can you describe what that means (folks looking at this class won't
necessarily
> know what this means).

Done.

  // Whether EnableNonClientDpiScaling was called successfully with this window.
  // This flag exists because EnableNonClientDpiScaling must be called during
  // WM_NCCREATE and EnableChildWindowDpiMessage is called after window
  // creation. We don't want to call both, so this helps us determine if a call
  // to EnableChildWindowDpiMessage is necessary.

[robliao, 2016-12-14 18:50:24]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 18:51:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-12-14 18:54:25]

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl.cc
File ui/gfx/win/window_impl.cc (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:297: WindowImpl* window =
reinterpret_cast<WindowImpl*>(GetWindowUserData(hwnd));
On 2016/12/14 18:50:19, robliao wrote:
> On 2016/12/14 02:42:41, sky wrote:
> > This isn't necessary for the if case, maybe move declaration above if and
move
> > this to else case?
> 
> While that is true, this seems more readable and GetWindowUserData is pretty
> cheap (it's just grabbing a pointer off of the underlying HWND and only for
> extra for WM_NCCREATE which is sent only once in an HWND's lifetime).

I personally think it's clearer overall to not do the lookup twice.

> Happy to change it if you feel strongly about it, but that would mean we would
> have to leave window uninitialized before the if-else check:
> 
> WindowImpl* window; // Uninitialized

How about setting it to null?

> if (message == WM_NCCREATE) {
>   ...
>   window = reinterpret_cast<>(cs->lpCreateParams)
>   ...
> } else {
>   window = reinterpret_cast<>(GetWindowUserData(hwnd))
> }
> 
> or check for WM_NCCREATE twice
> 
> const WindowImpl* window = (message == WM_NCCREATE) ? (Get WindowImpl from
> l_param) : GetWindowUserData(...)
> if (message == WM_NCCREATE) {
>   SetWindowLongData(hwnd, window);
>   ...
> }

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.h:582: // Whether EnableNonClientDpiScaling
was called successfully with this window.
On 2016/12/14 18:50:19, robliao wrote:
> On 2016/12/14 02:42:41, sky wrote:
> > Can you describe what that means (folks looking at this class won't
> necessarily
> > know what this means).
> 
> Done.
> 
>   // Whether EnableNonClientDpiScaling was called successfully with this
window.
>   // This flag exists because EnableNonClientDpiScaling must be called during
>   // WM_NCCREATE and EnableChildWindowDpiMessage is called after window
>   // creation. We don't want to call both, so this helps us determine if a
call
>   // to EnableChildWindowDpiMessage is necessary.

Excellent! But I was hoping you would describe what EnableNonClientDpiScaling()
does. But I'm ok with the improved description here.

[robliao, 2016-12-14 19:03:18]

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl.cc
File ui/gfx/win/window_impl.cc (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:297: WindowImpl* window =
reinterpret_cast<WindowImpl*>(GetWindowUserData(hwnd));
On 2016/12/14 18:54:24, sky wrote:
> On 2016/12/14 18:50:19, robliao wrote:
> > On 2016/12/14 02:42:41, sky wrote:
> > > This isn't necessary for the if case, maybe move declaration above if and
> move
> > > this to else case?
> > 
> > While that is true, this seems more readable and GetWindowUserData is pretty
> > cheap (it's just grabbing a pointer off of the underlying HWND and only for
> > extra for WM_NCCREATE which is sent only once in an HWND's lifetime).
> 
> I personally think it's clearer overall to not do the lookup twice.
> 
> > Happy to change it if you feel strongly about it, but that would mean we
would
> > have to leave window uninitialized before the if-else check:
> > 
> > WindowImpl* window; // Uninitialized
> 
> How about setting it to null?
> 
> > if (message == WM_NCCREATE) {
> >   ...
> >   window = reinterpret_cast<>(cs->lpCreateParams)
> >   ...
> > } else {
> >   window = reinterpret_cast<>(GetWindowUserData(hwnd))
> > }
> > 
> > or check for WM_NCCREATE twice
> > 
> > const WindowImpl* window = (message == WM_NCCREATE) ? (Get WindowImpl from
> > l_param) : GetWindowUserData(...)
> > if (message == WM_NCCREATE) {
> >   SetWindowLongData(hwnd, window);
> >   ...
> > }
> 

Done.

The main reason I don't like setting it to null is we never read (and shouldn't
read) from the value before it is set below.

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/2574933002/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.h:582: // Whether EnableNonClientDpiScaling
was called successfully with this window.
On 2016/12/14 18:54:24, sky wrote:
> On 2016/12/14 18:50:19, robliao wrote:
> > On 2016/12/14 02:42:41, sky wrote:
> > > Can you describe what that means (folks looking at this class won't
> > necessarily
> > > know what this means).
> > 
> > Done.
> > 
> >   // Whether EnableNonClientDpiScaling was called successfully with this
> window.
> >   // This flag exists because EnableNonClientDpiScaling must be called
during
> >   // WM_NCCREATE and EnableChildWindowDpiMessage is called after window
> >   // creation. We don't want to call both, so this helps us determine if a
> call
> >   // to EnableChildWindowDpiMessage is necessary.
> 
> Excellent! But I was hoping you would describe what
EnableNonClientDpiScaling()
> does. But I'm ok with the improved description here.

EnableNonClientDpiScaling is a public Windows function; I defer to MSDN for that
(https://msdn.microsoft.com/en-us/library/windows/desktop/mt748621(v=vs.85).aspx)

[sky, 2016-12-14 19:05:00]

LGTM

[robliao, 2016-12-14 19:06:48]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 19:07:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 20:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 20:42:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[robliao, 2016-12-14 20:58:26]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 20:59:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-12-14 21:42:49]

Patchset #4 (id:80001) has been deleted

[robliao, 2016-12-14 22:16:06]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 22:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-12-14 22:16:45]

FYI, fixed a bad test that assumed that it processed all messages (resulting in
a failure during WM_NCCREATE).

-    return true;
+    // Handle WM_NCCALCSIZE because the test below assumes there is no
+    // non-client area, affecting EventSystemLocationFromNative's client to
+    // screen coordinate transform.
+    return message == WM_NCCALCSIZE;

[sky, 2016-12-14 22:22:21]

SLGTM

[commit-bot: I haz the power, 2016-12-14 23:08:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 23:08:52]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-12-14 23:26:24]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-12-14 23:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2574933002/#ps100001
(title: "Fix Bad Test")

[commit-bot: I haz the power, 2016-12-14 23:26:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 23:36:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481757984522560,
"parent_rev": "972b78f53b998cb43bd99039c039a315c5542932", "commit_rev":
"bb70467f63856f4ca7014dc8325110010929a679"}

[commit-bot: I haz the power, 2016-12-14 23:36:47]

Description was changed from

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
performing the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787
==========

to

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
performing the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787

Review-Url: https://codereview.chromium.org/2574933002
==========

[commit-bot: I haz the power, 2016-12-14 23:36:48]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-12-14 23:39:30]

Description was changed from

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
performing the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787

Review-Url: https://codereview.chromium.org/2574933002
==========

to

==========
Call EnableNonClientDpiScaling

Windows versions after 10.0.14393.0 have EnableNonClientDpiScaling,
performing the same function as EnableChildWindowDpiMessage for us.

EnableChildWindowDpiMessage was also removed in 10.0.14393.0 if not
earlier.

This change also allows delegates to handle the WM_NCCREATE message,
required to properly call EnableNonClientDpiScaling.

BUG=642956, 658787

Committed: https://crrev.com/b297db871b54cd58c32c96897a7a59af295ba796
Cr-Commit-Position: refs/heads/master@{#438674}
==========

[commit-bot: I haz the power, 2016-12-14 23:39:33]

Patchset 4 (id:??) landed as
https://crrev.com/b297db871b54cd58c32c96897a7a59af295ba796
Cr-Commit-Position: refs/heads/master@{#438674}