Bug 34735 – [Chromium] OpenType font with CFF glyphs is not handled correctly on Windows XP

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Yusuke Sato]

Chromium bug: http://crbug.com/31879

OpenType font with CFF glyphs is not handled correctly on Windows XP. Nonexistent glyphs are treated as existent (http://crbug.com/31879, http://crbug.com/30759, and http://crbug.com/32510) and vice versa (http://crbug.com/29805).

fillBMPGlyphs() function in platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp should be modified so the function can detect OpenType fonts with CFF glyphs:

http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp?rev=45325#L137
136         int invalidGlyph = 0xFFFF;
137	    if (!isVistaOrNewer() && !(tm.tmPitchAndFamily & TMPF_TRUETYPE))    // <----- TMPF_TRUETYPE is not set for OpenType fonts with CFF glyphs!!
138	        invalidGlyph = 0x1F;

[Yusuke Sato]

Created attachment 48386 [details]
opentype_detection_v1

[WebKit Review Bot]

Attachment 48386 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:137:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Sato]

Created attachment 48387 [details]
opentype_detection_v2_style_fixed

[Adam Barth]

Comment on attachment 48387 [details]
opentype_detection_v2_style_fixed

Looks reasonable, but I wish we had a test for this change.

I'm not an expert on the Windows font system.  You might want to run this by someone who's an expert on GlyphPageTreeNodeChromiumWin before landing.

[Yusuke Sato]

(In reply to comment #4)
Adam, reviewers,

Thanks for the review! I agree that it's better to add a test for this issue. I'll create _v3 patch that has a test something like "LayoutTests/fast/css/font-face-opentype.html."

Before starting this, let me ask two questions:

- To write the test, a sample OpenType font is necessary. Though we have some TrueType sample fonts (e.g. LayoutTests/fast/css/resources/Ahem.ttf) in WebKit tree, but we don't have any OpenType (CFF) fonts yet. What kind of fonts can we add to the WebKit tree, in terms of license, etc?
- This issue is Chromium only (and Windows XP only). Does this make sense to put the test HTML on LayoutTests/fast/css/? Is there better place to live?

Thanks,
Yusuke

[Adam Barth]

> - To write the test, a sample OpenType font is necessary. Though we have some
> TrueType sample fonts (e.g. LayoutTests/fast/css/resources/Ahem.ttf) in WebKit
> tree, but we don't have any OpenType (CFF) fonts yet. What kind of fonts can we
> add to the WebKit tree, in terms of license, etc?

In general, we accept patches under BSD and LGPL 2.1.  Hopefully there exist OpenType fonts under that license.

> - This issue is Chromium only (and Windows XP only). Does this make sense to
> put the test HTML on LayoutTests/fast/css/? Is there better place to live?

As long as the test passes on other platforms, it's good to have the test coverage.  :)

LayoutTests/fast/css seems fine, unless you see a directory more specific to @font-face.

[Yusuke Sato]

Created attachment 48702 [details]
opentype_detection_v3

[Yusuke Sato]

(In reply to comment #7)
Added a layout test. This test passes on Chrome with the patch and Safari, but fails on Chrome current release. 
The patch against GlyphPageTreeNodeChromiumWin.cpp is not changed at all.

[Kent Tamura]

Comment on attachment 48702 [details]
opentype_detection_v3

> --- /dev/null
> +++ b/LayoutTests/platform/mac/fast/css/font-face-opentype-expected.txt

It's hard to understand why this expectation is correct. I recommend:
 - Add a PNG result, or
 - Check the result by JavaScript code such as element.offsetWidth==n*16.

[Yusuke Sato]

(In reply to comment #9)
Sure, I'll add a PNG result.

I think there's no way (except polling) to check if a @font-face remote font has loaded. The <body onload=""> handler does not help since it might be called before the font loading finishes. Therefore, checking the offsetWidth from JavaScript seems not to be a good idea.

[Yusuke Sato]

Created attachment 48705 [details]
opentype_detection_v4_pixel_test_added

[Kent Tamura]

Comment on attachment 48705 [details]
opentype_detection_v4_pixel_test_added

> diff --git a/LayoutTests/platform/mac/fast/css/font-face-opentype-expected.png b/LayoutTests/platform/mac/fast/css/font-face-opentype-expected.png
> new file mode 100644

ok, the test seems good.

[Dimitri Glazkov (Google)]

Comment on attachment 48705 [details]
opentype_detection_v4_pixel_test_added

ok.

[WebKit Commit Bot]

Comment on attachment 48705 [details]
opentype_detection_v4_pixel_test_added

Clearing flags on attachment: 48705

Committed r54855: <http://trac.webkit.org/changeset/54855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.