Implement downloadable fonts on Windows #15096

vvuk · 2017-01-18T15:49:51Z

This PR implements downloadable font support for Servo. It depends on new changes in webrender and dwrote, and adds a dependency on the truetype crate for pulling out basic font information. The original DirectWrite API does not provide an easy way to query font information direct from a FontFace (which is what you create from a FontFile). There are new DirectWrite APIs starting with Windows 10 that allow for this, but they are Win 10+ only, and winapi-rs does not yet have bindings for those versions of the interfaces (specificially, IDirectWriteFontFace3). The way to do this with DW is to go through a lot of pain in creating a custom collection and enumerator, add your font to your custom collection, then query the collection for its properties.

Instead, we just parse the truetype tables directly to pull out the few bits of information that we need. The truetype crate is ok, but I discovered some bugs (an update needs to get pushed to crates.io before this will build). It might be more worthwhile to just implement the tiny bit of truetype parsing that we need ourselves in Servo.

I'm guessing there are existing tests for downloadable fonts...

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-01-18T15:49:55Z

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!

emilio · 2017-01-18T15:51:39Z

components/gfx/platform/windows/font.rs

@@ -40,11 +42,211 @@ impl FontTableMethods for FontTable {
    }
 }

+fn make_tag(tag_bytes: &[u8]) -> FontTableTag {
+    assert!(tag_bytes.len() == 4);


nit: If you use assert_eq! this will display the length in the eventuality that it fails.

well, though it's only used statically.

emilio · 2017-01-18T15:54:08Z

components/gfx/platform/windows/font.rs

+            style: FontStyle::Normal,
+        };
+
+        if let Some(mut name_table_bytes) = face.get_font_table(make_tag(b"name")) {


nit: Maybe this would be a bit cleaner if you do something like:

let mut name_table_bytes = match face.get_font_table(make_tag(b"name")) { Some(table) => table, None => return Err(()), }; // Use name_table_bytes here.

This applies also to other places below.

vvuk · 2017-01-18T17:37:29Z

truetype 0.24.0 was just published to crates.io; going to rebase this

vvuk · 2017-01-18T18:35:30Z

Rebased and addressed review comments

emilio

Looks reasonable to me. Haven't audited the truetype crate, but it's (co-?)authored by @vvuk, so I think we can account you as responsible for it :)

License-wise it's fine (just for the record).

I've left mostly stylistic comments since most of the job is done in truetype.

r=me with it addressed

emilio · 2017-01-18T18:45:51Z

components/gfx/platform/windows/font.rs

+    let mut face_name_index = None;
+
+    for i in 0..records.len() {
+        // the truetype crate can only decode mac platform format names


Should we support this? If so, consider filling an issue and referencing it here.

Probably? I actually don't think we ever use the family and face name from downloaded fonts, since I believe you specify those in the @font-face anyway. If someone can confirm that, we can change this around to just give dummy UUIDs or something for family/face, and then the only thing left would be the OS/2 table, which is very simple to parse... in which case we could just do everything ourselves.

emilio · 2017-01-18T18:46:04Z

components/gfx/platform/windows/font.rs

+            continue;
+        }
+
+        if records[i].language_id != 0 {


Same for this I guess.

emilio · 2017-01-18T18:47:28Z

components/gfx/platform/windows/font.rs

+        let names = try_lossy!(NamingTable::read(&mut name_table_cursor));
+        let (family, face) = match names {
+            NamingTable::Format0(ref table) => {
+                if let Some((family_index, face_index)) = get_family_face_indices(&table.records) {


My previous comment also applied here and in the Format1 arm, though this is not a big deal.

Yeah; these were more compact so I didn't change them. I don't really like the mass duplicated code here anyway, but short of refactoring a bunch of stuff out I can't think of how I'd change it.

emilio · 2017-01-18T18:53:14Z

components/gfx/platform/windows/font.rs

+// names.  We want name_id 1 and 2, and we need to use platform_id == 1 and
+// language_id == 0 to avoid limitations in the truetype crate.  We *could* just
+// do our own parsing here, and use the offset/length data and pull the values out
+// ourselves.


Maybe make this a doc comment?

vvuk · 2017-01-18T19:34:42Z

Looks reasonable to me. Haven't audited the truetype crate, but it's (co-?)authored by @vvuk, so I think we can account you as responsible for it :)

Welllllllll... I made on PR for it, I don't know that it makes me an author :)

I filed #15103 for maybe fixing the language/encoding issues.

emilio · 2017-01-19T00:09:48Z

Oh, FYI you appear in the author list on https://crates.io/crates/truetype :)

Well, I opened #15105 and bodoni/truetype#5 regarding possible security implications we may need to care about in medium/long term.

Meanwhile this is ok to land IMO.

@bors-servo r+

bors-servo · 2017-01-19T00:09:48Z

📌 Commit a9611c3 has been approved by emilio

bors-servo · 2017-01-19T00:09:52Z

⌛ Testing commit a9611c3 with merge 1a7404e...

Implement downloadable fonts on Windows This PR implements downloadable font support for Servo. It depends on new changes in webrender and dwrote, and adds a dependency on the `truetype` crate for pulling out basic font information. The original DirectWrite API does not provide an easy way to query font information direct from a `FontFace` (which is what you create from a `FontFile`). There are new DirectWrite APIs starting with Windows 10 that allow for this, but they are Win 10+ only, and `winapi-rs` does not yet have bindings for those versions of the interfaces (specificially, `IDirectWriteFontFace3`). The way to do this with DW is to go through a lot of pain in creating a custom collection and enumerator, add your font to your custom collection, then query the collection for its properties. Instead, we just parse the truetype tables directly to pull out the few bits of information that we need. The `truetype` crate is ok, but I discovered some bugs (an update needs to get pushed to crates.io before this will build). It might be more worthwhile to just implement the tiny bit of truetype parsing that we need ourselves in Servo. I'm guessing there are existing tests for downloadable fonts... ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15096)

bors-servo · 2017-01-19T01:41:01Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned jdm Jan 18, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 18, 2017

emilio reviewed Jan 18, 2017

View reviewed changes

jdm added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Jan 18, 2017

jdm assigned emilio and unassigned jdm Jan 18, 2017

jdm added S-needs-rebase There are merge conflict errors. and removed S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. labels Jan 18, 2017

vvuk force-pushed the custom-font-files branch from 4d24edc to 704c945 Compare January 18, 2017 18:13

Implement downloadable fonts on Windows

a9611c3

vvuk force-pushed the custom-font-files branch from 704c945 to a9611c3 Compare January 18, 2017 18:35

jdm removed the S-needs-rebase There are merge conflict errors. label Jan 18, 2017

emilio reviewed Jan 18, 2017

View reviewed changes

vvuk mentioned this pull request Jan 18, 2017

Windows downloadable font support may need some fleshing out #15103

Closed

emilio mentioned this pull request Jan 19, 2017

How well does this crate handle invalid input? bodoni/truetype#5

Closed

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 19, 2017

bors-servo merged commit a9611c3 into servo:master Jan 19, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement downloadable fonts on Windows #15096

Implement downloadable fonts on Windows #15096

vvuk commented Jan 18, 2017 •

edited

highfive commented Jan 18, 2017

emilio Jan 18, 2017

emilio Jan 18, 2017

emilio Jan 18, 2017

vvuk commented Jan 18, 2017

vvuk commented Jan 18, 2017

emilio left a comment

emilio Jan 18, 2017

vvuk Jan 18, 2017

emilio Jan 18, 2017

emilio Jan 18, 2017

vvuk Jan 18, 2017

emilio Jan 18, 2017

vvuk commented Jan 18, 2017

emilio commented Jan 19, 2017

bors-servo commented Jan 19, 2017

bors-servo commented Jan 19, 2017

bors-servo commented Jan 19, 2017

Implement downloadable fonts on Windows #15096

Implement downloadable fonts on Windows #15096

Conversation

vvuk commented Jan 18, 2017 • edited

highfive commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvuk commented Jan 18, 2017

vvuk commented Jan 18, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvuk commented Jan 18, 2017

emilio commented Jan 19, 2017

bors-servo commented Jan 19, 2017

bors-servo commented Jan 19, 2017

bors-servo commented Jan 19, 2017

vvuk commented Jan 18, 2017 •

edited