Bug 99113 – Font’s fast code path doesn’t support kerning and ligatures

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

<rdar://problem/12470680>

Created attachment 168320 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

[kov's GTK+ EWS bot]

Comment on attachment 168320 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168320 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14256698

Created attachment 168322 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

[kov's GTK+ EWS bot]

Comment on attachment 168322 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168322 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14252745

[Early Warning System Bot]

Comment on attachment 168322 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168322 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14258756

[Peter Beverloo (cr-android ews)]

Comment on attachment 168322 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168322 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14256710

[WebKit Review Bot]

Comment on attachment 168322 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168322 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14249969

[Gyuyoung Kim]

Comment on attachment 168322 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168322 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14249988

Created attachment 168438 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

[Build Bot]

Comment on attachment 168438 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Attachment 168438 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14265369

Created attachment 168442 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

Created attachment 168465 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

[Tim Horton]

Comment on attachment 168465 [details]
Add kerning and ligature support on the fast code path (this patch excludes WebKitLibraries changes)

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

> Source/WebCore/platform/graphics/WidthIterator.cpp:104
> +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace)

Do we not have/use an enum for writing direction?

> Source/WebCore/platform/graphics/WidthIterator.cpp:130
> +    if (!ltr) {
> +        for (int i = 0, end = glyphBuffer->size() - 1; i < glyphBuffer->size() / 2; ++i, --end)
> +            glyphBuffer->swap(i, end);
> +    }

DRY? There's so little code here though, I guess it's ok.

(In reply to comment #13)
> (From update of attachment 168465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168465&action=review
> 
> > Source/WebCore/platform/graphics/WidthIterator.cpp:104
> > +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace)
> 
> Do we not have/use an enum for writing direction?

There is one, but WidthIterator is not using it yet.

> 
> > Source/WebCore/platform/graphics/WidthIterator.cpp:130
> > +    if (!ltr) {
> > +        for (int i = 0, end = glyphBuffer->size() - 1; i < glyphBuffer->size() / 2; ++i, --end)
> > +            glyphBuffer->swap(i, end);
> > +    }
> 
> DRY? There's so little code here though, I guess it's ok.

I will follow up with a patch moving this into GlyphBuffer.h, as it’s done in two more places.

Thanks for the review!

Fixed in <http://trac.webkit.org/r131365>.

[Dana Jansens]

This has broken the chromium mac build. It should be pretty easy to fix.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Builder/builds/33006/steps/compile/logs/stdio

The skia code needs to use width() as a function now. Height is not exposed on the advance struct anymore however. I could add it but that seems to be jumping the gun.

Can you please take a look and revert if this isn't easy to fix? Thanks.

[Dana Jansens]

nvm! http://trac.webkit.org/changeset/131374 thanks!

[Dana Jansens]

The following tests have started asserting on debug bots after this change:

Regressions: Unexpected crashes : (22)
  fast/css/first-letter-capitalized-edit-select-crash.html [ Crash ] 
  fast/css/text-rendering.html [ Crash ] 
  fast/ruby/modify-positioned-ruby-text-crash.html [ Crash ] 
  fast/text/complex-preferred-logical-widths.html [ Crash ] 
  fast/text/emphasis.html [ Crash ] 
  fast/text/font-kerning.html [ Crash ] 
  fast/text/font-variant-ligatures.html [ Crash ] 
  fast/text/international/text-spliced-font.html [ Crash ] 
  fast/text/international/vertical-text-glyph-test.html [ Crash ] 
  fast/text/international/vertical-text-metrics-test.html [ Crash ] 
  fast/text/ipa-tone-letters.html [ Crash ] 
  fast/text/justification-padding-mid-word.html [ Crash ] 
  fast/text/justify-ideograph-complex.html [ Crash ] 
  fast/text/justify-ideograph-leading-expansion.html [ Crash ] 
  fast/text/offsetForPosition-complex-fallback.html [ Crash ] 
  fast/text/regional-indicator-symobls.html [ Crash ] 
  fast/text/word-space-with-kerning-2.html [ Crash ] 
  fast/text/word-space-with-kerning.html [ Crash ] 
  fast/writing-mode/text-orientation-basic.html [ Crash ] 
  platform/chromium-linux/svg/text/text-with-geometric-precision.svg [ Crash ] 
  svg/W3C-SVG-1.1/filters-conv-01-f.svg [ Crash ] 
  svg/text/scaling-font-with-geometric-precision.html [ Crash ] 

The stacktrace for fast/css/first-letter-capitalized-edit-select-crash.html:
crash log for DumpRenderTree (pid 25383):
STDOUT: <empty>
STDERR: SHOULD NEVER BE REACHED
STDERR: third_party/WebKit/Source/WebCore/platform/graphics/SimpleFontData.h(206) : bool WebCore::SimpleFontData::applyTransforms(WebCore::GlyphBufferGlyph*, WebCore::GlyphBufferAdvance*, size_t, WebCore::TypesettingFeatures) const

STDERR: [25383:25383:17802909680655:ERROR:process_util_posix.cc(144)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7fccefb28fda]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7fccefb93579]
STDERR: 	0x7fcce8ac1af0
STDERR: 	WebCore::SimpleFontData::applyTransforms() [0x7fccf2de897e]
STDERR: 	WebCore::applyFontTransforms() [0x7fccf2de8474]
STDERR: 	WebCore::WidthIterator::advanceInternal<>() [0x7fccf2deae51]
STDERR: 	WebCore::WidthIterator::advance() [0x7fccf2de8749]
STDERR: 	WebCore::Font::floatWidthForSimpleText() [0x7fccf2da6bfa]
STDERR: 	WebCore::Font::width() [0x7fccf2d8ff28]
STDERR: 	WebCore::RenderText::computePreferredLogicalWidths() [0x7fccf38d6f66]
STDERR: 	WebCore::RenderText::width() [0x7fccf38d89bf]
STDERR: 	WebCore::textWidth() [0x7fccf37878dd]
STDERR: 	WebCore::RenderBlock::LineBreaker::nextLineBreak() [0x7fccf378a9fe]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x7fccf3783265]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloats() [0x7fccf3782b00]
STDERR: 	WebCore::RenderBlock::layoutInlineChildren() [0x7fccf378517a]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fccf37351fd]
STDERR: 	WebCore::RenderBlock::layout() [0x7fccf3734776]
STDERR: 	WebCore::RenderListItem::layout() [0x7fccf38588b8]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fccf373a44a]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fccf3739fc1]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fccf373521e]
STDERR: 	WebCore::RenderBlock::layout() [0x7fccf3734776]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fccf373a44a]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fccf3739fc1]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fccf373521e]
STDERR: 	WebCore::RenderBlock::layout() [0x7fccf3734776]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fccf373a44a]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fccf3739fc1]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fccf373521e]
STDERR: 	WebCore::RenderBlock::layout() [0x7fccf3734776]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fccf373a44a]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fccf3739fc1]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fccf373521e]
STDERR: 	WebCore::RenderBlock::layout() [0x7fccf3734776]
STDERR: 	WebCore::RenderView::layoutContent() [0x7fccf38f2222]
STDERR: 	WebCore::RenderView::layout() [0x7fccf38f27d6]

For fast/text/emphasis.html
crash log for DumpRenderTree (pid 27545):
STDOUT: <empty>
STDERR: SHOULD NEVER BE REACHED
STDERR: third_party/WebKit/Source/WebCore/platform/graphics/SimpleFontData.h(206) : bool WebCore::SimpleFontData::applyTransforms(WebCore::GlyphBufferGlyph*, WebCore::GlyphBufferAdvance*, size_t, WebCore::TypesettingFeatures) const

STDERR: [27545:27545:17803018430928:ERROR:process_util_posix.cc(144)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7fe5dd006fda]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7fe5dd071579]
STDERR: 	0x7fe5d5f9faf0
STDERR: 	WebCore::SimpleFontData::applyTransforms() [0x7fe5e02c697e]
STDERR: 	WebCore::applyFontTransforms() [0x7fe5e02c6474]
STDERR: 	WebCore::WidthIterator::advanceInternal<>() [0x7fe5e02c8e51]
STDERR: 	WebCore::WidthIterator::advance() [0x7fe5e02c6749]
STDERR: 	WebCore::Font::floatWidthForSimpleText() [0x7fe5e0284bfa]
STDERR: 	WebCore::Font::width() [0x7fe5e026df28]
STDERR: 	WebCore::RenderBlock::LineBreaker::nextLineBreak() [0x7fe5e0c67804]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x7fe5e0c61265]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloats() [0x7fe5e0c60b00]
STDERR: 	WebCore::RenderBlock::layoutInlineChildren() [0x7fe5e0c6317a]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fe5e0c131fd]
STDERR: 	WebCore::RenderBlock::layout() [0x7fe5e0c12776]
STDERR: 	WebCore::RenderObject::layoutIfNeeded() [0x7fe5e0be372b]
STDERR: 	WebCore::RenderBlock::insertFloatingObject() [0x7fe5e0c214f8]
STDERR: 	WebCore::RenderBlock::LineBreaker::skipLeadingWhitespace() [0x7fe5e0c655da]
STDERR: 	WebCore::RenderBlock::LineBreaker::nextLineBreak() [0x7fe5e0c664de]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x7fe5e0c61265]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloats() [0x7fe5e0c60b00]
STDERR: 	WebCore::RenderBlock::layoutInlineChildren() [0x7fe5e0c6317a]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fe5e0c131fd]
STDERR: 	WebCore::RenderBlock::layout() [0x7fe5e0c12776]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fe5e0c1844a]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fe5e0c17fc1]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fe5e0c1321e]
STDERR: 	WebCore::RenderBlock::layout() [0x7fe5e0c12776]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fe5e0c1844a]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fe5e0c17fc1]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fe5e0c1321e]
STDERR: 	WebCore::RenderBlock::layout() [0x7fe5e0c12776]

Here's a subset: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ftext%2Fjustify-ideograph-leading-expansion.html%2Cfast%2Ftext%2Fword-space-with-kerning-2.html%2Cfast%2Ftext%2Fcomplex-preferred-logical-widths.html%2Cfast%2Ftext%2Ffont-variant-ligatures.html%2Cfast%2Ftext%2Fjustification-padding-mid-word.html%2Cfast%2Ftext%2Fregional-indicator-symobls.html%2Cfast%2Ftext%2Fword-space-with-kerning.html%2Cfast%2Ftext%2Fjustify-ideograph-complex.html%2Cfast%2Ftext%2Fipa-tone-letters.html%2Cfast%2Ftext%2Femphasis.html%2Cfast%2Ftext%2Ffont-kerning.html%2Cfast%2Ftext%2FoffsetForPosition-complex-fallback.html

(In reply to comment #18)
> The following tests have started asserting on debug bots after this change:

I tried to fix that in <http://trac.webkit.org/r131375>.

(In reply to comment #19)
> (In reply to comment #18)
> > The following tests have started asserting on debug bots after this change:
> 
> I tried to fix that in <http://trac.webkit.org/r131375>.

This made things worse. I’ll fix that.

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > The following tests have started asserting on debug bots after this change:
> > 
> > I tried to fix that in <http://trac.webkit.org/r131375>.
> 
> This made things worse. I’ll fix that.

<http://trac.webkit.org/r131377>.