Add service protocol to get SkSLs #17300
Conversation
chinmaygarde
left a comment
There was a problem hiding this comment.
One comment about the thread used to service this and some very minor nits about naming convention. LGTM otherwise.
| const std::string_view ServiceProtocol::kGetDisplayRefreshRateExtensionName = | ||
| "_flutter.getDisplayRefreshRate"; | ||
| const std::string_view ServiceProtocol::kGetSkSLsExtensionName = | ||
| "_flutter.getSkSLs"; |
There was a problem hiding this comment.
Please don't forget to document this new method in the wiki entry on engine managed service protocol extensions. https://github.com/flutter/flutter/wiki/Engine-specific-Service-Protocol-extensions
There was a problem hiding this comment.
Created flutter/flutter#53199 to make sure that I don't forget it.
shell/common/shell.cc
Outdated
| std::bind(&Shell::OnServiceProtocolGetDisplayRefreshRate, this, | ||
| std::placeholders::_1, std::placeholders::_2)}; | ||
| service_protocol_handlers_[ServiceProtocol::kGetSkSLsExtensionName] = { | ||
| task_runners_.GetUITaskRunner(), |
There was a problem hiding this comment.
Nit: This doesn't have to be serviced on the UI task runner right (the writes are already atomic)? We can perform these operations on the IO thread. This will give fewer opportunities to block the UI thread. Granted its just debug and profile modes, but still.
There was a problem hiding this comment.
Sure, changed it to IO thread.
shell/common/shell.cc
Outdated
| char* b64_char = static_cast<char*>(b64_data->writable_data()); | ||
| SkBase64::Encode(sksl.second->data(), sksl.second->size(), b64_char); | ||
| b64_char[b64_size] = 0; // make it null terminated for printing | ||
| rapidjson::Value shaderValue(b64_char, response.GetAllocator()); |
There was a problem hiding this comment.
Nit: Please use engine ivar naming conventions. shader_value, etc..
There was a problem hiding this comment.
Oops. Missed those ones. Fixed.
| "{\"type\":\"GetSkSLs\",\"SkSLs\":{\"II\":\"eQ==\",\"IE\":\"eA==\"}}"); | ||
|
|
||
| // Cleanup files | ||
| fml::FileVisitor recursive_cleanup = [&recursive_cleanup]( |
There was a problem hiding this comment.
In a later patch, we should consider putting this in //flutter/testing or create a fixture that sets up an isolate temp directory per test run that gets automatically cleaned up.
chinmaygarde
left a comment
There was a problem hiding this comment.
BTW, the Fuchsia failures are due to the bot quarantine. Let's ignore those presubmits. But please make sure the non-fuchsia presubmits pass. LGTM.
Fixes flutter/flutter#53114