Fullscreen Exit Bubble should not elide its label....

Fullscreen Exit Bubble should not elide its label.

When drawing a Label it is important to adjust for
RTL not just when drawing, but also when computing
the size of the string.

Before this change, we'd compute using Font::GetStringWidth
(which uses ::GetTextExtentPoint32 *without* adjusting
the string for RTL) and draw using DoDrawText (which
adjusts for RTL and then draws using ::DrawText).

After this change, we now use SizeStringInt for
computing (so we now compute and draw using the
same ::DrawText function, thereby adjusting the 
string for RTL in both cases).

BUG=17949
TEST=Run chrome with --lang=he, press F11 and
the string in the bubble at the top should not
be elided.


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

[Finnur, 2009-11-02 23:59:26]

Dave, you were the last person to attempt such a change and backed out. I
noticed you were trying to fix something in the About box on some platform, is
there a bug for that?

[Peter Kasting, 2009-11-03 00:05:51]

LGTM.  If any callers call both GetTextSize() and GetHeightForWidth(), maybe we
should have a single call that does both (in addition to or in exchange for the
current functions).

http://codereview.chromium.org/354014/diff/1/2
File views/controls/label.cc (right):

http://codereview.chromium.org/354014/diff/1/2#newcode89
Line 89: // TODO(erikkay) With highlighted_ enabled, should we adjust the size
Nit: Looks like the indenting got unexpectedly horked here.

http://codereview.chromium.org/354014/diff/1/2#newcode267
Line 267: int w = MAXINT, h = 0;
Does |w| need to be MAXINT here?  It looks from SizeStringInt() like you could
just set it to 0?

[Finnur, 2009-11-03 00:31:52]

Fixed indentation problem and uploaded.

http://codereview.chromium.org/354014/diff/1/2
File views/controls/label.cc (right):

http://codereview.chromium.org/354014/diff/1/2#newcode267
Line 267: int w = MAXINT, h = 0;
Yes, it does look that way and that was the first thing I tested. Unfortunately,
it doesn't work. It calculates the label as 12 pixel wide (instead of 183).

[Peter Kasting, 2009-11-03 00:34:43]

On 2009/11/03 00:31:52, Finnur wrote:
> http://codereview.chromium.org/354014/diff/1/2#newcode267
> Line 267: int w = MAXINT, h = 0;
> Yes, it does look that way and that was the first thing I tested.
Unfortunately,
> it doesn't work. It calculates the label as 12 pixel wide (instead of 183).

If possible, please determine how the input value affects the output value and
add some comments to the declaration/implementation, or even better some
defensive code.  This is really nonintuitive.

http://codereview.chromium.org/354014/diff/5001/4002
File views/controls/label.cc (right):

http://codereview.chromium.org/354014/diff/5001/4002#newcode267
Line 267: int w = INT_MAX, h = 0;
Nit: std::numeric_limits<int>::max() is more The C++ Way

[Finnur, 2009-11-03 03:32:21]

I uploaded this after changing it to use std::numeric_limits.

As for the width being set to max, I think it is actually more intuitive than
setting it to 0 and have the internals of Canvas set it to 1 in order for
Windows to calculate some width based on some magic rules. But I asked levin
about this (who wrote that comment originally) and he said... Quote:

Let's say you have a string that looks like this:

"This is my cool string."

If you give this method a width of 0, (it will adjust it to 1 and) then it will
calc the width based on the widest word (likely "string.") and the height will
be based on 
"This
is
my
cool
string."

I think in your case you want the width to be based on "This is my cool string."
and the height to be based on the single line "This is my cool string." In that
case, you probably should pass in MAX_INT for width.

(end quote)

[DaveMoore, 2009-11-03 17:48:14]

LGTM, but we should at least add a comment indicating that it's invalid for
multiline labels...perhaps even check for it in the code.