Bug 34735 - [Chromium] OpenType font with CFF glyphs is not handled correctly on Windows XP
Summary: [Chromium] OpenType font with CFF glyphs is not handled correctly on Windows XP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-08 18:21 PST by Yusuke Sato
Modified: 2010-02-16 18:16 PST (History)
6 users (show)

See Also:


Attachments
opentype_detection_v1 (1.51 KB, patch)
2010-02-08 19:49 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
opentype_detection_v2_style_fixed (1.51 KB, patch)
2010-02-08 19:59 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
opentype_detection_v3 (9.24 KB, patch)
2010-02-13 06:20 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
opentype_detection_v4_pixel_test_added (46.51 KB, patch)
2010-02-13 08:43 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Sato 2010-02-08 18:21:07 PST
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;
Comment 1 Yusuke Sato 2010-02-08 19:49:35 PST
Created attachment 48386 [details]
opentype_detection_v1
Comment 2 WebKit Review Bot 2010-02-08 19:54:03 PST
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.
Comment 3 Yusuke Sato 2010-02-08 19:59:12 PST
Created attachment 48387 [details]
opentype_detection_v2_style_fixed
Comment 4 Adam Barth 2010-02-09 12:18:52 PST
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.
Comment 5 Yusuke Sato 2010-02-10 05:20:57 PST
(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
Comment 6 Adam Barth 2010-02-10 17:48:28 PST
> - 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.
Comment 7 Yusuke Sato 2010-02-13 06:20:06 PST
Created attachment 48702 [details]
opentype_detection_v3
Comment 8 Yusuke Sato 2010-02-13 06:22:55 PST
(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.
Comment 9 Kent Tamura 2010-02-13 06:50:29 PST
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.
Comment 10 Yusuke Sato 2010-02-13 08:42:33 PST
(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.
Comment 11 Yusuke Sato 2010-02-13 08:43:17 PST
Created attachment 48705 [details]
opentype_detection_v4_pixel_test_added
Comment 12 Kent Tamura 2010-02-13 08:53:05 PST
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.
Comment 13 Dimitri Glazkov (Google) 2010-02-16 12:46:47 PST
Comment on attachment 48705 [details]
opentype_detection_v4_pixel_test_added

ok.
Comment 14 WebKit Commit Bot 2010-02-16 18:16:32 PST
Comment on attachment 48705 [details]
opentype_detection_v4_pixel_test_added

Clearing flags on attachment: 48705

Committed r54855: <http://trac.webkit.org/changeset/54855>
Comment 15 WebKit Commit Bot 2010-02-16 18:16:38 PST
All reviewed patches have been landed.  Closing bug.