Bug 98100 – [Chromium] Should set unitsPerEm in SimpleFontDataSkia.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xianzhu Wang]

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.

[Xianzhu Wang]

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();
+    }
 }

[Xianzhu Wang]

Created attachment 166751 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166751 [details]
Patch

Attachment 166751 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14129363

[Xianzhu Wang]

Created attachment 166761 [details]
Fix cr-linux build

[Kenichi Ishibashi]

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?

[Xianzhu Wang]

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

[Kenichi Ishibashi]

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

[WebKit Review Bot]

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

[Xianzhu Wang]

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

[James Robinson]

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?

[Xianzhu Wang]

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

[Kenichi Ishibashi]

(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

[Build Bot]

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

[Xianzhu Wang]

Created attachment 166796 [details]
test case and font file updated

[Xianzhu Wang]

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

[Xianzhu Wang]

Created attachment 166907 [details]
For review

[Mike Reed]

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.

[Mike Reed]

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.

[Build Bot]

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

[WebKit Review Bot]

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

[Xianzhu Wang]

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

[Xianzhu Wang]

Created attachment 166926 [details]
Patch

[Xianzhu Wang]

James & Stephen, could you review the latest patch? Thanks!

[James Robinson]

Comment on attachment 166926 [details]
Patch

I don't think I know enough about this system to review his patch

[Stephen White]

Comment on attachment 166926 [details]
Patch

I don't know a great deal about this code, but this looks ok.  r=me

[WebKit Review Bot]

Comment on attachment 166926 [details]
Patch

Clearing flags on attachment: 166926

Committed r130402: <http://trac.webkit.org/changeset/130402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.