Fixes GetContainerBounds in tab_contents_view_mac.mm

Fixes GetContainerBounds in tab_contents_view_mac.mm to return the position of the view in screen coordinates.

Currently GetContainerBounds would return origin of (0,0) which is inconsistent with GTK and Windows implementation.

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

[Avi (use Gerrit), 2011-11-12 01:42:00]

First, it's unacceptable to not have a BUG line. If there is no bug currently
filed, file one just to track this.

Second, your code shows a lack of understanding of coordinate conversion.
Whenever dealing with bounds, frames, etc, you must always be conscious of what
coordinate system each variable is in, and convert between them appropriately.

http://codereview.chromium.org/8491062/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/8491062/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents_view_mac.mm:152: NSPoint origin =
[[cocoa_view_.get() window] frame].origin;
This line gets the window origin in global coordinates...

http://codereview.chromium.org/8491062/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents_view_mac.mm:156: NSSize size =
[cocoa_view_.get() frame].size;
...and this line gets the size of the view in its parent coordinates...

http://codereview.chromium.org/8491062/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents_view_mac.mm:157: origin.y = [screen
frame].size.height - origin.y - size.height;
...and this line mixes up values from two totally different objects and their
different coordinate systems...

http://codereview.chromium.org/8491062/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents_view_mac.mm:158: NSRect bounds =
NSMakeRect(origin.x, origin.y, size.width, size.height);
...as does this line. What is GetContainerBounds supposed to return? The
window's bounds? The TCV's? Please do so.

Please familiarize yourself with the -convertXxxx:toView: methods. Use them.

[pkotwicz, 2011-11-14 18:57:46]

I've done the bounds conversion more systematically. I wasn't able to find a
method to convert the size in terms of window coordinates to the size in terms
of screen coordinates.
I am aware of the NSWindow::convertRectToScreen method, however the method does
not seem to be defined in the version of cocoa we are pulling in.

I am flipping the y coordinate to put the returned coordinates in the same
coordinate space as BrowserWindowCocoa::GetBounds()

[Avi (use Gerrit), 2011-11-14 19:18:57]

As I mentioned before, you *must* have a BUG= line in your description.

BTW, window coordinates are guaranteed to be the same size as global
coordinates, so converting the origin and leaving the size alone is correct.

Additional comments below.

http://codereview.chromium.org/8491062/diff/5001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/8491062/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:153: NSRect
bounds_wrt_window = [cocoa_view_.get() convertRect:bounds toView:nil];
bounds_wrt_window is a horrible name. Just use bounds.

You don't need to annotate variable names with their coordinate systems. Just
don't mix/match.

http://codereview.chromium.org/8491062/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:157: window
convertBaseToScreen:bounds_wrt_window.origin];
You can't break there. You break like:

blah blah =
    [xxxx];

http://codereview.chromium.org/8491062/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:158: NSRect
bounds_wrt_screen = NSMakeRect(origin_wrt_screen.x,
horrible name; change

http://codereview.chromium.org/8491062/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:167: *out =
gfx::Rect(NSRectToCGRect(bounds_wrt_screen));
This is a change of functionality. Before, the bounds returned were in view
coordinates. With your new code, the bounds being returned are in global
coordinates. Which is correct? Where is that documented?

If it's not documented anywhere, please do so.

[pkotwicz, 2011-11-14 21:00:20]

Removed variable names with the coordinate system.
Created and added bug ID to description.
Changed description of GetContainerBounds in tab_contents_view.h to have the
same meaning as GetContainerBounds in tab_contents.h.

The method has been implemented in GTK and Windows and Views so that the
coordinates are in screen coordinates. The mac port of chromium doesn't use the
origin which is why this bug has not been uncovered earlier. However, I need the
API to be consistent among all the platforms in order to land
http://codereview.chromium.org/8523022/

[Avi (use Gerrit), 2011-11-14 21:27:14]

lgtm! thanks for hanging in there.

http://codereview.chromium.org/8491062/diff/8001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/8491062/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:160: bounds.size.height);
alternatively:

bounds.origin = [window convertBaseToScreen:bounds.origin]

instead of those two lines :)

http://codereview.chromium.org/8491062/diff/8001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents_view.h (right):

http://codereview.chromium.org/8491062/diff/8001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents_view.h:52: // the tab in the screen
coordinate system.
Nice clarification.