[web] Fix loading of fragment shader with space in name.#180919
[web] Fix loading of fragment shader with space in name.#180919auto-submit[bot] merged 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with loading fragment shaders that have spaces in their names on the web. The fix involves URI-encoding the asset key in FragmentProgram.fromAsset to align the web_ui implementation with dart:ui. A regression test is included to verify the fix. The changes look good, but I have one suggestion to add documentation to the modified public method to adhere to the repository's style guide.
| // encoded paths (replacing ' ' with '%20', for example). We perform | ||
| // the same encoding here so that users can load assets with the same | ||
| // key they have written in the pubspec. | ||
| final String encodedKey = Uri(path: Uri.encodeFull(assetKey)).path; |
There was a problem hiding this comment.
I'm curious why this is needed since the assetManager also does the Uri.encodeFull encoding before sending the request for the asset. It seems we are double-encoding the URI now.
From the AssetManager code (createFragmentProgram eventually calls assetManager.load(...))
String getAssetUrl(String asset) {
if (Uri.parse(asset).hasScheme) {
return Uri.encodeFull(asset);
}
return Uri.encodeFull('$_baseUrl$assetsDir/$asset');
}
Future<ByteData> load(String asset) async {
final String url = getAssetUrl(asset);
final HttpFetchResponse response = await httpFetch(url);
...
}There was a problem hiding this comment.
I see. It looks like the double encoding is necessary because the Flutter tool encodes the path, so we need to request the encoded path name and therefore it needs to be double encoded.
There was a problem hiding this comment.
Indeed, this was exactly the reason. Thanks a lot for your review!
|
LGTM |
Fixes #180862
Description
assetKeyencoding inFragmentProgram.fromAssetinweb_uito be consistent with the implementation inuiflutter/engine/src/flutter/lib/ui/painting.dart
Lines 5354 to 5369 in 7e176f8
Pre-launch Checklist
///).