Fix status bubble shadow being opaque, which made the bubble appear to be off...

Fix status bubble shadow being opaque, which made the bubble appear to be off by one pixel.  This reverts Brett's fix for issue 8651 and instead fixes that issue by ensuring the text bounding box does not overlap the status bubble shadow.  This also makes kShadowThickness (renamed for consistency with the browser frame) a class-scoped constant so the browser view can access it, eliminating a magic number.

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

[brettw, 2009-04-13 04:06:27]

http://codereview.chromium.org/67059/diff/1/2
File chrome/browser/views/status_bubble_views.cc (right):

http://codereview.chromium.org/67059/diff/1/2#newcode46
Line 46: static const int kTextHorizPadding = 1;
You changed the meaning of these things, I assume this included the border
before or something? Can you document what it means now? I can't really review
the math below without knowing that.

http://codereview.chromium.org/67059/diff/1/2#newcode410
Line 410: text_width, height - (kShadowThickness * 2) - 1);
Where did the +1 come from?

I would keep the old wrapping. It's easier to read with all the oerations in
there. Plus, I know of some people on our team that will always expand your
style to the old style, and I've decided that neither should happen unless it's
clearly better one way.

http://codereview.chromium.org/67059/diff/1/2#newcode414
Line 414: body_bounds.width(), body_bounds.height());
These two changes to DrawStringInt are even less useful since you didn't change
any code. I would put them back.

[Peter Kasting, 2009-04-13 04:45:01]

http://codereview.chromium.org/67059/diff/1/2
File chrome/browser/views/status_bubble_views.cc (right):

http://codereview.chromium.org/67059/diff/1/2#newcode46
Line 46: static const int kTextHorizPadding = 1;
On 2009/04/13 04:06:28, brettw wrote:
> You changed the meaning of these things, I assume this included the border
> before or something?

Yes, now it doesn't.  Also, an extra -1 has been taken from the padding value
since we manually subtract an additional 1 below.

I don't know how to write a comment that adds any value to the code here :(

http://codereview.chromium.org/67059/diff/1/2#newcode410
Line 410: text_width, height - (kShadowThickness * 2) - 1);
On 2009/04/13 04:06:28, brettw wrote:
> Where did the +1 come from?

What do you mean?  The +1 has always been there.  That's for the "text
highlight" (offset text underneath).

> I would keep the old wrapping. It's easier to read with all the oerations in
> there.

OK, will fix; I thought this was preferred team style.

>Plus, I know of some people on our team that will always expand your
> style to the old style,

Those people should be shot >:(

http://codereview.chromium.org/67059/diff/1/2#newcode414
Line 414: body_bounds.width(), body_bounds.height());
On 2009/04/13 04:06:28, brettw wrote:
> These two changes to DrawStringInt are even less useful since you didn't
change
> any code. I would put them back.

Sure, not a problem

[brettw, 2009-04-13 20:12:28]

On 2009/04/13 04:45:01, pkasting wrote:
> http://codereview.chromium.org/67059/diff/1/2
> File chrome/browser/views/status_bubble_views.cc (right):
> 
> http://codereview.chromium.org/67059/diff/1/2#newcode46
> Line 46: static const int kTextHorizPadding = 1;
> On 2009/04/13 04:06:28, brettw wrote:
> > You changed the meaning of these things, I assume this included the border
> > before or something?
> 
> Yes, now it doesn't.  Also, an extra -1 has been taken from the padding value
> since we manually subtract an additional 1 below.
> 
> I don't know how to write a comment that adds any value to the code here :(

"The -1 gives us more padding on the right side to keep the text from hitting
the wall and exploding."


> http://codereview.chromium.org/67059/diff/1/2#newcode410
> Line 410: text_width, height - (kShadowThickness * 2) - 1);
> On 2009/04/13 04:06:28, brettw wrote:
> > Where did the +1 come from?
> 
> What do you mean?  The +1 has always been there.  That's for the "text
> highlight" (offset text underneath).
> 
> > I would keep the old wrapping. It's easier to read with all the oerations in
> > there.
> 
> OK, will fix; I thought this was preferred team style.

Either way is OK, I think we should generally try to do the one that's most
clear, which depends on the situatio.

Brett

[Peter Kasting, 2009-04-13 20:27:52]

On 2009/04/13 20:12:28, brettw wrote:
> On 2009/04/13 04:45:01, pkasting wrote:
> > I don't know how to write a comment that adds any value to the code here :(
> 
> "The -1 gives us more padding on the right side to keep the text from hitting
> the wall and exploding."

CAP'N, SHE'S GONNA BLOW

[Peter Kasting, 2009-04-14 17:18:06]

New snapshot up.  I tried to write some more informative comments on those
constants.

[brettw, 2009-04-14 17:27:51]

LGTM