Repository navigation
fonts: Add font variations support for macOS - #38760
Conversation
simonwuelker
left a comment
There was a problem hiding this comment.
This looks good to me.
I wonder if some of the optimizations you did (VARIATION_AXIS_INFORMATION_CACHE, checking if specified variation values are different from the default) are worth having in the freetype version as well.
| let cache = VARIATION_AXIS_INFORMATION_CACHE | ||
| .0 | ||
| .get_or_init(Default::default); | ||
| if let Some(information) = cache.read().get(font_identifier) { | ||
| return information.clone(); | ||
| } |
There was a problem hiding this comment.
Is reading the variation axes on a font really expensive enough to warrant a cache? It doesn't look very expensive and we only read the variations once when instantiating the same font with different variation parameters.
There was a problem hiding this comment.
Perhaps it isn't very expensive. I'm happy to remove the cache for now if you think it isn't necessary. We can always add it back later. Is there anything else you'd like before landing this? (noticing that you said LGTM, but didn't approve the change). I'm happy to make whatever changes you like.
There was a problem hiding this comment.
Let's remove the cache for now and add it back if querying the axes becomes expensive.
There was a problem hiding this comment.
Okay. I've removed the new cache now. PTAL.
| let core_text_font_no_variations = | ||
| Self::core_text_font(font_identifier.clone(), data, clamped_pt_size, &[])?; |
There was a problem hiding this comment.
We could save some locking here by acquiring the write lock before the call and replacing it with a manual lookup, but that seems like premature optimization...
This change adds font variation support for macOS. The main bulk of the change is reading the default, min, and max values for each variation axis from the font and instantiating a new CoreText font with the appropriate values. As part of this process, there is a new cache for storing these values from each font loaded. In addition, fonts with variations are now properly cached in the CoreText font cache. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
dee464b to
97d4f5d
Compare
|
@simonwuelker You've enabled auto-merge but not actually approved the PR. Was that intentional? It won't be merged until it has an approving review. |
thanks |
This change adds font variation support for macOS. The main bulk of the change is reading the default, min, and max values for each variation axis from the font and instantiating a new CoreText font with the appropriate values. In addition, fonts with variations are now properly cached in the CoreText font cache. Testing: There are no tests for this change as we do not run WPT tests for macOS and font variations are currently turned off by default. Eventually, when the feature is turned on there will be test for it. These changes are just laying the groundwork for the full implementation. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
This change adds font variation support for macOS. The main bulk of the change is reading the default, min, and max values for each variation axis from the font and instantiating a new CoreText font with the appropriate values. In addition, fonts with variations are now properly cached in the CoreText font cache. Testing: There are no tests for this change as we do not run WPT tests for macOS and font variations are currently turned off by default. Eventually, when the feature is turned on there will be test for it. These changes are just laying the groundwork for the full implementation. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
This change adds font variation support for macOS. The main bulk of the change is reading the default, min, and max values for each variation axis from the font and instantiating a new CoreText font with the appropriate values. In addition, fonts with variations are now properly cached in the CoreText font cache. Testing: There are no tests for this change as we do not run WPT tests for macOS and font variations are currently turned off by default. Eventually, when the feature is turned on there will be test for it. These changes are just laying the groundwork for the full implementation. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
This change adds font variation support for macOS. The main bulk of the
change is reading the default, min, and max values for each variation
axis from the font and instantiating a new CoreText font with the
appropriate values.
In addition, fonts with variations are now properly cached in the
CoreText font cache.
Testing: There are no tests for this change as we do not run WPT tests for
macOS and font variations are currently turned off by default. Eventually,
when the feature is turned on there will be test for it. These changes
are just laying the groundwork for the full implementation.