Fixes vertical alignment of RenderText.

Fixes vertical alignment of RenderText.
(was: Issue 18848002: Shows Japanese and English mixed queries correctly.)

The CL 18848002 was reverted because the new unit test, which I added, depended on the specific fonts and didn't run well on some of platforms.

I've carefully chosen another font for the unit test, which is available on all platforms.

BUG=244323
TEST=Test manually.

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

[Yuki, 2013-07-16 09:10:05]

Hi Mike,

Could you review this CL because the last CL was reverted since my new test
didn't run well on some of platforms.

I've introduced PlatformFontFake so my test doesn't depend on any platform.

Only font_list_unittest.cc and ui_unittests.gyp were modified and no other files
were affected since the last CL.

Cheers,
Yuki Shiino

[msw, 2013-07-16 09:32:36]

In the future, upload the patch set that was reverted first, then it's easy for
reviewers to see how you fixed the failure.

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

https://codereview.chromium.org/19352002/diff/1/ui/gfx/font_list.cc#newcode86
ui/gfx/font_list.cc:86: : fonts_(fonts), common_height_(-1),
common_baseline_(-1) {
style nit: if the initializers don't fit on the same line as the signature, I
think they're supposed to be one-per-line. Sorry if my last review wasn't clear
about that.

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

https://codereview.chromium.org/19352002/diff/1/ui/gfx/font_list_unittest.cc#...
ui/gfx/font_list_unittest.cc:238: PlatformFontFake* pf_arial = new
PlatformFontFake("Arial", 10);
Rather than creating a fake font class, why not just measure the height and
baseline of the real fonts and use max(arial, helvetica) into the bottom checks?
That should be simpler and robust against platform differences in
height/baseline. You'll probably want to also check that the fonts have
different heights and/or baselines (so this test doesn't check a trivial case).

https://codereview.chromium.org/19352002/diff/1/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/19352002/diff/1/ui/ui_unittests.gypi#newcode123
ui/ui_unittests.gypi:123: 'gfx/platform_font_fake.cc',
These files aren't part of your CL.

[Yuki, 2013-07-16 09:59:57]

Let me explain why I introduced the fake font class.  The test in my last CL
passed on most of platforms, but "Sans serif" has 0 height and 0 baseline on Mac
OS X 10.6 and 10.7.  I don't fully understand why...

Logs are here:
http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg...
http://build.chromium.org/p/chromium.mac/builders/Mac%2010.7%20Tests%20%28dbg...

So I thought it's better to introduce the fake fonts rather than caring about
which font is available on which platform.  The logic I'd like to test doesn't
depend on actual fonts.

What do you think?

[msw, 2013-07-16 17:14:49]

On 2013/07/16 09:59:57, Yuki wrote:
> Let me explain why I introduced the fake font class.  The test in my last CL
> passed on most of platforms, but "Sans serif" has 0 height and 0 baseline on
Mac
> OS X 10.6 and 10.7.  I don't fully understand why...
> 
> Logs are here:
>
http://build.chromium.org/p/chromium.mac/builders/Mac%252010.6%2520Tests%2520...
>
http://build.chromium.org/p/chromium.mac/builders/Mac%252010.7%2520Tests%2520...
> 
> So I thought it's better to introduce the fake fonts rather than caring about
> which font is available on which platform.  The logic I'd like to test doesn't
> depend on actual fonts.
> 
> What do you think?

Give real fonts another try, hopefully it won't be too hard to find two with
reasonable, distinct heights and baselines on Win/Mac/Linux. I don't think it's
worthwhile to add 164 lines of a fake font class to test that FontList returns
an std::max of their values in one bare minimum test. Search our codebase for
examples and search online for "Cross platform fonts", "web safe fonts" or
similar. Otherwise, maybe disable the test on Mac...
https://code.google.com/p/chromium/codesearch#search/&q=Font%5C(%5C%22%20file...
http://www.ampsoft.net/webdesign-l/WindowsMacFonts.html
http://www.webspaceworks.com/resources/fonts-web-typography/49/

[msw, 2013-07-16 17:15:54]

CC'ing Alexei, he might have advice.

[Yuki, 2013-07-17 07:16:04]

Thanks for the comments.
I've updated the unit test and fixed coding style.

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

https://codereview.chromium.org/19352002/diff/1/ui/gfx/font_list.cc#newcode86
ui/gfx/font_list.cc:86: : fonts_(fonts), common_height_(-1),
common_baseline_(-1) {
On 2013/07/16 09:32:36, msw wrote:
> style nit: if the initializers don't fit on the same line as the signature, I
> think they're supposed to be one-per-line. Sorry if my last review wasn't
clear
> about that.

Done.

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

https://codereview.chromium.org/19352002/diff/1/ui/gfx/font_list_unittest.cc#...
ui/gfx/font_list_unittest.cc:238: PlatformFontFake* pf_arial = new
PlatformFontFake("Arial", 10);
On 2013/07/16 09:32:36, msw wrote:
> Rather than creating a fake font class, why not just measure the height and
> baseline of the real fonts and use max(arial, helvetica) into the bottom
checks?
> That should be simpler and robust against platform differences in
> height/baseline. You'll probably want to also check that the fonts have
> different heights and/or baselines (so this test doesn't check a trivial
case).

Done.

https://codereview.chromium.org/19352002/diff/1/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/19352002/diff/1/ui/ui_unittests.gypi#newcode123
ui/ui_unittests.gypi:123: 'gfx/platform_font_fake.cc',
On 2013/07/16 09:32:36, msw wrote:
> These files aren't part of your CL.

Done.

[msw, 2013-07-17 17:30:31]

LGTM with a couple minor comments, but I don't want to hold you up.
You should run try jobs on patch sets that are close to done. This CL would
especially benefit from tryjobs, to ensure the unit tests pass on all platforms.

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest...
ui/gfx/font_list_unittest.cc:247: EXPECT_NE(font1.GetHeight() -
font1.GetBaseline(),
Should this also EXPECT_NE(font1.GetHeight(), font2.GetHeight()); ?

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest...
ui/gfx/font_list_unittest.cc:253: EXPECT_LE(std::max(font1.GetHeight(),
font2.GetHeight()),
Should the height be calculated the same here as in FontList? (add the max
ascent and descent, instead of finding the max height directly). That's a subtle
but important piece of logic worth devising a separate test case for (that would
fail using a simple max of the heights), but its not critical if it's too much
work to find suitable fonts/sizes.

[Yuki, 2013-07-19 14:40:59]

Thanks for your review, and sorry that I was ooo two days.

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest...
ui/gfx/font_list_unittest.cc:247: EXPECT_NE(font1.GetHeight() -
font1.GetBaseline(),
On 2013/07/17 17:30:32, msw wrote:
> Should this also EXPECT_NE(font1.GetHeight(), font2.GetHeight()); ?

Since we specified the same font size, it's possible that they have the same
font height, having different ascent and descent, e.g. 12 ascent + 4 descent and
11 ascent + 5 descent.
So I don't want to require your EXPECT_NE.

I've changed the test's check condition, so we now guarantee
1. ascent of font1 != ascent of font2, as well as descent.
2. ascent of FontList == max(ascent of Fonts), as well as descent.

With the above test condition, I think it's okay that two fonts have
(accidentally) the same font height.  Even in that case, we're testing what we'd
like to guarantee.

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest...
ui/gfx/font_list_unittest.cc:253: EXPECT_LE(std::max(font1.GetHeight(),
font2.GetHeight()),
On 2013/07/17 17:30:32, msw wrote:
> Should the height be calculated the same here as in FontList? (add the max
> ascent and descent, instead of finding the max height directly). That's a
subtle
> but important piece of logic worth devising a separate test case for (that
would
> fail using a simple max of the heights), but its not critical if it's too much
> work to find suitable fonts/sizes.

Done.

[msw, 2013-07-19 17:30:33]

LGTM with nits. Looking forward to a similar RenderTextWin fix!

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/19352002/diff/24001/ui/gfx/font_list_unittest...
ui/gfx/font_list_unittest.cc:253: EXPECT_LE(std::max(font1.GetHeight(),
font2.GetHeight()),
On 2013/07/19 14:40:59, Yuki wrote:
> On 2013/07/17 17:30:32, msw wrote:
> > Should the height be calculated the same here as in FontList? (add the max
> > ascent and descent, instead of finding the max height directly). That's a
> subtle
> > but important piece of logic worth devising a separate test case for (that
> would
> > fail using a simple max of the heights), but its not critical if it's too
much
> > work to find suitable fonts/sizes.
> 
> Done.

nit: it'd be okay to keep this EXPECT_LE too, perhaps with a comment like:
"The combined height may be greater than the max height, since it's calculated
as the combined maximum ascent and descent."

https://codereview.chromium.org/19352002/diff/38001/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/19352002/diff/38001/ui/gfx/font_list_unittest...
ui/gfx/font_list_unittest.cc:253: // ascent of FontList == max(ascent of Fonts)
nit: Capitalize 'Ascent' here and 'Descent' below.

[msw, 2013-07-19 17:31:02]

Oh, please update your CL description to reflect the latest changes, thanks!

[commit-bot: I haz the power, 2013-07-20 13:59:23]

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

[commit-bot: I haz the power, 2013-07-20 15:42:48]

Change committed as 212792