Fix ScreenCapturerMac handling of secondary configurations.

Fix ScreenCapturerMac handling of secondary display configurations.

- Fix bugs in translating capture regions into display coordinates.
- Fix MacDeskopConfiguration to support inverse-Cartesian coordinates.

BUG=175261, 174090
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181885

[Wez, 2013-02-09 02:01:01]

PTAL

[Wez, 2013-02-09 02:16:39]

+sergeyu@ for media/../screen/ OWNERS.

[Wez, 2013-02-11 21:49:38]

On 2013/02/09 02:16:39, Wez wrote:
> +sergeyu@ for media/../screen/ OWNERS.

Ping.

[Jamie, 2013-02-11 22:12:44]

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.mm (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:33: // the specified
|reference| coordinates.
It's not clear to me from this comment what this function does. Can you clarify
it please?

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:108: // their
coordinates adjusting.
Why only secondary monitors?

[Wez, 2013-02-11 22:56:11]

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.mm (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:33: // the specified
|reference| coordinates.
On 2013/02/11 22:12:44, Jamie wrote:
> It's not clear to me from this comment what this function does. Can you
clarify
> it please?

Done.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:108: // their
coordinates adjusting.
On 2013/02/11 22:12:44, Jamie wrote:
> Why only secondary monitors?

The coordinate space is primary-monitor-relative.  Clarified.

[Sergey Ulanov, 2013-02-11 22:57:20]

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.h (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.h:45: enum Origin {
BottomLeftOrigin, TopLeftOrigin };
nit: types should be defined before the constructor.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.mm (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:54: [screen
respondsToSelector:@selector(convertRectToBacking:)]) {
nit: this line was indented correctly.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:68: : id(0),
nit: this line should be indented 4 spaces and the 3 following lines - 6.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:110:
InvertRectYOrigin(desktop_config.displays[0].bounds,
This doesn't look right to me. why invert bounds rect only for the first
display?
Shouldn't this be "display_config.bounds"?

https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
File media/video/capture/screen/mac/desktop_configuration.h (right):

https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
media/video/capture/screen/mac/desktop_configuration.h:46: MEDIA_EXPORT static
MacDesktopConfiguration GetCurrent(Origin origin);
Not sure if we need |origin| - this function is always called with
TopLeftOrigin.

[Wez, 2013-02-11 23:12:03]

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.h (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.h:45: enum Origin {
BottomLeftOrigin, TopLeftOrigin };
On 2013/02/11 22:57:20, sergeyu wrote:
> nit: types should be defined before the constructor.

Done.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.mm (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:54: [screen
respondsToSelector:@selector(convertRectToBacking:)]) {
On 2013/02/11 22:57:20, sergeyu wrote:
> nit: this line was indented correctly.

Done.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:68: : id(0),
On 2013/02/11 22:57:20, sergeyu wrote:
> nit: this line should be indented 4 spaces and the 3 following lines - 6.

Done.

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:110:
InvertRectYOrigin(desktop_config.displays[0].bounds,
On 2013/02/11 22:57:20, sergeyu wrote:
> This doesn't look right to me. why invert bounds rect only for the first
> display?
> Shouldn't this be "display_config.bounds"?

The first parameter is the coordinate scheme about which to invert, the second
is the in/out rect to invert.

[Wez, 2013-02-11 23:14:45]

https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
File media/video/capture/screen/mac/desktop_configuration.h (right):

https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
media/video/capture/screen/mac/desktop_configuration.h:46: MEDIA_EXPORT static
MacDesktopConfiguration GetCurrent(Origin origin);
On 2013/02/11 22:57:20, sergeyu wrote:
> Not sure if we need |origin| - this function is always called with
> TopLeftOrigin.

Mac's Cocoa uses Cartesian (bottom-left), so I could imagine uses of this helper
class that just return those coordinates.  That said, I have no strong feels
about keeping that option.

[Sergey Ulanov, 2013-02-11 23:16:02]

lgtm

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
File media/video/capture/screen/mac/desktop_configuration.mm (right):

https://chromiumcodereview.appspot.com/12221103/diff/1/media/video/capture/sc...
media/video/capture/screen/mac/desktop_configuration.mm:110:
InvertRectYOrigin(desktop_config.displays[0].bounds,
On 2013/02/11 23:12:03, Wez wrote:
> On 2013/02/11 22:57:20, sergeyu wrote:
> > This doesn't look right to me. why invert bounds rect only for the first
> > display?
> > Shouldn't this be "display_config.bounds"?
> 
> The first parameter is the coordinate scheme about which to invert, the second
> is the in/out rect to invert.

Ah, I see. Thanks for clarifying.

https://chromiumcodereview.appspot.com/12221103/diff/3004/media/video/capture...
File media/video/capture/screen/mac/desktop_configuration.h (right):

https://chromiumcodereview.appspot.com/12221103/diff/3004/media/video/capture...
media/video/capture/screen/mac/desktop_configuration.h:48: MEDIA_EXPORT static
MacDesktopConfiguration GetCurrent(Origin origin);
Do we really need |origin| here? This function is always called with
TopLeftOrigin.

[Sergey Ulanov, 2013-02-11 23:18:58]

On Mon, Feb 11, 2013 at 3:14 PM, <wez@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/12221103/diff/**
>
7001/media/video/capture/**screen/mac/desktop_**configuration.h<https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture/screen/mac/desktop_configuration.h>
> File media/video/capture/screen/**mac/desktop_configuration.h (right):
>
> https://chromiumcodereview.**appspot.com/12221103/diff/**
>
7001/media/video/capture/**screen/mac/desktop_**configuration.h#newcode46<https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture/screen/mac/desktop_configuration.h#newcode46>
> media/video/capture/screen/**mac/desktop_configuration.h:**46:
> MEDIA_EXPORT
> static MacDesktopConfiguration GetCurrent(Origin origin);
> On 2013/02/11 22:57:20, sergeyu wrote:
>
>> Not sure if we need |origin| - this function is always called with
>> TopLeftOrigin.
>>
>
> Mac's Cocoa uses Cartesian (bottom-left), so I could imagine uses of
> this helper class that just return those coordinates.  That said, I have
> no strong feels about keeping that option.
>

ok, I don't mind keeping it.


>
>
https://chromiumcodereview.**appspot.com/12221103/<https://chromiumcodereview...
>

[Jamie, 2013-02-11 23:56:15]

lgtm

https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
File media/video/capture/screen/mac/desktop_configuration.mm (right):

https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
media/video/capture/screen/mac/desktop_configuration.mm:38: rect->width(),
rect->height());
As discussed, please remove top() from the equation.

[Wez, 2013-02-12 01:38:53]

On 2013/02/11 23:56:15, Jamie wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
> File media/video/capture/screen/mac/desktop_configuration.mm (right):
> 
>
https://chromiumcodereview.appspot.com/12221103/diff/7001/media/video/capture...
> media/video/capture/screen/mac/desktop_configuration.mm:38: rect->width(),
> rect->height());
> As discussed, please remove top() from the equation.

Based on our discussion off-line about misleading use of "Cartesian", I've
re-phrased this as bottom-up vs top-down coordinates - happy to follow-up w/ a
CL to tweak that if it's still too confusing!

[commit-bot: I haz the power, 2013-02-12 01:41:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12221103/5002

[commit-bot: I haz the power, 2013-02-12 06:34:52]

Change committed as 181885