[web] Split all 1MB+ fallback fonts (including CJK) #56388

mdebbar · 2024-11-05T21:27:26Z

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

flutter-dashboard · 2024-11-05T21:27:30Z

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.

flutter-dashboard · 2024-11-05T21:38:16Z

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.

harryterkelsen

LGTM!

lib/web_ui/dev/roll_fallback_fonts.dart

mdebbar · 2024-11-05T21:57:31Z

I still need to do something about these.

harryterkelsen · 2024-11-05T22:01:58Z

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'"

mdebbar · 2024-11-05T22:03:17Z

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 list.contains(_notoSansSC) that need to change.

ditman

Just a small suggestion to improve error reporting a little bit.

lib/web_ui/dev/roll_fallback_fonts.dart

mdebbar · 2024-11-07T14:53:22Z

@harryterkelsen @ditman would you mind taking another look? I fixed the logic for picking a font based on user preferred language. I want to make sure that part is reviewed too.

ditman

I don't see anything major, it seems the font selection stays the same?

ditman · 2024-11-07T22:01:31Z

lib/web_ui/lib/src/engine/font_fallbacks.dart

+  @visibleForTesting
+  String get debugUserPreferredLanguage => _language;
+
+  @visibleForTesting
+  set debugUserPreferredLanguage(String value) {
+    _language = value;
+  }


Suggested change

@visibleForTesting

String get debugUserPreferredLanguage => _language;

@visibleForTesting

set debugUserPreferredLanguage(String value) {

_language = value;

}

@visibleForTesting

String debugUserPreferredLanguage = _language;

Same effect, less code? :)

Not really. With your suggestion, debugUserPreferredLanguage will get out of sync with _language, and the code below won't pick up changes to debugUserPreferredLanguage.

ditman · 2024-11-07T22:07:15Z

lib/web_ui/lib/src/engine/font_fallbacks.dart

+          _isNotoSansHK(font) ||
+          _isNotoSansJP(font) ||
+          _isNotoSansKR(font))) {
+        if (_language == 'zh-Hans' ||


Can _language ever be just 'zh'?

Not sure. Revising the logic wasn't a goal for this PR. I was just trying to make the logic work the same way after the font split.

That said, I think you're right in the other comment that the zh case is handled later by the catch-all.

ditman · 2024-11-07T22:15:10Z

lib/web_ui/lib/src/engine/font_fallbacks.dart

+      // If the list of best fonts are all CJK fonts, choose the best one based
+      // on user preferred language. Otherwise just choose the first font.
      if (bestFonts.every((NotoFont font) =>


Do we need this to check that all bestFonts are CJK, or should this kick in when there's "more than one"?

That's a good question, and we may want to revise some of the logic in this file. But, again, this is a non-goal for this PR 🙂

ditman · 2024-11-07T22:23:51Z

lib/web_ui/lib/src/engine/font_fallbacks.dart

-        } else if (bestFonts.contains(_notoSansSC)) {
-          bestFont = _notoSansSC;
+        } else {
+          final notoSansSC = bestFonts.firstWhereOrNull(_isNotoSansSC);


I guess this final catch-all solves all the possible corner cases of CJK fonts, by defaulting to simplified chinese :P

I'm adding a test for this (i.e. lang="zh")

ditman · 2024-11-07T22:27:37Z

lib/web_ui/test/ui/fallback_fonts_golden_test.dart

+    final commonCJKCodePoints = <int>[
+      0x56de, // CJK Unified Ideographs
+      0x700b, // CJK Unified Ideographs
+      0x9c57, // CJK Unified Ideographs
+    ];


I don't think testing in more than one codepoint adds much to the test? This is testing the language selection mostly, right?

The original idea was to find code points that would naturally default to different fonts when no language is specified (e.g. one code point that defaults to JP and another that would default to KR). But I couldn't find any unfortunately.

I think I'll use just one code point and get rid of the for loop.

ditman · 2024-11-07T22:36:16Z

lib/web_ui/test/ui/fallback_fonts_golden_test.dart

+      for (final lang in <String>['zh-Hans', 'zh-CN', 'zh-SG', 'zh-MY', 'en-US']) {
+        // Simplified Chinese is also prioritized when preferred language is en-US.
+        test('prioritizes Noto Sans SC for code-point=$codePointHex lang=$lang', () async {
+          await checkDownloadedFamilyForCharCode(codePoint, 'Noto Sans SC', userPreferredLanguage: lang);
+        });
+      }
+
+      // Traditional Chinese
+      for (final lang in <String>['zh-Hant', 'zh-TW', 'zh-MO']) {
+        test('prioritizes Noto Sans TC for code-point=$codePointHex lang=$lang', () async {
+          await checkDownloadedFamilyForCharCode(codePoint, 'Noto Sans TC', userPreferredLanguage: lang);
+        });
+      }
+
+      // Hong Kong


You could simplify all these tests with:

const fontFamilyToPreferredLanguage = Map<String, List<String>> { 'Noto Sans SC': <String>['zh-Hans', 'zh-CN', 'zh-SG'...], 'Noto Sans TC': <String>['zh-Hant', 'zh-TW', ...], ... }

And iterating over its entries to generate all the possible tests without duplicating any code? :)

(It might make the actual test harder to find when it fails if we try to search by the error message or line number though)

I was actually leaning the opposite direction. I prefer better readability/searchability. I was thinking of removing all for loops and doing:

test('prioritizes Noto Sans TC for code-point=0x700b lang=zh-Hant', () async { await checkDownloadedFamilyForCharCode(codePoint, 'Noto Sans TC', userPreferredLanguage: 'zh-Hant'); }); test('prioritizes Noto Sans TC for code-point=0x700b lang=zh-TW', () async { await checkDownloadedFamilyForCharCode(codePoint, 'Noto Sans TC', userPreferredLanguage: 'zh-TW'); }); test('prioritizes Noto Sans TC for code-point=0x700b lang=zh-MO', () async { await checkDownloadedFamilyForCharCode(codePoint, 'Noto Sans TC', userPreferredLanguage: 'zh-MO'); });

The code duplication isn't bad honestly. All the logic lives in checkDownloadedFamilyForCharCode.

Now that you suggested testing only one code point, I have an even stronger feeling to remove all the for loops 🙂

harryterkelsen

LGTM

flutter-dashboard · 2024-11-08T19:22:30Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #56388 at sha 6706ced

…r/engine#56388)

…158402) flutter/engine@6b77347...b7134d3 2024-11-08 [email protected] fml: Use CFRef where possible and add docs (flutter/engine#56436) 2024-11-08 [email protected] [web] Split all 1MB+ fallback fonts (including CJK) (flutter/engine#56388) 2024-11-08 [email protected] yapf: Add more detailed error message and TODO (flutter/engine#56458) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

auto-submit · 2024-11-20T19:09:45Z

Time to revert pull request flutter/engine/56388 has elapsed.
You need to open the revert manually and process as a regular pull request.

This reverts commit cfed62b.

…6388) 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#138288 Part of flutter#153974

[web] Split all 1MB+ fallback fonts (including CJK)

a4792ba

mdebbar requested review from ditman and harryterkelsen November 5, 2024 21:27

github-actions bot added the platform-web Code specifically for the web engine label Nov 5, 2024

mdebbar marked this pull request as draft November 5, 2024 21:34

harryterkelsen approved these changes Nov 5, 2024

View reviewed changes

lib/web_ui/dev/roll_fallback_fonts.dart Show resolved Hide resolved

lib/web_ui/dev/roll_fallback_fonts.dart Outdated Show resolved Hide resolved

mdebbar added 2 commits November 5, 2024 21:42

roll fonts

370df40

address review comments

32d2ca7

mdebbar marked this pull request as ready for review November 5, 2024 21:46

ditman approved these changes Nov 5, 2024

View reviewed changes

lib/web_ui/dev/roll_fallback_fonts.dart Show resolved Hide resolved

lib/web_ui/dev/roll_fallback_fonts.dart Outdated Show resolved Hide resolved

lib/web_ui/dev/roll_fallback_fonts.dart Show resolved Hide resolved

mdebbar added 3 commits November 6, 2024 18:49

fix language-based font picking

26fd31e

address review comments

41c0dc9

Merge branch 'main' into split_cjk

b143bfd

Merge branch 'main' into split_cjk

f325c1a

ditman approved these changes Nov 7, 2024

View reviewed changes

harryterkelsen approved these changes Nov 7, 2024

View reviewed changes

mdebbar added 2 commits November 8, 2024 12:54

remove for-loops in CJK tests

97ffd8d

Merge branch 'main' into split_cjk

6706ced

mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2024

flutter-dashboard bot added the will affect goldens label Nov 8, 2024

auto-submit bot merged commit cfed62b into flutter:main Nov 8, 2024
38 checks passed

mdebbar deleted the split_cjk branch November 8, 2024 20:48

engine-flutter-autoroll mentioned this pull request Nov 8, 2024

Roll Flutter Engine from 6b77347edfc5 to b7134d373ef8 (3 revisions) flutter/flutter#158402

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2024

cfed62b11 [web] Split all 1MB+ fallback fonts (including CJK) (flutte…

21d21d9

…r/engine#56388)

mdebbar added the revert Label used to revert changes in a closed and merged pull request. label Nov 20, 2024

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Nov 20, 2024

mdebbar added a commit that referenced this pull request Nov 20, 2024

Revert "[web] Split all 1MB+ fallback fonts (including CJK) (#56388)"

00dd358

This reverts commit cfed62b.

mdebbar mentioned this pull request Nov 20, 2024

Revert "[web] Split all 1MB+ fallback fonts (including CJK)" #56729

Closed

[web] Split all 1MB+ fallback fonts (including CJK) #56388

[web] Split all 1MB+ fallback fonts (including CJK) #56388

Uh oh!

Conversation

mdebbar commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Nov 5, 2024

Uh oh!

flutter-dashboard bot commented Nov 5, 2024

Uh oh!

harryterkelsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdebbar commented Nov 5, 2024

Uh oh!

harryterkelsen commented Nov 5, 2024

Uh oh!

mdebbar commented Nov 5, 2024

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdebbar commented Nov 7, 2024

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harryterkelsen left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Nov 8, 2024

Uh oh!

Uh oh!

auto-submit bot commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdebbar commented Nov 5, 2024 •

edited

Loading