Bug 135403 - Fonts forced to use non synthetic italics might be laid out with the incorrect baseline
Summary: Fonts forced to use non synthetic italics might be laid out with the incorrec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-29 16:14 PDT by Myles C. Maxfield
Modified: 2014-08-12 17:14 PDT (History)
9 users (show)

See Also:


Attachments
Patch (26.73 KB, patch)
2014-07-29 16:21 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (26.78 KB, patch)
2014-07-29 16:23 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (597.50 KB, application/zip)
2014-07-29 17:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (539.23 KB, application/zip)
2014-07-29 18:33 PDT, Build Bot
no flags Details
Patch (26.67 KB, patch)
2014-07-31 15:07 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (26.74 KB, patch)
2014-08-06 17:50 PDT, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-07-29 16:14:43 PDT
Fonts forced to use non synthetic italics might be laid out with the incorrect baseline
Comment 1 Myles C. Maxfield 2014-07-29 16:21:19 PDT
Created attachment 235712 [details]
Patch
Comment 2 Myles C. Maxfield 2014-07-29 16:23:14 PDT
Created attachment 235713 [details]
Patch
Comment 3 Myles C. Maxfield 2014-07-29 16:24:31 PDT
<rdar://problem/15114870>
Comment 4 Build Bot 2014-07-29 17:31:13 PDT
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
Comment 5 Build Bot 2014-07-29 17:31:16 PDT
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
Comment 6 Build Bot 2014-07-29 18:33:25 PDT
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
Comment 7 Build Bot 2014-07-29 18:33:28 PDT
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
Comment 8 Myles C. Maxfield 2014-07-31 15:07:07 PDT
Created attachment 235848 [details]
Patch
Comment 9 Pratik Solanki 2014-08-01 15:05:31 PDT
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.
Comment 10 Myles C. Maxfield 2014-08-06 17:50:05 PDT
Created attachment 236158 [details]
Patch
Comment 11 Darin Adler 2014-08-11 22:08:01 PDT
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?
Comment 12 Myles C. Maxfield 2014-08-12 10:44:43 PDT
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.
Comment 13 Myles C. Maxfield 2014-08-12 16:12:03 PDT
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.
Comment 14 Myles C. Maxfield 2014-08-12 16:14:12 PDT
https://trac.webkit.org/r172504
Comment 15 Brent Fulgham 2014-08-12 17:01:18 PDT
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)
Comment 16 Brent Fulgham 2014-08-12 17:14:22 PDT
Windows build fix landed in r172509. <https://trac.webkit.org/r172509>.