Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@liyuqian
Copy link
Contributor

@liyuqian
Copy link
Contributor Author

@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.

Copy link
Contributor

@chinmaygarde chinmaygarde left a 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.


#include "flutter/shell/common/persistent_cache.h"

#include <rapidjson/document.h>
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

static void MarkStrategySet() { strategy_set_ = true; }

static constexpr char kSkSLSubdirName[] = "sksl";
static constexpr char kAssetFileName[] = "shaders.json";
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@liyuqian liyuqian added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels Apr 22, 2020
@liyuqian liyuqian merged commit 31ecf87 into flutter:master Apr 22, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 23, 2020
@liyuqian liyuqian deleted the json_asset branch April 23, 2020 23:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update SkSL bundle format

3 participants