Multiple DPI Tracking for ScreenWin

Multiple DPI Tracking for ScreenWin

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, 501259

[robliao, 2016-02-09 19:25:29]

Description was changed from

==========
Multiple DPI Tracking for ScreenWin

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,501259
==========

to

==========
Multiple DPI Tracking for ScreenWin

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,501259
==========

[robliao, 2016-02-09 19:25:29]

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

[robliao, 2016-02-09 19:25:41]

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

[robliao, 2016-02-09 19:26:14]

Let's go ahead and get this show on the road!

[scottmg, 2016-02-09 20:21:36]

I'm still working on rect_util, but generally looks good.

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

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/screen_win.cc#newcode62
ui/gfx/screen_win.cc:62: // insufficent room to lay out its children. In these
cases, a DIP point could
insufficient

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

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/screen_win_unittest....
ui/gfx/screen_win_unittest.cc:711: TEST_F(ScreenWinTestTwoDisplays2x,
ScreenToDIPRects) {
Maybe a couple fractional scale tests here to codify what happens in the case of
1.25 or 1.5 for clamping, enclosing, etc?

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc#new...
ui/gfx/win/rect_util.cc:32: return gfx::Rect(-rect.y() -rect.height(), rect.x(),
binary - here, missing space before rect.height()

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc#new...
ui/gfx/win/rect_util.cc:36: gfx::Rect CoordinateReflectRectagleYAxis(const
gfx::Rect& rect) {
Rectagle -> Rectangle

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h#newc...
ui/gfx/win/rect_util.h:27: RectEdge GFX_EXPORT RectTouch(gfx::Rect ref,
gfx::Rect test);
The GFX_EXPORTs normally go before the return type.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h#newc...
ui/gfx/win/rect_util.h:27: RectEdge GFX_EXPORT RectTouch(gfx::Rect ref,
gfx::Rect test);
"RectTouch" is a bit terse. Mostly, I'd expect it to return bool.

Not coming up with a better name though. "TouchingEdgeForRect" or
"TouchingEdgeBetweenRects"

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h#newc...
ui/gfx/win/rect_util.h:67: // Returns the squared distance between two rects.
A little more explanation here, it isn't obvious what "squared distance between
two rects" means.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util_unitte...
File ui/gfx/win/rect_util_unittest.cc (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util_unitte...
ui/gfx/win/rect_util_unittest.cc:320: 
Maybe some non-integral tests here too.

(Also, extra blank line here)

[scottmg, 2016-02-09 21:24:26]

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc#new...
ui/gfx/win/rect_util.cc:184: ref_scaled_rect =
CanonicalizeTouchingEdge(ref_scaled_rect, orig_edge);
(I'm still trying to figure out how this works, but...)

Does this rely on ref being at the origin? The *Rotate*s are rotating around the
origin, right? If so, that should be DCHECK()d.

[robliao, 2016-02-10 22:50:04]

Thanks for the comments!

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

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/screen_win.cc#newcode62
ui/gfx/screen_win.cc:62: // insufficent room to lay out its children. In these
cases, a DIP point could
On 2016/02/09 20:21:36, scottmg wrote:
> insufficient

Done.

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

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/screen_win_unittest....
ui/gfx/screen_win_unittest.cc:711: TEST_F(ScreenWinTestTwoDisplays2x,
ScreenToDIPRects) {
On 2016/02/09 20:21:36, scottmg wrote:
> Maybe a couple fractional scale tests here to codify what happens in the case
of
> 1.25 or 1.5 for clamping, enclosing, etc?

Done.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc#new...
ui/gfx/win/rect_util.cc:32: return gfx::Rect(-rect.y() -rect.height(), rect.x(),
On 2016/02/09 20:21:36, scottmg wrote:
> binary - here, missing space before rect.height()

Oops, indeed. Done.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc#new...
ui/gfx/win/rect_util.cc:36: gfx::Rect CoordinateReflectRectagleYAxis(const
gfx::Rect& rect) {
On 2016/02/09 20:21:36, scottmg wrote:
> Rectagle -> Rectangle

Done.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.cc#new...
ui/gfx/win/rect_util.cc:184: ref_scaled_rect =
CanonicalizeTouchingEdge(ref_scaled_rect, orig_edge);
On 2016/02/09 21:24:26, scottmg wrote:
> (I'm still trying to figure out how this works, but...)
> 
> Does this rely on ref being at the origin? The *Rotate*s are rotating around
the
> origin, right? If so, that should be DCHECK()d.

All rectangles are assumed to have a coordinate origin of (0, 0). The rotation
transformations are performed with respect to (0, 0)

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h#newc...
ui/gfx/win/rect_util.h:27: RectEdge GFX_EXPORT RectTouch(gfx::Rect ref,
gfx::Rect test);
On 2016/02/09 20:21:36, scottmg wrote:
> The GFX_EXPORTs normally go before the return type.

Done and renamed to FindTouchingRectEdge.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util.h#newc...
ui/gfx/win/rect_util.h:67: // Returns the squared distance between two rects.
On 2016/02/09 20:21:36, scottmg wrote:
> A little more explanation here, it isn't obvious what "squared distance
between
> two rects" means.

Done and added some ASCII art.

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util_unitte...
File ui/gfx/win/rect_util_unittest.cc (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util_unitte...
ui/gfx/win/rect_util_unittest.cc:320: 
On 2016/02/09 20:21:36, scottmg wrote:
> Maybe some non-integral tests here too.
> 
> (Also, extra blank line here)

All of the SquaredDistance tests will be integral by nature. Added some 1.5f
scaling factor tests.

[scottmg, 2016-02-10 23:07:45]

non-owner lgtm insofar as I know what's going on :)

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util_unitte...
File ui/gfx/win/rect_util_unittest.cc (right):

https://codereview.chromium.org/1679393002/diff/1/ui/gfx/win/rect_util_unitte...
ui/gfx/win/rect_util_unittest.cc:320: 
On 2016/02/10 22:50:03, robliao wrote:
> On 2016/02/09 20:21:36, scottmg wrote:
> > Maybe some non-integral tests here too.
> > 
> > (Also, extra blank line here)
> 
> All of the SquaredDistance tests will be integral by nature. Added some 1.5f
> scaling factor tests.

Yeah, I meant the scale factor, not the rects. :)

https://codereview.chromium.org/1679393002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1679393002/diff/20001/ui/gfx/BUILD.gn#newcode226
ui/gfx/BUILD.gn:226: "win/rect_util.h",
rect_util_unittest.cc somewhere in this file?

[robliao, 2016-02-11 19:05:40]

oshima: Feel free to take a look. Thanks!

https://codereview.chromium.org/1679393002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1679393002/diff/20001/ui/gfx/BUILD.gn#newcode226
ui/gfx/BUILD.gn:226: "win/rect_util.h",
On 2016/02/10 23:07:45, scottmg wrote:
> rect_util_unittest.cc somewhere in this file?

Done.

[oshima, 2016-02-11 22:59:58]

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

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.cc#ne...
ui/gfx/screen_win.cc:165: DCHECK(g_screen_win_instance == nullptr);
DCHECK(!g_screen_win_instance) is more common

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.cc#ne...
ui/gfx/screen_win.cc:338: HWND hwnd = view ? GetHWNDFromNativeView(view) : NULL;
nit: nullptr throughout the CL

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.cc#ne...
ui/gfx/screen_win.cc:433: gfx::win::SquaredDistanceBetweenRects(dip_rect,
dip_bounds);
can this be manhattan distance?

you can also move the impl to here if this is only place used.

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

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:59: // independent of the origin. An alternative is to get
the enclosing rect,
The comment isn't clear to me. can you elaborate? (I understand what each
methods are doing)

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:45: rect = CoordinateRotateRectangle90(rect);
nit: simply return instead of assigning to rect. same for other places. You may
pass rect by const reference.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:53: }
indent. same for other places.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:103: DCHECK(new_min == new_max);
DCHECK_EQ

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:106: float percent = (float)(val - old_min) /
(float)old_delta;
static_cast

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:144: InRange(ref.right(), test.x(), test.right()))) {
this indent looks odd to me.

I believe this is testing if two overlaps. Can it be

!(test.right() < ref.x() || ref.right() < test.x())

? or use Range::Intersects

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h#...
ui/gfx/win/rect_util.h:27: GFX_EXPORT RectEdge FindTouchingRectEdge(gfx::Rect
ref, gfx::Rect test);
const &

[robliao, 2016-02-12 01:46:03]

Patchset #5 (id:80001) has been deleted

[robliao, 2016-02-12 01:46:08]

Patchset #4 (id:60001) has been deleted

[robliao, 2016-02-12 01:52:53]

Thanks for the comments!

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

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.cc#ne...
ui/gfx/screen_win.cc:165: DCHECK(g_screen_win_instance == nullptr);
On 2016/02/11 22:59:58, oshima wrote:
> DCHECK(!g_screen_win_instance) is more common

Done.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.cc#ne...
ui/gfx/screen_win.cc:338: HWND hwnd = view ? GetHWNDFromNativeView(view) : NULL;
On 2016/02/11 22:59:58, oshima wrote:
> nit: nullptr throughout the CL

Done.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.cc#ne...
ui/gfx/screen_win.cc:433: gfx::win::SquaredDistanceBetweenRects(dip_rect,
dip_bounds);
On 2016/02/11 22:59:58, oshima wrote:
> can this be manhattan distance?
> 
> you can also move the impl to here if this is only place used.

Using Manhattan distance seems like it would return the wrong result as it would
overshoot the actual direct distance between rectangles, favoring rectangles
lying on the same axis over diagonal ones.

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

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:59: // independent of the origin. An alternative is to get
the enclosing rect,
On 2016/02/11 22:59:58, oshima wrote:
> The comment isn't clear to me. can you elaborate? (I understand what each
> methods are doing)

This was taken from the original DPI scaling code. Updated.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:45: rect = CoordinateRotateRectangle90(rect);
On 2016/02/11 22:59:58, oshima wrote:
> nit: simply return instead of assigning to rect. same for other places. You
may
> pass rect by const reference.

Done.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:53: }
On 2016/02/11 22:59:58, oshima wrote:
> indent. same for other places.

Done.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:103: DCHECK(new_min == new_max);
On 2016/02/11 22:59:58, oshima wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:106: float percent = (float)(val - old_min) /
(float)old_delta;
On 2016/02/11 22:59:58, oshima wrote:
> static_cast

Done.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:144: InRange(ref.right(), test.x(), test.right()))) {
On 2016/02/11 22:59:58, oshima wrote:
> this indent looks odd to me.
> 
> I believe this is testing if two overlaps. Can it be
> 
> !(test.right() < ref.x() || ref.right() < test.x())
> 
> ? or use Range::Intersects

The parens certainly make it look odd, but I think this is the right break if we
group the statements together.
if (Statement1 &&
    Statement2
        Continuation of Statement 2
        Continuation of Statement 2
        Continuation of Statement 2)

As for the comparison, the rects actually may have negative coordinates, which
means we can't use gfx::Range (unsigned ints).

What's the motivation of directly inlining InRange into its boolean statement?

Variations of
ref.x() <= test.x() && test.x() <= ref.right()
!(ref.x() > test.x() || test.x() > ref.right())
!(test.x() < ref.x() || ref.right() < test.x())
repeated many times seem harder to read than
InRange(test.x(), ref.x(), ref.right()) especially as the expressions get
longer:
ref_original_rect.x() <= target_rect.right() && 
    target_rect.right() <= ref_original_rect.right())
!(ref_original_rect.x() > target_rect.right() ||
    target_rect.right() > ref_original_rect.right()))
!(target_rect.right() < ref_original_rect.x()  ||
    ref_original_rect.right() < target_rect.right())
vs.
InRange(target_rect.right(),
        ref_original_rect.x(),
        ref_original_rect.right())

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h#...
ui/gfx/win/rect_util.h:27: GFX_EXPORT RectEdge FindTouchingRectEdge(gfx::Rect
ref, gfx::Rect test);
On 2016/02/11 22:59:58, oshima wrote:
> const &

These actually started as const gfx::Rect& and then I removed them because
things got pretty messy as the first thing all of these functions would do is
copy all of the incoming values like so.

gfx::Rect ref_work(ref);
gfx::Rect test_work(test);

And then it was important that no code afterwards references ref or test ever
again otherwise they would be doing something very wrong. Inlining the copy
seemed like the right thing to do.

Do you have a better way that prevents the duplicate variable names?

[oshima, 2016-02-12 23:18:26]

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:144: InRange(ref.right(), test.x(), test.right()))) {
On 2016/02/12 01:52:53, robliao wrote:
> On 2016/02/11 22:59:58, oshima wrote:
> > this indent looks odd to me.
> > 
> > I believe this is testing if two overlaps. Can it be
> > 
> > !(test.right() < ref.x() || ref.right() < test.x())
> > 
> > ? or use Range::Intersects
> 
> The parens certainly make it look odd, but I think this is the right break if
we
> group the statements together.
> if (Statement1 &&
>     Statement2
>         Continuation of Statement 2
>         Continuation of Statement 2
>         Continuation of Statement 2)

It looks to me that this is more like

if (Statement &&
    (NestedStatement ||
     NestedStatement ||
     NestedStatement ||
     NestedStatement)) {


Both git cl format and emacs's google style script format this as

  for (int i = 0; i <= 3; ++i) {
    if (ref.bottom() == test.y() &&
        (InRange(test.x(), ref.x(), ref.right()) ||
         InRange(test.right(), ref.x(), ref.right()) ||
         InRange(ref.x(), test.x(), test.right()) ||
         InRange(ref.right(), test.x(), test.right()))) {


> 
> As for the comparison, the rects actually may have negative coordinates, which
> means we can't use gfx::Range (unsigned ints).

Bummer. I should have been fixed to use ints. There is gfx::RectF, which accepts
float
although it's probably overkill.

> 
> What's the motivation of directly inlining InRange into its boolean statement?
> 
> Variations of
> ref.x() <= test.x() && test.x() <= ref.right()
> !(ref.x() > test.x() || test.x() > ref.right())
> !(test.x() < ref.x() || ref.right() < test.x())
> repeated many times seem harder to read than
> InRange(test.x(), ref.x(), ref.right()) especially as the expressions get
> longer:
> ref_original_rect.x() <= target_rect.right() && 
>     target_rect.right() <= ref_original_rect.right())
> !(ref_original_rect.x() > target_rect.right() ||
>     target_rect.right() > ref_original_rect.right()))
> !(target_rect.right() < ref_original_rect.x()  ||
>     ref_original_rect.right() < target_rect.right())
> vs.
> InRange(target_rect.right(),
>         ref_original_rect.x(),
>         ref_original_rect.right())

What I'm saying is that the following single line does the same thing:

!(test.right() < ref.x() || ref.right() < test.x())

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h#...
ui/gfx/win/rect_util.h:27: GFX_EXPORT RectEdge FindTouchingRectEdge(gfx::Rect
ref, gfx::Rect test);
On 2016/02/12 01:52:53, robliao wrote:
> On 2016/02/11 22:59:58, oshima wrote:
> > const &
> 
> These actually started as const gfx::Rect& and then I removed them because
> things got pretty messy as the first thing all of these functions would do is
> copy all of the incoming values like so.
> 
> gfx::Rect ref_work(ref);
> gfx::Rect test_work(test);
> 
> And then it was important that no code afterwards references ref or test ever
> again otherwise they would be doing something very wrong. Inlining the copy
> seemed like the right thing to do.

For correctness, I'd rely on the unit tests rather than the use of copy, as
the actual effect can be opposite.
 
> Do you have a better way that prevents the duplicate variable names?

Not really, other than changing the logic not to rotate the rects.

If it were me, I'd get the intersect of two rects, (you can use what gfx::Rect
is doing, without the code that emptying out) then tell horizontal or vertical
edge by checking width/height, then compare the x/y with center point of the
intersect to decide which side. It's easier to follow and could be less code,
although I have no proof.

I'll leave it to your and owner though.

[robliao, 2016-02-13 01:27:49]

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:144: InRange(ref.right(), test.x(), test.right()))) {
On 2016/02/12 23:18:26, oshima wrote:
> On 2016/02/12 01:52:53, robliao wrote:
> > On 2016/02/11 22:59:58, oshima wrote:
> > > this indent looks odd to me.
> > > 
> > > I believe this is testing if two overlaps. Can it be
> > > 
> > > !(test.right() < ref.x() || ref.right() < test.x())
> > > 
> > > ? or use Range::Intersects
> > 
> > The parens certainly make it look odd, but I think this is the right break
if
> we
> > group the statements together.
> > if (Statement1 &&
> >     Statement2
> >         Continuation of Statement 2
> >         Continuation of Statement 2
> >         Continuation of Statement 2)
> 
> It looks to me that this is more like
> 
> if (Statement &&
>     (NestedStatement ||
>      NestedStatement ||
>      NestedStatement ||
>      NestedStatement)) {
> 
> 
> Both git cl format and emacs's google style script format this as
> 
>   for (int i = 0; i <= 3; ++i) {
>     if (ref.bottom() == test.y() &&
>         (InRange(test.x(), ref.x(), ref.right()) ||
>          InRange(test.right(), ref.x(), ref.right()) ||
>          InRange(ref.x(), test.x(), test.right()) ||
>          InRange(ref.right(), test.x(), test.right()))) {
> 
> 
> > 
> > As for the comparison, the rects actually may have negative coordinates,
which
> > means we can't use gfx::Range (unsigned ints).
> 
> Bummer. I should have been fixed to use ints. There is gfx::RectF, which
accepts
> float
> although it's probably overkill.
> 
> > 
> > What's the motivation of directly inlining InRange into its boolean
statement?
> > 
> > Variations of
> > ref.x() <= test.x() && test.x() <= ref.right()
> > !(ref.x() > test.x() || test.x() > ref.right())
> > !(test.x() < ref.x() || ref.right() < test.x())
> > repeated many times seem harder to read than
> > InRange(test.x(), ref.x(), ref.right()) especially as the expressions get
> > longer:
> > ref_original_rect.x() <= target_rect.right() && 
> >     target_rect.right() <= ref_original_rect.right())
> > !(ref_original_rect.x() > target_rect.right() ||
> >     target_rect.right() > ref_original_rect.right()))
> > !(target_rect.right() < ref_original_rect.x()  ||
> >     ref_original_rect.right() < target_rect.right())
> > vs.
> > InRange(target_rect.right(),
> >         ref_original_rect.x(),
> >         ref_original_rect.right())
> 
> What I'm saying is that the following single line does the same thing:
> 
> !(test.right() < ref.x() || ref.right() < test.x())
> 

Applied the formatting.

We're in agreement that InRange and the above boolean expression does the same
thing (proof of equivalence available in my comment).

Because this statement is repeated many times, I created InRange to make this
easier to read.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h#...
ui/gfx/win/rect_util.h:27: GFX_EXPORT RectEdge FindTouchingRectEdge(gfx::Rect
ref, gfx::Rect test);
On 2016/02/12 23:18:26, oshima wrote:
> On 2016/02/12 01:52:53, robliao wrote:
> > On 2016/02/11 22:59:58, oshima wrote:
> > > const &
> > 
> > These actually started as const gfx::Rect& and then I removed them because
> > things got pretty messy as the first thing all of these functions would do
is
> > copy all of the incoming values like so.
> > 
> > gfx::Rect ref_work(ref);
> > gfx::Rect test_work(test);
> > 
> > And then it was important that no code afterwards references ref or test
ever
> > again otherwise they would be doing something very wrong. Inlining the copy
> > seemed like the right thing to do.
> 
> For correctness, I'd rely on the unit tests rather than the use of copy, as
> the actual effect can be opposite.
>  
> > Do you have a better way that prevents the duplicate variable names?
> 
> Not really, other than changing the logic not to rotate the rects.
> 
> If it were me, I'd get the intersect of two rects, (you can use what gfx::Rect
> is doing, without the code that emptying out) then tell horizontal or vertical
> edge by checking width/height, then compare the x/y with center point of the
> intersect to decide which side. It's easier to follow and could be less code,
> although I have no proof.
> 
> I'll leave it to your and owner though.

Thanks for the intersecting rect pointer. I probably got carried away with the
transformations since ScaleAndPositionRect was on the mind.

I have the proof since I coded up the solution. It passes unit tests we'll go
ahead and use it. That also cleaned up RectRelativePosition.

This allows both of them to be const Rect&'s. ScaleAndPositionRect as well as
SquaredDistanceBetweenRects are more difficult to change, so I'm going to leave
them as is. It doesn't change the callers either way.

[oshima, 2016-02-16 06:03:26]

lgtm

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.cc...
ui/gfx/win/rect_util.cc:144: InRange(ref.right(), test.x(), test.right()))) {
On 2016/02/13 01:27:49, robliao wrote:
> On 2016/02/12 23:18:26, oshima wrote:
> > On 2016/02/12 01:52:53, robliao wrote:
> > > On 2016/02/11 22:59:58, oshima wrote:
> > > > this indent looks odd to me.
> > > > 
> > > > I believe this is testing if two overlaps. Can it be
> > > > 
> > > > !(test.right() < ref.x() || ref.right() < test.x())
> > > > 
> > > > ? or use Range::Intersects
> > > 
> > > The parens certainly make it look odd, but I think this is the right break
> if
> > we
> > > group the statements together.
> > > if (Statement1 &&
> > >     Statement2
> > >         Continuation of Statement 2
> > >         Continuation of Statement 2
> > >         Continuation of Statement 2)
> > 
> > It looks to me that this is more like
> > 
> > if (Statement &&
> >     (NestedStatement ||
> >      NestedStatement ||
> >      NestedStatement ||
> >      NestedStatement)) {
> > 
> > 
> > Both git cl format and emacs's google style script format this as
> > 
> >   for (int i = 0; i <= 3; ++i) {
> >     if (ref.bottom() == test.y() &&
> >         (InRange(test.x(), ref.x(), ref.right()) ||
> >          InRange(test.right(), ref.x(), ref.right()) ||
> >          InRange(ref.x(), test.x(), test.right()) ||
> >          InRange(ref.right(), test.x(), test.right()))) {
> > 
> > 
> > > 
> > > As for the comparison, the rects actually may have negative coordinates,
> which
> > > means we can't use gfx::Range (unsigned ints).
> > 
> > Bummer. I should have been fixed to use ints. There is gfx::RectF, which
> accepts
> > float
> > although it's probably overkill.
> > 
> > > 
> > > What's the motivation of directly inlining InRange into its boolean
> statement?
> > > 
> > > Variations of
> > > ref.x() <= test.x() && test.x() <= ref.right()
> > > !(ref.x() > test.x() || test.x() > ref.right())
> > > !(test.x() < ref.x() || ref.right() < test.x())
> > > repeated many times seem harder to read than
> > > InRange(test.x(), ref.x(), ref.right()) especially as the expressions get
> > > longer:
> > > ref_original_rect.x() <= target_rect.right() && 
> > >     target_rect.right() <= ref_original_rect.right())
> > > !(ref_original_rect.x() > target_rect.right() ||
> > >     target_rect.right() > ref_original_rect.right()))
> > > !(target_rect.right() < ref_original_rect.x()  ||
> > >     ref_original_rect.right() < target_rect.right())
> > > vs.
> > > InRange(target_rect.right(),
> > >         ref_original_rect.x(),
> > >         ref_original_rect.right())
> > 
> > What I'm saying is that the following single line does the same thing:
> > 
> > !(test.right() < ref.x() || ref.right() < test.x())
> > 
> 
> Applied the formatting.
> 
> We're in agreement that InRange and the above boolean expression does the same
> thing (proof of equivalence available in my comment).
> 
> Because this statement is repeated many times, I created InRange to make this
> easier to read.

Acknowledged.

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/40001/ui/gfx/win/rect_util.h#...
ui/gfx/win/rect_util.h:27: GFX_EXPORT RectEdge FindTouchingRectEdge(gfx::Rect
ref, gfx::Rect test);
On 2016/02/13 01:27:49, robliao wrote:
> On 2016/02/12 23:18:26, oshima wrote:
> > On 2016/02/12 01:52:53, robliao wrote:
> > > On 2016/02/11 22:59:58, oshima wrote:
> > > > const &
> > > 
> > > These actually started as const gfx::Rect& and then I removed them because
> > > things got pretty messy as the first thing all of these functions would do
> is
> > > copy all of the incoming values like so.
> > > 
> > > gfx::Rect ref_work(ref);
> > > gfx::Rect test_work(test);
> > > 
> > > And then it was important that no code afterwards references ref or test
> ever
> > > again otherwise they would be doing something very wrong. Inlining the
copy
> > > seemed like the right thing to do.
> > 
> > For correctness, I'd rely on the unit tests rather than the use of copy, as
> > the actual effect can be opposite.
> >  
> > > Do you have a better way that prevents the duplicate variable names?
> > 
> > Not really, other than changing the logic not to rotate the rects.
> > 
> > If it were me, I'd get the intersect of two rects, (you can use what
gfx::Rect
> > is doing, without the code that emptying out) then tell horizontal or
vertical
> > edge by checking width/height, then compare the x/y with center point of the
> > intersect to decide which side. It's easier to follow and could be less
code,
> > although I have no proof.
> > 
> > I'll leave it to your and owner though.
> 
> Thanks for the intersecting rect pointer. I probably got carried away with the
> transformations since ScaleAndPositionRect was on the mind.
> 
> I have the proof since I coded up the solution. It passes unit tests we'll go
> ahead and use it. That also cleaned up RectRelativePosition.
> 
> This allows both of them to be const Rect&'s. ScaleAndPositionRect as well as
> SquaredDistanceBetweenRects are more difficult to change, so I'm going to
leave
> them as is. It doesn't change the callers either way.

Ok. I'll leave the rest to owners review. (I don't own ui/gfx/win)

https://codereview.chromium.org/1679393002/diff/160001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/160001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:53: }
indent

[robliao, 2016-02-16 21:41:51]

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

[robliao, 2016-02-16 21:42:18]

sky: Please review ui/* in this CL. Thanks!

[sky, 2016-02-17 04:40:29]

Here's some initial, mostly minor feedback. I need a bit of time to thoroughly
understand the rect_util changes. I'll look more tomorrow.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:122: RectEdge FindTouchingRectEdge(const gfx::Rect& ref,
const gfx::Rect& test) {
Add description, in particular who the return value is relative to.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:135: } else if (max_x == min_right) {
nit: no else after return.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:16: namespace win {
This file name is very generic. I don't think we want it to become a dumping
ground. How about a more specific name with comments?

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:30: // Returns a scaled and positioned rect based off of
target_rect and context.
there is no context param.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:34: // scale factors.
Without looking at the implementation this description is confusing. It isn't
clear how ref_scaled_rect and ref_origin_rect related. I'm assuming
ref_scaled_rect is a scaled version of ref_origin_rect maybe with a different
origin. If that's true, then a scale and origin of the scaled would be less
error prone and less confusing.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:63: GFX_EXPORT gfx::Rect ScaleAndPositionRect(gfx::Rect
ref_scaled_rect,
const& on the params here and below. Use different names in the function if you
want to modify the incoming values.

[sky, 2016-02-17 17:28:48]

I don't understand the constraints of ScaleAndPositionRect enough to review it.
Seem my questions below.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:61: return CoordinateRotateRectangle90(rect);
Can you elaborate on why we need the rotation?

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:123: int max_x = std::max(ref.x(), test.x());
This functions seems more complex than it needs to be. Why not something like:

if (ref.right() == test.x())
  return RIGHT;
if (ref.x() == test.right())
  return LEFT;
if (ref.y() == test.bottom())
  return TOP;
if (ref.bottom() == test.y())
  return BOTTOM;
return NONE;

?
It's entirely possible I have the left/right backwards.As I said, I'm not sure
which rect the direction is relative to.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:165: NOTREACHED();
If that's the case, then 164 should be a DCHECK.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:26: // Returns |ref|'s RectEdge touching |test|.
Clarify if the two rects are guaranteed to touch.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:34: // scale factors.
On 2016/02/17 04:40:29, sky wrote:
> Without looking at the implementation this description is confusing. It isn't
> clear how ref_scaled_rect and ref_origin_rect related. I'm assuming
> ref_scaled_rect is a scaled version of ref_origin_rect maybe with a different
> origin. If that's true, then a scale and origin of the scaled would be less
> error prone and less confusing.

One more comment. It is also isn't clear if the rectangles always touch like you
have in the examples.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:65: gfx::Rect target_rect,
It's not clear if target_rect is scaled or not.

[robliao, 2016-02-18 00:34:40]

Thanks for the comments!

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:61: return CoordinateRotateRectangle90(rect);
On 2016/02/17 17:28:48, sky wrote:
> Can you elaborate on why we need the rotation?

The rotation allows us to take advantage of the rotational symmetry of
positioning a rectangle along an edge.

This avoids the need to consider cases for positioning on the right edge (need
to move the unscaled rectangle y location), top edge (need to move the unscaled
rectangle x location and position the bottom edge to match the top edge of the
reference rectangle), and left edge (need to move the unscaled y location and
position the right edge at the left edge of the reference rectangle).

All of these positions can be canonicalized to the same one, so the workflow
is...

[Rotational Transform - Canonicalize to desired orientation]
[Perform Scaling Calculations]
[Revert the Rotational Transform if necessary]

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:122: RectEdge FindTouchingRectEdge(const gfx::Rect& ref,
const gfx::Rect& test) {
On 2016/02/17 04:40:28, sky wrote:
> Add description, in particular who the return value is relative to.

Done for RectRelativePosition. The header contains this info:
// Returns |ref|'s RectEdge touching |test|.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:123: int max_x = std::max(ref.x(), test.x());
On 2016/02/17 17:28:48, sky wrote:
> This functions seems more complex than it needs to be. Why not something like:
> 
> if (ref.right() == test.x())
>   return RIGHT;
> if (ref.x() == test.right())
>   return LEFT;
> if (ref.y() == test.bottom())
>   return TOP;
> if (ref.bottom() == test.y())
>   return BOTTOM;
> return NONE;
> 
> ?
> It's entirely possible I have the left/right backwards.As I said, I'm not sure
> which rect the direction is relative to.

We need to do the intersection test because a rectangle may satisfy some of the
conditions in the suggested code snippet and still not touch. For example,
consider two rectangles REF and TEST:

x-values
0123456789
+---+
|REF|
+---+

    +----+
    |TEST|
    +----+

REF.right() == 4 == TEST.x()
The code snippet above would say that REF's right edge touches TEST when it
should return NONE.

The edge orientation is documented in the header file:
// Returns |ref|'s RectEdge touching |test|.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:135: } else if (max_x == min_right) {
On 2016/02/17 04:40:28, sky wrote:
> nit: no else after return.

Done.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:165: NOTREACHED();
On 2016/02/17 17:28:48, sky wrote:
> If that's the case, then 164 should be a DCHECK.

Done.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:16: namespace win {
On 2016/02/17 04:40:28, sky wrote:
> This file name is very generic. I don't think we want it to become a dumping
> ground. How about a more specific name with comments?

Given that this is primarily used by ScreenWin, how about
screen_win_rect_util.*?

I'm holding off on the rename to preserve the comment comparison chain.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:26: // Returns |ref|'s RectEdge touching |test|.
On 2016/02/17 17:28:48, sky wrote:
> Clarify if the two rects are guaranteed to touch.

Done.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:30: // Returns a scaled and positioned rect based off of
target_rect and context.
On 2016/02/17 04:40:29, sky wrote:
> there is no context param.

Clarified.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:34: // scale factors.
On 2016/02/17 17:28:48, sky wrote:
> On 2016/02/17 04:40:29, sky wrote:
> > Without looking at the implementation this description is confusing. It
isn't
> > clear how ref_scaled_rect and ref_origin_rect related. I'm assuming
> > ref_scaled_rect is a scaled version of ref_origin_rect maybe with a
different
> > origin. If that's true, then a scale and origin of the scaled would be less
> > error prone and less confusing.
> 
> One more comment. It is also isn't clear if the rectangles always touch like
you
> have in the examples.

Clarified the comment and added another example. For ScaleAndPositionRect, the
new origin and scale factor are insufficient to calculate the new position since
reference edge positioning is critical to getting the right scaled rectangle
position.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:63: GFX_EXPORT gfx::Rect ScaleAndPositionRect(gfx::Rect
ref_scaled_rect,
On 2016/02/17 04:40:29, sky wrote:
> const& on the params here and below. Use different names in the function if
you
> want to modify the incoming values.

Done.

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:65: gfx::Rect target_rect,
On 2016/02/17 17:28:48, sky wrote:
> It's not clear if target_rect is scaled or not.

Clarified via rename.

[sky, 2016-02-18 17:02:58]

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/200001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:16: namespace win {
On 2016/02/18 00:34:39, robliao wrote:
> On 2016/02/17 04:40:28, sky wrote:
> > This file name is very generic. I don't think we want it to become a dumping
> > ground. How about a more specific name with comments?
> 
> Given that this is primarily used by ScreenWin, how about
> screen_win_rect_util.*?
> 
> I'm holding off on the rename to preserve the comment comparison chain.

SGTM

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:129: int max_x = std::max(ref.x(), test.x());
I have a hard time reading this code because max_x implies the biggest
x-coordinate, but that's not what this is. It's the biggest left value. How
about max_left and max_top? That better matches min_bottom and min_right.

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:88: GFX_EXPORT gfx::Rect ScaleAndPositionRect(const
gfx::Rect& ref_scaled_rect,
Thanks for the comments. I think I understand what you're trying to do here now.
Why can't you take the top/left corner of unscaled_rect, apply the scale of ref
rect to get the origin of the resulting rect. For the size of the resulting rect
scale the unscaled_rects size by unscaled_rect_scale_factor. Isn't this
effectively what this function wants to do?

[robliao, 2016-02-19 00:26:29]

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.cc
File ui/gfx/win/rect_util.cc (right):

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.c...
ui/gfx/win/rect_util.cc:129: int max_x = std::max(ref.x(), test.x());
On 2016/02/18 17:02:58, sky wrote:
> I have a hard time reading this code because max_x implies the biggest
> x-coordinate, but that's not what this is. It's the biggest left value. How
> about max_left and max_top? That better matches min_bottom and min_right.

sgtm. Done. As an aside, the original naming from gfx::Rect was rx, ry, rr, rb,
which I had renamed here to [min|max]_[rect property name]

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h
File ui/gfx/win/rect_util.h (right):

https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h...
ui/gfx/win/rect_util.h:88: GFX_EXPORT gfx::Rect ScaleAndPositionRect(const
gfx::Rect& ref_scaled_rect,
On 2016/02/18 17:02:58, sky wrote:
> Thanks for the comments. I think I understand what you're trying to do here
now.
> Why can't you take the top/left corner of unscaled_rect, apply the scale of
ref
> rect to get the origin of the resulting rect. For the size of the resulting
rect
> scale the unscaled_rects size by unscaled_rect_scale_factor. Isn't this
> effectively what this function wants to do?

The sizing is straightforward. The positioning is tricky.

To rephrase, the proposal above is to have (correct me if I'm wrong)...
ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor)

One of the fun parts about MultiDPI is that while the reference rectangle may
have a scale factor of 1x, it can move between unscaled and scaled coordinate
systems.

Consider 3 monitors side-by-side:
[Left 3x], [Middle 1x], [Right 1x]
The left monitor will shrink when we go from unscaled to scaled. This means to
place the middle monitor, we need to know the size of the left monitor and the
scale factor. That's fine.

However, to position the right monitor, we need to figure out how much the
middle monitor moved from unscaled to scaled so that the right monitor can
remain side-by-side in scaled. This means having the scale factor is insufficent
to place the right monitor. We need to have the unscaled and scaled origins of
the middle monitor.

So now we have
ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor,
ref_unscaled.origin(), ref_scaled.origin())


The next case is case #2 in the comment.
If we only consider the reference scale, then the unscaled_rect origin doesn't
move. This means the bottom edge of the reference rectangle is no longer
perpendicular to the left edge of unscaled_rect with sufficient scaling.

This shows up in monitors that are aligned across the bottom:

+------+
| REF  +----+
|  1x  |    |
|      | 2x |   
+------+----+

Since REF 1x basically doesn't change between unscaled and scaled coordinates,
to maintain the bottom alignment of the unscaled_rect, we need to know how it
was positioned along the reference rect.

This led to the need to know the reference rect in both unscaled and scaled
coordinates for positioning.

[robliao, 2016-02-24 00:55:19]

On 2016/02/19 00:26:29, robliao wrote:
> https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.cc
> File ui/gfx/win/rect_util.cc (right):
> 
>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.c...
> ui/gfx/win/rect_util.cc:129: int max_x = std::max(ref.x(), test.x());
> On 2016/02/18 17:02:58, sky wrote:
> > I have a hard time reading this code because max_x implies the biggest
> > x-coordinate, but that's not what this is. It's the biggest left value. How
> > about max_left and max_top? That better matches min_bottom and min_right.
> 
> sgtm. Done. As an aside, the original naming from gfx::Rect was rx, ry, rr,
rb,
> which I had renamed here to [min|max]_[rect property name]
> 
> https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h
> File ui/gfx/win/rect_util.h (right):
> 
>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h...
> ui/gfx/win/rect_util.h:88: GFX_EXPORT gfx::Rect ScaleAndPositionRect(const
> gfx::Rect& ref_scaled_rect,
> On 2016/02/18 17:02:58, sky wrote:
> > Thanks for the comments. I think I understand what you're trying to do here
> now.
> > Why can't you take the top/left corner of unscaled_rect, apply the scale of
> ref
> > rect to get the origin of the resulting rect. For the size of the resulting
> rect
> > scale the unscaled_rects size by unscaled_rect_scale_factor. Isn't this
> > effectively what this function wants to do?
> 
> The sizing is straightforward. The positioning is tricky.
> 
> To rephrase, the proposal above is to have (correct me if I'm wrong)...
> ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor)
> 
> One of the fun parts about MultiDPI is that while the reference rectangle may
> have a scale factor of 1x, it can move between unscaled and scaled coordinate
> systems.
> 
> Consider 3 monitors side-by-side:
> [Left 3x], [Middle 1x], [Right 1x]
> The left monitor will shrink when we go from unscaled to scaled. This means to
> place the middle monitor, we need to know the size of the left monitor and the
> scale factor. That's fine.
> 
> However, to position the right monitor, we need to figure out how much the
> middle monitor moved from unscaled to scaled so that the right monitor can
> remain side-by-side in scaled. This means having the scale factor is
insufficent
> to place the right monitor. We need to have the unscaled and scaled origins of
> the middle monitor.
> 
> So now we have
> ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor,
> ref_unscaled.origin(), ref_scaled.origin())
> 
> 
> The next case is case #2 in the comment.
> If we only consider the reference scale, then the unscaled_rect origin doesn't
> move. This means the bottom edge of the reference rectangle is no longer
> perpendicular to the left edge of unscaled_rect with sufficient scaling.
> 
> This shows up in monitors that are aligned across the bottom:
> 
> +------+
> | REF  +----+
> |  1x  |    |
> |      | 2x |   
> +------+----+
> 
> Since REF 1x basically doesn't change between unscaled and scaled coordinates,
> to maintain the bottom alignment of the unscaled_rect, we need to know how it
> was positioned along the reference rect.
> 
> This led to the need to know the reference rect in both unscaled and scaled
> coordinates for positioning.

To update the review, oshima and I met and here's a summary of the discussion:
* ChromeOS in the past did not handle systems with more than 2 displays, which
is why display layout hasn't really been a problem for CrOS.
* oshima is working on a similar change for ash for 3+ display support.
  Inputs = Display Tree + Physical Pixels + Scale Factor
  Output = DIP displays in the correct position and size
* The code today is able to generate this tree relationship anchored to the
primary display.

oshima is going to land his change first. Then we'll need to perform some
refactoring to share the code since that code will initially live in ash.
After that, then the Windows code can consume the common layout code.

oshima and robliao will also sync up tomorrow on the further mechanics of
landing this.

[sky, 2016-02-24 17:11:35]

Thanks for the update! I'm glad to hear we can share code between the
two environments.

On Tue, Feb 23, 2016 at 4:55 PM,  <robliao@chromium.org> wrote:
> On 2016/02/19 00:26:29, robliao wrote:
>>
>>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.cc
>> File ui/gfx/win/rect_util.cc (right):
>>
>>
>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.c...
>> ui/gfx/win/rect_util.cc:129: int max_x = std::max(ref.x(), test.x());
>> On 2016/02/18 17:02:58, sky wrote:
>> > I have a hard time reading this code because max_x implies the biggest
>> > x-coordinate, but that's not what this is. It's the biggest left value.
>> > How
>> > about max_left and max_top? That better matches min_bottom and
>> > min_right.
>>
>> sgtm. Done. As an aside, the original naming from gfx::Rect was rx, ry,
>> rr,
> rb,
>> which I had renamed here to [min|max]_[rect property name]
>>
>>
>>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h
>> File ui/gfx/win/rect_util.h (right):
>>
>>
>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h...
>> ui/gfx/win/rect_util.h:88: GFX_EXPORT gfx::Rect ScaleAndPositionRect(const
>> gfx::Rect& ref_scaled_rect,
>> On 2016/02/18 17:02:58, sky wrote:
>> > Thanks for the comments. I think I understand what you're trying to do
>> > here
>> now.
>> > Why can't you take the top/left corner of unscaled_rect, apply the scale
>> > of
>> ref
>> > rect to get the origin of the resulting rect. For the size of the
>> > resulting
>> rect
>> > scale the unscaled_rects size by unscaled_rect_scale_factor. Isn't this
>> > effectively what this function wants to do?
>>
>> The sizing is straightforward. The positioning is tricky.
>>
>> To rephrase, the proposal above is to have (correct me if I'm wrong)...
>> ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor)
>>
>> One of the fun parts about MultiDPI is that while the reference rectangle
>> may
>> have a scale factor of 1x, it can move between unscaled and scaled
>> coordinate
>> systems.
>>
>> Consider 3 monitors side-by-side:
>> [Left 3x], [Middle 1x], [Right 1x]
>> The left monitor will shrink when we go from unscaled to scaled. This
>> means to
>> place the middle monitor, we need to know the size of the left monitor and
>> the
>> scale factor. That's fine.
>>
>> However, to position the right monitor, we need to figure out how much the
>> middle monitor moved from unscaled to scaled so that the right monitor can
>> remain side-by-side in scaled. This means having the scale factor is
> insufficent
>> to place the right monitor. We need to have the unscaled and scaled
>> origins of
>> the middle monitor.
>>
>> So now we have
>> ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor,
>> ref_unscaled.origin(), ref_scaled.origin())
>>
>>
>> The next case is case #2 in the comment.
>> If we only consider the reference scale, then the unscaled_rect origin
>> doesn't
>> move. This means the bottom edge of the reference rectangle is no longer
>> perpendicular to the left edge of unscaled_rect with sufficient scaling.
>>
>> This shows up in monitors that are aligned across the bottom:
>>
>> +------+
>> | REF +----+
>> | 1x | |
>> | | 2x |
>> +------+----+
>>
>> Since REF 1x basically doesn't change between unscaled and scaled
>> coordinates,
>> to maintain the bottom alignment of the unscaled_rect, we need to know how
>> it
>> was positioned along the reference rect.
>>
>> This led to the need to know the reference rect in both unscaled and
>> scaled
>> coordinates for positioning.
>
> To update the review, oshima and I met and here's a summary of the
> discussion:
> * ChromeOS in the past did not handle systems with more than 2 displays,
> which
> is why display layout hasn't really been a problem for CrOS.
> * oshima is working on a similar change for ash for 3+ display support.
> Inputs = Display Tree + Physical Pixels + Scale Factor
> Output = DIP displays in the correct position and size
> * The code today is able to generate this tree relationship anchored to the
> primary display.
>
> oshima is going to land his change first. Then we'll need to perform some
> refactoring to share the code since that code will initially live in ash.
> After that, then the Windows code can consume the common layout code.
>
> oshima and robliao will also sync up tomorrow on the further mechanics of
> landing this.
>
> https://codereview.chromium.org/1679393002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robliao, 2016-03-16 19:31:59]

On 2016/02/24 17:11:35, sky wrote:
> Thanks for the update! I'm glad to hear we can share code between the
> two environments.
> 
> On Tue, Feb 23, 2016 at 4:55 PM,  <mailto:robliao@chromium.org> wrote:
> > On 2016/02/19 00:26:29, robliao wrote:
> >>
> >>
> https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.cc
> >> File ui/gfx/win/rect_util.cc (right):
> >>
> >>
> >
>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.c...
> >> ui/gfx/win/rect_util.cc:129: int max_x = std::max(ref.x(), test.x());
> >> On 2016/02/18 17:02:58, sky wrote:
> >> > I have a hard time reading this code because max_x implies the biggest
> >> > x-coordinate, but that's not what this is. It's the biggest left value.
> >> > How
> >> > about max_left and max_top? That better matches min_bottom and
> >> > min_right.
> >>
> >> sgtm. Done. As an aside, the original naming from gfx::Rect was rx, ry,
> >> rr,
> > rb,
> >> which I had renamed here to [min|max]_[rect property name]
> >>
> >>
> >>
> https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h
> >> File ui/gfx/win/rect_util.h (right):
> >>
> >>
> >
>
https://codereview.chromium.org/1679393002/diff/220001/ui/gfx/win/rect_util.h...
> >> ui/gfx/win/rect_util.h:88: GFX_EXPORT gfx::Rect ScaleAndPositionRect(const
> >> gfx::Rect& ref_scaled_rect,
> >> On 2016/02/18 17:02:58, sky wrote:
> >> > Thanks for the comments. I think I understand what you're trying to do
> >> > here
> >> now.
> >> > Why can't you take the top/left corner of unscaled_rect, apply the scale
> >> > of
> >> ref
> >> > rect to get the origin of the resulting rect. For the size of the
> >> > resulting
> >> rect
> >> > scale the unscaled_rects size by unscaled_rect_scale_factor. Isn't this
> >> > effectively what this function wants to do?
> >>
> >> The sizing is straightforward. The positioning is tricky.
> >>
> >> To rephrase, the proposal above is to have (correct me if I'm wrong)...
> >> ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor)
> >>
> >> One of the fun parts about MultiDPI is that while the reference rectangle
> >> may
> >> have a scale factor of 1x, it can move between unscaled and scaled
> >> coordinate
> >> systems.
> >>
> >> Consider 3 monitors side-by-side:
> >> [Left 3x], [Middle 1x], [Right 1x]
> >> The left monitor will shrink when we go from unscaled to scaled. This
> >> means to
> >> place the middle monitor, we need to know the size of the left monitor and
> >> the
> >> scale factor. That's fine.
> >>
> >> However, to position the right monitor, we need to figure out how much the
> >> middle monitor moved from unscaled to scaled so that the right monitor can
> >> remain side-by-side in scaled. This means having the scale factor is
> > insufficent
> >> to place the right monitor. We need to have the unscaled and scaled
> >> origins of
> >> the middle monitor.
> >>
> >> So now we have
> >> ScaledRectOrigin(unscaled_rect.origin(), ref_scale_factor,
> >> ref_unscaled.origin(), ref_scaled.origin())
> >>
> >>
> >> The next case is case #2 in the comment.
> >> If we only consider the reference scale, then the unscaled_rect origin
> >> doesn't
> >> move. This means the bottom edge of the reference rectangle is no longer
> >> perpendicular to the left edge of unscaled_rect with sufficient scaling.
> >>
> >> This shows up in monitors that are aligned across the bottom:
> >>
> >> +------+
> >> | REF +----+
> >> | 1x | |
> >> | | 2x |
> >> +------+----+
> >>
> >> Since REF 1x basically doesn't change between unscaled and scaled
> >> coordinates,
> >> to maintain the bottom alignment of the unscaled_rect, we need to know how
> >> it
> >> was positioned along the reference rect.
> >>
> >> This led to the need to know the reference rect in both unscaled and
> >> scaled
> >> coordinates for positioning.
> >
> > To update the review, oshima and I met and here's a summary of the
> > discussion:
> > * ChromeOS in the past did not handle systems with more than 2 displays,
> > which
> > is why display layout hasn't really been a problem for CrOS.
> > * oshima is working on a similar change for ash for 3+ display support.
> > Inputs = Display Tree + Physical Pixels + Scale Factor
> > Output = DIP displays in the correct position and size
> > * The code today is able to generate this tree relationship anchored to the
> > primary display.
> >
> > oshima is going to land his change first. Then we'll need to perform some
> > refactoring to share the code since that code will initially live in ash.
> > After that, then the Windows code can consume the common layout code.
> >
> > oshima and robliao will also sync up tomorrow on the further mechanics of
> > landing this.
> >
> > https://codereview.chromium.org/1679393002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

A preview of what sharing code will look like is available here:
https://codereview.chromium.org/1813493002/

There are two additional changelists required that are not captured by the
preview:
1) Removing JSON dependencies from DisplayLayout and friends
2) Moving DisplayLayout and DisplayLayoutBuilder to ui/gfx/display