Bundle a local Roboto fallback for no-CDN web builds#184275
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to bundle a local Roboto font fallback for web builds when the kUseLocalCanvasKitFlag is enabled. The WebReleaseBundle target was modified to verify the presence of the Roboto font in the FontManifest.json and, if absent, copy the font from the engine source to the assets directory. Review feedback recommends refactoring hardcoded engine font paths into constants in both the implementation and test files to enhance maintainability and adhere to readability principles.
| final File sourceRobotoFont = environment.fileSystem.file( | ||
| environment.fileSystem.path.join( | ||
| Cache.flutterRoot!, | ||
| 'engine', | ||
| 'src', | ||
| 'flutter', | ||
| 'txt', | ||
| 'third_party', | ||
| 'fonts', | ||
| 'Roboto-Regular.ttf', | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The path to the Roboto font is constructed from multiple hardcoded string literals. This is brittle and can lead to breakage if the engine's internal directory structure changes. It also impacts readability.
To improve maintainability and align with the style guide's principle to "Optimize for readability", I recommend extracting this path into a private, top-level constant. This provides a single source of truth for the path and makes the code easier to understand and update.
Here's how you could refactor it:
-
Add a constant at the top of the file:
const String _kEngineRobotoFontPath = 'engine/src/flutter/txt/third_party/fonts/Roboto-Regular.ttf';
-
Use this constant in the
_bundleLocalRobotoFallbackmethod:final File sourceRobotoFont = environment.fileSystem.file( environment.fileSystem.path.join( Cache.flutterRoot!, _kEngineRobotoFontPath, ), );
This change will make the dependency on this specific engine file more explicit and easier to manage in the future.
References
- Optimize for readability: Code is read more often than it is written. (link)
|
Thanks for the contribution @samanzamani! It look like you'll need to sign the CLA before this can be submitted. Is there an issue filed that this PR is addressing? I don't have any serious concerns with this change, but I'd like to check in with the team to make sure this is the right way to handle this case. We might want to put this behavior behind a flag instead, or this might just be working as intended with the solution being to manually add the font to the manifest explicitly. I'm personally not too familiar with the default set of fonts, so I'm also not sure if limiting this to Roboto is sufficient. cc @mdebbar for input on the PR and @justinmc for confirmation on the set of "default" fonts. |
90b99db to
9dfd709
Compare
|
Hi @bkonyi , thanks for the review. Regarding the CLA — my first commit was authored with a local machine email instead of my GitHub-associated email. I've just rebased and amended both commits with the correct email and force-pushed. The CLA check should pass now. As for the PR itself — there isn't a specific issue filed for this. I noticed that when using --web-resources-cdn flag with local CanvasKit, the Roboto fallback font wasn't being bundled if it wasn't already in the font manifest. This caused missing font issues in offline/no-CDN web builds. |
There was a problem hiding this comment.
Looks good to me!
We've always wanted to do this for Roboto and fallback fonts on the web, so this change is welcome!
To answer @bkonyi's question, yes, Roboto is the only font that we download by default on the web. We download fallback fonts for non-Latin languages, but that happens on-demand. Starting with Roboto being available offline is a good step forward. Thank you for the contribution @samanzamani !
You need to rebase to resolve the ci.yaml validation error.
|
Hi @samanzamani! It looks like there's some analysis errors that are going to block this from being merged. |
3c30892 to
e9ab31c
Compare
|
Hi @bkonyi . I rebased this PR onto the latest upstream master, resolved the merge conflict, and fixed the flutter_tools analysis lints that were blocking it. |
|
@mdebbar can I get you to reapprove this? |
|
autosubmit label was removed for flutter/flutter/184275, 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. |
|
auto label is removed for flutter/flutter/184275, Failed to enqueue flutter/flutter/184275 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected.. |
Problem
Solution
Tests
00:00 +0 -1: loading test/general.shard/build_system/targets/web_test.dart [E]
Failed to load "test/general.shard/build_system/targets/web_test.dart": Does not exist.
00:00 +0 -1: Some tests failed.
Failing tests:
test/general.shard/build_system/targets/web_test.dart: loading test/general.shard/build_system/targets/web_test.dart