Always aligns text at vertically center (Textfield, Label).

Always aligns text at vertically center (Textfield, Label).

This CL makes Textfield and Label put their content text at vertically center respecting the cap height.

Also this CL removes vertical alignment support, which we no longer need.

I think this CL fixes all issues related to vertical alignment and font height.

BUG=146236, 264436
TEST=Test manually + ui_unittests

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

[Yuki, 2013-10-18 16:48:24]

Could you guys review this CL?

pkasting@: Could you mainly review files under c/b/ui/views/ ?

msw@ and asvitkine@: Could you review ui/gfx/ and ui/views/ ?

Cheers,
Yuki Shiino

[Peter Kasting, 2013-10-18 21:11:09]

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:116: // Given a
containing |height| and a |base_font_list|, shrinks the font list
Nit: font list -> font size

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:118: // vertically
centered.  Returns the shrinked font list.
Nit: shrinked -> correctly-sized

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:125:
(font_list.GetBaseline() - font_list.GetCapHeight());
Nit: Indent 4, not even

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:126: if ((y_offset >=
0 &&
Nit: Parens around binary subexprs (omnibox subsystem style)

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:250: bubble_font_list,
kBubbleInteriorVerticalPadding,
Instead of passing this interior padding value to all these bubbles, can't we
just eliminate this arg entirely?  If the font size we've calculated above is
small enough, then vertically centering it within the entire bubble height seems
like it should work fine.

Indeed, given how the border in these classes is implemented, I think the
current behavior actually pushes the font too low, because you're going to add
this border on top, but not on bottom, and then center the font in the remaining
height.

You can test by visiting paypal.com and looking at the font placement within the
green bubble at the beginning of the omnibox.

[Yuki, 2013-10-21 09:44:40]

Worked on pkasting's comments.

Also fixed several bugs.
- PlatformFontPango::GetCapHeight() should return ascent rather than height as
an approximation.
- RenderText::GetAlignmentOffset() should call GetBaseline() in order to get the
right value.  |baseline_| could be an obsolete value.
- For the baseline calculation in RenderText::UpdateCachedBoundsAndOffset(), I
forgot to take into account internal leading.
- Fixed TEST_F(RenderTextTest, GetTextOffset) unit test.

I tested the CL on Windows and GNU/Linux Aura(Views).

Cheers,
Yuki Shiino

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:116: // Given a
containing |height| and a |base_font_list|, shrinks the font list
On 2013/10/18 21:11:09, Peter Kasting wrote:
> Nit: font list -> font size

Done.

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:118: // vertically
centered.  Returns the shrinked font list.
On 2013/10/18 21:11:09, Peter Kasting wrote:
> Nit: shrinked -> correctly-sized

Done.

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:125:
(font_list.GetBaseline() - font_list.GetCapHeight());
On 2013/10/18 21:11:09, Peter Kasting wrote:
> Nit: Indent 4, not even

Done.

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:126: if ((y_offset >=
0 &&
On 2013/10/18 21:11:09, Peter Kasting wrote:
> Nit: Parens around binary subexprs (omnibox subsystem style)

Done.

https://codereview.chromium.org/25039002/diff/129001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:250: bubble_font_list,
kBubbleInteriorVerticalPadding,
On 2013/10/18 21:11:09, Peter Kasting wrote:
> Instead of passing this interior padding value to all these bubbles, can't we
> just eliminate this arg entirely?  If the font size we've calculated above is
> small enough, then vertically centering it within the entire bubble height
seems
> like it should work fine.
> 
> Indeed, given how the border in these classes is implemented, I think the
> current behavior actually pushes the font too low, because you're going to add
> this border on top, but not on bottom, and then center the font in the
remaining
> height.
> 
> You can test by visiting http://paypal.com and looking at the font placement
within the
> green bubble at the beginning of the omnibox.

I've removed kBubbleInteriorVerticalPadding, and it's working fine.

[Peter Kasting, 2013-10-21 23:29:22]

LGTM

https://codereview.chromium.org/25039002/diff/319001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/ev_bubble_view.cc (left):

https://codereview.chromium.org/25039002/diff/319001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/ev_bubble_view.cc:9: 
Nit: Leave this and the other blank line below that got removed in place; this
is consistent with selected_keyword_view.cc and content_setting_image_view.cc

[Yuki, 2013-10-22 06:13:35]

Alexei and Mike, could you take a look at this CL?

Thanks,
Yuki Shiino

https://codereview.chromium.org/25039002/diff/319001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/ev_bubble_view.cc (left):

https://codereview.chromium.org/25039002/diff/319001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/ev_bubble_view.cc:9: 
On 2013/10/21 23:29:22, Peter Kasting wrote:
> Nit: Leave this and the other blank line below that got removed in place; this
> is consistent with selected_keyword_view.cc and content_setting_image_view.cc

Done.

[Alexei Svitkine (slow), 2013-10-22 15:01:00]

Could you explain a little bit about the need for both GetBaseline() and
GetBaselineOfTextLayout() functions on RenderText? Why are both needed and how
they relate to each other and why is one protected while the other one is public
(i.e. what's the effect on callers)?

I'd like to get some high-level explanations about your approach here before I
start commenting on the code. Thanks!

[Yuki, 2013-10-22 16:23:26]

On 2013/10/22 15:01:00, Alexei Svitkine wrote:
> Could you explain a little bit about the need for both GetBaseline() and
> GetBaselineOfTextLayout() functions on RenderText? Why are both needed and how
> they relate to each other and why is one protected while the other one is
public
> (i.e. what's the effect on callers)?
> 
> I'd like to get some high-level explanations about your approach here before I
> start commenting on the code. Thanks!

For the given display rect and font list, we'd like a RenderText to always use
the same baseline regardless of its content text.  The baseline is determined
from the font list and display rect centering the cap height.

GetBaseline() (i.e. baseline_) is this fixed baseline.  It never changes unless
the display rect or font list is changed.

While GetBaseline() always returns the same value, GetBaselineOfTextLayout()
returns a (possibly) different baseline depending on the content text, because
some characters may have very tall ascent and/or decent, or English and Japanese
characters may have different ascent.
NOTE: As you see, GetBaselineOfTextLayout() is the same as the previous
implementation of GetBaseline().  It's the baseline returned by the underlying
layout engine.

In order to keep the same baseline regardless of the content text, we have to
adjust the y offset internally.  This is done in
RenderText::GetAlignmentOffset().

GetBaselineOfTextLayout() is just for implementation, so it's not public.  The
actual implementation depends on a platform, and the method should be overridden
by RenderText{Linux,Mac,Win} so it's not private.

The reason why GetBaseline() is public is because the clients which implement
View::GetBaseline() need to call RenderText::GetBaseline(), and just backward
compatibility.

Does this explain enough?

[Alexei Svitkine (slow), 2013-10-22 17:25:29]

Thanks for the explanation. The approach sounds good, just a few comments below.

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1144: (font_list().GetBaseline() -
font_list().GetCapHeight())));
Nit: Can you cache font_list's GetCapHeight() and GetBaseline() in local vars to
avoid multiple calls?

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1145: baseline_ = font_list().GetBaseline() +
baseline_shift;
Nit: Can you make this block a free-standing function in the anon namespace that
takes a font_list and a display_rect_ and returns the baseline?

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:400: // GetAlignmentOffset() takes into account the
difference between them.
Can you expand this comment to make the purpose more clear. (The purpose being
so that we can align multiple runs that have different fonts together in the
implementation, right?)

[msw, 2013-10-23 01:18:00]

I just have mostly minor comments.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:123: while (true) {
nit: rewrite to avoid while(true), and/or return base_font_list at the bottom.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:124: const int
y_offset = (height - font_list.GetCapHeight()) / 2 -
nit: this calculation isn't entirely straightforward, can you add a comment?
An ASCII diagram of these measurements (in ui/gfx/font.h?) would be even better.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:234: int
location_height = GetInternalHeight(true);
nit: const.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:238: // The bubble
background images have 1 px thick edges, which we don't want to
nit: combine this comment with the one above (wrap this on the line above).

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:576: int text_width;
nit: please explicitly initialize this to 0.

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1126: // Determine the baseline so the text is placed at
vertically center.
nit: s/is placed at vertically center/appears vertically centered/

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1132: int min_shift;
nit: explicitly init min_shift and max_shift to 0.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1145: baseline_ = font_list().GetBaseline() +
baseline_shift;
This only needs to be invalidated on changes to the display_rect or font_list,
not every time the cursor bounds change. Can you instead just set baseline_ = 0
(or maybe INT_MAX) on SetFontList and SetDisplayRect, and then calculate this
value lazily on GetBaseline or something like that?

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h
File ui/gfx/render_text.h (left):

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#old...
ui/gfx/render_text.h:173: void SetVerticalAlignment(VerticalAlignment
alignment);
Hmm, I wonder if this will be needed for multi-line labels, but I guess we can
always restore this functionality if and when it's actually needed.

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:398: // Returns the baseline of the layouted text.  The
return value depends on
nit: s/layouted/laid out/ (or just say "current text" instead)

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:399: // the text and its layout while the return value of
GetBaseline() doesn't.
Add to this comment to explain exactly when/why this value will differ from
GetBaseLine(), which I suppose is only when the current text requires a fallback
font with a bigger cap height or similar, right?

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:401: virtual int GetBaselineOfTextLayout() = 0;
nit: I'd prefer GetLayoutTextBaseline (or GetBaselineOfLayoutText is ok).

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:632: // The baseline of the text.  This is determined from
height and cap height
nit: "from the height"

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1143: render_text->SetDisplayRect(Rect(0, 0,
std::numeric_limits<int>::max(),
Hmm, an int max width shouldn't break anything, but you could just use 0 or some
arbitrary constant like 100 instead, for simplicity here and below.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1310: // Widen display area's size by
kEnlargementX.
nit: "Widen the display area by |kEnlargementX|", but this comment is
superfluous anyway.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1330: // Enlarge the display height twice.  This
allows the |render_text| puts the
nit: simplify this comment to "Check that text is vertically centered within
taller display rects." and remove the other comment.

[Yuki, 2013-10-24 14:32:53]

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:123: while (true) {
On 2013/10/23 01:18:00, msw wrote:
> nit: rewrite to avoid while(true), and/or return base_font_list at the bottom.

Done.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:124: const int
y_offset = (height - font_list.GetCapHeight()) / 2 -
On 2013/10/23 01:18:00, msw wrote:
> nit: this calculation isn't entirely straightforward, can you add a comment?
> An ASCII diagram of these measurements (in ui/gfx/font.h?) would be even
better.

Done.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:124: const int
y_offset = (height - font_list.GetCapHeight()) / 2 -
On 2013/10/23 01:18:00, msw wrote:
> nit: this calculation isn't entirely straightforward, can you add a comment?
> An ASCII diagram of these measurements (in ui/gfx/font.h?) would be even
better.

Done.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:234: int
location_height = GetInternalHeight(true);
On 2013/10/23 01:18:00, msw wrote:
> nit: const.

Done.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:238: // The bubble
background images have 1 px thick edges, which we don't want to
On 2013/10/23 01:18:00, msw wrote:
> nit: combine this comment with the one above (wrap this on the line above).

Done.

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:576: int text_width;
On 2013/10/23 01:18:00, msw wrote:
> nit: please explicitly initialize this to 0.

I think it's better not to initialize this to 0.

The following if-else-clauses initialize this variable.  If it's not initialized
and used, then compilers warn it.

In the case that we initialize this to 0 here, even if we forgot to assign a
value in the else-clause, compilers do not warn anything.  And then, the value 0
is used.

So I think it's better to leave it uninitialized in this case.

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1126: // Determine the baseline so the text is placed at
vertically center.
On 2013/10/23 01:18:00, msw wrote:
> nit: s/is placed at vertically center/appears vertically centered/

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1132: int min_shift;
On 2013/10/23 01:18:00, msw wrote:
> nit: explicitly init min_shift and max_shift to 0.

The same reason here.
I think it's better to leave them uninitialized.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1144: (font_list().GetBaseline() -
font_list().GetCapHeight())));
On 2013/10/22 17:25:30, Alexei Svitkine wrote:
> Nit: Can you cache font_list's GetCapHeight() and GetBaseline() in local vars
to
> avoid multiple calls?

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1145: baseline_ = font_list().GetBaseline() +
baseline_shift;
On 2013/10/22 17:25:30, Alexei Svitkine wrote:
> Nit: Can you make this block a free-standing function in the anon namespace
that
> takes a font_list and a display_rect_ and returns the baseline?

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1145: baseline_ = font_list().GetBaseline() +
baseline_shift;
On 2013/10/23 01:18:00, msw wrote:
> This only needs to be invalidated on changes to the display_rect or font_list,
> not every time the cursor bounds change. Can you instead just set baseline_ =
0
> (or maybe INT_MAX) on SetFontList and SetDisplayRect, and then calculate this
> value lazily on GetBaseline or something like that?

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h
File ui/gfx/render_text.h (left):

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#old...
ui/gfx/render_text.h:173: void SetVerticalAlignment(VerticalAlignment
alignment);
On 2013/10/23 01:18:00, msw wrote:
> Hmm, I wonder if this will be needed for multi-line labels, but I guess we can
> always restore this functionality if and when it's actually needed.

Okay, let me remove them now.

I think, if RenderText provides GetPreferredSize(), the clients can align the
RenderText as they want.  Just as another option.

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:398: // Returns the baseline of the layouted text.  The
return value depends on
On 2013/10/23 01:18:00, msw wrote:
> nit: s/layouted/laid out/ (or just say "current text" instead)

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:399: // the text and its layout while the return value of
GetBaseline() doesn't.
On 2013/10/23 01:18:00, msw wrote:
> Add to this comment to explain exactly when/why this value will differ from
> GetBaseLine(), which I suppose is only when the current text requires a
fallback
> font with a bigger cap height or similar, right?

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:400: // GetAlignmentOffset() takes into account the
difference between them.
On 2013/10/22 17:25:30, Alexei Svitkine wrote:
> Can you expand this comment to make the purpose more clear. (The purpose being
> so that we can align multiple runs that have different fonts together in the
> implementation, right?)

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:401: virtual int GetBaselineOfTextLayout() = 0;
On 2013/10/23 01:18:00, msw wrote:
> nit: I'd prefer GetLayoutTextBaseline (or GetBaselineOfLayoutText is ok).

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:632: // The baseline of the text.  This is determined from
height and cap height
On 2013/10/23 01:18:00, msw wrote:
> nit: "from the height"

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1143: render_text->SetDisplayRect(Rect(0, 0,
std::numeric_limits<int>::max(),
On 2013/10/23 01:18:00, msw wrote:
> Hmm, an int max width shouldn't break anything, but you could just use 0 or
some
> arbitrary constant like 100 instead, for simplicity here and below.

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1310: // Widen display area's size by
kEnlargementX.
On 2013/10/23 01:18:00, msw wrote:
> nit: "Widen the display area by |kEnlargementX|", but this comment is
> superfluous anyway.

Done.

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1330: // Enlarge the display height twice.  This
allows the |render_text| puts the
On 2013/10/23 01:18:00, msw wrote:
> nit: simplify this comment to "Check that text is vertically centered within
> taller display rects." and remove the other comment.

Done.

[Alexei Svitkine (slow), 2013-10-24 15:03:54]

lgtm with a nit

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

https://codereview.chromium.org/25039002/diff/839001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:75: return baseline + baseline_shift;
Nit: I think this is more readable:

  const int baseline_shift =
      (display_height - cap_height) / 2 - internal_leading;
  return baseline + std::max(min_shift, std::min(max_shift, baseline_shift));

https://codereview.chromium.org/25039002/diff/839001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:683: baseline_ =
DetermineBaselineCenteringText(display_rect(), font_list());
Nit: Add DCHECK_NE(kInvalidBaseline, baseline_);

[Yuki, 2013-10-24 15:17:35]

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

https://codereview.chromium.org/25039002/diff/839001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:75: return baseline + baseline_shift;
On 2013/10/24 15:03:55, Alexei Svitkine wrote:
> Nit: I think this is more readable:
> 
>   const int baseline_shift =
>       (display_height - cap_height) / 2 - internal_leading;
>   return baseline + std::max(min_shift, std::min(max_shift, baseline_shift));

Done.

https://codereview.chromium.org/25039002/diff/839001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:683: baseline_ =
DetermineBaselineCenteringText(display_rect(), font_list());
On 2013/10/24 15:03:55, Alexei Svitkine wrote:
> Nit: Add DCHECK_NE(kInvalidBaseline, baseline_);

Done.

[Peter Kasting, 2013-10-24 20:12:05]

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:576: int text_width;
On 2013/10/24 14:32:54, Yuki wrote:
> On 2013/10/23 01:18:00, msw wrote:
> > nit: please explicitly initialize this to 0.
> 
> I think it's better not to initialize this to 0.
> 
> The following if-else-clauses initialize this variable.  If it's not
initialized
> and used, then compilers warn it.
> 
> In the case that we initialize this to 0 here, even if we forgot to assign a
> value in the else-clause, compilers do not warn anything.  And then, the value
0
> is used.
> 
> So I think it's better to leave it uninitialized in this case.

What about initializing it to (width() - text_x - edge_item_padding_) and
dropping the else clause?

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1132: int min_shift;
On 2013/10/24 14:32:54, Yuki wrote:
> On 2013/10/23 01:18:00, msw wrote:
> > nit: explicitly init min_shift and max_shift to 0.
> 
> The same reason here.
> I think it's better to leave them uninitialized.

How about this simpler version:

  int min_shift = std::max(0, display_height - font_height);
  int max_shift = abs(display_height - font_height);

[Yuki, 2013-10-25 15:21:08]

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/25039002/diff/589001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:576: int text_width;
On 2013/10/24 20:12:05, Peter Kasting wrote:
> On 2013/10/24 14:32:54, Yuki wrote:
> > On 2013/10/23 01:18:00, msw wrote:
> > > nit: please explicitly initialize this to 0.
> > 
> > I think it's better not to initialize this to 0.
> > 
> > The following if-else-clauses initialize this variable.  If it's not
> initialized
> > and used, then compilers warn it.
> > 
> > In the case that we initialize this to 0 here, even if we forgot to assign a
> > value in the else-clause, compilers do not warn anything.  And then, the
value
> 0
> > is used.
> > 
> > So I think it's better to leave it uninitialized in this case.
> 
> What about initializing it to (width() - text_x - edge_item_padding_) and
> dropping the else clause?

Done.

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

https://codereview.chromium.org/25039002/diff/589001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1132: int min_shift;
On 2013/10/24 20:12:05, Peter Kasting wrote:
> On 2013/10/24 14:32:54, Yuki wrote:
> > On 2013/10/23 01:18:00, msw wrote:
> > > nit: explicitly init min_shift and max_shift to 0.
> > 
> > The same reason here.
> > I think it's better to leave them uninitialized.
> 
> How about this simpler version:
> 
>   int min_shift = std::max(0, display_height - font_height);
>   int max_shift = abs(display_height - font_height);

Done.

[msw, 2013-10-25 17:18:26]

LGTM, thanks!

[commit-bot: I haz the power, 2013-10-25 18:03:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/25039002/1099001

[commit-bot: I haz the power, 2013-10-25 20:41:11]

Change committed as 231107

[Daniel Erat, 2013-11-04 17:14:20]

On 2013/10/25 20:41:11, I haz the power (commit-bot) wrote:
> Change committed as 231107

This breaks the omnibox font on Chrome OS (http://crbug.com/314688).

[knuckles, 2014-09-18 11:34:16]

On 2013/11/04 17:14:20, Daniel Erat wrote:
> On 2013/10/25 20:41:11, I haz the power (commit-bot) wrote:
> > Change committed as 231107
> 
> This breaks the omnibox font on Chrome OS (http://crbug.com/314688).

Can someone please take a look at this report?
https://code.google.com/p/chromium/issues/detail?id=411908
Text positioning still seems broken. We have a patch!