[webkit-reviews] review granted: [Bug 135403] Fonts forced to use non synthetic italics might be laid out with the incorrect baseline : [Attachment 236158] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 135403: Fonts forced to use non synthetic italics might be laid out with
the incorrect baseline
https://bugs.webkit.org/show_bug.cgi?id=135403

Attachment 236158: Patch
https://bugs.webkit.org/attachment.cgi?id=236158&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236158&action=review


I’m saying review+ but I am not super-happy with this. See comments below.

> Source/WebCore/ChangeLog:17
> +	   alphabetic baseline; otherwise, use the ideographic baseline). By
passing "true" to
> +	   the isTextOrientationFallback argument to SimpleFontData::create(),
we preserve this
> +	   hasVerticalGlyphs flag.

This comment is very confusing. It sounds like it is saying we should be
passing "true", but the patch actually changes us from passing "true" to
passing "false".

Can you write a simpler comment that is less confusing?

> Source/WebCore/platform/graphics/SimpleFontData.cpp:245
> -	   m_derivedFontData->nonSyntheticItalic =
create(nonSyntheticItalicFontPlatformData, isCustomFont(), false, true);
> +	   m_derivedFontData->nonSyntheticItalic =
create(nonSyntheticItalicFontPlatformData, isCustomFont(), false, false);

Never was there a more eloquent argument against boolean constant arguments.

I suggest omitting both false arguments; false is the default for both of them.


> LayoutTests/ChangeLog:10
> +	   The second <p> goes through the nonSyntheicItalicFontData()
codepath. Make sure
> +	   that it is laid out using the correct baseline.

Typo: nonSyntheic

Why does only the second <p> go through that path? I see no difference between
the first and second <p> that would explain that. Are you saying that it’s
different because the glyphs are cached?

Can this test be a reference test somehow? I can imagine making the text color
white for the first paragraph and then in the reference having only one
paragraph. But that would only work if the font cache was flushed, I guess?