Bug 106009 - [Qt] Support -webkit-font-smoothing
Summary: [Qt] Support -webkit-font-smoothing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-03 03:24 PST by Allan Sandfeld Jensen
Modified: 2013-04-04 06:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.98 KB, patch)
2013-01-03 04:52 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2013-01-03 05:26 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2013-03-19 08:45 PDT, Allan Sandfeld Jensen
jturcotte: review+
Details | Formatted Diff | Diff
Patch (6.12 KB, patch)
2013-03-19 10:48 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-01-03 03:24:36 PST
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.
Comment 1 Allan Sandfeld Jensen 2013-01-03 04:52:12 PST
Created attachment 181163 [details]
Patch
Comment 2 Pierre Rossi 2013-01-03 05:09:20 PST
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 ?
Comment 3 Allan Sandfeld Jensen 2013-01-03 05:13:26 PST
(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.
Comment 4 Allan Sandfeld Jensen 2013-01-03 05:15:27 PST
Oh yeah, and finally the image differences are often too small for Qt ImageDiff to pick them up. See bug #94782
Comment 5 Allan Sandfeld Jensen 2013-01-03 05:26:54 PST
Created attachment 181165 [details]
Patch

Ignore Font::shouldUseSmoothing for now.
Comment 6 Allan Sandfeld Jensen 2013-03-19 08:45:33 PDT
Created attachment 193839 [details]
Patch
Comment 7 Jocelyn Turcotte 2013-03-19 10:30:07 PDT
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.
Comment 8 Allan Sandfeld Jensen 2013-03-19 10:48:22 PDT
Created attachment 193859 [details]
Patch
Comment 9 Allan Sandfeld Jensen 2013-03-20 03:52:22 PDT
Comment on attachment 193839 [details]
Patch

The two ifdefs are both to minimize baselines and each branch sets StyleStrategy so they are related.
Comment 10 Jocelyn Turcotte 2013-03-20 04:09:43 PDT
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.
Comment 11 Allan Sandfeld Jensen 2013-03-20 04:24:38 PDT
Committed r146324: <http://trac.webkit.org/changeset/146324>
Comment 12 Allan Sandfeld Jensen 2013-04-04 06:18:46 PDT
Committed r147623: <http://trac.webkit.org/changeset/147623>