Round glyph width and bounds if !SkPaint.isSubpixelText() in complex path

Round glyph width and bounds if !SkPaint.isSubpixelText() in complex path

This patch fixes HarfBuzzFace to round widths and bounds of glyphs
when SkPaint.isSubpixelText() is false.

SimpleFontData::platformWidthForGlyph() and platformBoundsForGlyph()
have this rounding. This difference in HarfBuzzFace caused poor text
positioning only in complex path.

Unlike SimpleFontData that uses round() for glyphBounds, this CL uses
roundOut() to avoid possible glyph rendering outside the visual
overflow rect. The same fix for SimpleFontData is in a separate CL[1].

[1] https://codereview.chromium.org/1326563003/

BUG=452914
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201592

[kojii, 2015-08-29 19:30:06]

Patchset #3 (id:40001) has been deleted

[kojii, 2015-08-29 19:30:44]

Patchset #3 (id:60001) has been deleted

[kojii, 2015-08-30 08:12:11]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2015-08-30 08:14:04]

PTAL.

The relevant part in SimpleFontData are getSkiaBoundsForGlyph:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
and platformWidthForGlyph:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[jbroman, 2015-08-30 16:08:06]

https://codereview.chromium.org/1316843005/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzFace.cpp (right):

https://codereview.chromium.org/1316843005/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzFace.cpp:155: if
(!paint->isSubpixelText()) {
drive-by question: Are these used to populate the visual overflow rect (and if
so, can use rounding here cause part of the glyph to render outside the visual
overflow rect)?

I'm only asking because I've been fighting bugs where we rounded when we should
have enclosed (roundOut, in Skia terms) the rectangle, and I don't want more of
them. :)

[kojii, 2015-08-30 18:05:30]

On 2015/08/30 16:08:06, jbroman wrote:
>
https://codereview.chromium.org/1316843005/diff/100001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/HarfBuzzFace.cpp (right):
> 
>
https://codereview.chromium.org/1316843005/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/HarfBuzzFace.cpp:155: if
> (!paint->isSubpixelText()) {
> drive-by question: Are these used to populate the visual overflow rect (and if
> so, can use rounding here cause part of the glyph to render outside the visual
> overflow rect)?
> 
> I'm only asking because I've been fighting bugs where we rounded when we
should
> have enclosed (roundOut, in Skia terms) the rectangle, and I don't want more
of
> them. :)

That's a good point. I'm not very familiar with visual overflows, but AFAIU,
yes, glyphBounds can populate visual overflow if glyphs are higher than
ascent/descent, or wider than advance.

In this case, whether actual pixel renders outside of rounded box or not
probably depends on the font renderer, so I think it's possible. It should be
happening today in the simple code path in
SimpleFontData::getSkiaBoundsForGlyph().

Do you have test cases that reproduce today where glyphs are rendered outside of
the visual overflow? I can then confirm if changing
SimpleFontData::getSkiaBoundsForGlyph() to roundOut() can fix the issue, and in
that case, this CL should also be fixed.

If you don't, from your comments, I'm leaning to fix both to roundOut() since
it's logically possible, and see what reviewers would say.

[kojii, 2015-08-31 08:56:52]

PTAL again, round() were changed to roundOut().

Visual overflow clipping test was fixed by this change:
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

[jbroman, 2015-09-01 15:07:42]

On 2015/08/31 at 08:56:52, kojii wrote:
> PTAL again, round() were changed to roundOut().
> 
> Visual overflow clipping test was fixed by this change:
>
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

Thanks! lgtm for addressing that, but naturally you should still get drott or
eae to approve. :)

[eae, 2015-09-01 16:24:38]

LGTM w/nit

The test is going to become obsolete pretty soon (when we remove the simple
path), perhaps a pixel test would be better?

https://codereview.chromium.org/1316843005/diff/180001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzFace.cpp (right):

https://codereview.chromium.org/1316843005/diff/180001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzFace.cpp:155: if
(!paint->isSubpixelText()) {
Would you mind adding a comment here as to why we're using roundOut?

[kojii, 2015-09-02 01:06:25]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-09-02 01:06:25]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1316843005/#ps200001
(title: "eae nits")

[commit-bot: I haz the power, 2015-09-02 01:06:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316843005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316843005/200001

[eae, 2015-09-02 01:06:53]

Thanks!

[commit-bot: I haz the power, 2015-09-02 01:11:38]

Committed patchset #9 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201592