[web] Split all 1MB+ fallback fonts (including CJK) #56388
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
| if (failedUrl != null) { | ||
| throw ToolExit('Could not find license attribution at: $failedUrl'); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: unnecessary empty line
| return urlString.substring(expectedUrlPrefix.length); | ||
| } | ||
|
|
||
| Future<String?> _checkForLicenseAttributions( |
There was a problem hiding this comment.
document that this method returns a non-null String with the non-conforming license URL if the license fails the check
|
I still need to do something about these. |
I think instead of comparing the font to the (single) Noto Sans SC, TC, etc you can basically replace the checks for "if font == _notoSansSC" with "if font.name == 'Noto Sans SC'" |
Yep, that and there are also some |
| 'Noto Sans Cuneiform', | ||
| 'Noto Sans Duployan', | ||
| 'Noto Sans Egyptian Hieroglyphs', |
There was a problem hiding this comment.
If we don't want to bloat the main file in the general case... Have we thought of trimming the list of fallback fonts to "scripts currently in use today", with an opt-in to the full set? (Maybe vending 2 different versions of the font metadata file?)
| @@ -65,6 +65,13 @@ class RollFallbackFontsCommand extends Command<bool> | |||
| ...await _processSplitFallbackFonts(client, splitFallbackFonts), | |||
| ...await _processFallbackFonts(client, apiFallbackFonts), | |||
| ]; | |||
|
|
|||
| final String? failedUrl = await _checkForLicenseAttributions(client, fallbackFontInfo); | |||
There was a problem hiding this comment.
Maybe return a List<String> or Set<String> with all the fonts that might have failed, and then do if (failedUrls.isNotEmpty) to throw? That way we can report all the fonts that have a problem at once.
| final String urlString = fontUri.toString(); | ||
| if (!urlString.startsWith(expectedUrlPrefix)) { | ||
| throw ToolExit('Unexpected url format received from Google Fonts API: $urlString.'); | ||
| } | ||
| return urlString.substring(expectedUrlPrefix.length); |
There was a problem hiding this comment.
This should could be a regex :P (now we have two problems!)
By splitting all large fallback fonts (1MB+) into smaller parts, we get faster downloads and fast decoding.
Some fonts are split into 100+ parts, and that's causing
main.dart.js's size to grow by ~47KB (Brotli-compressed). The increase in size is due to the extra data we have to store about all the parts of these fonts.The PR also makes changes to ensure we don't download the same license file 100 times (once for each part of the split font).
Fixes flutter/flutter#138288
Part of flutter/flutter#153974