-
Notifications
You must be signed in to change notification settings - Fork 6k
Read SkSL from json asset #17861
Read SkSL from json asset #17861
Conversation
|
@jonahwilliams : let's forget about iOS Metal for now until developers ask for it. This should unblock us on iOS32 and Android. The Metal iOS would still load those SkSLs but they probably won't eliminate the compilation jank for now. |
chinmaygarde
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.
Some minor nits. Otherwise lgtm.
shell/common/persistent_cache.cc
Outdated
|
|
||
| #include "flutter/shell/common/persistent_cache.h" | ||
|
|
||
| #include <rapidjson/document.h> |
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.
Nit: Square brackets for system headers and quotes for others. This goes at the bottom with the others.
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.
Done.
| SkBase64 decoder; | ||
| auto error = decoder.decode(input.c_str(), input.length()); | ||
| if (error != SkBase64::Error::kNoError) { | ||
| FML_LOG(ERROR) << "Base64 can't decode: " << input; |
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.
Nit: Also dump the error maybe?
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.
Done.
shell/common/persistent_cache.h
Outdated
| static void MarkStrategySet() { strategy_set_ = true; } | ||
|
|
||
| static constexpr char kSkSLSubdirName[] = "sksl"; | ||
| static constexpr char kAssetFileName[] = "shaders.json"; |
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.
This seems like an all too common name and may collide with other assets. How about io.flutter.shaders.json or something more indicative of its purpose?
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.
Done.
Fixes flutter/flutter#55219