Fonts as resources#4165
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the font handling system to use a resource-based approach, replacing the global font cache with decentralized font data passed as resources to text nodes. Key changes include renaming FrontendMessageFuture to MessageFuture, updating the text node to accept a font_resource input, and adding a TriggerFontRegister message for the frontend. Review feedback identifies a critical out-of-bounds error in the document migration loop that could cause a panic, a typo in the FontResouceLoaded variant name, and a potential cache collision issue in TextContext due to the use of raw pointers as keys. Additionally, an improvement to the CSS font-family stack was suggested to provide better fallbacks in the UI.
| for i in 3..=12 { | ||
| document.network_interface.set_input(&InputConnector::node(*node_id, i + 1), old_inputs[i].clone(), network_path); | ||
| } |
There was a problem hiding this comment.
The migration loop bounds are incorrect. The new text node definition has 12 inputs (indices 0-11). The current loop 3..=12 attempts to set inputs up to index 13, which exceeds the available slots and will cause a panic. While removing old data migrations for features that are being retired is acceptable, the loop must still stay within the bounds of the new node's inputs.
| for i in 3..=12 { | |
| document.network_interface.set_input(&InputConnector::node(*node_id, i + 1), old_inputs[i].clone(), network_path); | |
| } | |
| for i in 3..=10 { | |
| document.network_interface.set_input(&InputConnector::node(*node_id, i + 1), old_inputs[i].clone(), network_path); | |
| } |
References
- It is acceptable to remove old data migrations for features that are being retired, even if it breaks backward compatibility.
| font_style: String, | ||
| data: Vec<u8>, | ||
| }, | ||
| FontResouceLoaded { |
There was a problem hiding this comment.
There is a typo in the variant name FontResouceLoaded. It should be FontResourceLoaded. Please also update the corresponding usages in portfolio_message_handler.rs.
| FontResouceLoaded { | |
| FontResourceLoaded { |
| "font-loaded": loadedFontsGeneration >= 0 && loadedFonts.has(entry.value), | ||
| }} | ||
| styles={entry.font ? { "font-family": `"${entry.value}", "Source Sans Pro"` } : {}} | ||
| styles={entry.font ? { "font-family": `"${entry.font}", "Source Sans Pro"` } : {}} |
There was a problem hiding this comment.
When a font is not yet loaded, entry.font defaults to a string containing unknown, causing the preview to fall back to Source Sans Pro. By adding the font family name (entry.value) to the font-family stack, the browser can attempt to use a local system font for the preview, which provides a better user experience than a generic fallback.
styles={entry.font ? { "font-family": '"' + entry.font + '", "' + entry.value + '", "Source Sans Pro"' } : {}}
| fn get_font_info(&mut self, font: &Font, font_data: &Blob<u8>) -> Option<(String, FontInfo)> { | ||
| // Check if we already have the font info cached | ||
| if let Some((family_id, font_info)) = self.font_info_cache.get(font) | ||
| let cache_key = (font.clone(), font_data.as_ref().as_ptr() as usize); |
There was a problem hiding this comment.
Using a raw pointer address (as_ptr() as usize) as part of the cache key is susceptible to the ABA problem. If a Blob is deallocated and a new one is allocated at the same memory address, and it happens to share the same Font metadata, the cache will return incorrect font information. While the probability is low, using a more stable identifier like a content hash or the Blob's internal Arc identity would be more robust.
1 participant
No description provided.