At least on chromium-linux and chromium-android, unitsPerEm is not set according to the information in the font, causing at least problems in OpenTypeVerticalData when calculating vertical advance.
I'm making the final patch containing a layout test. Here is the patch for preview: diff --git a/Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp b/Source/WebCore/platform/graphics/sk index 3027e05..21f9962 100644 --- a/Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp +++ b/Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp @@ -159,6 +159,11 @@ void SimpleFontData::platformInit() } } } + + if (SkAdvancedTypefaceMetrics* advancedMetrics = paint.getTypeface()->getAdvancedTypefaceMetrics(SkAdvance + m_fontMetrics.setUnitsPerEm(advancedMetrics->fEmSize); + advancedMetrics->unref(); + } }
Created attachment 166751 [details] Patch
Comment on attachment 166751 [details] Patch Attachment 166751 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14129363
Created attachment 166761 [details] Fix cr-linux build
Comment on attachment 166761 [details] Fix cr-linux build View in context: https://bugs.webkit.org/attachment.cgi?id=166761&action=review > Source/WebCore/ChangeLog:7 > + Please add a description. > LayoutTests/fast/writing-mode/vertical-font-vmtx-units-per-em.html:14 > + src: url('resources/DroidSansFallback-reduced.ttf'); I'm not sure we can add the font to the repository. What is the license of the font?
(In reply to comment #5) > (From update of attachment 166761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166761&action=review > > > LayoutTests/fast/writing-mode/vertical-font-vmtx-units-per-em.html:14 > > + src: url('resources/DroidSansFallback-reduced.ttf'); > > I'm not sure we can add the font to the repository. What is the license of the font? It's of Apache 2.0 license. Actually any font with vhea and vmtx table and unitPerEm != 1000 can be used for the test. I'm not familiar with font files. It'd be good if the existing MikibaFont13.ttf meets the requirements.
(In reply to comment #6) > > I'm not sure we can add the font to the repository. What is the license of the font? > > It's of Apache 2.0 license. I see. I'd like to hear reviewers opinion on this. > Actually any font with vhea and vmtx table and unitPerEm != 1000 can be used for the test. I'm not familiar with font files. It'd be good if the existing MikibaFont13.ttf meets the requirements. Unfortunately, MaikibaFont13.ttf doesn't contain vhea/vmtx tables. The patch itself looks good to me.
Comment on attachment 166761 [details] Fix cr-linux build Attachment 166761 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14124553 New failing tests: fast/writing-mode/vertical-font-vmtx-units-per-em.html
(In reply to comment #8) > (From update of attachment 166761 [details]) > Attachment 166761 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14124553 > > New failing tests: > fast/writing-mode/vertical-font-vmtx-units-per-em.html I suppose the ews would give me something like layout-test-results.zip. Where can I get it? The test passes locally.
Test results aren't uploaded anywhere. I believe it's possible to retrieve from the bots but not trivial. Does the test pass locally for you on linux?
(In reply to comment #10) > Test results aren't uploaded anywhere. I believe it's possible to retrieve from the bots but not trivial. Does the test pass locally for you on linux? Yes. It passes locally for both chromium-linux and chromium-android. Actually the test expectations in the patch were locally generated.
(In reply to comment #11) > (In reply to comment #10) > > Test results aren't uploaded anywhere. I believe it's possible to retrieve from the bots but not trivial. Does the test pass locally for you on linux? > > Yes. It passes locally for both chromium-linux and chromium-android. Actually the test expectations in the patch were locally generated. Could you try to use setTimeout() to make sure the font is actually loaded? You can find an example at fast/writing-mode/japanese-rl-text-with-broken-font.html
Comment on attachment 166761 [details] Fix cr-linux build Attachment 166761 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14123536 New failing tests: fast/writing-mode/vertical-font-vmtx-units-per-em.html
Created attachment 166796 [details] test case and font file updated
(In reply to comment #12) > > Could you try to use setTimeout() to make sure the font is actually loaded? You can find an example at fast/writing-mode/japanese-rl-text-with-broken-font.html Thanks. That must be the reason.
Created attachment 166907 [details] For review
SkTypeface::getUnitsPerEm() will be more efficient (or the same) than your change. If android has it, you will be able to skip the #ifdef as well.
wait, its better than I thought. Clank gets its own copy of Skia, so we can be sure that they will have this API too.
Comment on attachment 166907 [details] For review Attachment 166907 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14132702 New failing tests: fast/writing-mode/vertical-font-vmtx-units-per-em.html
Comment on attachment 166907 [details] For review Attachment 166907 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14132707 New failing tests: fast/writing-mode/vertical-font-vmtx-units-per-em.html
(In reply to comment #18) > wait, its better than I thought. Clank gets its own copy of Skia, so we can be sure that they will have this API too. (In reply to comment #17) > SkTypeface::getUnitsPerEm() will be more efficient (or the same) than your change. If android has it, you will be able to skip the #ifdef as well. Thanks for the information. I thought it is still unavailable. I should have checked that. Chromium-android downstream has merged that.
Created attachment 166926 [details] Patch
James & Stephen, could you review the latest patch? Thanks!
Comment on attachment 166926 [details] Patch I don't think I know enough about this system to review his patch
Comment on attachment 166926 [details] Patch I don't know a great deal about this code, but this looks ok. r=me
Comment on attachment 166926 [details] Patch Clearing flags on attachment: 166926 Committed r130402: <http://trac.webkit.org/changeset/130402>
All reviewed patches have been landed. Closing bug.