Pixel perfection on tabstrip for non-Aero frame. This gets both ends of the ...

Pixel perfection on tabstrip for non-Aero frame.  This gets both ends of the tabstrip positioned correctly for normal and incognito windows, in both restored and maximized modes.The chief change was to correctly transform the bounds returned by the non-client view into the BrowserView's coordinate system, as the Views system expected.  To make this not break the world I then had to actually return the right coordinates in the non-client view :).  Updated comments on function declarations to hopefully match reality.For convenience this also introduces a function to get the horizontal border size and cleans up existing code to use it.BUG=5054
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=7976

[sky, 2009-01-13 21:25:48]

Other than the Rect change, everything else LGTM.

http://codereview.chromium.org/18019/diff/1/2
File base/gfx/rect.h (right):

http://codereview.chromium.org/18019/diff/1/2#newcode65
Line 65: gfx::Point& origin() { return origin_; }
I'm not to keen on exposing the implementation like this, but if Brett or Ben is
OK with it, some am I.

[Peter Kasting, 2009-01-13 21:29:28]

http://codereview.chromium.org/18019/diff/1/2
File base/gfx/rect.h (right):

http://codereview.chromium.org/18019/diff/1/2#newcode65
Line 65: gfx::Point& origin() { return origin_; }
On 2009/01/13 21:25:48, sky wrote:
> I'm not to keen on exposing the implementation like this,

Why is that?  This is just syntactic sugar around the two existing
origin-related functions, since we already allow all callers to get and set the
origin.

Without doing this, in a couple of places (both old code and new) we need to do:
Point p = x.origin();
p.transform();
x.set_origin(p);

I don't see why this is a desirable pattern.  It caused an error I spent a half
hour tracking down, since I originally wrote code like the above that didn't
make the final x.set_origin(p) call because I didn't see that I used |x| much
later in the function.  Shorter, clearer, less error-prone code seems like a
plus :)

[Peter Kasting, 2009-01-13 21:54:12]

Take another look.

[sky, 2009-01-13 22:00:39]

Consider updating the docs of GetBoundsForTabStrip too. It's still confusing to
read them then look at your change.

http://codereview.chromium.org/18019/diff/205/15
File chrome/browser/views/frame/opaque_non_client_view.cc (right):

http://codereview.chromium.org/18019/diff/205/15#newcode825
Line 825: &toolbar_bounds.origin());
Don't you need to change this one back?

[Peter Kasting, 2009-01-13 22:09:30]

On 2009/01/13 22:00:39, sky wrote:
> Consider updating the docs of GetBoundsForTabStrip too. It's still confusing
to
> read them then look at your change.

Updated declaration comments in various files.  The Aero one probably doesn't
match what the code there actually does but I'll fix that soon.

> Line 825: &toolbar_bounds.origin());
> Don't you need to change this one back?

Yes, caught that before your comment actually :)

[sky, 2009-01-13 22:21:14]

LGTM