[Mac] Add gfx::ScreenPoint[To|From]NSPoint.

[Mac] Add gfx::ScreenPoint[To|From]NSPoint.

Similarly to ScreenRect[To|From]NSRect, we want to avoid a proliferation of the
following patterns for doing this conversion:

gfx::Point point(ns_point.x,
                 NSMaxY([[[NSScreen screens] objectAtIndex:0] frame]) - ns_point.y);

Or alternatively, creating an NSRect, converting with gfx::ScreenRectFromNSRect(),
and taking the origin.

BUG=None

Committed: https://crrev.com/148c98707491251276ec2380e269e1b7f0edc699
Cr-Commit-Position: refs/heads/master@{#342914}

[jackhou1, 2015-08-11 03:34:15]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2015-08-11 04:01:01]

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
File ui/gfx/mac/coordinate_conversion.h (right):

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
CL description nits:

"Add gfx::ScreenPoint..."

Then, it needs a brief rationale. Like, "We want to avoid a proliferation of the
`[NSScreen screens] objectAtIndex:0...` pattern."

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
File ui/gfx/mac/coordinate_conversion.mm (right):

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:9: #include "ui/gfx/geometry/rect.h"
nit: #include point?

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:23: NSRect ScreenRectToNSRect(const
gfx::Rect& rect) {
nit: remove gfx::

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:31: return gfx::Rect(rect.origin.x,
nit: remove gfx::

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:37: NSPoint ScreenPointToNSPoint(const
gfx::Point& point) {
nit: remove gfx::

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:41: gfx::Point ScreenPointFromNSPoint(const
NSPoint& point) {
nit: remove gfx::

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:42: return gfx::Point(point.x,
PrimaryDisplayHeight() - point.y);
nit: remove gfx::

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion_unittest.mm:109:
TEST_F(MacCoordinateConversionTest, ScreenPointToFromNSPoint) {
nit: boring comment. I guess something like

// Test point conversions using the same setup as ScreenRectToFromNSRect, but
// using only the origin.

[tapted, 2015-08-11 04:01:15]

oh and lgtm after all those nits

[jackhou1, 2015-08-11 06:40:56]

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
File ui/gfx/mac/coordinate_conversion.h (right):

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2015/08/11 04:01:01, tapted wrote:
> CL description nits:
> 
> "Add gfx::ScreenPoint..."
> 
> Then, it needs a brief rationale. Like, "We want to avoid a proliferation of
the
> `[NSScreen screens] objectAtIndex:0...` pattern."

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
File ui/gfx/mac/coordinate_conversion.mm (right):

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:9: #include "ui/gfx/geometry/rect.h"
On 2015/08/11 04:01:01, tapted wrote:
> nit: #include point? 

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:23: NSRect ScreenRectToNSRect(const
gfx::Rect& rect) {
On 2015/08/11 04:01:01, tapted wrote:
> nit: remove gfx::

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:31: return gfx::Rect(rect.origin.x,
On 2015/08/11 04:01:01, tapted wrote:
> nit: remove gfx::

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:37: NSPoint ScreenPointToNSPoint(const
gfx::Point& point) {
On 2015/08/11 04:01:01, tapted wrote:
> nit: remove gfx::

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:41: gfx::Point ScreenPointFromNSPoint(const
NSPoint& point) {
On 2015/08/11 04:01:01, tapted wrote:
> nit: remove gfx::

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion.mm:42: return gfx::Point(point.x,
PrimaryDisplayHeight() - point.y);
On 2015/08/11 04:01:01, tapted wrote:
> nit: remove gfx::

Done.

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/1287573002/diff/1/ui/gfx/mac/coordinate_conve...
ui/gfx/mac/coordinate_conversion_unittest.mm:109:
TEST_F(MacCoordinateConversionTest, ScreenPointToFromNSPoint) {
On 2015/08/11 04:01:01, tapted wrote:
> nit: boring comment. I guess something like
> 
> // Test point conversions using the same setup as ScreenRectToFromNSRect, but
> // using only the origin.

Done.

[jackhou1, 2015-08-11 06:41:47]

jackhou@chromium.org changed reviewers:
+ msw@chromium.org

[jackhou1, 2015-08-11 06:41:47]

msw, please review for OWNERS

[msw, 2015-08-11 17:41:48]

lgtm with a nit and a q.

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
File ui/gfx/mac/coordinate_conversion.mm (right):

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
ui/gfx/mac/coordinate_conversion.mm:16: // The height of the primary display,
which OSX defines as the monitor with the
q: Does this really support extended displays as the test seems to check? I
guess it gives off-primary-screen coords using the origin of the primary screen?
Does that really work? (just curious)

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
ui/gfx/mac/coordinate_conversion_unittest.mm:88: EXPECT_EQ(gfx_rect,
ScreenRectFromNSRect(ns_rect));
nit: I'm not strongly opinionated here, but some folks prefer to compare the
strings so any failures are more legible.

[jackhou1, 2015-08-11 22:01:12]

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
File ui/gfx/mac/coordinate_conversion.mm (right):

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
ui/gfx/mac/coordinate_conversion.mm:16: // The height of the primary display,
which OSX defines as the monitor with the
On 2015/08/11 17:41:48, msw wrote:
> q: Does this really support extended displays as the test seems to check? I
> guess it gives off-primary-screen coords using the origin of the primary
screen?
> Does that really work? (just curious)

Yeah, this is the same for rects and is how we position windows on those
screens. (I.e negative position coordinates)

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
ui/gfx/mac/coordinate_conversion_unittest.mm:88: EXPECT_EQ(gfx_rect,
ScreenRectFromNSRect(ns_rect));
On 2015/08/11 17:41:48, msw wrote:
> nit: I'm not strongly opinionated here, but some folks prefer to compare the
> strings so any failures are more legible.

gfx::Rect gets pretty good failure messages these days. It looks like this:

Value of: ScreenRectFromNSRect(ns_rect)
  Actual: 0,0 50x50
Expected: gfx_rect
Which is: 1,2 40x40

[jackhou1, 2015-08-11 22:01:22]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-08-11 22:01:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1287573002/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-08-11 22:01:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287573002/20001

[msw, 2015-08-11 22:02:02]

lgtm, thanks for the explanations.

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
File ui/gfx/mac/coordinate_conversion.mm (right):

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
ui/gfx/mac/coordinate_conversion.mm:16: // The height of the primary display,
which OSX defines as the monitor with the
On 2015/08/11 22:01:12, jackhou1 wrote:
> On 2015/08/11 17:41:48, msw wrote:
> > q: Does this really support extended displays as the test seems to check? I
> > guess it gives off-primary-screen coords using the origin of the primary
> screen?
> > Does that really work? (just curious)
> 
> Yeah, this is the same for rects and is how we position windows on those
> screens. (I.e negative position coordinates)

Acknowledged.

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/1287573002/diff/20001/ui/gfx/mac/coordinate_c...
ui/gfx/mac/coordinate_conversion_unittest.mm:88: EXPECT_EQ(gfx_rect,
ScreenRectFromNSRect(ns_rect));
On 2015/08/11 22:01:12, jackhou1 wrote:
> On 2015/08/11 17:41:48, msw wrote:
> > nit: I'm not strongly opinionated here, but some folks prefer to compare the
> > strings so any failures are more legible.
> 
> gfx::Rect gets pretty good failure messages these days. It looks like this:
> 
> Value of: ScreenRectFromNSRect(ns_rect)
>   Actual: 0,0 50x50
> Expected: gfx_rect
> Which is: 1,2 40x40

Acknowledged.

[commit-bot: I haz the power, 2015-08-11 22:06:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-11 22:07:29]

Patchset 2 (id:??) landed as
https://crrev.com/148c98707491251276ec2380e269e1b7f0edc699
Cr-Commit-Position: refs/heads/master@{#342914}