[RenderText] Adding vertical alignment options

[RenderText] Adding vertical alignment options.

This change adds vertical alignment options using similar terms to html layout.  The macros are prefixed with VALIGN for vertical alignment.  The three positions available are TOP, MIDDLE, BOTTOM.

This change also includes a unit test for alignment options.

BUG=469362

[dschuyler, 2015-03-24 23:54:48]

dschuyler@chromium.org changed reviewers:
+ msw@chromium.org

[dschuyler, 2015-03-24 23:56:53]

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

https://codereview.chromium.org/1013543006/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:1168: if (multiline_) {
This hasn't been tested on Windows yet.  Do these look correct?

[dschuyler, 2015-03-26 18:42:14]

Sorry about the empty message.  I should have said ping :)

The Mac version is not getting the alignment quite right.  I'll look into that,
but does the rest of the direction of this CL look generally ok?

[msw, 2015-03-26 20:10:37]

Sorry, I've had a rather heavy review load lately, including more critical crash
fixes and regression fixes that need to be merged to beta/stable... I'll look
soon, but feel free to iterate in the mean time.

[msw, 2015-03-26 22:55:45]

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1172: offset.set_y(lines_.back().preceding_heights);
I don't think this is correct... Shouldn't it just be 0?

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1176: // Vertically center the text.
nit: change this comment to DCHECK_EQ(VALIGN_MIDDLE, vertical_alignment_);

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1185: // Vertically center the text.
ditto nit: change this comment to DCHECK_EQ(VALIGN_MIDDLE, vertical_alignment_);

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:649: FRIEND_TEST_ALL_PREFIXES(RenderTextTest,
VerticalAlignement);
nit: "Alignment"

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:212: EnsureSolidRect(SK_ColorWHITE, 0, rect.y() +
rect.height(), stride_,
rect.y() + rect.height() == rect.bottom()

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:213: row_count_ - rect.y() - rect.height());
- rect.y() - rect.height() == - rect.bottom()

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:221: EnsureSolidRect(SK_ColorWHITE, rect.x() +
rect.width(), rect.y(),
rect.x() + rect.width() == rect.right()

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:222: stride_ - rect.x() - rect.width(),
rect.height());
- rect.x() - rect.width() == - rect.right()

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2616: TEST_F(RenderTextTest, VerticalAlignement)
{
nit: "Alignment"

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2617: SCOPED_TRACE("VerticalAlignement");
What's the use of this SCOPED_TRACE? The failing test should be readily
identifiable without this. (otherwise, ditto nit: "Alignment")

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2637: {ALIGN_LEFT, VALIGN_TOP, kTestSize,
kTestSize},
nit: outdent all the these lines by 2 spaces

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2648: skia::RefPtr<SkSurface> surface =
skia::AdoptRef(
I think painting the text and checking the painted pixels is an overly complex
and expensive way of checking alignment calculations. This test should check
GetAlignmentOffset() (or GetLineOffset()) values for each of the settings,
similar to the GetTextOffset or Multiline_HorizontalAlignment tests.

You can probably just extend the GetTextOffset test with vertical alignments,
and add a Multiline_VerticalAlignment test (or check the same info for multiline
RenderText in the same GetTextOffset test)?

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2788: // TODO(dschuyler): After platform issued
are resolved below, change to
nit: "issues"

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2839: SCOPED_TRACE("TextDoesClip");
Ditto, what's the use of this SCOPED_TRACE? The failing test should be readily
identifiable without this.

[dschuyler, 2015-03-27 21:20:35]

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1172: offset.set_y(lines_.back().preceding_heights);
On 2015/03/26 22:55:44, msw wrote:
> I don't think this is correct... Shouldn't it just be 0?

I'm not sure.  If it's a single line or if it's done for the whole block of text
(which is something I'm not clear on yet), then 0 would be correct.  If it's
done per-line on multiline text, it would need to change as we progress through
the lines.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1176: // Vertically center the text.
On 2015/03/26 22:55:44, msw wrote:
> nit: change this comment to DCHECK_EQ(VALIGN_MIDDLE, vertical_alignment_);

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1185: // Vertically center the text.
On 2015/03/26 22:55:45, msw wrote:
> ditto nit: change this comment to DCHECK_EQ(VALIGN_MIDDLE,
vertical_alignment_);

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:212: EnsureSolidRect(SK_ColorWHITE, 0, rect.y() +
rect.height(), stride_,
On 2015/03/26 22:55:45, msw wrote:
> rect.y() + rect.height() == rect.bottom()

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:213: row_count_ - rect.y() - rect.height());
On 2015/03/26 22:55:45, msw wrote:
> - rect.y() - rect.height() == - rect.bottom()

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:221: EnsureSolidRect(SK_ColorWHITE, rect.x() +
rect.width(), rect.y(),
On 2015/03/26 22:55:45, msw wrote:
> rect.x() + rect.width() == rect.right()

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:222: stride_ - rect.x() - rect.width(),
rect.height());
On 2015/03/26 22:55:45, msw wrote:
> - rect.x() - rect.width() == - rect.right()

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2616: TEST_F(RenderTextTest, VerticalAlignement)
{
On 2015/03/26 22:55:45, msw wrote:
> nit: "Alignment"

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2617: SCOPED_TRACE("VerticalAlignement");
On 2015/03/26 22:55:45, msw wrote:
> What's the use of this SCOPED_TRACE? The failing test should be readily
> identifiable without this. (otherwise, ditto nit: "Alignment")

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2637: {ALIGN_LEFT, VALIGN_TOP, kTestSize,
kTestSize},
On 2015/03/26 22:55:45, msw wrote:
> nit: outdent all the these lines by 2 spaces

Afaik, it's an outstanding bug in cl format.  I.e. this is the git cl format of
this code :(

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2648: skia::RefPtr<SkSurface> surface =
skia::AdoptRef(
On 2015/03/26 22:55:45, msw wrote:
> I think painting the text and checking the painted pixels is an overly complex
> and expensive way of checking alignment calculations. This test should check
> GetAlignmentOffset() (or GetLineOffset()) values for each of the settings,
> similar to the GetTextOffset or Multiline_HorizontalAlignment tests.
> 
> You can probably just extend the GetTextOffset test with vertical alignments,
> and add a Multiline_VerticalAlignment test (or check the same info for
multiline
> RenderText in the same GetTextOffset test)?

I put in some tests for the get alignment offset.  They don't seem to catch
errors on Mac where the text still draws in the wrong place.  Though maybe the
pixel testing should be an entirely separate test from a set/get alignment test?
 I.e. This kind of test does seem useful to do - are you suggesting that it be
done as a separate test?  (That separate test would also need to set alignment
values to see the issues).

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2788: // TODO(dschuyler): After platform issued
are resolved below, change to
On 2015/03/26 22:55:45, msw wrote:
> nit: "issues"

Done.

[msw, 2015-03-30 19:19:12]

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1172: offset.set_y(lines_.back().preceding_heights);
On 2015/03/27 21:20:34, dschuyler wrote:
> On 2015/03/26 22:55:44, msw wrote:
> > I don't think this is correct... Shouldn't it just be 0?
> 
> I'm not sure.  If it's a single line or if it's done for the whole block of
text
> (which is something I'm not clear on yet), then 0 would be correct.  If it's
> done per-line on multiline text, it would need to change as we progress
through
> the lines.

This should probably be 0 (as in your latest patch set). The vertical offset
contributed from the alignment is a single value for for the whole multi-line
block of text (though the horizontal alignment offset differs by line for
center-/right-aligned text). The per-line vertical offset is derived from the
preceding heights and applied in RenderText::GetLineOffset(). Please build
views_examples_exe and add controls to experiment with the text rendering
examples there to verify that this new functionality appears correct.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2648: skia::RefPtr<SkSurface> surface =
skia::AdoptRef(
On 2015/03/27 21:20:34, dschuyler wrote:
> On 2015/03/26 22:55:45, msw wrote:
> > I think painting the text and checking the painted pixels is an overly
complex
> > and expensive way of checking alignment calculations. This test should check
> > GetAlignmentOffset() (or GetLineOffset()) values for each of the settings,
> > similar to the GetTextOffset or Multiline_HorizontalAlignment tests.
> > 
> > You can probably just extend the GetTextOffset test with vertical
alignments,
> > and add a Multiline_VerticalAlignment test (or check the same info for
> multiline
> > RenderText in the same GetTextOffset test)?
> 
> I put in some tests for the get alignment offset.  They don't seem to catch
> errors on Mac where the text still draws in the wrong place.  Though maybe the
> pixel testing should be an entirely separate test from a set/get alignment
test?
>  I.e. This kind of test does seem useful to do - are you suggesting that it be
> done as a separate test?  (That separate test would also need to set alignment
> values to see the issues).

How many ms do the pixel tests take on the bots? I suspect their value
diminishes with the cost of each new test. I don't think we ultimately want to
do this expensive testing on every variation of settings that affect text
rendering. Just checking the alignment/line offset here should suffice, as long
as some other test(s) perform a pixel check with non-zero line offsets
(TextDoesntClip setting the display rect should suffice). ie. We should have
separate tests for (1) checking alignment offsets given the alignment setting
and (2) for checking that the text paints respecting some alignment offset(s).

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2854: const Size
string_size(render_text->GetContentWidth(),
I don't think this is correct, the cursor shouldn't be painting here, so the
size difference (if there is any) is disingenuous.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2925: SCOPED_TRACE("TextDoesClip");
Remove this scoped trace.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2941: const Size
string_size(render_text->GetContentWidth(),
Ditto.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/text_constants.h...
ui/gfx/text_constants.h:28: VALIGN_TOP = 0,  // Align the text's top edge with
that of its display area.
nit: one fewer space before the comments on this line and the next line.

[dschuyler, 2015-03-31 18:38:15]

It looks like the canvas skia was also trying to wrap multi-line text.  I'm
seeing better results by letting RenderText handle the wrapping instead.  Is the
removal of the repeated multi-line code likely to be an issue - what would be a
good test for me to check?

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1172: offset.set_y(lines_.back().preceding_heights);
On 2015/03/30 19:19:12, msw wrote:
> On 2015/03/27 21:20:34, dschuyler wrote:
> > On 2015/03/26 22:55:44, msw wrote:
> > > I don't think this is correct... Shouldn't it just be 0?
> > 
> > I'm not sure.  If it's a single line or if it's done for the whole block of
> text
> > (which is something I'm not clear on yet), then 0 would be correct.  If it's
> > done per-line on multiline text, it would need to change as we progress
> through
> > the lines.
> 
> This should probably be 0 (as in your latest patch set). The vertical offset
> contributed from the alignment is a single value for for the whole multi-line
> block of text (though the horizontal alignment offset differs by line for
> center-/right-aligned text). The per-line vertical offset is derived from the
> preceding heights and applied in RenderText::GetLineOffset(). Please build
> views_examples_exe and add controls to experiment with the text rendering
> examples there to verify that this new functionality appears correct.

Done.

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2648: skia::RefPtr<SkSurface> surface =
skia::AdoptRef(
On 2015/03/30 19:19:12, msw wrote:
> On 2015/03/27 21:20:34, dschuyler wrote:
> > On 2015/03/26 22:55:45, msw wrote:
> > > I think painting the text and checking the painted pixels is an overly
> complex
> > > and expensive way of checking alignment calculations. This test should
check
> > > GetAlignmentOffset() (or GetLineOffset()) values for each of the settings,
> > > similar to the GetTextOffset or Multiline_HorizontalAlignment tests.
> > > 
> > > You can probably just extend the GetTextOffset test with vertical
> alignments,
> > > and add a Multiline_VerticalAlignment test (or check the same info for
> > multiline
> > > RenderText in the same GetTextOffset test)?
> > 
> > I put in some tests for the get alignment offset.  They don't seem to catch
> > errors on Mac where the text still draws in the wrong place.  Though maybe
the
> > pixel testing should be an entirely separate test from a set/get alignment
> test?
> >  I.e. This kind of test does seem useful to do - are you suggesting that it
be
> > done as a separate test?  (That separate test would also need to set
alignment
> > values to see the issues).
> 
> How many ms do the pixel tests take on the bots? I suspect their value
> diminishes with the cost of each new test. I don't think we ultimately want to
> do this expensive testing on every variation of settings that affect text
> rendering. Just checking the alignment/line offset here should suffice, as
long
> as some other test(s) perform a pixel check with non-zero line offsets
> (TextDoesntClip setting the display rect should suffice). ie. We should have
> separate tests for (1) checking alignment offsets given the alignment setting
> and (2) for checking that the text paints respecting some alignment offset(s).

Done.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2854: const Size
string_size(render_text->GetContentWidth(),
On 2015/03/30 19:19:12, msw wrote:
> I don't think this is correct, the cursor shouldn't be painting here, so the
> size difference (if there is any) is disingenuous.

The value of cursor_enabled_ defaults to true.  It's not checking whether the
cursor is being drawn, but whether it is enabled.  The cursor_visible_ flag
which defaults to false is a separate data member.  I don't know the background
on why those two values are separate.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2925: SCOPED_TRACE("TextDoesClip");
On 2015/03/30 19:19:12, msw wrote:
> Remove this scoped trace.

Done.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2941: const Size
string_size(render_text->GetContentWidth(),
On 2015/03/30 19:19:12, msw wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/text_constants.h...
ui/gfx/text_constants.h:28: VALIGN_TOP = 0,  // Align the text's top edge with
that of its display area.
On 2015/03/30 19:19:12, msw wrote:
> nit: one fewer space before the comments on this line and the next line.

git cl format insists on adding the spaces back.  Should I let cl format win or
try to force the spacing?

[msw, 2015-04-01 15:07:18]

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2854: const Size
string_size(render_text->GetContentWidth(),
On 2015/03/31 18:38:14, dschuyler wrote:
> On 2015/03/30 19:19:12, msw wrote:
> > I don't think this is correct, the cursor shouldn't be painting here, so the
> > size difference (if there is any) is disingenuous.
> 
> The value of cursor_enabled_ defaults to true.  It's not checking whether the
> cursor is being drawn, but whether it is enabled.  The cursor_visible_ flag
> which defaults to false is a separate data member.  I don't know the
background
> on why those two values are separate.

Ah, you should probably disable the cursor for these tests, and revert to using
just GetStringSize(). In fact, the cursor should probably be disabled by default
(or perhaps GetStringSize should take it into account, as you suggested in the
sizing bug); having the cursor enabled for any non-textfield RenderText
instances (labels, etc.) might be causing a fair number of sizing/alignment
issues. The separate visible flag is for making the cursor blink, or hiding it
when not focused.

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

https://codereview.chromium.org/1013543006/diff/80001/ui/gfx/text_constants.h...
ui/gfx/text_constants.h:28: VALIGN_TOP = 0,  // Align the text's top edge with
that of its display area.
On 2015/03/31 18:38:14, dschuyler wrote:
> On 2015/03/30 19:19:12, msw wrote:
> > nit: one fewer space before the comments on this line and the next line.
> 
> git cl format insists on adding the spaces back.  Should I let cl format win
or
> try to force the spacing?

I'd force the spacing tighter (like many other enums here), or optionally
shorten the 1st and 3rd comment by removing the word 'edge', so the bottom
comment can be aligned with the top two.

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

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/canvas.h#newcode73
ui/gfx/canvas.h:73: // Specifies the alignment for text rendered with the
DrawStringRect method.
Let's not add these flags unless they are imminently needed (I hope we can
deprecate the canvas string drawing functions eventually).

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (left):

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/canvas_skia.cc#...
ui/gfx/canvas_skia.cc:197: WordWrapBehavior wrap_behavior = IGNORE_LONG_WORDS;
The new code should configure RenderText's word wrap behavior from these flags.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/canvas_skia.cc#...
ui/gfx/canvas_skia.cc:201: wrap_behavior = ELIDE_LONG_WORDS;
RenderTextHarfBuzz doesn't yet support ELIDE_LONG_WORDS.

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

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/canvas_skia.cc#...
ui/gfx/canvas_skia.cc:113: render_text->SetMultiline((flags &
Canvas::MULTI_LINE) != 0);
This will break RenderTextMac, which doesn't support multiline. We do want
RenderTextHarfBuzz to handle multiline layout internally, so you can land these
changes (hopefully in a prerequisite CL), but you'll need to update Canvas to
support both codepaths, toggling on RenderText::MultilineSupported().

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:1225: text_rect.Inset(GetAlignmentOffset(0).x(),
GetAlignmentOffset(0).y(), 0, 0);
nit: cache the alignment offset instead of calling this twice.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2731: TEST_F(RenderTextTest, VerticalAlignment) {
Can you add a separate test for multiline vertical alignment? Or integrate that
here if it doesn't explode the test complexity.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2733: const wchar_t* string = L"g Hello g";
nit: inline this below

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2735: render_text->ApplyStyle(BOLD, true,
Range(0, 3));
Why apply bold? Do you check this somehow?

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2736: const Size kCanvasSize(171, 50);
nit: where did 171 come from? That seems awfully specific. If you intend to
force line-wrapping, you can't depend on glyph widths being consistent across
platforms. You'll need to either add line breaks to the text content itself, or
use set_glyph_width_for_test().

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2745: ((kCanvasSize.height()) / 2) - 1 +
nit: remove the "kCanvasSize.height()" parens, optionally the /2 parens too.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2751: HorizontalAlignment horizontal;
Is testing vertical alignment functionality for each horizontal alignment
necessary? Consider reducing the complexity of the test by just checking the
vertical alignment and ignoring the horizontal offsets; and feel free to refine
the existing horizontal alignment tests.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2767: skia::RefPtr<SkSurface> surface =
skia::AdoptRef(
Remove this, it's no longer necessary.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2769: render_text->SetColor(SK_ColorBLACK);
ditto: Remove this, it's no longer necessary.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2774:
render_text->set_clip_to_display_rect(false);
ditto: Remove this, it's no longer necessary.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2778:
SCOPED_TRACE(base::StringPrintf("VerticalAlignement H %i, V %i",
nit: "Alignment", but again, the test name isn't necessary in SCOPED_TRACE, you
could nix that part of the string. When authoring tests, it can be helpful to
force your tests to fail locally and look at the actual output.

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2781: surface->getCanvas()->clear(SK_ColorWHITE);
ditto: Remove this, it's no longer necessary.

https://codereview.chromium.org/1013543006/diff/160001/ui/views/examples/text...
File ui/views/examples/text_example.cc (right):

https://codereview.chromium.org/1013543006/diff/160001/ui/views/examples/text...
ui/views/examples/text_example.cc:51: const char* kVerticalAligments[] = {
optional nit: defy git-cl-format and fit this on one line like those above:
const char* kVerticalAligments[] = { "Default", "Top", "Middle", "Bottom", };

https://codereview.chromium.org/1013543006/diff/160001/ui/views/examples/text...
ui/views/examples/text_example.cc:235: flags |= gfx::Canvas::TEXT_ALIGN_TOP;
Ah, I see why you added the canvas flags now... It'd be nice to avoid those and
either (1) extend the multi-line example with RenderText with vertical alignment
options, (2) extend the Label example with vertical alignment options (3) make a
RenderText example on par with this canvas text rendering example... These
changes are entirely optional, adding the flags for this example isn't so bad...
we'll just eventually want to avoid those flags and improve these examples.

[Jun Mukai, 2015-04-02 00:53:12]

mukai@chromium.org changed reviewers:
+ mukai@chromium.org

[Jun Mukai, 2015-04-02 00:53:12]

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1013543006/diff/160001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:276: // TODO(ckocagil): Add vertical alignment and line
spacing support instead.
Please remove this TODO.