Bug 135403 – Fonts forced to use non synthetic italics might be laid out with the incorrect baseline

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Fonts forced to use non synthetic italics might be laid out with the incorrect baseline

[Myles C. Maxfield]

Created attachment 235712 [details]
Patch

[Myles C. Maxfield]

Created attachment 235713 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15114870>

[Build Bot]

Comment on attachment 235713 [details]
Patch

Attachment 235713 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4751354992001024

New failing tests:
fast/text/international/synthesized-italic-vertical-latin-double.html

[Build Bot]

Created attachment 235718 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235713 [details]
Patch

Attachment 235713 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6569534907482112

New failing tests:
fast/text/international/synthesized-italic-vertical-latin-double.html

[Build Bot]

Created attachment 235723 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

Created attachment 235848 [details]
Patch

[Pratik Solanki]

Comment on attachment 235848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235848&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=135403

Please add a link to the radar.

[Myles C. Maxfield]

Created attachment 236158 [details]
Patch

[Darin Adler]

Comment on attachment 236158 [details]
Patch

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?

[Myles C. Maxfield]

Comment on attachment 236158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236158&action=review

>> Source/WebCore/ChangeLog:17
>> +        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?

Whoops; this was a typo; I meant to say "by passing 'false.'" I've removed some of the details in the above paragraph which should make it easier to understand.

>> Source/WebCore/platform/graphics/SimpleFontData.cpp:245
>> +        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.

Done.

>> LayoutTests/ChangeLog:10
>> +        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?

Yep - when the glyphs aren't cached, we don't ask the existing SimpleFontData to provide a non-synthetic-italic version of itself (because there is no existing SimpleFontData), so this code path gets skipped entirely.

I'll see what I can do about a reftest.

[Myles C. Maxfield]

Comment on attachment 236158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236158&action=review

>>> LayoutTests/ChangeLog:10
>>> +        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?
> 
> Yep - when the glyphs aren't cached, we don't ask the existing SimpleFontData to provide a non-synthetic-italic version of itself (because there is no existing SimpleFontData), so this code path gets skipped entirely.
> 
> I'll see what I can do about a reftest.

Done.

[Myles C. Maxfield]

https://trac.webkit.org/r172504

[Brent Fulgham]

This broke the Windows build due to some missing symbols. I'll fix it.

     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "class WebCore::FontCache & __cdecl WebCore::fontCache(void)" (?fontCache@WebCore@@YAAAVFontCache@1@XZ) referenced in function "public: void __thiscall WebCore::Internals::invalidateFontCache(void)" (?invalidateFontCache@Internals@WebCore@@QAEXXZ)
     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::FontCache::invalidate(void)" (?invalidate@FontCache@WebCore@@QAEXXZ) referenced in function "public: void __thiscall WebCore::Internals::invalidateFontCache(void)" (?invalidateFontCache@Internals@WebCore@@QAEXXZ)

[Brent Fulgham]

Windows build fix landed in r172509. <https://trac.webkit.org/r172509>.