Conditionally draw a bezel below text fields, used for the omnibox....

Conditionally draw a bezel below text fields, used for the omnibox.
BUG=49793
TEST=visual inspection. unit tests shouldn't leak or crash on drawing.

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

[Scott Hess - ex-Googler, 2010-07-21 17:40:47]

is the bezel already on the other items on the trunk, or is that pending?

http://codereview.chromium.org/3042014/diff/1/3
File chrome/browser/cocoa/styled_text_field_cell.h (right):

http://codereview.chromium.org/3042014/diff/1/3#newcode42
chrome/browser/cocoa/styled_text_field_cell.h:42: - (BOOL)shouldDrawBezel;
Once upon a time, I was thinking we should respect -isBezeled.  But our border
is maybe too custom?

http://codereview.chromium.org/3042014/diff/4001/5003
File chrome/browser/cocoa/styled_text_field_cell.mm (right):

http://codereview.chromium.org/3042014/diff/4001/5003#newcode177
chrome/browser/cocoa/styled_text_field_cell.mm:177: NSHeight(cellFrame) - 0.5,
Should probably be NSMaxY(), just in case y != 0.0.  Yeah, so unlikely.

http://codereview.chromium.org/3042014/diff/4001/5003#newcode179
chrome/browser/cocoa/styled_text_field_cell.mm:179: 1.0);
I think the x-axis adjustment would be easier to follow with
  bezelRect = NSInsetRect(bezelRect, radius - 0.5, 0.0);

http://codereview.chromium.org/3042014/diff/4001/5003#newcode180
chrome/browser/cocoa/styled_text_field_cell.mm:180: NSFrameRect(bezelRect);
This doesn't seem right - a single pixel frame around a single-pixel-high rect,
centered between pixels?

[pink (ping after 24hrs), 2010-07-21 18:03:01]

comments addressed.

http://codereview.chromium.org/3042014/diff/1/3
File chrome/browser/cocoa/styled_text_field_cell.h (right):

http://codereview.chromium.org/3042014/diff/1/3#newcode42
chrome/browser/cocoa/styled_text_field_cell.h:42: - (BOOL)shouldDrawBezel;
On 2010/07/21 17:40:48, shess wrote:
> Once upon a time, I was thinking we should respect -isBezeled.  But our border
> is maybe too custom?

I feel ours is too custom to re-use that cocoa property.

http://codereview.chromium.org/3042014/diff/4001/5003
File chrome/browser/cocoa/styled_text_field_cell.mm (right):

http://codereview.chromium.org/3042014/diff/4001/5003#newcode177
chrome/browser/cocoa/styled_text_field_cell.mm:177: NSHeight(cellFrame) - 0.5,
On 2010/07/21 17:40:48, shess wrote:
> Should probably be NSMaxY(), just in case y != 0.0.  Yeah, so unlikely.

Done.

http://codereview.chromium.org/3042014/diff/4001/5003#newcode179
chrome/browser/cocoa/styled_text_field_cell.mm:179: 1.0);
On 2010/07/21 17:40:48, shess wrote:
> I think the x-axis adjustment would be easier to follow with
>   bezelRect = NSInsetRect(bezelRect, radius - 0.5, 0.0);
> 

Done.

http://codereview.chromium.org/3042014/diff/4001/5003#newcode180
chrome/browser/cocoa/styled_text_field_cell.mm:180: NSFrameRect(bezelRect);
On 2010/07/21 17:40:48, shess wrote:
> This doesn't seem right - a single pixel frame around a single-pixel-high
rect,
> centered between pixels?

I don't follow. It works.

[Scott Hess - ex-Googler, 2010-07-21 18:09:01]

LGTM, except using NSFrameRect() to fill a rect.  Do not like.

http://codereview.chromium.org/3042014/diff/4001/5003
File chrome/browser/cocoa/styled_text_field_cell.mm (right):

http://codereview.chromium.org/3042014/diff/4001/5003#newcode180
chrome/browser/cocoa/styled_text_field_cell.mm:180: NSFrameRect(bezelRect);
On 2010/07/21 18:03:01, pink wrote:
> On 2010/07/21 17:40:48, shess wrote:
> > This doesn't seem right - a single pixel frame around a single-pixel-high
rect,
> > centered between pixels?
> 
> I don't follow. It works.

NSFrameRect() is for drawing a single-pixel border inside the bounds of a
rectangle.  Using it here implies that you're drawing a frame, but the rectangle
being only a single pixel high implies that you probably aren't drawing a frame.
 If you mean NSRectFill(), use NSRectFill().

[pink (ping after 24hrs), 2010-07-21 18:27:59]

On Wed, Jul 21, 2010 at 2:09 PM,  <shess@chromium.org> wrote:
> LGTM, except using NSFrameRect() to fill a rect.  Do not like.

Fixed, changed to NSRectFill().

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com