linux: Make RenderTextPango avoid clipping underlines.

linux: Make RenderTextPango avoid clipping underlines.

Adjust underline positions so the lines won't get clipped. I
think that the inaccuracy may be occurring due to our
conversion from PangoFontDescriptions (usually containing a
point-based size) to a rounded pixel-based size and then
back to a PangoFontDescription. Luckily, RenderTextPango
will be replaced by RenderTextHarfBuzz soon.

Also delete some dead code in PlatformFontPango for
computing underline metrics.

BUG=393117
TEST=manual: checked that the "Import bookmarks now..."
     underline is visible after setting my UI font to
     "Ubuntu 11"

Committed: https://crrev.com/e22312d2327d29a092171a6ab13de12654a36b25
Cr-Commit-Position: refs/heads/master@{#292537}

[Daniel Erat, 2014-08-28 20:37:23]

derat@chromium.org changed reviewers:
+ asvitkine@chromium.org, msw@chromium.org

[Daniel Erat, 2014-08-28 20:37:23]

Mike, would you mind building this to check if it also fixes the problem when
using whichever font configuration you have?

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

https://codereview.chromium.org/509093004/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:67: int descent_pixels =
PANGO_PIXELS(pango_font_metrics_get_descent(metrics));
it's also possible that this isn't the right fix and it just happens to work. is
the underline even supposed to fit within the descent? am i misinterpreting the
units here and treating a point that's in the middle of a pixel as being at the
edge of a pixel instead?

i mainly just want to fix this for the default ubuntu config in a way that can
be merged back to 38 so we can limp to 39 and delete RenderTextPango in favor of
RenderTextHarfBuzz... :-/

[Alexei Svitkine (slow), 2014-08-29 00:07:46]

Seems reasonable to me. LGTM.

[Daniel Erat, 2014-08-29 00:09:02]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-08-29 00:10:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/509093004/1

[msw, 2014-08-29 00:41:55]

lgtm with a question. Please ensure this doesn't break underlines for your
original font settings where this bug didn't repro.

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

https://codereview.chromium.org/509093004/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:67: int descent_pixels =
PANGO_PIXELS(pango_font_metrics_get_descent(metrics));
On 2014/08/28 20:37:23, Daniel Erat wrote:
> it's also possible that this isn't the right fix and it just happens to work.
is
> the underline even supposed to fit within the descent? am i misinterpreting
the
> units here and treating a point that's in the middle of a pixel as being at
the
> edge of a pixel instead?
> 
> i mainly just want to fix this for the default ubuntu config in a way that can
> be merged back to 38 so we can limp to 39 and delete RenderTextPango in favor
of
> RenderTextHarfBuzz... :-/

I honestly don't know the Pango underline code that well. Could there be an
issue in our ordering of quantizing/PANGO_PIXEL'ing versus actually using
|underline_position_| in SkiaTextRenderer::DrawUnderline? If this works for now,
it's probably okay, given the impending switch to HarfBuzz. Feel free to ping
Alexei, Dave, or other relevant authors for more knowledgeable review.

[commit-bot: I haz the power, 2014-08-29 01:12:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[commit-bot: I haz the power, 2014-08-29 01:27:24]

Committed patchset #1 (id:1) as d8872d912640ac6936e2ef8f03e2e8e8d6ed9867

[Daniel Erat, 2014-08-29 02:49:44]

On 2014/08/29 00:41:55, msw wrote:
> lgtm with a question. Please ensure this doesn't break underlines for your
> original font settings where this bug didn't repro.

oh, didn't mention it, but yes, i tested this. i didn't see any changes to the
DejaVu Sans 10/11 and Ubuntu 10 underlining with this change; it just fixed
Ubuntu 11.

> https://codereview.chromium.org/509093004/diff/1/ui/gfx/render_text_pango.cc
> File ui/gfx/render_text_pango.cc (right):
> 
>
https://codereview.chromium.org/509093004/diff/1/ui/gfx/render_text_pango.cc#...
> ui/gfx/render_text_pango.cc:67: int descent_pixels =
> PANGO_PIXELS(pango_font_metrics_get_descent(metrics));
> On 2014/08/28 20:37:23, Daniel Erat wrote:
> > it's also possible that this isn't the right fix and it just happens to
work.
> is
> > the underline even supposed to fit within the descent? am i misinterpreting
> the
> > units here and treating a point that's in the middle of a pixel as being at
> the
> > edge of a pixel instead?
> > 
> > i mainly just want to fix this for the default ubuntu config in a way that
can
> > be merged back to 38 so we can limp to 39 and delete RenderTextPango in
favor
> of
> > RenderTextHarfBuzz... :-/
> 
> I honestly don't know the Pango underline code that well. Could there be an
> issue in our ordering of quantizing/PANGO_PIXEL'ing versus actually using
> |underline_position_| in SkiaTextRenderer::DrawUnderline? If this works for
now,
> it's probably okay, given the impending switch to HarfBuzz. Feel free to ping
> Alexei, Dave, or other relevant authors for more knowledgeable review.

[commit-bot: I haz the power, 2014-09-10 03:04:52]

Patchset 1 (id:??) landed as
https://crrev.com/e22312d2327d29a092171a6ab13de12654a36b25
Cr-Commit-Position: refs/heads/master@{#292537}