Fonts forced to use non synthetic italics might be laid out with the incorrect baseline
Created attachment 235712 [details] Patch
Created attachment 235713 [details] Patch
<rdar://problem/15114870>
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
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 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
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
Created attachment 235848 [details] Patch
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.
Created attachment 236158 [details] Patch
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 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 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.
https://trac.webkit.org/r172504
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)
Windows build fix landed in r172509. <https://trac.webkit.org/r172509>.