Adds Get(Root)WindowRect handlers for the Mac....

Adds Get(Root)WindowRect handlers for the Mac.

Fixes http://crbug.com/8829 .


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11980

[pink (ping after 24hrs), 2009-03-18 15:59:41]

http://codereview.chromium.org/42336/diff/31/1007
File chrome/browser/renderer_host/resource_message_filter_mac.mm (right):

http://codereview.chromium.org/42336/diff/31/1007#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
2009

http://codereview.chromium.org/42336/diff/31/1007#newcode18
Line 18: NSPoint global_upper_left = [[view window]
convertBaseToScreen:upper_left];
this is still in cocoa coordinates (bottom-left relative), right? Doesn't the
calling code expect a top-left relative rect? If not, maybe you should put in a
comment?

http://codereview.chromium.org/42336/diff/31/1007#newcode32
Line 32: *rect = gfx::Rect(NSRectToCGRect(frame));
same here.

[Avi (use Gerrit), 2009-03-18 16:43:45]

http://codereview.chromium.org/42336/diff/31/1007
File chrome/browser/renderer_host/resource_message_filter_mac.mm (right):

http://codereview.chromium.org/42336/diff/31/1007#newcode18
Line 18: NSPoint global_upper_left = [[view window]
convertBaseToScreen:upper_left];
On 2009/03/18 15:59:42, pink wrote:
> this is still in cocoa coordinates (bottom-left relative), right?

No, this is explicitly doing a conversion to top-left origin coordinates, thus
the variable named |upper_left|. Please re-read the code.

http://codereview.chromium.org/42336/diff/31/1007#newcode32
Line 32: *rect = gfx::Rect(NSRectToCGRect(frame));
On 2009/03/18 15:59:42, pink wrote:
> same here.

This is different. I believed screen coordinates were upper-left origin, but
after some research it appears that I was mistaken. Fixing.

[Avi (use Gerrit), 2009-03-18 16:58:57]

On 2009/03/18 16:43:45, Avi wrote:
> No, this is explicitly doing a conversion to top-left origin coordinates

Bzzt, wrong. Bad Avi! (Thx, pink.)

Updated.

[pink (ping after 24hrs), 2009-03-18 17:06:13]

LGTM with 2 questions.

http://codereview.chromium.org/42336/diff/34/40
File chrome/browser/renderer_host/resource_message_filter_mac.mm (right):

http://codereview.chromium.org/42336/diff/34/40#newcode14
Line 14: gfx::Rect NSRectToRect(const NSRect rect, NSScreen* screen) {
is it worth putting into the name something about the conversion? I know this
file is tiny and it's easy to read the function comment, but just reading the
calling func itself doesn't imply that there is an origin conversion going on.
Implicit things like that always raise flags to me. Maybe just a comment on the
line that calls it stating that an origin conversion is occurring? *shrug*

http://codereview.chromium.org/42336/diff/34/40#newcode27
Line 27: NSRect bounds = [view bounds];
is it worth being defensive about |view| being nil here? That would cause
|bounds| to be full of garbage, which may cause bigger problems up the call
stack.

[Avi (use Gerrit), 2009-03-18 17:10:34]

http://codereview.chromium.org/42336/diff/34/40
File chrome/browser/renderer_host/resource_message_filter_mac.mm (right):

http://codereview.chromium.org/42336/diff/34/40#newcode14
Line 14: gfx::Rect NSRectToRect(const NSRect rect, NSScreen* screen) {
On 2009/03/18 17:06:13, pink wrote:
> is it worth putting into the name something about the conversion?

Hmmm. I always thought that gfx::Rect was always an UL-based origin rect, so
that converting to it meant origin conversion. See BaseView where I do the same
thing.

http://codereview.chromium.org/42336/diff/34/40#newcode27
Line 27: NSRect bounds = [view bounds];
On 2009/03/18 17:06:13, pink wrote:
> is it worth being defensive about |view| being nil here?

After a DCHECK? You're thinking about the other changelist? I'd be more inclined
to track down why that's happening rather than pull the DCHECK.