Bug 76048 – [GTK] fast/text/font-kerning.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

Test added in r104678 fails: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r104693%20(16077)/fast/text/font-kerning-diffs.html

Can you debug this? A good place to start would be to look at Font::typesettingFeatures() as you enter Font::codePath() for each line in the test.

[Philippe Normand]

(In reply to comment #1)
> Can you debug this? A good place to start would be to look at Font::typesettingFeatures() as you enter Font::codePath() for each line in the test.

Will do, thanks for the hints Dan!

See also bug 76050.

[Philippe Normand]

Hum I can't reproduce this failure here :(

[arno.]

I can reproduce the bug.
Actually, for "-webkit-font-kerning: none; text-rendering: optimizelegibility", the complex text path is taken. That's probably correct because there may be some ligatures.
But unfortunately, it's not possible to disable kerning (nor ligatures) in the complex codepath. I tried to find a way to tell pango to not enable kerning, but I didn't suceed.

[arno.]

Created attachment 158643 [details]
testcase

[Denis Nomiyama (dnomi)]

Created attachment 202669 [details]
Patch

[Martin Robinson]

Comment on attachment 202669 [details]
Patch

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

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:502
> +        if (m_font->fontDescription().kerning() == FontDescription::NoneKerning) {
> +            Vector<hb_feature_t, 5> features = m_features;
> +            hb_feature_t kerning = { HB_TAG('k', 'e', 'r', 'n'), 0, 0, static_cast<unsigned>(-1) };
> +            features.append(kerning);
> +            hb_shape(harfBuzzFont.get(), harfBuzzBuffer.get(), features.data(), features.size());
> +        } else

Hrm. A more likely location for this change is HarfBuzzShaper::setFontFeatures where the rest of the feature are set, no? Aren't you also adding 'kern' to the feature list here instead of removing it?

[Denis Nomiyama (dnomi)]

Comment on attachment 202669 [details]
Patch

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

>> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:502
>> +        } else
> 
> Hrm. A more likely location for this change is HarfBuzzShaper::setFontFeatures where the rest of the feature are set, no? Aren't you also adding 'kern' to the feature list here instead of removing it?

Good point about setFontFeatures(). The idea is to add 'kern' as 0 (disabled) to the feature list in case of NoneKerning. I will also include a case for NormalKerning.

[Denis Nomiyama (dnomi)]

Created attachment 202724 [details]
Patch

[EFL EWS Bot]

Comment on attachment 202724 [details]
Patch

Attachment 202724 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/651023

[Denis Nomiyama (dnomi)]

Comment on attachment 202724 [details]
Patch

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

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:357
> +        kerning = { HarfBuzzFace::kernTag, 0, 0, static_cast<unsigned>(-1) };

I will remove this cheesy initialisation that requires c++11.

[Denis Nomiyama (dnomi)]

Created attachment 202791 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 202791 [details]
Patch

Clearing flags on attachment: 202791

Committed r152411: <http://trac.webkit.org/changeset/152411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.