Repository navigation
iOS: test GPU availability and task queue behaviour - #190836
Conversation
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds unit tests to FlutterEngineTest.mm to verify GPU state behavior during shell creation and to ensure that platform messages are delivered on the correct threads depending on the task queue configuration. It also introduces a helper class TestPlatformMessageResponse to track message completion. Feedback suggests making the constructor of TestPlatformMessageResponse private to prevent accidental stack allocation of a ref-counted object, and omitting the unused parameter in Complete to avoid compiler warnings.
| class TestPlatformMessageResponse : public flutter::PlatformMessageResponse { | ||
| public: | ||
| static fml::RefPtr<TestPlatformMessageResponse> Create() { | ||
| return fml::AdoptRef(new TestPlatformMessageResponse()); | ||
| } | ||
| void Complete(std::unique_ptr<fml::Mapping> data) override { is_complete_ = true; } | ||
| void CompleteEmpty() override { is_complete_ = true; } | ||
| }; |
There was a problem hiding this comment.
To prevent accidental stack allocation or direct instantiation of TestPlatformMessageResponse (which is ref-counted and should only be managed via fml::RefPtr), the constructor should be made private. Additionally, the unused parameter data in Complete can be omitted to avoid potential compiler warnings about unused parameters.
class TestPlatformMessageResponse : public flutter::PlatformMessageResponse {
public:
static fml::RefPtr<TestPlatformMessageResponse> Create() {
return fml::AdoptRef(new TestPlatformMessageResponse());
}
void Complete(std::unique_ptr<fml::Mapping>) override { is_complete_ = true; }
void CompleteEmpty() override { is_complete_ = true; }
private:
TestPlatformMessageResponse() = default;
};
There was a problem hiding this comment.
the constructor should be made private.
Good bot. Done.
Additionally, the unused parameter data in Complete can be omitted to avoid potential compiler warnings about unused parameters.
🤦♂️ Bad bot. No it can't. This is an override of a pure virtual method in the flutter::PlatformMessageResponse abstract base class. It's literally marked override. We don't exactly have the option to change the signature. You're right removing the param won't complain about unused parameters... because it won't even compile 😂.
Also, we explicitly compile with -Wno-unused-parameter, so there's no way we'd get those warnings even if this were legit feedback, which it isn't. You can see this yourself in compile_commands.json.
Adds tests for two behaviours the embedder API migration has to preserve, both at the layer a plugin or the framework actually sees rather than the layer the current implementation happens to use: First is GPU availability: Currently, `createShell:` re-derives `isGpuDisabled` from the live view controller or application state immediately before creating the shell, and clobbers whatever the property was set to beforehand. This is an easy thing to get wrong later: the "obvious" handling via `FlutterEngineRun` would be to take the initial GPU availability as a project argument, but passing the property straight through would resurrect exactly the stale value this code goes out of its way to drop. Second is Task queues: `PlatformMessageHandlerIosTest` covers `PlatformMessageHandlerIos` behaviour on its own, but nothing covered the path a plugin takes to reach it, through * `-[FlutterEngine makeBackgroundTaskQueue]` -> * `-[FlutterEngine setMessageHandlerOnChannel:binaryMessageHandler:taskQueue:]` `EmbedderPlatformMessageHandler` always trampolines to the platform thread, so both halves of that contract will have to be rebuilt explicitly for embedder API migration. The GPU test locks in the current behaviour. I'll address the TODO in the next patch to clean up the API. But this is a nice way to document/verify the behavioural diff in the tests. Issue: flutter#112232 Issue: flutter#190835
ed75e86 to
2278d88
Compare
Previously, a value assigned to `isGpuDisabled` before the engine ran was silently discarded: `createShell:` sampled the current lifecycle state by assigning to the public property immediately before creating the shell, so the caller's value never reached `Shell::Create`. The sampling is necessary — lifecycle notifications only report transitions, so an engine created while the app is already backgrounded never receives one — but it belongs where its input changes, not where the shell is built. We now sample when the engine is created and when a view controller is attached or detached, and `createShell:` passes `_isGpuDisabled` through untouched. For an app the observable behaviour is unchanged, but a pre-run assignment now survives to the shell and is corrected by the next real lifecycle transition rather than by a write the caller cannot see. Sampling also has to cope with not being able to tell. An app extension has no `UIApplication` to consult, and reaches the scene only through an attached view controller whose view is in a window — which neither `setViewController:` call site has yet. That is also the only configuration in which `setUpLifecycleNotifications:` registers the scene observers, so treating "cannot tell" as "in the foreground" would let an attach undo a `sceneDidEnterBackground:` and re-enable the GPU in the background, which is what gets an app killed. Where neither source is readable we leave `isGpuDisabled` as it stands, the last observed transition being the better answer. The test added in flutter#190836 asserted the old behaviour and is inverted here, so this lands as a behavioural diff. Fixes: flutter#190835 Issue: flutter#112232
Adds tests for two behaviours the embedder API migration has to preserve, both at the layer a plugin or the framework actually sees rather than the layer the current implementation happens to use:
First is GPU availability:
Currently,
createShell:re-derivesisGpuDisabledfrom the live view controller or application state immediately before creating the shell, and clobbers whatever the property was set to beforehand. This is an easy thing to get wrong later: the "obvious" handling viaFlutterEngineRunwould be to take the initial GPU availability as a project argument, but passing the property straight through would resurrect exactly the stale value this code goes out of its way to drop.Second is Task queues:
PlatformMessageHandlerIosTestcoversPlatformMessageHandlerIosbehaviour on its own, but nothing covered the path a plugin takes to reach it, through-[FlutterEngine makeBackgroundTaskQueue]-[FlutterEngine setMessageHandlerOnChannel:binaryMessageHandler:taskQueue:]EmbedderPlatformMessageHandleralways trampolines to the platform thread, so both halves of that contract will have to be rebuilt explicitly for embedder API migration.The GPU test locks in the current behaviour. I'll address the TODO in the next patch to clean up the API. But this is a nice way to document/verify the behavioural diff in the tests.
Issue: #112232
Issue: #190835
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.