This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Update DisplayList tests to explicitly select the Roboto font #47493
Conversation
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
Member
Author
flar
approved these changes
Oct 30, 2023
Contributor
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
LGTM - other than my question about the previous benchmark fixtures...
| } | ||
| } | ||
|
|
||
| fixtures_location("display_list_benchmarks_fixtures") { |
Member
Author
There was a problem hiding this comment.
I did not see any references to fixtures in the DL benchmarks. The benchmarks appear to be running as expected with this change.
Contributor
|
LGTM once the compilation issue is sorted out |
8 tasks
jason-simmons
force-pushed
the
dl_test_roboto_font
branch
3 times, most recently
from
d52b91d to
c39d0d4
Compare
jason-simmons
force-pushed
the
dl_test_roboto_font
branch
from
c39d0d4 to
061f107
Compare
Skia is removing the API for constructing a default typeface (see https://issues.skia.org/issues/305780908)
jason-simmons
force-pushed
the
dl_test_roboto_font
branch
from
061f107 to
b77cf68
Compare
jason-simmons
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 2, 2023
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 2, 2023
…137767) flutter/engine@4ba585b...be49d65 2023-11-02 [email protected] Roll Fuchsia Linux SDK from pvAbHwPJtOuEpHJN7... to YS7vrarYI-cHvhuBT... (flutter/engine#47602) 2023-11-02 [email protected] Update DisplayList tests to explicitly select the Roboto font (flutter/engine#47493) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from pvAbHwPJtOuE to YS7vrarYI-cH 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
8 tasks
kjlubick
added a commit
that referenced
this pull request
Dec 6, 2023
In https://g-issues.skia.org/issues/305780908 Skia is removing the default SkFontMgr. Previous work consolidated all references to txt/platform.h and this replaces those last references. I attempted to mirror the existing functionality, which still responds to GN flags and the target platform. After this PR, Flutter should not be depending on the default fontmgr (and the defines in flutter_defines.gni) will maintain that behavior until the legacy functions/methods are deleted from Skia. There were a few tests that I missed on an earlier PR which relied on the default font (helper added in #47493). These tests were failing because they were making some assertions related to TextBlobs, which didn't work if the (now-empty) Typeface they loaded had no glyphs. Thus, I added a few extra asserts to make sure these textblobs *had* glyphs which make the failing tests less mysterious, should this issue crop up again. I cleaned up Flutter's BUILD.gn file for Skia a bit, deleting unused targets related to the font managers. This involved fixing an implicit dependency from //third_party/glfw/ to `Gdi32.lib` on Windows. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
jason-simmons
pushed a commit
to jason-simmons/flutter_engine
that referenced
this pull request
Dec 7, 2023
…ter#48571) In https://g-issues.skia.org/issues/305780908 Skia is removing the default SkFontMgr. Previous work consolidated all references to txt/platform.h and this replaces those last references. I attempted to mirror the existing functionality, which still responds to GN flags and the target platform. After this PR, Flutter should not be depending on the default fontmgr (and the defines in flutter_defines.gni) will maintain that behavior until the legacy functions/methods are deleted from Skia. There were a few tests that I missed on an earlier PR which relied on the default font (helper added in flutter#47493). These tests were failing because they were making some assertions related to TextBlobs, which didn't work if the (now-empty) Typeface they loaded had no glyphs. Thus, I added a few extra asserts to make sure these textblobs *had* glyphs which make the failing tests less mysterious, should this issue crop up again. I cleaned up Flutter's BUILD.gn file for Skia a bit, deleting unused targets related to the font managers. This involved fixing an implicit dependency from //third_party/glfw/ to `Gdi32.lib` on Windows. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 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.
Skia is removing the API for constructing a default typeface (see https://issues.skia.org/issues/305780908)
Fixes flutter/flutter#137565