fonts: Port macOS font code to use objc2-* crates#41711
Conversation
servo-highfive
added
the
S-awaiting-review
|
@madsmtm I would be grateful if you could take a look at this one. I'm particularly unsure if I'm converting between the opaque versions of |
mrobinson
force-pushed
the
port-mac-fonts-to-use-objc2
branch
from
d6ae1b2 to
95d4c70
Compare
did you mean generation? |
Indeed. I've fixed this typo. |
madsmtm
left a comment
madsmtm
left a comment
There was a problem hiding this comment.
This is probably how I'd do the transition myself, so +1 from me.
Not that happy with how some things are a bit more cumbersome now, as I noted in servo/core-foundation-rs#729 there's still work to do on objc2-core-text (and it shows in this PR).
Two things I'm noting in particular is that it's annoying that madsmtm/objc2@90d4c33 is not yet released, and that things like CTFontManagerCopyAvailableFontFamilyNames still return untyped CFArrays (would be a breaking change, so I'm still on fixing that), but if you have other gripes, I'd love to know!
| &variation_keys | ||
| .iter() | ||
| .map(CFRetained::as_ref) | ||
| .collect::<Vec<_>>(), | ||
| &variation_values | ||
| .iter() | ||
| .map(CFRetained::as_ref) | ||
| .collect::<Vec<_>>(), |
There was a problem hiding this comment.
Remark: Ideally, objc2-core-foundation would allow you to avoid these extra allocations (probably by having variants of CFDictionary::from_slices similar to CFArray::from_retained_objects).
| .iter() | ||
| .map(|variation| CFNumber::new_f32(variation.value)) | ||
| .collect(); | ||
| let values_dict: CFRetained<CFDictionary<CFNumber, CFNumber>> = CFDictionary::from_slices( |
There was a problem hiding this comment.
FYI, I tend to prefer:
| let values_dict: CFRetained<CFDictionary<CFNumber, CFNumber>> = CFDictionary::from_slices( | |
| let values_dict = CFDictionary::<CFNumber, CFNumber>::from_slices( |
It makes it a bit easier on the eyes.
There was a problem hiding this comment.
Yeah, that looks a lot better.
| &glyphs[0], | ||
| self.ctfont.advances_for_glyphs( | ||
| CTFontOrientation::Default, | ||
| NonNull::new(&mut (glyph as CGGlyph))?, |
There was a problem hiding this comment.
Tip: You can use NonNull::from when converting references.
| NonNull::new(&mut (glyph as CGGlyph))?, | |
| NonNull::from(&mut (glyph as CGGlyph)), |
Similarly in a few other places.
mrobinson
changed the title
objc2-* cratesobjc2-* crates
|
Thanks for the review @madsmtm. We also end up having to make another copy of font data due to the lack of |
mrobinson
force-pushed
the
port-mac-fonts-to-use-objc2
branch
from
95d4c70 to
6fa15f4
Compare
|
Okay. I think this is ready for review now. |
servo-highfive
added
S-needs-rebase
mrobinson
force-pushed
the
port-mac-fonts-to-use-objc2
branch
from
6fa15f4 to
18524b4
Compare
servo-highfive
added
S-awaiting-review
This change moves Servo's macOS font code away from using our homegrown `core-*` crates and toward the more general-purpose `objc2-*` crates. Development of these crates is more active and they use automatic code general to have more complete coverage of the relevant platform APIs. In addition, this means that it is easier to understand Servo's code if you are familiar with the platform APIs as the `objc2` crate are a more direct Rust wrapper over them. In comparison, our wrappers had more batteries-included behavior that was less flexible. This change: - is the first step toward more flexible font fallback on macOS (servo#41426) - means we can now remove our manually FFI'd font variation code. Signed-off-by: Martin Robinson <[email protected]>
mrobinson
force-pushed
the
port-mac-fonts-to-use-objc2
branch
from
18524b4 to
99c646b
Compare
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
5 participants
This change moves Servo's macOS font code away from using our homegrown
core-*crates and toward the more general-purposeobjc2-*crates.Development of these crates is more active and they use automatic code
generation to have more complete coverage of the relevant platform APIs. In
addition, this means that it is easier to understand Servo's code if you
are familiar with the platform APIs as the
objc2crate are a moredirect Rust wrapper over them. In comparison, our wrappers had more
batteries-included behavior that was less flexible.
This change:
Testing: This should not change behavior and macOS is currently untested via WPT on the Ci.