-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Reland "Speed up first asset load by using the binary-formatted asset manifest for image resolution #121322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
auto-submit
merged 14 commits into
flutter:master
from
andrewkolos:reland-use-binary-asset-manifest-for-asset-resolution
Mar 6, 2023
Merged
Reland "Speed up first asset load by using the binary-formatted asset manifest for image resolution #121322
auto-submit
merged 14 commits into
flutter:master
from
andrewkolos:reland-use-binary-asset-manifest-for-asset-resolution
Mar 6, 2023
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
5244790 to
e00f17a
Compare
…-asset-manifest-for-asset-resolution
This reverts commit e69cee3.
…-asset-manifest-for-asset-resolution
jonahwilliams
approved these changes
Mar 6, 2023
Contributor
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to the asset loading API sound reasonable.
LGTM!
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 8, 2023
…ed asset manifest for image resolution (flutter/flutter#121322)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 8, 2023
…ed asset manifest for image resolution (flutter/flutter#121322)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 8, 2023
…ed asset manifest for image resolution (flutter/flutter#121322)
11 tasks
8 tasks
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 10, 2023
…ed asset manifest for image resolution (flutter/flutter#121322)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 10, 2023
…ed asset manifest for image resolution (flutter/flutter#121322)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
autosubmit
Merge PR when tree becomes green via auto submit App
c: contributor-productivity
Team-specific productivity, code health, technical debt.
framework
flutter/packages/flutter repository. See also f: labels.
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.
Relands #118782 with some changes.
The original PR added a behavior change--when trying to load an asset and we don't find its key in the asset manifest file, we would throw an error (code). The idea was that this would provide a more useful error message than the one that would get shown when the image inevitably fails to load (code). However, this caused the
errorBuilderfunctionality ofImageto not work in the case of a missing asset (this was revealed by an internal test breakage). Fixing this appears non-trivial, so I think it's better to scope this out of this PR and perhaps look at it again later.This also makes a small update to the recently-added
AssetManifest.getAssetVariantsAPI (still in beta). When given a key that is not found within the asset manifest file, the method will returnnullinstead of throwing an error. Throwing an error is a bit too opinionated for an API that's just meant to provide read-access to the asset manifest file. The tradeoff is that callers will have to do null checking/assertions.Let me know if you feel strongly that this should be its own PR.
Pre-launch Checklist
///).