Bug 106009 – [Qt] Support -webkit-font-smoothing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

Qt currently does not respond to the -webkit-font-smoothing CSS property. It should be relatively simple to connect it to corresponding QFont properties though.

[Allan Sandfeld Jensen]

Created attachment 181163 [details]
Patch

[Pierre Rossi]

Comment on attachment 181163 [details]
Patch

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

LGTM but I'm just curious about the tests. Post-landing gardening expected ?

> Source/WebCore/ChangeLog:10
> +        Covered by existing tests.

Is it really ? No test expectations to update ?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 181163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181163&action=review
> 
> LGTM but I'm just curious about the tests. Post-landing gardening expected ?
> 
> > Source/WebCore/ChangeLog:10
> > +        Covered by existing tests.
> 
> Is it really ? No test expectations to update ?

Actually this is somewhat complicated. First of all it only touches the image results, and they are not currently clean. Secondly it differs between WK2 and WK1. It turns out WebKitTestRunner always sets Font::ShouldUseSmoothing to false.

I am considering making it ignore Font::ShouldUseSmoothing() to begin with. That should make it possible to only update the tests that explicitly disable font smoothing.

[Allan Sandfeld Jensen]

Oh yeah, and finally the image differences are often too small for Qt ImageDiff to pick them up. See bug #94782

[Allan Sandfeld Jensen]

Created attachment 181165 [details]
Patch

Ignore Font::shouldUseSmoothing for now.

[Allan Sandfeld Jensen]

Created attachment 193839 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 193839 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:79
> +#else

Those two logics are independent and one of them should be removed soon. I'm of the opinion that it would make it cleaner to have
#if < 5.1
featureA
#endif
#if >= 5.1
featureB
#endif

No strong feeling though.

[Allan Sandfeld Jensen]

Created attachment 193859 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 193839 [details]
Patch

The two ifdefs are both to minimize baselines and each branch sets StyleStrategy so they are related.

[Jocelyn Turcotte]

Comment on attachment 193839 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:77
>      // Kept enabled for Qt < 5.1 to maintain stable baselines for 5.0.

If you feel like it, you could reword this comment to cover the added block, or add a new one.

r=me as discussed.

[Allan Sandfeld Jensen]

Committed r146324: <http://trac.webkit.org/changeset/146324>

[Allan Sandfeld Jensen]

Committed r147623: <http://trac.webkit.org/changeset/147623>