Summary: | [Qt] Support -webkit-font-smoothing | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||
Component: | Layout and Rendering | Assignee: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | noam, ossy, pierre.rossi, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Allan Sandfeld Jensen
2013-01-03 03:24:36 PST
Created attachment 181163 [details]
Patch
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 ? (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. Oh yeah, and finally the image differences are often too small for Qt ImageDiff to pick them up. See bug #94782 Created attachment 181165 [details]
Patch
Ignore Font::shouldUseSmoothing for now.
Created attachment 193839 [details]
Patch
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. Created attachment 193859 [details]
Patch
Comment on attachment 193839 [details]
Patch
The two ifdefs are both to minimize baselines and each branch sets StyleStrategy so they are related.
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. Committed r146324: <http://trac.webkit.org/changeset/146324> Committed r147623: <http://trac.webkit.org/changeset/147623> |