WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
98100
[Chromium] Should set unitsPerEm in SimpleFontDataSkia.cpp
https://bugs.webkit.org/show_bug.cgi?id=98100
Summary
[Chromium] Should set unitsPerEm in SimpleFontDataSkia.cpp
Xianzhu Wang
Reported
2012-10-01 17:12:44 PDT
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.
Attachments
Patch
(286.25 KB, patch)
2012-10-02 14:29 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
Fix cr-linux build
(286.41 KB, patch)
2012-10-02 15:10 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
test case and font file updated
(287.57 KB, patch)
2012-10-02 18:39 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
For review
(286.93 KB, patch)
2012-10-03 09:02 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
Patch
(274.58 KB, patch)
2012-10-03 11:14 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Xianzhu Wang
Comment 1
2012-10-01 17:17:41 PDT
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
Comment 2
2012-10-02 14:29:44 PDT
Created
attachment 166751
[details]
Patch
WebKit Review Bot
Comment 3
2012-10-02 14:51:44 PDT
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
Comment 4
2012-10-02 15:10:00 PDT
Created
attachment 166761
[details]
Fix cr-linux build
Kenichi Ishibashi
Comment 5
2012-10-02 15:28:17 PDT
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
Comment 6
2012-10-02 15:47:56 PDT
(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
Comment 7
2012-10-02 15:57:46 PDT
(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 8
2012-10-02 16:01:48 PDT
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
Comment 9
2012-10-02 16:25:22 PDT
(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
Comment 10
2012-10-02 16:43:41 PDT
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
Comment 11
2012-10-02 16:56:17 PDT
(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
Comment 12
2012-10-02 17:14:18 PDT
(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 13
2012-10-02 17:44:29 PDT
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
Comment 14
2012-10-02 18:39:38 PDT
Created
attachment 166796
[details]
test case and font file updated
Xianzhu Wang
Comment 15
2012-10-02 18:41:24 PDT
(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
Comment 16
2012-10-03 09:02:14 PDT
Created
attachment 166907
[details]
For review
Mike Reed
Comment 17
2012-10-03 09:17:57 PDT
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
Comment 18
2012-10-03 09:18:41 PDT
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 19
2012-10-03 09:37:57 PDT
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 20
2012-10-03 10:06:41 PDT
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
Comment 21
2012-10-03 10:24:46 PDT
(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
Comment 22
2012-10-03 11:14:07 PDT
Created
attachment 166926
[details]
Patch
Xianzhu Wang
Comment 23
2012-10-03 15:33:53 PDT
James & Stephen, could you review the latest patch? Thanks!
James Robinson
Comment 24
2012-10-03 17:13:02 PDT
Comment on
attachment 166926
[details]
Patch I don't think I know enough about this system to review his patch
Stephen White
Comment 25
2012-10-03 18:48:51 PDT
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 26
2012-10-04 09:37:46 PDT
Comment on
attachment 166926
[details]
Patch Clearing flags on attachment: 166926 Committed
r130402
: <
http://trac.webkit.org/changeset/130402
>
WebKit Review Bot
Comment 27
2012-10-04 09:37:53 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug