Add line height setting to views::Label & use it for notifications.

Add line height setting to views::Label & use it for notifications.

This fixes the line spacing of notification so it's 18px on all
platforms (instead of the previous 17px on Chrome OS and 15px on
Windows).

To do this, support for a line height property was added to the
BoundedLabel that NotificationView uses, to the Label that BoundedLabel
uses, and to the Canvas that Label uses.

BUG=225871
R=mukai@chromium.org,msw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195076

[dharcourt, 2013-04-17 07:26:31]

This change list intends to add settable line height support to views' Label and
to the Canvas it uses to draw its text. This line height support is needed for
notifications.

This is a preliminary review to flush out issues and get direction before coding
gets too far. Please let me know if you see anything wrong with this approach or
if you have suggestions of things to change or other avenues to pursue.

Mike, ui/gfx files are for your review. Note that I haven't implemented any
support for this in canvas_mac.cc. Please let me know if you have suggestions of
a direction to go in for that (for example, feel free to let me know if we don't
need to implement this now as notifications doesn't use views and gfx on Mac
:-).

Sadrul, files in ui/views are for your review.

Jun, ui/message_center files are for you.

Thanks for the review!

    - Charles

[Jun Mukai, 2013-04-17 17:08:57]

lgtm with nits, but you have to wait for replies from msw and sadrul. thanks!

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_unittest.cc
File ui/gfx/canvas_unittest.cc (right):

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_unittest.cc#new...
ui/gfx/canvas_unittest.cc:50: // Line height only supported on SKIA.
Not so sure but it seems to be written as 'Skia', not full capital :)

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_unittest.cc#new...
ui/gfx/canvas_unittest.cc:51: #if defined(OS_CHROMEOS)
build/common.gypi says that canvas_skia.cc is used for all platforms except for
MAC and ANDROID. Can you change the condition to follow that?

[dharcourt, 2013-04-17 17:52:21]

Jun, thanks for your comment, I implemented your fixes.

Mike, I (finally) read the comments at the top of canvas_mac and it seems clear
that if it doesn't support multi-line it can't support line heights, so I've
updated the comments in that file and this change list may be closer to final
than I thought.

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_mac.mm
File ui/gfx/canvas_mac.mm (right):

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_mac.mm#newcode43
ui/gfx/canvas_mac.mm:43: int line_height,  // Ignored for now.
Since multi-line isn't implemented for canvas_mac, line_height naturally won't
be, so I removed the comment here and updated the multi-line comment above.

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_unittest.cc
File ui/gfx/canvas_unittest.cc (right):

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_unittest.cc#new...
ui/gfx/canvas_unittest.cc:50: // Line height only supported on SKIA.
On 2013/04/17 17:08:57, Jun Mukai wrote:
> Not so sure but it seems to be written as 'Skia', not full capital :)

You're right. Fixed.

https://codereview.chromium.org/14322007/diff/1/ui/gfx/canvas_unittest.cc#new...
ui/gfx/canvas_unittest.cc:51: #if defined(OS_CHROMEOS)
On 2013/04/17 17:08:57, Jun Mukai wrote:
> build/common.gypi says that canvas_skia.cc is used for all platforms except
for
> MAC and ANDROID. Can you change the condition to follow that?

Much better, thanks. Fixed.

[msw, 2013-04-17 20:12:59]

This mostly looks good. Just a couple general Qs and nits.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.cc#newcode436
ui/gfx/canvas.cc:436: -1,  // Default line height.
nit: skip the -1 comment, the function decl comment is sufficient.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h#newcode131
ui/gfx/canvas.h:131: int line_height,  // Only on SKIA and -1 = default.
nit: Skia. Consider moving this up to the function comment.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h#newcode131
ui/gfx/canvas.h:131: int line_height,  // Only on SKIA and -1 = default.
Should 0 mean default too? (and be used instead of -1?)
I don't think 0 line heights make sense or should be supported.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h#newcode331
ui/gfx/canvas.h:331: int line_height,  // Only on SKIA and -1 = default.
nit: Skia. Consider moving this up to the function comment.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_android.cc
File ui/gfx/canvas_android.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_android.cc#n...
ui/gfx/canvas_android.cc:15: int* width, int* height,
nit: split args onto individual lines while you're here.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_mac.mm
File ui/gfx/canvas_mac.mm (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_mac.mm#newco...
ui/gfx/canvas_mac.mm:42: int line_height,
nit: consider a DLOG_IF(WARNING, line_height != -1) << "Line heights are not
implemented."; here and below. You could also optionally add a similar check for
(flags & Canvas::MULTI_LINE)

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:205: h += (i > 0 && line_height >= 0) ? line_height :
string_size.height();
Should line_height really be ignored if the text isn't multi-line?

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:322: const int text_height =
render_text->GetStringSize().height();
nit: Move this above to use in line_height/line_padding init; then:
const int line_padding = (line_height < 0) ? 0 : text_height - line_height;
(you'd need to rename the other |text_height|, maybe to |total_height|?)

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:324: rect += Vector2d(0, (text_bounds.height() -
text_height) / 2);
Again, Should line_height really be ignored if the text isn't multi-line?

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc
File ui/gfx/canvas_unittest.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:14: /* Test fixture
***************************************************************/
nit: this doesn't seem terribly common or helpful, i'd remove it.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:34: /* Test cases
*****************************************************************/
nit: this doesn't seem terribly common or helpful, i'd remove it.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:50: // Line height only supported on Skia.
nit: "Line height *is* only..."

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:52: #define MAYBE_StringSizeWithLineHeight
DISABLED_StringSizeWithLineHeight
It seems more common to just wrap the test in #if defined blocks.
But I actually like this, it's easier to track DISABLED_* tests.

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/button/t...
File ui/views/controls/button/text_button.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/button/t...
ui/views/controls/button/text_button.cc:501: -1,  // Default line height.
nit: skip the -1 comment, the function decl comment is sufficient.

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/label.cc...
ui/views/controls/label.cc:238: int flags = ComputeDrawStringFlags();
nit: const

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/tree/tre...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/tree/tre...
ui/views/controls/tree/tree_view.cc:663: font_, &width, &height, -1,  // -1 =
Default line height.
nit: skip the -1 comment, the function decl comment is sufficient.

[dharcourt, 2013-04-17 23:29:40]

Mike, thanks for the suggestions. I've implemented them and tried to answer your
questions. Let me know if the answers seem reasonable or if I should really
change the meaning I current have for line_height.

   - C=

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.cc#newcode436
ui/gfx/canvas.cc:436: -1,  // Default line height.
On 2013/04/17 20:12:59, msw wrote:
> nit: skip the -1 comment, the function decl comment is sufficient.

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h#newcode131
ui/gfx/canvas.h:131: int line_height,  // Only on SKIA and -1 = default.
On 2013/04/17 20:12:59, msw wrote:
> nit: Skia. Consider moving this up to the function comment.

Done and done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas.h#newcode331
ui/gfx/canvas.h:331: int line_height,  // Only on SKIA and -1 = default.
On 2013/04/17 20:12:59, msw wrote:
> nit: Skia. Consider moving this up to the function comment.

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_android.cc
File ui/gfx/canvas_android.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_android.cc#n...
ui/gfx/canvas_android.cc:15: int* width, int* height,
On 2013/04/17 20:12:59, msw wrote:
> nit: split args onto individual lines while you're here.

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_mac.mm
File ui/gfx/canvas_mac.mm (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_mac.mm#newco...
ui/gfx/canvas_mac.mm:42: int line_height,
On 2013/04/17 20:12:59, msw wrote:
> nit: consider a DLOG_IF(WARNING, line_height != -1) << "Line heights are not
> implemented."; here and below. You could also optionally add a similar check
for
> (flags & Canvas::MULTI_LINE)

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:205: h += (i > 0 && line_height >= 0) ? line_height :
string_size.height();
On 2013/04/17 20:12:59, msw wrote:
> Should line_height really be ignored if the text isn't multi-line?

I think so. Changing the line height should change the distance between
baselines of successive lines of text, but not the amount of space above and
below the text as a whole. To make this work the line height must be ignored for
the first or last line of text. This should make text layout more predictable.

For example consider text that needs 9 pixels of padding above it and 8 pixels
of padding below it to get 12 pixels of white space before the top of the first
line's capitals and below the baseline of the last line (because the font
already provides 3 pixels above its capitals and 4 pixels below its baseline).
You should be able to change the line spacing of that text without changing the
amount of padding you need to achieve the 12 pixels of space.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:322: const int text_height =
render_text->GetStringSize().height();
On 2013/04/17 20:12:59, msw wrote:
> nit: Move this above to use in line_height/line_padding init; then:
> const int line_padding = (line_height < 0) ? 0 : text_height - line_height;
> (you'd need to rename the other |text_height|, maybe to |total_height|?)

I don't think I can do that because the line height above is computed on a
per-line basis, with possibly different GetStringSize().height() values returned
for different lines. I can't use this text_height for all lines.

Am I missing something?

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:324: rect += Vector2d(0, (text_bounds.height() -
text_height) / 2);
On 2013/04/17 20:12:59, msw wrote:
> Again, Should line_height really be ignored if the text isn't multi-line?

I think so, as described above.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc
File ui/gfx/canvas_unittest.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:14: /* Test fixture
***************************************************************/
On 2013/04/17 20:12:59, msw wrote:
> nit: this doesn't seem terribly common or helpful, i'd remove it.

And now it's gone :-).

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:34: /* Test cases
*****************************************************************/
On 2013/04/17 20:12:59, msw wrote:
> nit: this doesn't seem terribly common or helpful, i'd remove it.

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:50: // Line height only supported on Skia.
On 2013/04/17 20:12:59, msw wrote:
> nit: "Line height *is* only..."

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/gfx/canvas_unittest.cc#...
ui/gfx/canvas_unittest.cc:52: #define MAYBE_StringSizeWithLineHeight
DISABLED_StringSizeWithLineHeight
On 2013/04/17 20:12:59, msw wrote:
> It seems more common to just wrap the test in #if defined blocks.
> But I actually like this, it's easier to track DISABLED_* tests.

I'm glad my ignorance in this regard led to better trackability :-).

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/button/t...
File ui/views/controls/button/text_button.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/button/t...
ui/views/controls/button/text_button.cc:501: -1,  // Default line height.
On 2013/04/17 20:12:59, msw wrote:
> nit: skip the -1 comment, the function decl comment is sufficient.

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/label.cc...
ui/views/controls/label.cc:238: int flags = ComputeDrawStringFlags();
On 2013/04/17 20:12:59, msw wrote:
> nit: const

Done.

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/tree/tre...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/14322007/diff/4001/ui/views/controls/tree/tre...
ui/views/controls/tree/tree_view.cc:663: font_, &width, &height, -1,  // -1 =
Default line height.
On 2013/04/17 20:12:59, msw wrote:
> nit: skip the -1 comment, the function decl comment is sufficient.

Done.

[msw, 2013-04-18 03:58:52]

LGTM with a couple nits; thanks for the explanations.

https://codereview.chromium.org/14322007/diff/3004/ui/message_center/views/bo...
File ui/message_center/views/bounded_label.h (right):

https://codereview.chromium.org/14322007/diff/3004/ui/message_center/views/bo...
ui/message_center/views/bounded_label.h:41: void SetLineHeight(int height);  //
Pass in -1 for default height.
nit: s/-1/0/ or "height <= 0"

https://codereview.chromium.org/14322007/diff/3004/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/14322007/diff/3004/ui/views/controls/label.h#...
ui/views/controls/label.h:121: // Default is -1, indicating the distance between
lines should be the standard
nit: s/-1/0/

[dharcourt, 2013-04-18 17:50:27]

Thanks for catching those!

https://codereview.chromium.org/14322007/diff/3004/ui/message_center/views/bo...
File ui/message_center/views/bounded_label.h (right):

https://codereview.chromium.org/14322007/diff/3004/ui/message_center/views/bo...
ui/message_center/views/bounded_label.h:41: void SetLineHeight(int height);  //
Pass in -1 for default height.
On 2013/04/18 03:58:52, msw wrote:
> nit: s/-1/0/ or "height <= 0"

Done.

https://codereview.chromium.org/14322007/diff/3004/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/14322007/diff/3004/ui/views/controls/label.h#...
ui/views/controls/label.h:121: // Default is -1, indicating the distance between
lines should be the standard
On 2013/04/18 03:58:52, msw wrote:
> nit: s/-1/0/

Done.

[sadrul, 2013-04-18 17:53:51]

ui/views/ lgtm

[dharcourt, 2013-04-18 17:54:04]

Hi Scott, can I get an OWNERS review of the changes to the following files:

  ui/views/controls/button/text_button.cc
  ui/views/controls/label.cc
  ui/views/controls/label.h
  ui/views/controls/tree/tree_view.cc
  ui/views/view_text_utils.cc

All but the label.{cc,h} changes are trivial adjustments for a canvas.h API
change, and the label.{cc,h} changes should be pretty straightforward too.

Thanks in advance,

    - Charles

[dharcourt, 2013-04-18 17:56:57]

Hi Scott, please ignore my last comment, Sadrul reviewed and LGTM'ed this and
it's all good to go.

[commit-bot: I haz the power, 2013-04-18 17:57:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/14322007/24001

[commit-bot: I haz the power, 2013-04-18 21:38:31]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-19 00:50:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/14322007/24001

[commit-bot: I haz the power, 2013-04-19 03:40:00]

Change committed as 195076