Bug 145492 – [WinCairo] ClearType should be enabled on Cairo port

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Karlen Simonyan]

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).

[Karlen Simonyan]

Created attachment 253959 [details]
ClearType fix for custom fonts

[Jinyoung Hur]

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 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]

Created attachment 256237 [details]
ClearType fix for custom fonts (v2)

[Karlen Simonyan]

Created attachment 256238 [details]
ClearType fix for custom fonts (v3)

[Karlen Simonyan]

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 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]

(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]

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]

Created attachment 256315 [details]
ClearType fix for custom fonts (v4)

[WebKit Commit Bot]

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]

The patch needs a ChangeLog entry.  Use perl Tools/Scripts/prepare-ChangeLog to prepare an example

[Alex Christensen]

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]

Created attachment 256400 [details]
ClearType fix for custom fonts (v5)

[WebKit Commit Bot]

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]

(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 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]

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.