Fix font sizes being incorrect when using --force-device-scale-factor.

Fix font sizes being incorrect when using --force-device-scale-factor.

Normalizes font sizes received from Windows based on the unforced
device scale factor, so that when they're scaled up later they end up
the right sized for the forced scale.

BUG=675933
Review-Url: https://codereview.chromium.org/2695523002
Cr-Commit-Position: refs/heads/master@{#450869}
Committed: https://chromium.googlesource.com/chromium/src/+/6cf3aecafe0e4790a0aa0dd8f44602acce121a2e

[Bret, 2017-02-12 02:58:43]

Description was changed from

==========
Fix font sizes being incorrect when using --force-device-scale-factor.

Normalizes font sizes received from Windows based on the unforced
device scale factor, so that when they're scaled up later they end up
the right sized for the forced scale.

BUG=675933
==========

to

==========
Fix font sizes being incorrect when using --force-device-scale-factor.

Normalizes font sizes received from Windows based on the unforced
device scale factor, so that when they're scaled up later they end up
the right sized for the forced scale.

BUG=675933
==========

[Bret, 2017-02-12 02:58:44]

bsep@chromium.org changed reviewers:
+ jshin@chromium.org, robliao@chromium.org

[Bret, 2017-02-12 02:58:51]

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

[commit-bot: I haz the power, 2017-02-12 02:59:01]

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

[Bret, 2017-02-12 03:10:51]

robliao: ui/display/win
jshin: ui/base/l10n

Rob, I don't really like exposing GetUnforcedDeviceScaleFactor, can you think of
a better way?

[robliao, 2017-02-13 17:57:33]

https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
File ui/base/l10n/l10n_util_win.cc (right):

https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
ui/base/l10n/l10n_util_win.cc:161:
AdjustUIFontForDIP(display::win::GetUnforcedDeviceScaleFactor(), logfont);
Just to make sure I understand this correctly, the issue here is that the font
system completely works in pixels and we perform a conversion to make things DIP
in views, correct?

If that's the case, then I don't see an alternative other than to provide the
actual DPI to the font system. See comment on display.

[Bret, 2017-02-14 20:29:20]

https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
File ui/base/l10n/l10n_util_win.cc (right):

https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
ui/base/l10n/l10n_util_win.cc:161:
AdjustUIFontForDIP(display::win::GetUnforcedDeviceScaleFactor(), logfont);
On 2017/02/13 17:57:33, robliao wrote:
> Just to make sure I understand this correctly, the issue here is that the font
> system completely works in pixels and we perform a conversion to make things
DIP
> in views, correct?
> 
> If that's the case, then I don't see an alternative other than to provide the
> actual DPI to the font system. See comment on display.

Our font system works in DIPs, as expected. This value is coming from Windows,
so it's in pixels.

I'm not sure what comment you're referring to.

[robliao, 2017-02-14 20:43:09]

https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
File ui/base/l10n/l10n_util_win.cc (right):

https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
ui/base/l10n/l10n_util_win.cc:161:
AdjustUIFontForDIP(display::win::GetUnforcedDeviceScaleFactor(), logfont);
On 2017/02/14 20:29:20, Bret Sepulveda wrote:
> On 2017/02/13 17:57:33, robliao wrote:
> > Just to make sure I understand this correctly, the issue here is that the
font
> > system completely works in pixels and we perform a conversion to make things
> DIP
> > in views, correct?
> > 
> > If that's the case, then I don't see an alternative other than to provide
the
> > actual DPI to the font system. See comment on display.
> 
> Our font system works in DIPs, as expected. This value is coming from Windows,
> so it's in pixels.
> 
> I'm not sure what comment you're referring to.

That's fun. I think what I meant to say is let's pipe this through display. 

display::Display already provides the forced DPI, it stands to reason that it
should also provide the unforced one if someone really needs it.

[Bret, 2017-02-15 02:02:36]

On 2017/02/14 20:43:09, robliao wrote:
>
https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
> File ui/base/l10n/l10n_util_win.cc (right):
> 
>
https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
> ui/base/l10n/l10n_util_win.cc:161:
> AdjustUIFontForDIP(display::win::GetUnforcedDeviceScaleFactor(), logfont);
> On 2017/02/14 20:29:20, Bret Sepulveda wrote:
> > On 2017/02/13 17:57:33, robliao wrote:
> > > Just to make sure I understand this correctly, the issue here is that the
> font
> > > system completely works in pixels and we perform a conversion to make
things
> > DIP
> > > in views, correct?
> > > 
> > > If that's the case, then I don't see an alternative other than to provide
> the
> > > actual DPI to the font system. See comment on display.
> > 
> > Our font system works in DIPs, as expected. This value is coming from
Windows,
> > so it's in pixels.
> > 
> > I'm not sure what comment you're referring to.
> 
> That's fun. I think what I meant to say is let's pipe this through display. 
> 
> display::Display already provides the forced DPI, it stands to reason that it
> should also provide the unforced one if someone really needs it.

Sorry, I'm still confused. Do you want a Display::GetUnforcedDeviceScaleFactor
function? It doesn't seem like it has access to that information from there,
since it just has the final number which could be forced or unforced. I don't
really understanding the plumbing of Display though.

[robliao, 2017-02-15 02:22:21]

On 2017/02/15 02:02:36, Bret Sepulveda wrote:
> On 2017/02/14 20:43:09, robliao wrote:
> >
>
https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
> > File ui/base/l10n/l10n_util_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
> > ui/base/l10n/l10n_util_win.cc:161:
> > AdjustUIFontForDIP(display::win::GetUnforcedDeviceScaleFactor(), logfont);
> > On 2017/02/14 20:29:20, Bret Sepulveda wrote:
> > > On 2017/02/13 17:57:33, robliao wrote:
> > > > Just to make sure I understand this correctly, the issue here is that
the
> > font
> > > > system completely works in pixels and we perform a conversion to make
> things
> > > DIP
> > > > in views, correct?
> > > > 
> > > > If that's the case, then I don't see an alternative other than to
provide
> > the
> > > > actual DPI to the font system. See comment on display.
> > > 
> > > Our font system works in DIPs, as expected. This value is coming from
> Windows,
> > > so it's in pixels.
> > > 
> > > I'm not sure what comment you're referring to.
> > 
> > That's fun. I think what I meant to say is let's pipe this through display. 
> > 
> > display::Display already provides the forced DPI, it stands to reason that
it
> > should also provide the unforced one if someone really needs it.
> 
> Sorry, I'm still confused. Do you want a Display::GetUnforcedDeviceScaleFactor
> function? It doesn't seem like it has access to that information from there,
> since it just has the final number which could be forced or unforced. I don't
> really understanding the plumbing of Display though.

Discussed offline.

We really want the System DPI in this case, so getting this information from the
display doesn't really make sense.

Instead, we're going to expose this in
display::ScreenWin::GetSystemScaleFactor (feel free to discuss the name).
to avoid having folks use dpi.h (I'm trying to scale back usage of this
include).

[Bret, 2017-02-15 02:40:57]

On 2017/02/15 02:22:21, robliao wrote:
> On 2017/02/15 02:02:36, Bret Sepulveda wrote:
> > On 2017/02/14 20:43:09, robliao wrote:
> > >
> >
>
https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
> > > File ui/base/l10n/l10n_util_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2695523002/diff/20001/ui/base/l10n/l10n_util_...
> > > ui/base/l10n/l10n_util_win.cc:161:
> > > AdjustUIFontForDIP(display::win::GetUnforcedDeviceScaleFactor(), logfont);
> > > On 2017/02/14 20:29:20, Bret Sepulveda wrote:
> > > > On 2017/02/13 17:57:33, robliao wrote:
> > > > > Just to make sure I understand this correctly, the issue here is that
> the
> > > font
> > > > > system completely works in pixels and we perform a conversion to make
> > things
> > > > DIP
> > > > > in views, correct?
> > > > > 
> > > > > If that's the case, then I don't see an alternative other than to
> provide
> > > the
> > > > > actual DPI to the font system. See comment on display.
> > > > 
> > > > Our font system works in DIPs, as expected. This value is coming from
> > Windows,
> > > > so it's in pixels.
> > > > 
> > > > I'm not sure what comment you're referring to.
> > > 
> > > That's fun. I think what I meant to say is let's pipe this through
display. 
> > > 
> > > display::Display already provides the forced DPI, it stands to reason that
> it
> > > should also provide the unforced one if someone really needs it.
> > 
> > Sorry, I'm still confused. Do you want a
Display::GetUnforcedDeviceScaleFactor
> > function? It doesn't seem like it has access to that information from there,
> > since it just has the final number which could be forced or unforced. I
don't
> > really understanding the plumbing of Display though.
> 
> Discussed offline.
> 
> We really want the System DPI in this case, so getting this information from
the
> display doesn't really make sense.
> 
> Instead, we're going to expose this in
> display::ScreenWin::GetSystemScaleFactor (feel free to discuss the name).
> to avoid having folks use dpi.h (I'm trying to scale back usage of this
> include).

This is done.

[jungshik at Google, 2017-02-15 18:27:33]

ui/base/l10n: LGTM

[robliao, 2017-02-15 18:43:16]

Looks generally good. Two comments.

https://codereview.chromium.org/2695523002/diff/40001/ui/base/l10n/l10n_util_...
File ui/base/l10n/l10n_util_win_unittest.cc (right):

https://codereview.chromium.org/2695523002/diff/40001/ui/base/l10n/l10n_util_...
ui/base/l10n/l10n_util_win_unittest.cc:53: lf.lfHeight = 18;
Document where 18 originated (even if it's arbitrary) and make it constexpr to
be used in the EXPECT_EQ statement below.

https://codereview.chromium.org/2695523002/diff/40001/ui/display/win/screen_w...
File ui/display/win/screen_win.h (right):

https://codereview.chromium.org/2695523002/diff/40001/ui/display/win/screen_w...
ui/display/win/screen_win.h:105: // Returns the system's global scale factor. In
most cases you should prefer
Possible rephrasing. What do you think?

Returns the system's global scale factor ignoring --force-device-scale-factor.
Only call this if you are working with Windows metrics global to the display.
Otherwise you should call GetScaleFactorForHWND() to handle per-monitor scale
factors.

[Bret, 2017-02-15 20:43:16]

https://codereview.chromium.org/2695523002/diff/40001/ui/base/l10n/l10n_util_...
File ui/base/l10n/l10n_util_win_unittest.cc (right):

https://codereview.chromium.org/2695523002/diff/40001/ui/base/l10n/l10n_util_...
ui/base/l10n/l10n_util_win_unittest.cc:53: lf.lfHeight = 18;
On 2017/02/15 18:43:15, robliao wrote:
> Document where 18 originated (even if it's arbitrary) and make it constexpr to
> be used in the EXPECT_EQ statement below.

Done.

https://codereview.chromium.org/2695523002/diff/40001/ui/display/win/screen_w...
File ui/display/win/screen_win.h (right):

https://codereview.chromium.org/2695523002/diff/40001/ui/display/win/screen_w...
ui/display/win/screen_win.h:105: // Returns the system's global scale factor. In
most cases you should prefer
On 2017/02/15 18:43:16, robliao wrote:
> Possible rephrasing. What do you think?
> 
> Returns the system's global scale factor ignoring --force-device-scale-factor.
> Only call this if you are working with Windows metrics global to the display.
> Otherwise you should call GetScaleFactorForHWND() to handle per-monitor scale
> factors.

Yeah yours is clearer. I edited it slightly.

[robliao, 2017-02-15 22:59:42]

lgtm + nit. Thanks!

https://codereview.chromium.org/2695523002/diff/60001/ui/display/win/screen_w...
File ui/display/win/screen_win.h (right):

https://codereview.chromium.org/2695523002/diff/60001/ui/display/win/screen_w...
ui/display/win/screen_win.h:109: // you are on.
Nit: correct scale factor for the target monitor.
Ending sentences with a preposition :-D.

[Bret, 2017-02-15 23:13:46]

https://codereview.chromium.org/2695523002/diff/60001/ui/display/win/screen_w...
File ui/display/win/screen_win.h (right):

https://codereview.chromium.org/2695523002/diff/60001/ui/display/win/screen_w...
ui/display/win/screen_win.h:109: // you are on.
On 2017/02/15 22:59:41, robliao wrote:
> Nit: correct scale factor for the target monitor.
> Ending sentences with a preposition :-D.

Okay, Bishop Lowth. I edited it anyway since target is more generic than what
monitor you're drawing to.

[Bret, 2017-02-15 23:14:04]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-02-15 23:14:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org,
robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/2695523002/#ps80001
(title: "comment tweak")

[commit-bot: I haz the power, 2017-02-15 23:15:38]

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

[robliao, 2017-02-15 23:17:46]

Still lgtm. Thanks!

[commit-bot: I haz the power, 2017-02-16 00:08:38]

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

[commit-bot: I haz the power, 2017-02-16 00:08:39]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Bret, 2017-02-16 02:34:17]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-02-16 02:34:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, jshin@chromium.org
Link to the patchset: https://codereview.chromium.org/2695523002/#ps100001
(title: "fix test link")

[commit-bot: I haz the power, 2017-02-16 02:35:49]

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, 2017-02-16 04:20:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487212457914370,
"parent_rev": "be2c7115703a6610b25128809eb388ac28d07285", "commit_rev":
"6cf3aecafe0e4790a0aa0dd8f44602acce121a2e"}

[commit-bot: I haz the power, 2017-02-16 04:22:29]

Description was changed from

==========
Fix font sizes being incorrect when using --force-device-scale-factor.

Normalizes font sizes received from Windows based on the unforced
device scale factor, so that when they're scaled up later they end up
the right sized for the forced scale.

BUG=675933
==========

to

==========
Fix font sizes being incorrect when using --force-device-scale-factor.

Normalizes font sizes received from Windows based on the unforced
device scale factor, so that when they're scaled up later they end up
the right sized for the forced scale.

BUG=675933

Review-Url: https://codereview.chromium.org/2695523002
Cr-Commit-Position: refs/heads/master@{#450869}
Committed:
https://chromium.googlesource.com/chromium/src/+/6cf3aecafe0e4790a0aa0dd8f446...
==========

[commit-bot: I haz the power, 2017-02-16 04:22:30]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6cf3aecafe0e4790a0aa0dd8f446...