Fix FontTemplateDescriptor under FreeType #19928

jonleighton · 2018-02-01T18:13:52Z

Issue #17321. Under Linux, using "font-family: sans-serif" previously
caused Servo to select the "UltraLight" face (of DejaVu Sans). There were
two reasons for this:

Font weight was only retrieved from the OS/2 table for bold faces.
This neglected to retrieve the weight information for "lighter than
normal" weight faces. This meant that the UltraLight face appeared as
normal weight, and was selected.
Retrieval of font stretch information from the OS/2 table was not
implemented at all.

This change is

highfive · 2018-02-01T18:13:57Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify gfx code, but no tests are modified. Please consider adding a test!

jonleighton · 2018-02-01T18:15:02Z

I would like to implement tests for this change, but need some guidance. I asked about this in IRC but got no response:

Hi! I was just looking at a font-related issue and decided to run tests/wpt/web-platform-tests/css/css-fonts on my machine. I got a bunch of failures, and then I realised that these tests are not run on the CI (it's configured in tests/wpt/include.ini). Why is that? What would be the best way to ensure a font-related change is properly tested?

I see two options (or both):

Enable the css-fonts WPT tests
Write unit tests

Advice please!

jdm · 2018-02-01T18:29:40Z

Go ahead and enable the tests. They were added when we weren't paying attention; there's no reason I know of not to run them in Servo.


        Enable css-fonts WPT tests

See servo#19928 (comment) Note that some of these tests require you to install the fonts found in tests/wpt/web-platform-tests/css/fonts.


        Auto merge of #19930 - jonleighton:enable-wpt-css-fonts, r=jdm

@jdm

Enable css-fonts WPT tests See #19928 (comment) Note that some of these tests require you to install the fonts found in tests/wpt/web-platform-tests/css/fonts. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19930)


        Enable css-fonts WPT tests

See servo#19928 (comment) Note that some of these tests require you to install the fonts found in tests/wpt/web-platform-tests/css/fonts.


        Auto merge of #19930 - jonleighton:enable-wpt-css-fonts, r=jdm

@jdm

Enable css-fonts WPT tests See #19928 (comment) Note that some of these tests require you to install the fonts found in tests/wpt/web-platform-tests/css/fonts. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19930)


        Fix missing whitespace between functions

jonleighton · 2018-02-03T21:13:51Z

I've been looking into how to test this and have hit several roadblocks after enabling the css-fonts tests.

The change to how the font weight is detected is captured by css/css-fonts/font-weight-normal-001.xht. (I haven't found an existing test which captures the font stretch fix.) However, it doesn't easily show this bug fix for two reasons:

That test is marked as a manual test in WPT, even though it's actually a reftest. Therefore it won't run on the CI. See web-platform-tests/wpt#9374.
That test fails still fails after my fix due to presumably other bugs in how we are implementing the font matching algorithm. The rendering of the test changes, but it still doesn't pass (per my manual testing with and without this patch).

Therefore I propose to write a unit test to encapsulate this change.

jdm · 2018-02-04T05:06:21Z

A unit test sounds sensible, in that case.

jonleighton · 2018-02-04T17:09:44Z

OK, I've added a test. I tried to use the existing test fonts in tests/wpt/web-platform-tests/css/fonts/CSSTest/, but there were no condensed/expanded versions to test the font-stretch detection. So I added the DejaVu fonts. The addition of the descriptor() method yielded a nice cleanup for the existing methods which load the descriptor.

jonleighton · 2018-02-04T17:11:41Z

components/gfx/font_context.rs

@@ -9,7 +9,7 @@ use font_cache_thread::FontCacheThread;
 use font_template::FontTemplateDescriptor;
 use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
 use platform::font::FontHandle;
-use platform::font_context::FontContextHandle;
+pub use platform::font_context::FontContextHandle;


I had to do this for the test, which doesn't seem ideal but I struggled to find an alternative. In the existing code, a FontContext instantiates a FontContextHandle, but instantiating a FontContext requires lots of dependencies (some of them external to this crate too).

jonleighton · 2018-02-05T19:34:42Z

The test I wrote failed on Windows but it's fixed now.

jonleighton · 2018-02-06T13:52:14Z

@emilio has reviewed and says he's happy with the code, but he's unsure about whether there are licensing issues caused by the inclusion of the DejaVu fonts for testing: https://mozilla.logbot.info/servo/20180206#c14250864-c14250932

metajack · 2018-02-06T14:32:44Z

The license is fine.

jdm · 2018-02-06T15:02:02Z

@bors-servo r=emilio,jack

bors-servo · 2018-02-06T15:02:03Z

📌 Commit ecf441d has been approved by emilio,jack

bors-servo · 2018-02-06T15:02:09Z

⌛ Testing commit ecf441d with merge b03899e...


        Auto merge of #19928 - jonleighton:issue-17321, r=emilio,jack

Fix FontTemplateDescriptor under FreeType Issue #17321. Under Linux, using "font-family: sans-serif" previously caused Servo to select the "UltraLight" face (of DejaVu Sans). There were two reasons for this: 1. Font weight was only retrieved from the OS/2 table for bold faces. This neglected to retrieve the weight information for "lighter than normal" weight faces. This meant that the UltraLight face appeared as normal weight, and was selected. 2. Retrieval of font stretch information from the OS/2 table was not implemented at all.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19928)

bors-servo · 2018-02-06T15:18:59Z

💔 Test failed - mac-dev-unit


        Auto merge of #795 - jonleighton:patch-1, r=jdm

@metajack

Add my SSH key In servo/servo#19928 @metajack suggested that I could use one of the builders to work on Mac-specific code and @jdm requested I submit my key in a PR here.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/795)

bors-servo · 2018-02-07T20:05:09Z

💔 Test failed - mac-dev-unit


        Fix FontTemplateDescriptor under FreeType

Issue #17321. Under Linux, using "font-family: sans-serif" previously caused Servo to select the "UltraLight" face (of DejaVu Sans). There were two reasons for this: 1. Font weight was only retrieved from the OS/2 table for bold faces. This neglected to retrieve the weight information for "lighter than normal" weight faces. This meant that the UltraLight face appeared as normal weight, and was selected. 2. Retrieval of font stretch information from the OS/2 table was not implemented at all.

jdm · 2018-02-08T13:35:03Z

@bors-servo r+

bors-servo · 2018-02-08T13:35:04Z

📌 Commit 446b0e4 has been approved by jdm

bors-servo · 2018-02-08T14:03:22Z

⌛ Testing commit 446b0e4 with merge 2032b46...


        Auto merge of #19928 - jonleighton:issue-17321, r=jdm

Fix FontTemplateDescriptor under FreeType Issue #17321. Under Linux, using "font-family: sans-serif" previously caused Servo to select the "UltraLight" face (of DejaVu Sans). There were two reasons for this: 1. Font weight was only retrieved from the OS/2 table for bold faces. This neglected to retrieve the weight information for "lighter than normal" weight faces. This meant that the UltraLight face appeared as normal weight, and was selected. 2. Retrieval of font stretch information from the OS/2 table was not implemented at all.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19928)

bors-servo · 2018-02-08T14:21:09Z

💔 Test failed - linux-dev

jdm · 2018-02-08T14:25:51Z

@bors-servo retry

freed up space on servo-linux6

bors-servo · 2018-02-08T15:37:15Z

⌛ Testing commit 446b0e4 with merge 1ed6010...


        Auto merge of #19928 - jonleighton:issue-17321, r=jdm

Fix FontTemplateDescriptor under FreeType Issue #17321. Under Linux, using "font-family: sans-serif" previously caused Servo to select the "UltraLight" face (of DejaVu Sans). There were two reasons for this: 1. Font weight was only retrieved from the OS/2 table for bold faces. This neglected to retrieve the weight information for "lighter than normal" weight faces. This meant that the UltraLight face appeared as normal weight, and was selected. 2. Retrieval of font stretch information from the OS/2 table was not implemented at all.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19928)

bors-servo · 2018-02-08T16:07:48Z

💔 Test failed - linux-rel-wpt

jdm · 2018-02-08T16:17:41Z

@bors-servo retry

#13509

bors-servo · 2018-02-08T16:17:44Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css1, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3...

bors-servo · 2018-02-08T16:44:18Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 1ed6010 to master...

highfive assigned metajack Feb 1, 2018

highfive added the S-awaiting-review label Feb 1, 2018

jonleighton added a commit to jonleighton/servo that referenced this pull request Feb 1, 2018

Enable css-fonts WPT tests

caae3eb

See servo#19928 (comment) Note that some of these tests require you to install the fonts found in tests/wpt/web-platform-tests/css/fonts.

jonleighton mentioned this pull request Feb 1, 2018

Enable css-fonts WPT tests #19930

Merged

jonleighton added a commit to jonleighton/servo that referenced this pull request Feb 1, 2018

Enable css-fonts WPT tests

f578ec9

See servo#19928 (comment) Note that some of these tests require you to install the fonts found in tests/wpt/web-platform-tests/css/fonts.

Fix missing whitespace between functions

e9b9398

jonleighton force-pushed the jonleighton:issue-17321 branch from 9f867ac to 38622c7 Feb 4, 2018

jonleighton reviewed Feb 4, 2018

View changes

jonleighton force-pushed the jonleighton:issue-17321 branch 2 times, most recently from fd68b22 to ecf441d Feb 4, 2018

highfive assigned emilio and unassigned metajack Feb 6, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 6, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 7, 2018

jonleighton force-pushed the jonleighton:issue-17321 branch from ae1020f to 446b0e4 Feb 7, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Feb 7, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 8, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 8, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 8, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 8, 2018

bors-servo merged commit 446b0e4 into servo:master Feb 8, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

jonleighton mentioned this pull request Feb 8, 2018

Fix detection of font weight on Mac #19998

Open

servo / servo

Fix FontTemplateDescriptor under FreeType #19928

Fix FontTemplateDescriptor under FreeType #19928

jonleighton commented Feb 1, 2018 •

edited by SimonSapin

highfive commented Feb 1, 2018

jonleighton commented Feb 1, 2018

jdm commented Feb 1, 2018

jonleighton commented Feb 3, 2018

jdm commented Feb 4, 2018

jonleighton commented Feb 4, 2018

This comment has been minimized.

jonleighton commented Feb 5, 2018

jonleighton commented Feb 6, 2018

metajack commented Feb 6, 2018

jdm commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 7, 2018

jdm commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

jdm commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

jdm commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

servo / servo

Join GitHub today

Fix FontTemplateDescriptor under FreeType #19928

Fix FontTemplateDescriptor under FreeType #19928

Conversation

jonleighton commented Feb 1, 2018 • edited by SimonSapin

highfive commented Feb 1, 2018

jonleighton commented Feb 1, 2018

jdm commented Feb 1, 2018

jonleighton commented Feb 3, 2018

jdm commented Feb 4, 2018

jonleighton commented Feb 4, 2018

This comment has been minimized.

jonleighton Feb 4, 2018

jonleighton commented Feb 5, 2018

jonleighton commented Feb 6, 2018

metajack commented Feb 6, 2018

jdm commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 7, 2018

jdm commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

jdm commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

jdm commented Feb 8, 2018

bors-servo commented Feb 8, 2018

bors-servo commented Feb 8, 2018

jonleighton commented Feb 1, 2018 •

edited by SimonSapin