Make bigger string size in RenderTextHarfBuzz.

Make bigger string size in RenderTextHarfBuzz.

Some diacritics appear above the ascents, and some glyphs
like underscore may appear below descent. However, our code
base seems to assume a line from the ascent and the descent.

This CL makes enough string size for the gap between the top
bearing line and the ascent and the gap between bottom
bearing line and the descent so that they are drawn inside
of the specified area.

BUG=459812
R=msw@chromium.org, ckocagil@chromium.org
TEST=gfx_unittests still succeeds

[msw, 2015-02-21 00:07:40]

lgtm

[Jun Mukai, 2015-02-23 17:58:35]

The CQ bit was checked by mukai@chromium.org

[Jun Mukai, 2015-02-23 17:58:46]

The CQ bit was unchecked by mukai@chromium.org

[Jun Mukai, 2015-02-23 19:28:23]

New patchsets have been uploaded after l-g-t-m from msw@chromium.org

[Jun Mukai, 2015-02-23 21:56:18]

PTAL

The code changed very well -- it seems simply replacing fAscent/fDescent by
fTop/fBottom isn't a good idea.

[msw, 2015-02-24 23:31:36]

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:391: if (line == &lines_.front()) {
Why does only the first line add the extra height between |max_top_| and
|max_ascent_|? Why is that different than your change to make all lines add the
extra height between |max_bottom_| and |max_descent_|? Add an explanatory
comment in one or both places.

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

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1377: // If it's not empty, render_text's height
could be slightly bigger than
nit: |render_text|'s. Also, what does this mean by "If it's not empty"? Could
|render_text|'s height here be zero? Why would that be the case?

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1378: // the font height for the distance from
the top bearing line to the ascender.
nit: try to keep the wording of these comments as consistent as possible.

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1419: // render text height may not be exactly
same to smaller_font's height
nits: "|render_text|'s height", "exactly the same as", "|smaller_font|'s".

[Jun Mukai, 2015-02-25 23:49:19]

Done, but I noticed that styled_label_unittest.cc is still broken, and it seems
I'll have to fix the layout logic of styled_label (it assumes a line height
equals to the height between ascent / descent).

Also this would affect to the multiline logic of views::Label of
https://codereview.chromium.org/867003002/, so I'll leave this as-is and revisit
this once that CL is landed.

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:391: if (line == &lines_.front()) {
On 2015/02/24 23:31:35, msw wrote:
> Why does only the first line add the extra height between |max_top_| and
> |max_ascent_|? Why is that different than your change to make all lines add
the
> extra height between |max_bottom_| and |max_descent_|? Add an explanatory
> comment in one or both places.

Comments added.  Our code base seems to assume line height for the height
between ascent and descent, therefore changing this to the height between top
and bottom would make extra space between lines.

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

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1377: // If it's not empty, render_text's height
could be slightly bigger than
On 2015/02/24 23:31:36, msw wrote:
> nit: |render_text|'s. Also, what does this mean by "If it's not empty"? Could
> |render_text|'s height here be zero? Why would that be the case?

Fixed. I meant the text isn't empty (in this case the text is single character
whitespace " ")

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1378: // the font height for the distance from
the top bearing line to the ascender.
On 2015/02/24 23:31:35, msw wrote:
> nit: try to keep the wording of these comments as consistent as possible.

Done (I think...)

https://codereview.chromium.org/943093002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:1419: // render text height may not be exactly
same to smaller_font's height
On 2015/02/24 23:31:36, msw wrote:
> nits: "|render_text|'s height", "exactly the same as", "|smaller_font|'s".

Done.

[msw, 2015-02-26 00:47:21]

Okay, this makes way more sense now that I'm reading it again. +CC Dan and Peter
in case they have concerns or insight that I'm missing. Let me know when you
want another review pass.

https://codereview.chromium.org/943093002/diff/100001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/943093002/diff/100001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:268: // and the comments in AdvanceLine() below.
nit: you can remove this line "// and the comments in AdvanceLine() below."

https://codereview.chromium.org/943093002/diff/100001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:394: // Line height is assumed to be the height
between the ascent and the
nit: consider just using a comment like that above: "Add extra space above the
first line, because some characters (eg. A-ring) may be drawn above the ascent.
See http://crbug.com/459812"

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

https://codereview.chromium.org/943093002/diff/100001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:1378: // If the text is not empty,
|render_text|'s height could be slightly bigger
nit:   // |render_text| may be taller than the font to accommodate some glyphs.

https://codereview.chromium.org/943093002/diff/100001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:1422: // |render_text| height may not be exactly
same to |smaller_font|'s height
nit:   // |render_text| may be taller than the font to accommodate some glyphs.

https://codereview.chromium.org/943093002/diff/100001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:1440: // Again, it could be slightly different
from font's height, see above.
nit:  // |render_text| may be taller than the font to accommodate some glyphs.

[Peter Kasting, 2015-02-26 01:04:05]

The premise of this CL sounds strange.  I commented on the bug.