Revert "Keep glyphs for variable fonts (#183857)"#184147
Merged
Merged
Conversation
gmackall
approved these changes
Mar 25, 2026
jason-simmons
added
the
autosubmit
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the font subsetting logic in main.cc to accommodate changes in HarfBuzz 3.0, specifically by re-adding explicit dropping of layout tables (GSUB/GPOS/GDEF) for non-variable fonts, which are now kept by default in the new HarfBuzz version. The previous logic for retaining all glyphs in variable fonts was removed, relying on the new HarfBuzz behavior to prevent rendering issues. A corresponding Python test case in test.py was adjusted to reflect these changes in variable font subsetting. Feedback includes improving the clarity and professionalism of a C++ comment in main.cc for better readability and maintainability, and reformatting a Python test case in test.py to a single line to enhance readability, both aligning with respective Google Style Guides.
Comment on lines
+74
to
+79
| // we can only drop GSUB/GPOS/GDEF for non variable fonts, they may be | ||
| // needed for variable fonts (guessing we need to keep all of these, but | ||
| // in Material Symbols Icon variable fonts if we drop the GSUB table (they | ||
| // do not have GPOS/DEF) then the Fill=1,Weight=100 variation is rendered | ||
| // incorrect. (and other variations are probably less noticibly | ||
| // incorrect)) |
Contributor
There was a problem hiding this comment.
This comment is hard to follow due to its informal language (e.g., "guessing") and structure. It can be rephrased for clarity and professionalism, which will improve code readability and maintainability.
According to the repository's style guide, which points to the Google C++ Style Guide, comments should be clear and explain the 'why'. Here is a suggested improvement:
Suggested change
| // we can only drop GSUB/GPOS/GDEF for non variable fonts, they may be | |
| // needed for variable fonts (guessing we need to keep all of these, but | |
| // in Material Symbols Icon variable fonts if we drop the GSUB table (they | |
| // do not have GPOS/DEF) then the Fill=1,Weight=100 variation is rendered | |
| // incorrect. (and other variations are probably less noticibly | |
| // incorrect)) | |
| // Drop layout tables for non-variable fonts only. | |
| // For variable fonts, these tables may be required for correct rendering. | |
| // For example, dropping the GSUB table in Material Symbols variable | |
| // fonts causes rendering issues for some variations (e.g. Fill=1, | |
| // Weight=100). | |
| // |
References
- C++ code should follow the Google C++ Style Guide. This guide emphasizes that comments should be readable and explain the rationale behind the code. The current comment is unclear and could be improved to better meet this standard. (link)
Comment on lines
+50
to
+51
| (False, '1variable.ttf', VARIABLE_MATERIAL_TTF, [r'57348' | ||
| ]), # False because different codepoint |
Contributor
There was a problem hiding this comment.
The current formatting of this test case is split across two lines in a way that harms readability. Combining it into a single line would make the code easier to understand.
(False, '1variable.ttf', VARIABLE_MATERIAL_TTF, [r'57348']), # False because different codepointReferences
- The repository style guide specifies using
yapfand following the Google Python Style Guide, which prioritizes readability. While auto-formatters are helpful, manual adjustments are sometimes necessary to improve clarity. (link)
auto-submit
Bot
removed
the
autosubmit
Contributor
|
autosubmit label was removed for flutter/flutter/184147, because - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
This reverts commit 8ae690c. Retaining every glyph in all variable fonts caused a major size increase for apps where the font subsetter worked adequately. For now, apps that are seeing issues can opt out of font subsetting by using --no-tree-shake-icons.
jason-simmons
force-pushed
the
revert_pr_183857
branch
from
4e7de84 to
1ba5421
Compare
jason-simmons
added
CICD
flutter-dashboard
Bot
removed
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
This was referenced Mar 26, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
mboetger
pushed a commit
to mboetger/flutter
that referenced
this pull request
Mar 26, 2026
) This reverts commit 8ae690c. Retaining every glyph in all variable fonts caused a major size increase for apps where the font subsetter worked adequately. For now, apps that are seeing issues can opt out of font subsetting by using --no-tree-shake-icons.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 26, 2026
auto-submit Bot
pushed a commit
to flutter/packages
that referenced
this pull request
Mar 26, 2026
…11372) Manual roll Flutter from 82d96ef98a33 to e79bf6cbc140 (32 revisions) Manual roll requested by [email protected] flutter/flutter@82d96ef...e79bf6c 2026-03-26 [email protected] Roll Dart SDK from a3af8949863e to 7587a31814c6 (1 revision) (flutter/flutter#184213) 2026-03-26 [email protected] Adds rockchip series to block list for vulkan. (flutter/flutter#184207) 2026-03-26 [email protected] [ios]add ci/cd label to ios triage (flutter/flutter#184214) 2026-03-26 [email protected] Roll Skia from 10c97361d8f3 to bee5a06ef578 (1 revision) (flutter/flutter#184203) 2026-03-26 [email protected] Roll Fuchsia Linux SDK from rS5ezRgehkw26fKRX... to BIlBJNOlKjQeRFoFy... (flutter/flutter#184197) 2026-03-26 [email protected] Roll Packages from 5909bdd to 0dd2410 (3 revisions) (flutter/flutter#184198) 2026-03-26 [email protected] fix: use atomic write for engine.stamp to prevent race conditions (flutter/flutter#184131) 2026-03-26 [email protected] Roll Skia from 77dfb68002cd to 10c97361d8f3 (1 revision) (flutter/flutter#184195) 2026-03-26 [email protected] Roll Dart SDK from 349dbbbdba99 to a3af8949863e (1 revision) (flutter/flutter#184194) 2026-03-26 [email protected] Roll Skia from 81ef2238cb09 to 77dfb68002cd (12 revisions) (flutter/flutter#184186) 2026-03-26 [email protected] Revert "Keep glyphs for variable fonts (#183857)" (flutter/flutter#184147) 2026-03-26 [email protected] Expand simple shape path optimization logic and move it from dl_dispatcher to dl_builder (flutter/flutter#184096) 2026-03-26 [email protected] Fix merge changelog workflow. (flutter/flutter#184145) 2026-03-26 [email protected] Add notes on HCPP to `Android-Platform-Views.md` (flutter/flutter#183859) 2026-03-26 [email protected] Adds explicit name to the cicd label job. (flutter/flutter#184070) 2026-03-26 [email protected] Roll Dart SDK from 26f80b9403f5 to 349dbbbdba99 (3 revisions) (flutter/flutter#184176) 2026-03-26 [email protected] Collect impeller analytics for appbundles (flutter/flutter#184146) 2026-03-26 [email protected] [Dot shorthands] Migrate examples/api/lib/material (flutter/flutter#183963) 2026-03-26 [email protected] Roll Dart DevTools to a version with correct CIPD tags (flutter/flutter#184172) 2026-03-25 [email protected] Pipes ScrollCacheExtent through more scroll views (flutter/flutter#184078) 2026-03-25 [email protected] Add widget of the week link in SensitiveContent documentation (flutter/flutter#183972) 2026-03-25 [email protected] Roll Packages from 8dcfd11 to 5909bdd (13 revisions) (flutter/flutter#184123) 2026-03-25 [email protected] Roll Dart SDK from c48f0c954d86 to 26f80b9403f5 (1 revision) (flutter/flutter#184117) 2026-03-25 [email protected] Roll Skia from 51725ae49ef5 to 81ef2238cb09 (3 revisions) (flutter/flutter#184115) 2026-03-25 [email protected] Roll Fuchsia Linux SDK from M3sjWggTQmP2AD4bS... to rS5ezRgehkw26fKRX... (flutter/flutter#184114) 2026-03-25 [email protected] Add SensitiveContent widget sample code (flutter/flutter#183846) 2026-03-25 [email protected] SelectableRegion should passthrough constraints to child unmodified (flutter/flutter#184083) 2026-03-25 [email protected] Roll Dart SDK from ce171d5026ff to c48f0c954d86 (2 revisions) (flutter/flutter#184105) 2026-03-25 [email protected] Roll Skia from 789ad6b12776 to 51725ae49ef5 (3 revisions) (flutter/flutter#184106) 2026-03-25 [email protected] Roll Skia from f4e59f82dc69 to 789ad6b12776 (7 revisions) (flutter/flutter#184098) 2026-03-25 [email protected] Roll Dart SDK from b08bd0a3ee39 to ce171d5026ff (4 revisions) (flutter/flutter#184095) 2026-03-24 [email protected] [flutter_goldens] Remove dead check on null being in a list of non-nullables (flutter/flutter#183938) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 ...
ahmedsameha1
pushed a commit
to ahmedsameha1/flutter
that referenced
this pull request
Apr 14, 2026
) This reverts commit 8ae690c. Retaining every glyph in all variable fonts caused a major size increase for apps where the font subsetter worked adequately. For now, apps that are seeing issues can opt out of font subsetting by using --no-tree-shake-icons.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts commit 8ae690c.
Retaining every glyph in all variable fonts caused a major size increase for apps where the font subsetter worked adequately.
For now, apps that are seeing issues can opt out of font subsetting by using --no-tree-shake-icons.