Enable hidpi on Linux, refactor a bit on Windows to share

Enable hidpi on Linux, refactor a bit on Windows to share

Mostly seems to work on Linux now.

I'm sure there's a variety of to-be-uncovered problems (the use of
gfx::Point all over that might be DIP or might be pixel sucks), but I
think it's better-enough that it's useful to get landed.

I mostly only tested at 2x (as in, not 1.343x that I'm sure people
will complain about). The assets that are ripped out of gtk are still
1x and then scaled up. I wasn't sure how to get a higher res version,
but that can probably be a follow up anyway.

The Windows code is refactored mostly to share DIPToScreenXYZ, but no
intended change in behaviour on Windows. There's a lot of files
touched for this, but they're all tiny changes.

The main changes are in:

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
ui/gfx/pango_util.cc
ui/gfx/dpi.cc

(I will probably also exclude the switch-flip in build/common.gypi
when landing and do it separately so that the lots-of-small-changes
can hopefully avoid revert-reland cycles)

R=erg@chromium.org, oshima@chromium.org, ananta@chromium.org
BUG=143619

[scottmg, 2014-10-15 23:48:28]

Patchset #3 (id:40001) has been deleted

[Elliot Glaysher, 2014-10-16 00:27:39]

erg@chromium.org changed reviewers:
+ erg@chromium.org

[Elliot Glaysher, 2014-10-16 00:27:40]

https://codereview.chromium.org/659883002/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/659883002/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:536: return
ScreenToDIPRect(gfx::Rect(x, y, width, height));
So bounds in screen are also dips?

[scottmg, 2014-10-16 22:54:39]

Patchset #4 (id:80001) has been deleted

[scottmg, 2014-10-16 22:57:39]

scottmg@chromium.org changed reviewers:
+ ananta@chromium.org, oshima@chromium.org

[scottmg, 2014-10-16 23:06:38]

Patchset #5 (id:120001) has been deleted

[scottmg, 2014-10-16 23:09:02]

Patchset #5 (id:120001) has been deleted

[scottmg, 2014-10-17 00:09:12]

I'd appreciate it if all three of you could take a look.

(it looks big, but most of it is trivial changes for moving ui/gfx/win/dpi to
ui/gfx/dpi.)

https://codereview.chromium.org/659883002/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/659883002/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:536: return
ScreenToDIPRect(gfx::Rect(x, y, width, height));
On 2014/10/16 00:27:40, Elliot Glaysher wrote:
> So bounds in screen are also dips?

Yeah, that's confusing, but that one's Screen/Client not Screen/DIP. (I think. I
certainly could be totally wrong.)

https://codereview.chromium.org/659883002/diff/160001/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

https://codereview.chromium.org/659883002/diff/160001/ui/gfx/pango_util.cc#ne...
ui/gfx/pango_util.cc:144:
pango_cairo_context_set_resolution(pango_layout_get_context(layout), 96.0);
This seems odd, but seems to be what's required as it's scaled elsewhere on
desktop linux once the global dpi is modified. I'm not sure if this will be
problematic for cros though? (similarly below)

[ananta, 2014-10-17 00:49:07]

https://codereview.chromium.org/659883002/diff/180001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/659883002/diff/180001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:623:
supported_scale_factors.push_back(SCALE_FACTOR_125P);
Why do we need to push the fractional scale factors for linux?. Can't it work on
the same lines as windows?

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc#ne...
ui/gfx/pango_util.cc:144:
pango_cairo_context_set_resolution(pango_layout_get_context(layout), 96.0);
Is hardcoding to 96 here and below the right thing to do?. GetPangoDPI() would
also return the scale from the delegate.

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc#ne...
ui/gfx/pango_util.cc:187: return static_cast<int>((96.0 / 72.0) *
ditto

[scottmg, 2014-10-17 23:18:42]

https://codereview.chromium.org/659883002/diff/180001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/659883002/diff/180001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:623:
supported_scale_factors.push_back(SCALE_FACTOR_125P);
On 2014/10/17 00:49:06, ananta wrote:
> Why do we need to push the fractional scale factors for linux?. Can't it work
on
> the same lines as windows?

Done.

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc#ne...
ui/gfx/pango_util.cc:144:
pango_cairo_context_set_resolution(pango_layout_get_context(layout), 96.0);
On 2014/10/17 00:49:06, ananta wrote:
> Is hardcoding to 96 here and below the right thing to do?. GetPangoDPI() would
> also return the scale from the delegate.

I'm not sure. The behaviour I'm seeing without making this 96 is that the fonts
end up scaled twice. i.e. when the desktop is 2x, the fonts are 4x.

AFAICT, this is because the device/display is 2x, so we don't also want to take
it into account here and scale the fonts a second time. (I'm a little confused
how it all fits together on Linux though.)

[oshima, 2014-10-20 21:12:00]

https://codereview.chromium.org/659883002/diff/270001/ui/gfx/dpi.cc
File ui/gfx/dpi.cc (right):

https://codereview.chromium.org/659883002/diff/270001/ui/gfx/dpi.cc#newcode64
ui/gfx/dpi.cc:64: DCHECK_NE(0.0f, scale);
what happens if the scale is between 0.0 and 1.0 ?

https://codereview.chromium.org/659883002/diff/270001/ui/gfx/dpi.cc#newcode248
ui/gfx/dpi.cc:248: #endif  // OS_WIN
consider moving win impl to dip_win.cc ?

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:270: 
remove extra new line

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:271: float
device_scale_factor = 1.0f;
isn't it better to initialize this once here?

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:301:
gfx::Size(output_info->mm_width, output_info->mm_height))) {
Since it's no longer using the xrandr data, you can eliminate this check.

https://codereview.chromium.org/659883002/diff/270001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (right):

https://codereview.chromium.org/659883002/diff/270001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:32: #include "ui/gfx/dpi.h"
is this used?

[scottmg, 2014-10-22 18:54:15]

Thanks

https://codereview.chromium.org/659883002/diff/270001/ui/gfx/dpi.cc
File ui/gfx/dpi.cc (right):

https://codereview.chromium.org/659883002/diff/270001/ui/gfx/dpi.cc#newcode64
ui/gfx/dpi.cc:64: DCHECK_NE(0.0f, scale);
On 2014/10/20 21:11:59, oshima wrote:
> what happens if the scale is between 0.0 and 1.0 ?

Assets and fonts seem to be rendered at 1.0, but some other adjustments are
scaled by the scale factor. Perhaps clamp to >= 1.0? I don't think that path
would ever have been tried on Windows.

https://codereview.chromium.org/659883002/diff/270001/ui/gfx/dpi.cc#newcode248
ui/gfx/dpi.cc:248: #endif  // OS_WIN
On 2014/10/20 21:11:59, oshima wrote:
> consider moving win impl to dip_win.cc ?

Done.

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:270: 
On 2014/10/20 21:11:59, oshima wrote:
> remove extra new line

Done.

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:271: float
device_scale_factor = 1.0f;
On 2014/10/20 21:11:59, oshima wrote:
> isn't it better to initialize this once here?

Done.

https://codereview.chromium.org/659883002/diff/270001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:301:
gfx::Size(output_info->mm_width, output_info->mm_height))) {
On 2014/10/20 21:11:59, oshima wrote:
> Since it's no longer using the xrandr data, you can eliminate this check.

Done.

https://codereview.chromium.org/659883002/diff/270001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (right):

https://codereview.chromium.org/659883002/diff/270001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:32: #include "ui/gfx/dpi.h"
On 2014/10/20 21:11:59, oshima wrote:
> is this used?

Done.

[oshima, 2014-10-22 19:48:07]

lgtm with a nit.

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc#ne...
ui/gfx/pango_util.cc:187: return static_cast<int>((96.0 / 72.0) *
On 2014/10/17 00:49:06, ananta wrote:
> ditto

can you define const for these values with description?

[scottmg, 2014-10-22 20:35:05]

Thanks

On 2014/10/22 19:48:07, oshima wrote:
> lgtm with a nit.
> 
> https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc
> File ui/gfx/pango_util.cc (right):
> 
>
https://codereview.chromium.org/659883002/diff/180001/ui/gfx/pango_util.cc#ne...
> ui/gfx/pango_util.cc:187: return static_cast<int>((96.0 / 72.0) *
> On 2014/10/17 00:49:06, ananta wrote:
> > ditto
> 
> can you define const for these values with description?

Done.

[scottmg, 2014-10-22 20:35:26]

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

[scottmg, 2014-10-22 20:35:51]

+sky, could you take a look + OWNERS?

[sky, 2014-10-22 20:45:15]

Are we sure we want to go this route? I'm of the opinion views should be pixel
based. I haven't had time to write up the documentation and plan out the work.

[scottmg, 2014-10-22 20:50:01]

On 2014/10/22 20:45:15, sky wrote:
> Are we sure we want to go this route? I'm of the opinion views should be pixel
> based. I haven't had time to write up the documentation and plan out the work.

I'm not sure, I think you're probably right because we still have various
snapping/alignment bugs on Windows.

Linux is unusable at the moment on hidpi though. I think having it the same as
Windows would be an improvement in the meantime.

[sky, 2014-10-22 20:54:21]

Fair enough. Let me look at the patch.

On Wed, Oct 22, 2014 at 1:50 PM,  <scottmg@chromium.org> wrote:
> On 2014/10/22 20:45:15, sky wrote:
>>
>> Are we sure we want to go this route? I'm of the opinion views should be
>> pixel
>> based. I haven't had time to write up the documentation and plan out the
>> work.
>
>
> I'm not sure, I think you're probably right because we still have various
> snapping/alignment bugs on Windows.
>
> Linux is unusable at the moment on hidpi though. I think having it the same
> as
> Windows would be an improvement in the meantime.
>
> https://codereview.chromium.org/659883002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-10-22 21:11:18]

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
File ui/gfx/dpi.h (right):

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h#newcode13
ui/gfx/dpi.h:13: namespace gfx {
I have always found this file confusing. It's a mix of functions used for
different purposes: testing, initialization and general purposes.

Can we clean it up, perhaps separating into files, so that the purpose is clear?
I'm hoping this file can remain general purpose and have dpi_init and
dpi_testing.

I also think it needs to be made clear when you want to use scale factor in this
file vs scale factor from Display.

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h#newcode30
ui/gfx/dpi.h:30: GFX_EXPORT bool IsInHighDPIMode();
Can this function be nuked and callers use  GetDPIScale() > 1.0?

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h#newcode54
ui/gfx/dpi.h:54: GFX_EXPORT bool IsHighDPIEnabled();
Can this be nuked too?

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc
File ui/gfx/dpi_linux.cc (right):

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc#new...
ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
This ifdef shouldn't be necessary, right?

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp#newcode160
ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
I suspect dpi_linux should not be present on chromeos.

[scottmg, 2014-10-22 23:56:18]

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
File ui/gfx/dpi.h (right):

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h#newcode13
ui/gfx/dpi.h:13: namespace gfx {
On 2014/10/22 21:11:18, sky wrote:
> I have always found this file confusing. It's a mix of functions used for
> different purposes: testing, initialization and general purposes.
> 
> Can we clean it up, perhaps separating into files, so that the purpose is
clear?
> I'm hoping this file can remain general purpose and have dpi_init and
> dpi_testing.
> 
> I also think it needs to be made clear when you want to use scale factor in
this
> file vs scale factor from Display.

Yes, I find it very confusing too, since it seems like it should just be one
float that determines the scale between points<->pixels.

I started trying to clean it up:
- delete IsInHighDPIMode()
- move ForceHighDPISupportForTesting to dpi_testing.h
- delete IsDeviceScaleFactorSet
- move EnableHighDPISupport to chrome/app
- delete IsHighDPIEnabled
- and mostly, we delete GetDeviceScaleFactor(), and have all callers switch to
GetDPI or GetDPIScale. 

I think it's too big to include in this CL though.

I don't really understand the difference between display and this file, but I
think Display is mostly a debug/testing thing. I think the dpi related things in
display should be merge into here.

Umm, yeah. So we need to clean this up.

It'd be easier to do all that after this CL to avoid having to rebase this CL on
the cleanup one, but if you prefer I can do the cleanup first.

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc
File ui/gfx/dpi_linux.cc (right):

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc#new...
ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
On 2014/10/22 21:11:18, sky wrote:
> This ifdef shouldn't be necessary, right?

Done.

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp#newcode160
ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
On 2014/10/22 21:11:18, sky wrote:
> I suspect dpi_linux should not be present on chromeos.

Done.

[oshima, 2014-10-23 00:50:30]

On 2014/10/22 23:56:18, scottmg wrote:
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
> File ui/gfx/dpi.h (right):
> 
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h#newcode13
> ui/gfx/dpi.h:13: namespace gfx {
> On 2014/10/22 21:11:18, sky wrote:
> > I have always found this file confusing. It's a mix of functions used for
> > different purposes: testing, initialization and general purposes.
> > 
> > Can we clean it up, perhaps separating into files, so that the purpose is
> clear?
> > I'm hoping this file can remain general purpose and have dpi_init and
> > dpi_testing.
> > 
> > I also think it needs to be made clear when you want to use scale factor in
> this
> > file vs scale factor from Display.
> 
> Yes, I find it very confusing too, since it seems like it should just be one
> float that determines the scale between points<->pixels.
> 
> I started trying to clean it up:
> - delete IsInHighDPIMode()
> - move ForceHighDPISupportForTesting to dpi_testing.h
> - delete IsDeviceScaleFactorSet
> - move EnableHighDPISupport to chrome/app
> - delete IsHighDPIEnabled
> - and mostly, we delete GetDeviceScaleFactor(), and have all callers switch to
> GetDPI or GetDPIScale. 
> 
> I think it's too big to include in this CL though.
> 
> I don't really understand the difference between display and this file, but I
> think Display is mostly a debug/testing thing. I think the dpi related things
in
> display should be merge into here.

Scale factor should belong to display in general as each display can have
different
scale factor. This is true on mac and chromeos at least. Scale factor happens to
be
the same on linux and current windows impl, although someone told me that newer
windows
(don't know which) can support different scale factors on each display.
I'm not expert on windows, so I may be completely wrong though.

> 
> Umm, yeah. So we need to clean this up.
> 
> It'd be easier to do all that after this CL to avoid having to rebase this CL
on
> the cleanup one, but if you prefer I can do the cleanup first.
> 
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc
> File ui/gfx/dpi_linux.cc (right):
> 
>
https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc#new...
> ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
> On 2014/10/22 21:11:18, sky wrote:
> > This ifdef shouldn't be necessary, right?
> 
> Done.
> 
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
> File ui/gfx/gfx.gyp (right):
> 
>
https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp#newcode160
> ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
> On 2014/10/22 21:11:18, sky wrote:
> > I suspect dpi_linux should not be present on chromeos.
> 
> Done.

[sky, 2014-10-23 04:22:51]

You seem to have started breaking this up. Thanks! And double thanks
for the cleanup!

On Wed, Oct 22, 2014 at 4:56 PM,  <scottmg@chromium.org> wrote:
>
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
> File ui/gfx/dpi.h (right):
>
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h#newcode13
> ui/gfx/dpi.h:13: namespace gfx {
> On 2014/10/22 21:11:18, sky wrote:
>>
>> I have always found this file confusing. It's a mix of functions used
>
> for
>>
>> different purposes: testing, initialization and general purposes.
>
>
>> Can we clean it up, perhaps separating into files, so that the purpose
>
> is clear?
>>
>> I'm hoping this file can remain general purpose and have dpi_init and
>> dpi_testing.
>
>
>> I also think it needs to be made clear when you want to use scale
>
> factor in this
>>
>> file vs scale factor from Display.
>
>
> Yes, I find it very confusing too, since it seems like it should just be
> one float that determines the scale between points<->pixels.
>
> I started trying to clean it up:
> - delete IsInHighDPIMode()
> - move ForceHighDPISupportForTesting to dpi_testing.h
> - delete IsDeviceScaleFactorSet
> - move EnableHighDPISupport to chrome/app
> - delete IsHighDPIEnabled
> - and mostly, we delete GetDeviceScaleFactor(), and have all callers
> switch to GetDPI or GetDPIScale.
>
> I think it's too big to include in this CL though.
>
> I don't really understand the difference between display and this file,
> but I think Display is mostly a debug/testing thing. I think the dpi
> related things in display should be merge into here.
>
> Umm, yeah. So we need to clean this up.
>
> It'd be easier to do all that after this CL to avoid having to rebase
> this CL on the cleanup one, but if you prefer I can do the cleanup
> first.
>
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc
> File ui/gfx/dpi_linux.cc (right):
>
>
https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc#new...
> ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
> On 2014/10/22 21:11:18, sky wrote:
>>
>> This ifdef shouldn't be necessary, right?
>
>
> Done.
>
> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
> File ui/gfx/gfx.gyp (right):
>
>
https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp#newcode160
> ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
> On 2014/10/22 21:11:18, sky wrote:
>>
>> I suspect dpi_linux should not be present on chromeos.
>
>
> Done.
>
> https://codereview.chromium.org/659883002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2014-10-23 04:29:42]

On Wed, Oct 22, 2014 at 5:50 PM, <oshima@chromium.org> wrote:

> On 2014/10/22 23:56:18, scottmg wrote:
>
>> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
>> File ui/gfx/dpi.h (right):
>>
>
>  https://codereview.chromium.org/659883002/diff/330001/ui/
>> gfx/dpi.h#newcode13
>> ui/gfx/dpi.h:13: namespace gfx {
>> On 2014/10/22 21:11:18, sky wrote:
>> > I have always found this file confusing. It's a mix of functions used
>> for
>> > different purposes: testing, initialization and general purposes.
>> >
>> > Can we clean it up, perhaps separating into files, so that the purpose
>> is
>> clear?
>> > I'm hoping this file can remain general purpose and have dpi_init and
>> > dpi_testing.
>> >
>> > I also think it needs to be made clear when you want to use scale
>> factor in
>> this
>> > file vs scale factor from Display.
>>
>
>  Yes, I find it very confusing too, since it seems like it should just be
>> one
>> float that determines the scale between points<->pixels.
>>
>
>  I started trying to clean it up:
>> - delete IsInHighDPIMode()
>> - move ForceHighDPISupportForTesting to dpi_testing.h
>> - delete IsDeviceScaleFactorSet
>> - move EnableHighDPISupport to chrome/app
>> - delete IsHighDPIEnabled
>> - and mostly, we delete GetDeviceScaleFactor(), and have all callers
>> switch to
>> GetDPI or GetDPIScale.
>>
>
>  I think it's too big to include in this CL though.
>>
>
>  I don't really understand the difference between display and this file,
>> but I
>> think Display is mostly a debug/testing thing. I think the dpi related
>> things
>>
> in
>
>> display should be merge into here.
>>
>
> Scale factor should belong to display in general as each display can have
> different
> scale factor. This is true on mac and chromeos at least. Scale factor
> happens to
> be
> the same on linux and current windows impl, although someone told me that
> newer
> windows
> (don't know which) can support different scale factors on each display.
> I'm not expert on windows, so I may be completely wrong though.


That makes sense. Does that work on CrOS/Mac at moment? (i.e. all
conversions between points/pixel have a handle to a Display somehow?)

I was just thinking of the moving the override code where we force
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/display.cc&...
into a shared location so that there's only once place where we decide the
scale. Does that seem ok?

Per-monitor DPI in Win 8.1+, but I don't think we've looked into supporting
it yet.


>
>
>
>  Umm, yeah. So we need to clean this up.
>>
>
>  It'd be easier to do all that after this CL to avoid having to rebase
>> this CL
>>
> on
>
>> the cleanup one, but if you prefer I can do the cleanup first.
>>
>
>  https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc
>> File ui/gfx/dpi_linux.cc (right):
>>
>
>
> https://codereview.chromium.org/659883002/diff/330001/ui/
> gfx/dpi_linux.cc#newcode26
>
>> ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
>> On 2014/10/22 21:11:18, sky wrote:
>> > This ifdef shouldn't be necessary, right?
>>
>
>  Done.
>>
>
>  https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
>> File ui/gfx/gfx.gyp (right):
>>
>
>
> https://codereview.chromium.org/659883002/diff/330001/ui/
> gfx/gfx.gyp#newcode160
>
>> ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
>> On 2014/10/22 21:11:18, sky wrote:
>> > I suspect dpi_linux should not be present on chromeos.
>>
>
>  Done.
>>
>
>
>
> https://codereview.chromium.org/659883002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-10-23 04:54:31]

On 2014/10/23 04:29:42, scottmg wrote:
> On Wed, Oct 22, 2014 at 5:50 PM, <mailto:oshima@chromium.org> wrote:
> 
> > On 2014/10/22 23:56:18, scottmg wrote:
> >
> >> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
> >> File ui/gfx/dpi.h (right):
> >>
> >
> >  https://codereview.chromium.org/659883002/diff/330001/ui/
> >> gfx/dpi.h#newcode13
> >> ui/gfx/dpi.h:13: namespace gfx {
> >> On 2014/10/22 21:11:18, sky wrote:
> >> > I have always found this file confusing. It's a mix of functions used
> >> for
> >> > different purposes: testing, initialization and general purposes.
> >> >
> >> > Can we clean it up, perhaps separating into files, so that the purpose
> >> is
> >> clear?
> >> > I'm hoping this file can remain general purpose and have dpi_init and
> >> > dpi_testing.
> >> >
> >> > I also think it needs to be made clear when you want to use scale
> >> factor in
> >> this
> >> > file vs scale factor from Display.
> >>
> >
> >  Yes, I find it very confusing too, since it seems like it should just be
> >> one
> >> float that determines the scale between points<->pixels.
> >>
> >
> >  I started trying to clean it up:
> >> - delete IsInHighDPIMode()
> >> - move ForceHighDPISupportForTesting to dpi_testing.h
> >> - delete IsDeviceScaleFactorSet
> >> - move EnableHighDPISupport to chrome/app
> >> - delete IsHighDPIEnabled
> >> - and mostly, we delete GetDeviceScaleFactor(), and have all callers
> >> switch to
> >> GetDPI or GetDPIScale.
> >>
> >
> >  I think it's too big to include in this CL though.
> >>
> >
> >  I don't really understand the difference between display and this file,
> >> but I
> >> think Display is mostly a debug/testing thing. I think the dpi related
> >> things
> >>
> > in
> >
> >> display should be merge into here.
> >>
> >
> > Scale factor should belong to display in general as each display can have
> > different
> > scale factor. This is true on mac and chromeos at least. Scale factor
> > happens to
> > be
> > the same on linux and current windows impl, although someone told me that
> > newer
> > windows
> > (don't know which) can support different scale factors on each display.
> > I'm not expert on windows, so I may be completely wrong though.
> 
> 
> That makes sense. Does that work on CrOS/Mac at moment? (i.e. all
> conversions between points/pixel have a handle to a Display somehow?)

Not all code directly uses Display. For example, layer (in compositor) has
device scale factor
as cache, and gets updated when the associated display changes, but display is
always the source of
the scale factor information.

> I was just thinking of the moving the override code where we force
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/display.cc&...
> into a shared location so that there's only once place where we decide the
> scale. Does that seem ok?

Sorry, I didn't understand. Can you elaborate?

> 
> Per-monitor DPI in Win 8.1+, but I don't think we've looked into supporting
> it yet.
> 
> 
> >
> >
> >
> >  Umm, yeah. So we need to clean this up.
> >>
> >
> >  It'd be easier to do all that after this CL to avoid having to rebase
> >> this CL
> >>
> > on
> >
> >> the cleanup one, but if you prefer I can do the cleanup first.
> >>
> >
> >  https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi_linux.cc
> >> File ui/gfx/dpi_linux.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/659883002/diff/330001/ui/
> > gfx/dpi_linux.cc#newcode26
> >
> >> ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
> >> On 2014/10/22 21:11:18, sky wrote:
> >> > This ifdef shouldn't be necessary, right?
> >>
> >
> >  Done.
> >>
> >
> >  https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
> >> File ui/gfx/gfx.gyp (right):
> >>
> >
> >
> > https://codereview.chromium.org/659883002/diff/330001/ui/
> > gfx/gfx.gyp#newcode160
> >
> >> ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
> >> On 2014/10/22 21:11:18, sky wrote:
> >> > I suspect dpi_linux should not be present on chromeos.
> >>
> >
> >  Done.
> >>
> >
> >
> >
> > https://codereview.chromium.org/659883002/
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2014-10-23 23:59:44]

On Wed, Oct 22, 2014 at 9:54 PM, <oshima@chromium.org> wrote:

> On 2014/10/23 04:29:42, scottmg wrote:
>
>  On Wed, Oct 22, 2014 at 5:50 PM, <mailto:oshima@chromium.org> wrote:
>>
>
>  > On 2014/10/22 23:56:18, scottmg wrote:
>> >
>> >> https://codereview.chromium.org/659883002/diff/330001/ui/gfx/dpi.h
>> >> File ui/gfx/dpi.h (right):
>> >>
>> >
>> >  https://codereview.chromium.org/659883002/diff/330001/ui/
>> >> gfx/dpi.h#newcode13
>> >> ui/gfx/dpi.h:13: namespace gfx {
>> >> On 2014/10/22 21:11:18, sky wrote:
>> >> > I have always found this file confusing. It's a mix of functions used
>> >> for
>> >> > different purposes: testing, initialization and general purposes.
>> >> >
>> >> > Can we clean it up, perhaps separating into files, so that the
>> purpose
>> >> is
>> >> clear?
>> >> > I'm hoping this file can remain general purpose and have dpi_init and
>> >> > dpi_testing.
>> >> >
>> >> > I also think it needs to be made clear when you want to use scale
>> >> factor in
>> >> this
>> >> > file vs scale factor from Display.
>> >>
>> >
>> >  Yes, I find it very confusing too, since it seems like it should just
>> be
>> >> one
>> >> float that determines the scale between points<->pixels.
>> >>
>> >
>> >  I started trying to clean it up:
>> >> - delete IsInHighDPIMode()
>> >> - move ForceHighDPISupportForTesting to dpi_testing.h
>> >> - delete IsDeviceScaleFactorSet
>> >> - move EnableHighDPISupport to chrome/app
>> >> - delete IsHighDPIEnabled
>> >> - and mostly, we delete GetDeviceScaleFactor(), and have all callers
>> >> switch to
>> >> GetDPI or GetDPIScale.
>> >>
>> >
>> >  I think it's too big to include in this CL though.
>> >>
>> >
>> >  I don't really understand the difference between display and this file,
>> >> but I
>> >> think Display is mostly a debug/testing thing. I think the dpi related
>> >> things
>> >>
>> > in
>> >
>> >> display should be merge into here.
>> >>
>> >
>> > Scale factor should belong to display in general as each display can
>> have
>> > different
>> > scale factor. This is true on mac and chromeos at least. Scale factor
>> > happens to
>> > be
>> > the same on linux and current windows impl, although someone told me
>> that
>> > newer
>> > windows
>> > (don't know which) can support different scale factors on each display.
>> > I'm not expert on windows, so I may be completely wrong though.
>>
>
>
>  That makes sense. Does that work on CrOS/Mac at moment? (i.e. all
>> conversions between points/pixel have a handle to a Display somehow?)
>>
>
> Not all code directly uses Display. For example, layer (in compositor) has
> device scale factor
> as cache, and gets updated when the associated display changes, but
> display is
> always the source of
> the scale factor information.
>
>  I was just thinking of the moving the override code where we force
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/ui/gfx/display.cc&rcl=1413930987&l=58
>
>> into a shared location so that there's only once place where we decide the
>> scale. Does that seem ok?
>>
>
> Sorry, I didn't understand. Can you elaborate?
>
>
I just meant moving the parsing of --force-device-scale-factor into dpi.cc
mostly so that there was one place that set a global overridden scale.

But now that I've tidied it up, it seems clear that Windows is wrong now
(it was probably "right" before win81) in that it uses a global scale
rather than a per-display one. I filed
https://code.google.com/p/chromium/issues/detail?id=426656 . Doesn't
immediately help Linux unfortunately. :(


>
>
>  Per-monitor DPI in Win 8.1+, but I don't think we've looked into
>> supporting
>> it yet.
>>
>
>
>  >
>> >
>> >
>> >  Umm, yeah. So we need to clean this up.
>> >>
>> >
>> >  It'd be easier to do all that after this CL to avoid having to rebase
>> >> this CL
>> >>
>> > on
>> >
>> >> the cleanup one, but if you prefer I can do the cleanup first.
>> >>
>> >
>> >  https://codereview.chromium.org/659883002/diff/330001/ui/
>> gfx/dpi_linux.cc
>> >> File ui/gfx/dpi_linux.cc (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/659883002/diff/330001/ui/
>> > gfx/dpi_linux.cc#newcode26
>> >
>> >> ui/gfx/dpi_linux.cc:26: #if defined(OS_LINUX)
>> >> On 2014/10/22 21:11:18, sky wrote:
>> >> > This ifdef shouldn't be necessary, right?
>> >>
>> >
>> >  Done.
>> >>
>> >
>> >  https://codereview.chromium.org/659883002/diff/330001/ui/gfx/gfx.gyp
>> >> File ui/gfx/gfx.gyp (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/659883002/diff/330001/ui/
>> > gfx/gfx.gyp#newcode160
>> >
>> >> ui/gfx/gfx.gyp:160: 'dpi_linux.cc',
>> >> On 2014/10/22 21:11:18, sky wrote:
>> >> > I suspect dpi_linux should not be present on chromeos.
>> >>
>> >
>> >  Done.
>> >>
>> >
>> >
>> >
>> > https://codereview.chromium.org/659883002/
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/659883002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.