Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now. All callers of the old functions
have been directed to use the new functions.

The visual behavior of Chrome should not change with this CL.

BUG=426656

[robliao, 2015-10-28 17:51:08]

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

[robliao, 2015-10-28 17:51:08]

Here's a first shot and the first part of handling multiple monitor DPI.

[robliao, 2015-10-28 17:53:17]

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

[commit-bot: I haz the power, 2015-10-28 17:54:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1426933002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1426933002/1

[robliao, 2015-10-28 17:55:09]

Description was changed from

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display on gfx::Screen, ScreenWin
seems like an appropriate home for these now.

An additional change is that to make the conversion between screen pixels and
device-independent pixels more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

to

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now.

An additional change is that to make the conversion between screen pixels and
device-independent pixels more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

[robliao, 2015-10-28 18:12:59]

Description was changed from

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now.

An additional change is that to make the conversion between screen pixels and
device-independent pixels more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

to

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now.

An additional change is that to make the conversion between screen pixels and
device-independent points more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

[scottmg, 2015-10-28 18:48:20]

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

[scottmg, 2015-10-28 18:48:21]

Great stuff!

I'm a little concerned/sad that we still diverge from CrOS. Is this necessary
because of the fixed origins? I need to read that code over again to try to
understand why we have to do that.

(I don't own much/anything here, so adding sky and oshima for OWNERS review and
for more experience with dpi)

https://codereview.chromium.org/1426933002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1426933002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:314: point =
gfx::ScreenWin::DIPToScreenPoint(point);
Will this need an HWND in the future?

https://codereview.chromium.org/1426933002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1426933002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:1265: host ?
host->GetAcceleratedWidget() : nullptr, bounds);
What do we do when nullptr?

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcode14
ui/gfx/screen_win.cc:14: #include "base/win/scoped_hdc.h"
I don't think this got used.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
ui/gfx/screen_win.cc:113: float device_scale_factor =
display.device_scale_factor();
DCHECK that device_scale_factor >= 1 (CrOS has some 0.75 devices)

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
ui/gfx/screen_win.cc:138: ScreenToDIPSize(hwnd, pixel_bounds.size()));
indent

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
ui/gfx/screen_win.cc:151: DIPToScreenSize(hwnd, dip_bounds.size()));
indent

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h
File ui/gfx/screen_win.h (right):

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h#newcode40
ui/gfx/screen_win.h:40: // Converts a client DIP point relative to |hwnd| to a
client point.
Is this point or pixel?

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h#newcode42
ui/gfx/screen_win.h:42: static Point DIPToClientPoint(HWND hwnd, const Point&
dip_pount);
"pount"

[commit-bot: I haz the power, 2015-10-28 19:39:52]

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

[commit-bot: I haz the power, 2015-10-28 19:39:53]

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

[robliao, 2015-10-29 00:30:24]

Thanks for the comments!

> I'm a little concerned/sad that we still diverge from CrOS. Is this necessary
because of the fixed origins? I need to read that code over again to try to
understand why we have to do that.

Yeah, I'm sad too, but it seems to allow us to avoid a rectangle counting
computation.

With movable DIP origins, we would need to compute and cache the location
mapping from physical to DIP origins.
This computation would generally depend on all rects to the left and top of the
rectangle in question, but becomes
interesting with this arrangement:
|--|
|1x|--|
|--|2x|
|2x|--|
|--|
1x = 1x Device Scale Factor
2x = 2x Device Scale Factor

There are a few ways to lay out the above physically, one including maintaining
the rectangular property of the right
rectangle and another one that does not. I don't have a third monitor set up for
Windows 10 so I don't know what it does ;-).

Assuming windows does this physically...

|--| |----|
|1x| |2x  |
|----|    |
|2x  |----|
|    |
|----|

We have an interesting mapping of physical to logical pixels. One could also
imagine moving the right rectangle down.
In either case, the origin Physical->DIP mapping will depend on the physical
layout.

Either way, fixed origins do simplify the scheme. If there is a strong desire to
have movable origins, I can investigate that more.

https://codereview.chromium.org/1426933002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1426933002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:314: point =
gfx::ScreenWin::DIPToScreenPoint(point);
On 2015/10/28 18:48:21, scottmg wrote:
> Will this need an HWND in the future?

At the moment, I don't think so. Callers that have points sensitive to the HWND
context should probably be using the Client point converters.

The common case seems to be HWND free screen points. If there is a compelling
case otherwise, then I could see selected uses take an HWND.

An interesting case in the multiple DPI world is if an HWND spans multiple
monitors and the app asks for a screen point that resides outside of the nearest
monitor.

In certain cases (like GetCursorPos) for virtualized applications, Windows will
actually provide a point referenced to the nearest monitor in the HWND's
context. This means there is some overlap in the physical screen pixel space. 

Incidentally, we are broken today with Windows default virtualization. When the
Chrome window is associated to one monitor and the Chrome menu button is drawn
on another monitor of a different DPI, the menu will render on the Chrome
monitor and not underneath the button. Of course this presents interesting
questions to the Window manager as it needs to render the menu in a different
DPI as well, but it looks like it does not handle this either.

https://codereview.chromium.org/1426933002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1426933002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:1265: host ?
host->GetAcceleratedWidget() : nullptr, bounds);
On 2015/10/28 18:48:21, scottmg wrote:
> What do we do when nullptr?

This will tell ScreenToDIPRect to use the nearest monitor to the current rect
instead of the hwnd of the nearest monitor. Since we don't have an HWND, we
don't have a context yet.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcode14
ui/gfx/screen_win.cc:14: #include "base/win/scoped_hdc.h"
On 2015/10/28 18:48:21, scottmg wrote:
> I don't think this got used.

Good catch. I had a plan that used HDC's before but decided not to go with it.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
ui/gfx/screen_win.cc:113: float device_scale_factor =
display.device_scale_factor();
On 2015/10/28 18:48:21, scottmg wrote:
> DCHECK that device_scale_factor >= 1 (CrOS has some 0.75 devices)

Added the DCHECK where we create the display in GetDisplay.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
ui/gfx/screen_win.cc:138: ScreenToDIPSize(hwnd, pixel_bounds.size()));
On 2015/10/28 18:48:21, scottmg wrote:
> indent

Done.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
ui/gfx/screen_win.cc:151: DIPToScreenSize(hwnd, dip_bounds.size()));
On 2015/10/28 18:48:21, scottmg wrote:
> indent

Done.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h
File ui/gfx/screen_win.h (right):

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h#newcode40
ui/gfx/screen_win.h:40: // Converts a client DIP point relative to |hwnd| to a
client point.
On 2015/10/28 18:48:21, scottmg wrote:
> Is this point or pixel?

pixel point for consistency with the above. As part of a future refactor, we
should probably agree on a terminology for physical screen pixels vs DIP pixels.
We're currently conflating Screen to represent the dimension format as well as
the coordinate system.

https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h#newcode42
ui/gfx/screen_win.h:42: static Point DIPToClientPoint(HWND hwnd, const Point&
dip_pount);
On 2015/10/28 18:48:21, scottmg wrote:
> "pount"

Done.

[sky, 2015-10-29 15:51:51]

https://codereview.chromium.org/1426933002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/1426933002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:323:
tabstrip_bounds = gfx::ScreenWin::DIPToClientRect(GetHWND(),
Is there a reason for changing the name of these functions? Isn't screen rect
more consistent with what the function returns and naming else where?

I also think the scale factor should be available at the widget level so that
you don't have to dig out the hwnd.

https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h
File ui/gfx/screen_win.h (right):

https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:11: #include "ui/gfx/geometry/point.h"
you should be able to forward declare the types.

https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:38: static Point ClientToDIPPoint(HWND hwnd, const Point&
client_point);
What is a client point?

https://codereview.chromium.org/1426933002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1426933002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:702:
GetDisplayNearestWindow(root).device_scale_factor();
Why can't this code be made to work consistently?

[oshima, 2015-10-29 19:01:52]

On 2015/10/29 00:30:24, robliao (OOO through Sept. 2) wrote:
> Thanks for the comments!
> 
> > I'm a little concerned/sad that we still diverge from CrOS. Is this
necessary
> because of the fixed origins? I need to read that code over again to try to
> understand why we have to do that.
> 
> Yeah, I'm sad too, but it seems to allow us to avoid a rectangle counting
> computation.
> 
> With movable DIP origins, we would need to compute and cache the location
> mapping from physical to DIP origins.
> This computation would generally depend on all rects to the left and top of
the
> rectangle in question, but becomes
> interesting with this arrangement:
> |--|
> |1x|--|
> |--|2x|
> |2x|--|
> |--|
> 1x = 1x Device Scale Factor
> 2x = 2x Device Scale Factor
> 
> There are a few ways to lay out the above physically, one including
maintaining
> the rectangular property of the right
> rectangle and another one that does not. I don't have a third monitor set up
for
> Windows 10 so I don't know what it does ;-).
> 
> Assuming windows does this physically...
> 
> |--| |----|
> |1x| |2x  |
> |----|    |
> |2x  |----|
> |    |
> |----|
> 
> We have an interesting mapping of physical to logical pixels. One could also
> imagine moving the right rectangle down.
> In either case, the origin Physical->DIP mapping will depend on the physical
> layout.

If you have displays coorinates in DIP, you can convert DIP to physical coords
by
1) Find display the DIP coordinate is in
2) Then convert DIP to physical in that display.

But there may be scenario I'm not aware of, and I won't oppose if this is the
right approach.
Maybe we can have a quick offline chat?

> 
> Either way, fixed origins do simplify the scheme. If there is a strong desire
to
> have movable origins, I can investigate that more.
> 
>
https://codereview.chromium.org/1426933002/diff/1/chrome/browser/ui/views/tab...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
> 
>
https://codereview.chromium.org/1426933002/diff/1/chrome/browser/ui/views/tab...
> chrome/browser/ui/views/tabs/tab_strip.cc:314: point =
> gfx::ScreenWin::DIPToScreenPoint(point);
> On 2015/10/28 18:48:21, scottmg wrote:
> > Will this need an HWND in the future?
> 
> At the moment, I don't think so. Callers that have points sensitive to the
HWND
> context should probably be using the Client point converters.
> 
> The common case seems to be HWND free screen points. If there is a compelling
> case otherwise, then I could see selected uses take an HWND.
> 
> An interesting case in the multiple DPI world is if an HWND spans multiple
> monitors and the app asks for a screen point that resides outside of the
nearest
> monitor.
> 
> In certain cases (like GetCursorPos) for virtualized applications, Windows
will
> actually provide a point referenced to the nearest monitor in the HWND's
> context. This means there is some overlap in the physical screen pixel space. 
> 
> Incidentally, we are broken today with Windows default virtualization. When
the
> Chrome window is associated to one monitor and the Chrome menu button is drawn
> on another monitor of a different DPI, the menu will render on the Chrome
> monitor and not underneath the button. Of course this presents interesting
> questions to the Window manager as it needs to render the menu in a different
> DPI as well, but it looks like it does not handle this either.
> 
>
https://codereview.chromium.org/1426933002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/1426933002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1265: host ?
> host->GetAcceleratedWidget() : nullptr, bounds);
> On 2015/10/28 18:48:21, scottmg wrote:
> > What do we do when nullptr?
> 
> This will tell ScreenToDIPRect to use the nearest monitor to the current rect
> instead of the hwnd of the nearest monitor. Since we don't have an HWND, we
> don't have a context yet.
> 
> https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc
> File ui/gfx/screen_win.cc (right):
> 
>
https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcode14
> ui/gfx/screen_win.cc:14: #include "base/win/scoped_hdc.h"
> On 2015/10/28 18:48:21, scottmg wrote:
> > I don't think this got used.
> 
> Good catch. I had a plan that used HDC's before but decided not to go with it.
> 
>
https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
> ui/gfx/screen_win.cc:113: float device_scale_factor =
> display.device_scale_factor();
> On 2015/10/28 18:48:21, scottmg wrote:
> > DCHECK that device_scale_factor >= 1 (CrOS has some 0.75 devices)
> 
> Added the DCHECK where we create the display in GetDisplay.
> 
>
https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
> ui/gfx/screen_win.cc:138: ScreenToDIPSize(hwnd, pixel_bounds.size()));
> On 2015/10/28 18:48:21, scottmg wrote:
> > indent
> 
> Done.
> 
>
https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.cc#newcod...
> ui/gfx/screen_win.cc:151: DIPToScreenSize(hwnd, dip_bounds.size()));
> On 2015/10/28 18:48:21, scottmg wrote:
> > indent
> 
> Done.
> 
> https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h
> File ui/gfx/screen_win.h (right):
> 
>
https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h#newcode40
> ui/gfx/screen_win.h:40: // Converts a client DIP point relative to |hwnd| to a
> client point.
> On 2015/10/28 18:48:21, scottmg wrote:
> > Is this point or pixel?
> 
> pixel point for consistency with the above. As part of a future refactor, we
> should probably agree on a terminology for physical screen pixels vs DIP
pixels.
> We're currently conflating Screen to represent the dimension format as well as
> the coordinate system.
> 
>
https://codereview.chromium.org/1426933002/diff/1/ui/gfx/screen_win.h#newcode42
> ui/gfx/screen_win.h:42: static Point DIPToClientPoint(HWND hwnd, const Point&
> dip_pount);
> On 2015/10/28 18:48:21, scottmg wrote:
> > "pount"
> 
> Done.

[robliao, 2015-11-07 00:56:40]

After a busy week, I finally have time to get to this review! Thanks for the
comments.

oshima> Maybe we can have a quick offline chat?
How does next Monday work for you?

https://codereview.chromium.org/1426933002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/1426933002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:323:
tabstrip_bounds = gfx::ScreenWin::DIPToClientRect(GetHWND(),
On 2015/10/29 15:51:50, sky wrote:
> Is there a reason for changing the name of these functions? Isn't screen rect
> more consistent with what the function returns and naming else where?
> 
> I also think the scale factor should be available at the widget level so that
> you don't have to dig out the hwnd.

When I audited this function, I found the calculation was not performed relative
to the screen coordinate system but rather the HWND's or client's coordinate
system, hence the change. This is necessary since scaling a client means we
don't have to worry about screen origins. For rects calculated with respect to
screen origins, a transformation is possible especially if an HWND spans
multiple monitors since this implementation anchors the screen origins to handle
the multiple DPIs.

The HWND seemed to be a good lowest common denominator everyone had. My hand was
forced by TooltipWin which reasons over HWNDs and has no access to views or
widgets, so HWNDs are used for commonality.

https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h
File ui/gfx/screen_win.h (right):

https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:11: #include "ui/gfx/geometry/point.h"
On 2015/10/29 15:51:51, sky wrote:
> you should be able to forward declare the types.

Indeed! Done.

https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:38: static Point ClientToDIPPoint(HWND hwnd, const Point&
client_point);
On 2015/10/29 15:51:51, sky wrote:
> What is a client point?

It's generally a point in an HWND's coordinate system. I used terminology from
Windows
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms633503(v=vs.85).aspx),
but if there is a more Chromium idiomatic term, I'm happy to update this.

https://codereview.chromium.org/1426933002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1426933002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:702:
GetDisplayNearestWindow(root).device_scale_factor();
On 2015/10/29 15:51:51, sky wrote:
> Why can't this code be made to work consistently?

This came down to the fixed origin coordinate between physical and DIP scales.
This implementation performs fixed origin scaling. The code here presumes
movable origin scaling. I plan on talking to oshima about this next Monday if
you're interested.

[oshima, 2015-11-07 01:45:36]

On Fri, Nov 6, 2015 at 4:56 PM, <robliao@chromium.org> wrote:

> After a busy week, I finally have time to get to this review! Thanks for
> the
> comments.
>
> oshima> Maybe we can have a quick offline chat?
> How does next Monday work for you?


Sure, just book it on my calendar.


>
>
https://codereview.chromium.org/1426933002/diff/20001/chrome/browser/ui/views...
> File
> chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
> (right):
>
>
>
https://codereview.chromium.org/1426933002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:323:
> tabstrip_bounds = gfx::ScreenWin::DIPToClientRect(GetHWND(),
> On 2015/10/29 15:51:50, sky wrote:
>
>> Is there a reason for changing the name of these functions? Isn't
>>
> screen rect
>
>> more consistent with what the function returns and naming else where?
>>
>
> I also think the scale factor should be available at the widget level
>>
> so that
>
>> you don't have to dig out the hwnd.
>>
>
> When I audited this function, I found the calculation was not performed
> relative to the screen coordinate system but rather the HWND's or
> client's coordinate system, hence the change. This is necessary since
> scaling a client means we don't have to worry about screen origins. For
> rects calculated with respect to screen origins, a transformation is
> possible especially if an HWND spans multiple monitors since this
> implementation anchors the screen origins to handle the multiple DPIs.
>
> The HWND seemed to be a good lowest common denominator everyone had. My
> hand was forced by TooltipWin which reasons over HWNDs and has no access
> to views or widgets, so HWNDs are used for commonality.
>
> https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h
> File ui/gfx/screen_win.h (right):
>
>
>
https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h#new...
> ui/gfx/screen_win.h:11: #include "ui/gfx/geometry/point.h"
> On 2015/10/29 15:51:51, sky wrote:
>
>> you should be able to forward declare the types.
>>
>
> Indeed! Done.
>
>
>
https://codereview.chromium.org/1426933002/diff/20001/ui/gfx/screen_win.h#new...
> ui/gfx/screen_win.h:38: static Point ClientToDIPPoint(HWND hwnd, const
> Point& client_point);
> On 2015/10/29 15:51:51, sky wrote:
>
>> What is a client point?
>>
>
> It's generally a point in an HWND's coordinate system. I used
> terminology from Windows
> (
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms633503(v=vs.85).aspx
> ),
> but if there is a more Chromium idiomatic term, I'm happy to update
> this.
>
>
>
https://codereview.chromium.org/1426933002/diff/20001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
>
>
>
https://codereview.chromium.org/1426933002/diff/20001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:702:
> GetDisplayNearestWindow(root).device_scale_factor();
> On 2015/10/29 15:51:51, sky wrote:
>
>> Why can't this code be made to work consistently?
>>
>
> This came down to the fixed origin coordinate between physical and DIP
> scales. This implementation performs fixed origin scaling. The code here
> presumes movable origin scaling. I plan on talking to oshima about this
> next Monday if you're interested.
>
> https://codereview.chromium.org/1426933002/
>

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

[robliao, 2015-12-02 23:19:36]

Patchset #4 (id:60001) has been deleted

[robliao, 2015-12-15 22:54:54]

Patchset #4 (id:80001) has been deleted

[robliao, 2015-12-18 09:19:10]

Patchset #4 (id:100001) has been deleted

[robliao, 2015-12-18 09:24:37]

Patchset #5 (id:140001) has been deleted

[robliao, 2015-12-18 09:40:21]

Patchset #5 (id:160001) has been deleted

[robliao, 2015-12-18 09:45:43]

I'm at a point where the DIP layout is converging (at least the tests are) so
here's a preview as to what this is looking like. Most of the added complexity
revolves around laying out the DIP monitors and mapping physical and DIP to each
other.

There are certainly some rough edges around the delta, and I don't intend on
landing it until after the holidays when I get back to the states. 

However, feel free to take a look to see where this is going.

[robliao, 2016-01-12 00:13:29]

Patchset #6 (id:200001) has been deleted

[robliao, 2016-01-13 00:35:50]

Patchset #6 (id:220001) has been deleted

[robliao, 2016-01-13 00:41:34]

Alright. I think things are converging here. Feel free to take a look. Thanks!

[robliao, 2016-01-15 18:11:03]

On 2016/01/13 00:41:34, robliao wrote:
> Alright. I think things are converging here. Feel free to take a look. Thanks!

Ping! Feel free to take a look. I've updated it to b840e2f

[oshima, 2016-01-15 18:26:03]

can you update the description with brief description of what this CL changes
existing code?

https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
ui/gfx/display.h:61: virtual ~Display();
Did you add subclass? I haven't look at other changes, but gfx::Display is
copyable by design, so it shouldn't have subclasses.

[robliao, 2016-01-15 19:25:51]

Description was changed from

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now.

An additional change is that to make the conversion between screen pixels and
device-independent points more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

to

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now. All callers of the old functions
have been directed to use the new functions.

An additional change is that to make the conversion between screen pixels and
device-independent points more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

[robliao, 2016-01-15 19:26:15]

Description was changed from

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now. All callers of the old functions
have been directed to use the new functions.

An additional change is that to make the conversion between screen pixels and
device-independent points more tractable, the origins will be fixed regardless
of DPI, similar to how Windows handles DPI virtualization for applications.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

to

==========
Refactor Windows DPI Point, Rect, and Size for Multiple Monitor DPI Awareness

In Windows 8.1 and above, a machine with multiple monitors can have a different
DPI for each monitor. This means that we can no longer use global DPI scaling
to convert between screen pixels and device-independent points. Additionally,
top-level windows can span multiple monitors. Fortunately, the window is affine
to a DPI, so within a window, a single DPI will apply.

To begin to accommodate multiple-DPIs, these Windows scaling functions will
now take an HWND to determine the DPI context and then perform the scale. Given
that the DPI will be determined by gfx::Display in gfx::Screen, ScreenWin
seems like an appropriate home for these now. All callers of the old functions
have been directed to use the new functions.

The visual behavior of Chrome should not change with this CL.

BUG=426656
==========

[robliao, 2016-01-15 19:28:16]

Added
"All callers of the old functions
have been directed to use the new functions."

FYI, one of my assumptions regarding the native screen was wrong with regards to
unit tests, so I'm investigating how to fix that. Most of the code here remains
in light of that.

https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
ui/gfx/display.h:61: virtual ~Display();
On 2016/01/15 18:26:03, oshima wrote:
> Did you add subclass? I haven't look at other changes, but gfx::Display is
> copyable by design, so it shouldn't have subclasses.

Yes, but it doesn't preclude copying. ScreenWin::DisplayWin maintains the
untransformed screen pixel bounds. I figured all outside uses of Display were
outbound of the Screen, so DisplayWin is fully contained within ScreenWin. When
a gfx::Display needs to be handed out, a DisplayWin is always copied to a
gfx::Display.

[oshima, 2016-01-15 19:55:53]

https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
ui/gfx/display.h:61: virtual ~Display();
On 2016/01/15 19:28:16, robliao wrote:
> On 2016/01/15 18:26:03, oshima wrote:
> > Did you add subclass? I haven't look at other changes, but gfx::Display is
> > copyable by design, so it shouldn't have subclasses.
> 
> Yes, but it doesn't preclude copying. ScreenWin::DisplayWin maintains the
> untransformed screen pixel bounds. I figured all outside uses of Display were
> outbound of the Screen, so DisplayWin is fully contained within ScreenWin.
When
> a gfx::Display needs to be handed out, a DisplayWin is always copied to a
> gfx::Display.

Style guide recommends to avoid it, and even if it works now, I still want to
avoid it because it's error prone.
Looks like you just want to remember physical bounds inside ScreenWin. Can you
just keep the mapping inside it?

[robliao, 2016-01-15 20:10:46]

On 2016/01/15 19:55:53, oshima wrote:
> https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
> File ui/gfx/display.h (right):
> 
>
https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
> ui/gfx/display.h:61: virtual ~Display();
> On 2016/01/15 19:28:16, robliao wrote:
> > On 2016/01/15 18:26:03, oshima wrote:
> > > Did you add subclass? I haven't look at other changes, but gfx::Display is
> > > copyable by design, so it shouldn't have subclasses.
> > 
> > Yes, but it doesn't preclude copying. ScreenWin::DisplayWin maintains the
> > untransformed screen pixel bounds. I figured all outside uses of Display
were
> > outbound of the Screen, so DisplayWin is fully contained within ScreenWin.
> When
> > a gfx::Display needs to be handed out, a DisplayWin is always copied to a
> > gfx::Display.
> 
> Style guide recommends to avoid it, and even if it works now, I still want to
> avoid it because it's error prone.
> Looks like you just want to remember physical bounds inside ScreenWin. Can you
> just keep the mapping inside it?

The derived class ensured a nice 1:1 relationship between a display and a
pixel_bounds.
It also kept the initialization of displays as a single vector unit rather than
having a
vector and map that needed to be set.

Would you be opposed to having something like...

struct DisplayWin {
  gfx::Display display;
  gfx::rect pixel_bounds
}

[robliao, 2016-01-15 21:27:18]

Patchset #8 (id:280001) has been deleted

[robliao, 2016-01-15 21:41:17]

On 2016/01/15 20:10:46, robliao wrote:
> On 2016/01/15 19:55:53, oshima wrote:
> > https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
> > File ui/gfx/display.h (right):
> > 
> >
>
https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
> > ui/gfx/display.h:61: virtual ~Display();
> > On 2016/01/15 19:28:16, robliao wrote:
> > > On 2016/01/15 18:26:03, oshima wrote:
> > > > Did you add subclass? I haven't look at other changes, but gfx::Display
is
> > > > copyable by design, so it shouldn't have subclasses.
> > > 
> > > Yes, but it doesn't preclude copying. ScreenWin::DisplayWin maintains the
> > > untransformed screen pixel bounds. I figured all outside uses of Display
> were
> > > outbound of the Screen, so DisplayWin is fully contained within ScreenWin.
> > When
> > > a gfx::Display needs to be handed out, a DisplayWin is always copied to a
> > > gfx::Display.
> > 
> > Style guide recommends to avoid it, and even if it works now, I still want
to
> > avoid it because it's error prone.
> > Looks like you just want to remember physical bounds inside ScreenWin. Can
you
> > just keep the mapping inside it?
> 
> The derived class ensured a nice 1:1 relationship between a display and a
> pixel_bounds.
> It also kept the initialization of displays as a single vector unit rather
than
> having a
> vector and map that needed to be set.
> 
> Would you be opposed to having something like...
> 
> struct DisplayWin {
>   gfx::Display display;
>   gfx::rect pixel_bounds
> }

Went ahead and converted the code to something similar to the struct proposal.

[oshima, 2016-01-15 21:41:59]

On 2016/01/15 20:10:46, robliao wrote:
> On 2016/01/15 19:55:53, oshima wrote:
> > https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
> > File ui/gfx/display.h (right):
> > 
> >
>
https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
> > ui/gfx/display.h:61: virtual ~Display();
> > On 2016/01/15 19:28:16, robliao wrote:
> > > On 2016/01/15 18:26:03, oshima wrote:
> > > > Did you add subclass? I haven't look at other changes, but gfx::Display
is
> > > > copyable by design, so it shouldn't have subclasses.
> > > 
> > > Yes, but it doesn't preclude copying. ScreenWin::DisplayWin maintains the
> > > untransformed screen pixel bounds. I figured all outside uses of Display
> were
> > > outbound of the Screen, so DisplayWin is fully contained within ScreenWin.
> > When
> > > a gfx::Display needs to be handed out, a DisplayWin is always copied to a
> > > gfx::Display.
> > 
> > Style guide recommends to avoid it, and even if it works now, I still want
to
> > avoid it because it's error prone.
> > Looks like you just want to remember physical bounds inside ScreenWin. Can
you
> > just keep the mapping inside it?
> 
> The derived class ensured a nice 1:1 relationship between a display and a
> pixel_bounds.
>
> It also kept the initialization of displays as a single vector unit rather
than
> having a
> vector and map that needed to be set.
> 
> Would you be opposed to having something like...
> 
> struct DisplayWin {
>   gfx::Display display;
>   gfx::rect pixel_bounds
> }


Fine with me, but please get feedback from windows folks as well.

[robliao, 2016-01-16 02:16:45]

On 2016/01/15 21:41:59, oshima wrote:
> On 2016/01/15 20:10:46, robliao wrote:
> > On 2016/01/15 19:55:53, oshima wrote:
> > > https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h
> > > File ui/gfx/display.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1426933002/diff/260001/ui/gfx/display.h#newco...
> > > ui/gfx/display.h:61: virtual ~Display();
> > > On 2016/01/15 19:28:16, robliao wrote:
> > > > On 2016/01/15 18:26:03, oshima wrote:
> > > > > Did you add subclass? I haven't look at other changes, but
gfx::Display
> is
> > > > > copyable by design, so it shouldn't have subclasses.
> > > > 
> > > > Yes, but it doesn't preclude copying. ScreenWin::DisplayWin maintains
the
> > > > untransformed screen pixel bounds. I figured all outside uses of Display
> > were
> > > > outbound of the Screen, so DisplayWin is fully contained within
ScreenWin.
> > > When
> > > > a gfx::Display needs to be handed out, a DisplayWin is always copied to
a
> > > > gfx::Display.
> > > 
> > > Style guide recommends to avoid it, and even if it works now, I still want
> to
> > > avoid it because it's error prone.
> > > Looks like you just want to remember physical bounds inside ScreenWin. Can
> you
> > > just keep the mapping inside it?
> > 
> > The derived class ensured a nice 1:1 relationship between a display and a
> > pixel_bounds.
> >
> > It also kept the initialization of displays as a single vector unit rather
> than
> > having a
> > vector and map that needed to be set.
> > 
> > Would you be opposed to having something like...
> > 
> > struct DisplayWin {
> >   gfx::Display display;
> >   gfx::rect pixel_bounds
> > }
> 
> 
> Fine with me, but please get feedback from windows folks as well.

Will do! After looking at some of the test interaction, I've decided with a more
substantial movement of display information out of Screen. I'll let you know
when it's safe to look again. Thanks!

[robliao, 2016-01-21 01:59:21]

scottmg: Per oshima@, feel free to take a look at the latest patch. Thanks!

[scottmg, 2016-01-21 03:51:25]

On 2016/01/21 01:59:21, robliao wrote:
> scottmg: Per oshima@, feel free to take a look at the latest patch. Thanks!

Oof "+2691 lines". Can we split this up at all? e.g. ui/gfx/win/rect_util* looks
like it could be separately landed first.

I'll start taking a look tomorrow, but I think sky@ probably understood the end
state better than I did here.

[robliao, 2016-01-21 17:07:40]

On 2016/01/21 03:51:25, scottmg wrote:
> On 2016/01/21 01:59:21, robliao wrote:
> > scottmg: Per oshima@, feel free to take a look at the latest patch. Thanks!
> 
> Oof "+2691 lines". Can we split this up at all? e.g. ui/gfx/win/rect_util*
looks
> like it could be separately landed first.
> 
> I'll start taking a look tomorrow, but I think sky@ probably understood the
end
> state better than I did here.

We could land rect_util first, but there wouldn't be any users of it aside from
the unit tests. The display manager separation is potentially separable from
this CL.

[robliao, 2016-01-21 17:47:27]

On 2016/01/21 17:07:40, robliao wrote:
> On 2016/01/21 03:51:25, scottmg wrote:
> > On 2016/01/21 01:59:21, robliao wrote:
> > > scottmg: Per oshima@, feel free to take a look at the latest patch.
Thanks!
> > 
> > Oof "+2691 lines". Can we split this up at all? e.g. ui/gfx/win/rect_util*
> looks
> > like it could be separately landed first.
> > 
> > I'll start taking a look tomorrow, but I think sky@ probably understood the
> end
> > state better than I did here.
> 
> We could land rect_util first, but there wouldn't be any users of it aside
from
> the unit tests. The display manager separation is potentially separable from
> this CL.

Yeah, I think that should be doable. I'm going to sequence this roughly as
follows:
1) Screen DPI scaling harmonization for aura.
2) ScreenWin --> DisplayManager refactor. This was mainly for the benefit of
unit testing the multiple DPI stuff, but should be workable in today's world.
3) Multiple DPI Support.

I should note that a majority of those lines are unit test lines. (980 for
screen_win_unittest and 322 for rect_util_unittest). If that sounds good to you
folks, I'll go ahead and start on that.

[oshima, 2016-01-21 17:47:44]

one nit and one qq.

https://codereview.chromium.org/1426933002/diff/320001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1426933002/diff/320001/ui/gfx/display.h#newco...
ui/gfx/display.h:25: // Display is designed to be copied, so don't inherit from
this class!
can you make the class final instead? I believe it's allowed now.

https://codereview.chromium.org/1426933002/diff/320001/ui/wm/core/default_scr...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/1426933002/diff/320001/ui/wm/core/default_scr...
ui/wm/core/default_screen_position_client.cc:26: gfx::Rect dip_bounds =
screen->ScreenToDIPRectInWindow(window, screen_bounds);
is this screen_bounds in native screen coordinates?

[scottmg, 2016-01-21 17:59:47]

On 2016/01/21 17:47:27, robliao wrote:
> On 2016/01/21 17:07:40, robliao wrote:
> > On 2016/01/21 03:51:25, scottmg wrote:
> > > On 2016/01/21 01:59:21, robliao wrote:
> > > > scottmg: Per oshima@, feel free to take a look at the latest patch.
> Thanks!
> > > 
> > > Oof "+2691 lines". Can we split this up at all? e.g. ui/gfx/win/rect_util*
> > looks
> > > like it could be separately landed first.
> > > 
> > > I'll start taking a look tomorrow, but I think sky@ probably understood
the
> > end
> > > state better than I did here.
> > 
> > We could land rect_util first, but there wouldn't be any users of it aside
> from
> > the unit tests. The display manager separation is potentially separable from
> > this CL.
> 
> Yeah, I think that should be doable. I'm going to sequence this roughly as
> follows:
> 1) Screen DPI scaling harmonization for aura.
> 2) ScreenWin --> DisplayManager refactor. This was mainly for the benefit of
> unit testing the multiple DPI stuff, but should be workable in today's world.
> 3) Multiple DPI Support.
> 
> I should note that a majority of those lines are unit test lines. (980 for
> screen_win_unittest and 322 for rect_util_unittest). If that sounds good to
you
> folks, I'll go ahead and start on that.

If it's possible to split it into 3, that would be great.

[robliao, 2016-01-21 18:19:20]

https://codereview.chromium.org/1426933002/diff/320001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1426933002/diff/320001/ui/gfx/display.h#newco...
ui/gfx/display.h:25: // Display is designed to be copied, so don't inherit from
this class!
On 2016/01/21 17:47:44, oshima wrote:
> can you make the class final instead? I believe it's allowed now.

Will do! That can be a separate CL too.

https://codereview.chromium.org/1426933002/diff/320001/ui/wm/core/default_scr...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/1426933002/diff/320001/ui/wm/core/default_scr...
ui/wm/core/default_screen_position_client.cc:26: gfx::Rect dip_bounds =
screen->ScreenToDIPRectInWindow(window, screen_bounds);
On 2016/01/21 17:47:44, oshima wrote:
> is this screen_bounds in native screen coordinates?

We really need that units refactor.

GetOriginInScreen returns a Screen point in DIP units.
root_window->GetHost()->GetBounds() returns a rect in Screen coordinates in
physical pixel units (In Windows, it calls directly into GetClientRect + a
ClientToScreen call)

The conversion step turns it into screen coordinates + DIP units.

[oshima, 2016-01-21 18:31:11]

On 2016/01/21 18:19:20, robliao wrote:
> https://codereview.chromium.org/1426933002/diff/320001/ui/gfx/display.h
> File ui/gfx/display.h (right):
> 
>
https://codereview.chromium.org/1426933002/diff/320001/ui/gfx/display.h#newco...
> ui/gfx/display.h:25: // Display is designed to be copied, so don't inherit
from
> this class!
> On 2016/01/21 17:47:44, oshima wrote:
> > can you make the class final instead? I believe it's allowed now.
> 
> Will do! That can be a separate CL too.
> 
>
https://codereview.chromium.org/1426933002/diff/320001/ui/wm/core/default_scr...
> File ui/wm/core/default_screen_position_client.cc (right):
> 
>
https://codereview.chromium.org/1426933002/diff/320001/ui/wm/core/default_scr...
> ui/wm/core/default_screen_position_client.cc:26: gfx::Rect dip_bounds =
> screen->ScreenToDIPRectInWindow(window, screen_bounds);
> On 2016/01/21 17:47:44, oshima wrote:
> > is this screen_bounds in native screen coordinates?
> 
> We really need that units refactor.
> 
> GetOriginInScreen returns a Screen point in DIP units.
> root_window->GetHost()->GetBounds() returns a rect in Screen coordinates in
> physical pixel units (In Windows, it calls directly into GetClientRect + a
> ClientToScreen call)

If that's only the difference, how about having WTH::GetBoundsInNavie and
GetBoundsInDIP (which are the same on chromeos/mac but differ on Windows), and
use that instead?

> 
> The conversion step turns it into screen coordinates + DIP units.