RESOLVED FIXED 145492
[WinCairo] ClearType should be enabled on Cairo port
https://bugs.webkit.org/show_bug.cgi?id=145492
Summary [WinCairo] ClearType should be enabled on Cairo port
Karlen Simonyan
Reported 2015-05-30 02:06:06 PDT
Created attachment 253958 [details] Cairo port text rendering behaviour for custom fonts with and without ClearType Overview: by default Cairo port uses CAIRO_ANTIALIAS_GRAY for text antialiasing, which results to using ANTIALIASED_QUALITY for Unicscribe text rendering on Windows platform instead of CLEARTYPE_QUALITY (corresponding Cairo antialiasing mode is CAIRO_ANTIALIAS_BEST). Steps to reproduce: Open any web page with embedded fonts (http://www.apple.com/mac/, for example). Actual result: some amount of text is rendered thin and semi-transparent (as it should NOT be). Expected result: smooth text as shown on the right side of the attachment (wincairo_text_rendering_diff.png).
Attachments
Cairo port text rendering behaviour for custom fonts with and without ClearType (52.27 KB, image/png)
2015-05-30 02:06 PDT, Karlen Simonyan
no flags
ClearType fix for custom fonts (681 bytes, patch)
2015-05-30 02:22 PDT, Karlen Simonyan
no flags
ClearType fix for custom fonts (v2) (676 bytes, patch)
2015-07-06 14:15 PDT, Karlen Simonyan
no flags
ClearType fix for custom fonts (v3) (676 bytes, patch)
2015-07-06 14:20 PDT, Karlen Simonyan
no flags
ClearType fix for custom fonts (v4) (787 bytes, patch)
2015-07-07 13:11 PDT, Karlen Simonyan
achristensen: review-
ClearType fix for custom fonts (v5) (1.45 KB, patch)
2015-07-08 12:51 PDT, Karlen Simonyan
bfulgham: review+
Karlen Simonyan
Comment 1 2015-05-30 02:22:57 PDT
Created attachment 253959 [details] ClearType fix for custom fonts
Jinyoung Hur
Comment 2 2015-07-03 09:58:21 PDT
It seems that this patch might help improving the quality of custom font rendering on Windows. Is there anybody to review this?
Jinyoung Hur
Comment 3 2015-07-05 17:48:41 PDT
Comment on attachment 253959 [details] ClearType fix for custom fonts Please fix the Bot Status, "patch does not apply...". It seems that your patch has a CR(carriage return) character, which is a violation of the WebKit coding convention. After correction, review flags should be set as "?" in order to be reviewed by any reviewers.
Karlen Simonyan
Comment 4 2015-07-06 14:15:44 PDT
Created attachment 256237 [details] ClearType fix for custom fonts (v2)
Karlen Simonyan
Comment 5 2015-07-06 14:20:00 PDT
Created attachment 256238 [details] ClearType fix for custom fonts (v3)
Karlen Simonyan
Comment 6 2015-07-06 14:26:02 PDT
Comment on attachment 256238 [details] ClearType fix for custom fonts (v3) >Index: FontPlatformDataCairoWin.cpp >=================================================================== >--- Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp (revision 186376) >+++ Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp (working copy) >@@ -77,7 +77,7 @@ > // We force antialiasing and disable hinting to provide consistent > // typographic qualities for custom fonts on all platforms. > cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE); >- cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY); >+ cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_BEST); > > if (syntheticOblique()) { > static const float syntheticObliqueSkew = -tanf(14 * acosf(0) / 90);
Karlen Simonyan
Comment 7 2015-07-06 14:27:05 PDT
Comment on attachment 256238 [details] ClearType fix for custom fonts (v3) >Index: FontPlatformDataCairoWin.cpp >=================================================================== >--- Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp (revision 186376) >+++ Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp (working copy) >@@ -77,7 +77,7 @@ > // We force antialiasing and disable hinting to provide consistent > // typographic qualities for custom fonts on all platforms. > cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE); >- cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY); >+ cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_BEST); > > if (syntheticOblique()) { > static const float syntheticObliqueSkew = -tanf(14 * acosf(0) / 90);
Karlen Simonyan
Comment 8 2015-07-06 14:28:54 PDT
(In reply to comment #3) > Comment on attachment 253959 [details] > ClearType fix for custom fonts > > Please fix the Bot Status, "patch does not apply...". > It seems that your patch has a CR(carriage return) character, which is a > violation of the WebKit coding convention. > After correction, review flags should be set as "?" in order to be reviewed > by any reviewers. Thanks Jinyoung for good point!
Jinyoung Hur
Comment 9 2015-07-06 19:27:30 PDT
You're welcome! However, your patch still looks not ready for a review. It seems that you have uploaded your patch manually rather than use the patch-uploading script, "webkit-patch". I believe the sign, "patch does not apply...", would disappear if you revise your patch using the script. There is an explanation how to use it in this page. (https://www.webkit.org/coding/contributing.html)
Karlen Simonyan
Comment 10 2015-07-07 13:11:33 PDT
Created attachment 256315 [details] ClearType fix for custom fonts (v4)
WebKit Commit Bot
Comment 11 2015-07-07 13:14:08 PDT
Attachment 256315 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alex Christensen
Comment 12 2015-07-07 17:06:51 PDT
The patch needs a ChangeLog entry. Use perl Tools/Scripts/prepare-ChangeLog to prepare an example
Alex Christensen
Comment 13 2015-07-08 10:17:41 PDT
Comment on attachment 256315 [details] ClearType fix for custom fonts (v4) This is likely an improvement that should go into WebKit, but please prepare a ChangeLog entry like the changes on trac.webkit.org
Karlen Simonyan
Comment 14 2015-07-08 12:51:46 PDT
Created attachment 256400 [details] ClearType fix for custom fonts (v5)
WebKit Commit Bot
Comment 15 2015-07-08 12:54:47 PDT
Attachment 256400 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Karlen Simonyan
Comment 16 2015-07-08 13:13:59 PDT
(In reply to comment #13) > Comment on attachment 256315 [details] > ClearType fix for custom fonts (v4) > > This is likely an improvement that should go into WebKit, but please prepare > a ChangeLog entry like the changes on trac.webkit.org Hi Alex! I've prepared new patch (https://bugs.webkit.org/attachment.cgi?id=256400) with ChangeLog entry. Hope everything is correct for now. Thank you.
Brent Fulgham
Comment 17 2015-07-08 14:41:59 PDT
Comment on attachment 256400 [details] ClearType fix for custom fonts (v5) View in context: https://bugs.webkit.org/attachment.cgi?id=256400&action=review This seems reasonable. r=me. > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). Do we not have any tests that exercise this code? At the very least, I would expect we would need to rebaseline test due to the improved quality of the font rendering.
Alex Christensen
Comment 18 2015-07-09 11:42:51 PDT
http://trac.webkit.org/changeset/186599 > Do we not have any tests that exercise this code? At the very least, I would expect we would need to rebaseline test due to the improved quality of the font rendering. WinCairo does not have layout test expectations right now. If it did, we would need to rebaseline some tests.
Note You need to log in before you can comment on or make changes to this bug.