Bug 65903 – [Chromium] Implement text shaping with font-feature-settings on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kenichi Ishibashi]

Implement text shaping with font-feature-settings CSS property.  This bug entry is for Chromium Linux port.

[Kenichi Ishibashi]

Created attachment 103342 [details]
Patch V0

[Gustavo Noronha (kov)]

Comment on attachment 103342 [details]
Patch V0

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

[WebKit Review Bot]

Comment on attachment 103342 [details]
Patch V0

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

[Gyuyoung Kim]

Comment on attachment 103342 [details]
Patch V0

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

[Kenichi Ishibashi]

Created attachment 103343 [details]
Patch V1 (fix compile error)

[Evan Martin]

Comment on attachment 103343 [details]
Patch V1 (fix compile error)

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

This looks great, but I am a bit worried about performance...

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:212
> +        HB_GSUB_Clear_Features(hbFace->gsub);

Is this clearing any leftover cached gsub data on the hbFace?

I wonder if it is expensive to reload this data repeatedly; we call setupFontForScriptRun for every span of text on a page...

[Kenichi Ishibashi]

Created attachment 103444 [details]
Patch V2

[Kenichi Ishibashi]

Comment on attachment 103343 [details]
Patch V1 (fix compile error)

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

Hi Evan,

Thank you for your review.  I've revised the patch.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:212
>> +        HB_GSUB_Clear_Features(hbFace->gsub);
> 
> Is this clearing any leftover cached gsub data on the hbFace?
> 
> I wonder if it is expensive to reload this data repeatedly; we call setupFontForScriptRun for every span of text on a page...

As far as I checked, HB_{GPOS,GSUB}_Clear_Features() just reset some fields to zero and don't clean cached data. I think it is not so expensive.  I revised the patch to reduce calling setupFontFeatures() either way.

[Evan Martin]

Comment on attachment 103444 [details]
Patch V2

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

LGTM

> Source/WebCore/platform/graphics/Font.cpp:274
> +        return Complex;

One more question: this change here affects all WebKit ports.  Will it be a problem for the other ports?

I guess .featureSettings() is only available when the page uses these new CSS keywords, which should be very rare, right?

[Adam Barth]

Comment on attachment 103444 [details]
Patch V2

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

The harfbuzz code is a mystery to me, but I believe that evmar knows that he's talking about.  :)

>> Source/WebCore/platform/graphics/Font.cpp:274
>> +    if (m_fontDescription.featureSettings() && m_fontDescription.featureSettings()->size() > 0)
>> +        return Complex;
> 
> One more question: this change here affects all WebKit ports.  Will it be a problem for the other ports?
> 
> I guess .featureSettings() is only available when the page uses these new CSS keywords, which should be very rare, right?

I'm not an expert here, but this seems ok to me.  -webkit-font-feature-settings kicking us into complex mode shouldn't be too much of a problem.

[WebKit Review Bot]

Comment on attachment 103444 [details]
Patch V2

Clearing flags on attachment: 103444

Committed r92783: <http://trac.webkit.org/changeset/92783>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

Thank you for r+ing, Adam!

(In reply to comment #10)
> > I guess .featureSettings() is only available when the page uses these new CSS keywords, which should be very rare, right?

Yes, .featureSettings() is only available when there are -webkit-font-feature-settings properties on the page, and it should be very rare.

[Andrei Popescu]

It looks like the css3/font-feature-settings-rendering.html LT added in

http://trac.webkit.org/changeset/92783

is missing expectations for Chromium. The actual output also looks wrong:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=css3%2Ffont-feature-settings-rendering.html

I think the test was marked as failing for MAC and WIN in Chromium test expectations file, when it should have been skipped or marked as missing expectations.

See http://trac.webkit.org/changeset/92837

[Kenichi Ishibashi]

(In reply to comment #14)
> I think the test was marked as failing for MAC and WIN in Chromium test expectations file, when it should have been skipped or marked as missing expectations.

My fault.  Thank you for fixing expectations.