[web] Add font fallback service#185314
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the FallbackFontService to provide a more resilient font fallback system for Flutter Web, utilizing Skia-driven discovery and a greedy selection algorithm with built-in retry logic. The review feedback identifies potential null-safety issues when accessing the font fallback manager and registry in the HTML renderer and recommends using for-in loops instead of forEach for side effects to comply with Effective Dart style guidelines.
| /// Adds a list of missing code points to be processed. | ||
| void addMissingCodePoints(List<int> codePoints) { | ||
| var added = false; | ||
| final FontFallbackManager manager = renderer.fontCollection.fontFallbackManager!; |
There was a problem hiding this comment.
The use of the non-null assertion operator (!) on renderer.fontCollection.fontFallbackManager is risky. While this service is primarily intended for the CanvasKit and Skwasm renderers, it is a global singleton. In environments where the HTML renderer is active, fontFallbackManager is null (as seen in WebFontCollection), which will cause a runtime exception here. A defensive null check should be added to ensure the service fails gracefully or remains inactive when the manager is unavailable. This pattern should also be applied to other occurrences in this file (lines 122, 260, 364, 483, 552).
| final FontFallbackManager manager = renderer.fontCollection.fontFallbackManager!; | |
| final FontFallbackManager? manager = renderer.fontCollection.fontFallbackManager; | |
| if (manager == null) { | |
| return; | |
| } |
There was a problem hiding this comment.
There is no HTML renderer. This code will only ever be hit from the Skwasm or CanvasKit renderers.
| final bool success = await renderer.fontCollection.fallbackFontRegistry!.loadFallbackFont( | ||
| font.name, | ||
| bytes, | ||
| ); |
There was a problem hiding this comment.
Similar to the fontFallbackManager access, using ! on fallbackFontRegistry is unsafe if the HTML renderer is active. Adding a null check here ensures robustness across different rendering backends.
final FallbackFontRegistry? registry = renderer.fontCollection.fallbackFontRegistry;
if (registry == null) {
break;
}
final bool success = await registry.loadFallbackFont(
font.name,
bytes,
);There was a problem hiding this comment.
There is no HTML renderer. This code will only ever be hit from the Skwasm or CanvasKit renderers.
| } | ||
|
|
||
| // Fire and Forget: Start downloads for the selected fonts. | ||
| newFonts.forEach(_startDownloadTask); |
There was a problem hiding this comment.
According to Effective Dart (which the Flutter style guide follows), it is preferred to use a for-in loop instead of forEach for side effects, especially when dealing with asynchronous tasks, even if they are not awaited. This improves readability and is more idiomatic.
for (final NotoFont font in newFonts) {
_startDownloadTask(font);
}References
- Effective Dart: Style recommends using for-in loops over forEach for side effects. (link)
There was a problem hiding this comment.
I get an analyzer warning if I switch from using forEach to for-in. I can't find the forEach suggestion in the Effective Dart: Style document. Hallucination?
mdebbar
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the comprehensive tests!
|
autosubmit label was removed for flutter/flutter/185314, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
ca5cc93 to
58efc1d
Compare
This change introduces a centralized
FallbackFontServiceto resolve infinite download loops through resilient failure tracking while significantly boosting text rendering performance by shifting missing character discovery directly to the underlying graphics engine.By replacing expensive string-based character scanning with a "source of truth" query to Skia for unresolved codepoints during layout, this update eliminates redundant processing and layout thrashing during paragraph building. The new service acts as a stateful coordinator that manages font downloads independently of the UI cycle, implementing a robust retry-and-recovery logic that marks failed assets as permanently unavailable to break infinite re-layout loops.
Fixes #184449
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.