[iOS] Improve LaunchEngine implementation/API/docs#185200
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request refactors the LaunchEngine class to use a state-based lifecycle management system, replacing the engine property with an acquireEngine() method. The FlutterAppDelegate and associated tests were updated to use this new API. Review feedback highlighted logic errors in FlutterAppDelegate.mm where incorrect methods were called during the refactor, as well as stale documentation references in LaunchEngine.swift.
Alrighty... flutter/cocoon#5024 |
efe5854 to
d17fa8b
Compare
Refactors the internal state management of `LaunchEngine` to more idiomatic Swift, makes the possible states clear, and makes invalid states impossible: e.g. `didTakeEngine == true` but `_engine != nil`. LaunchEngine has three valid states: 1. Uninitialized: Initial state. The engine has not yet been created. 2. Created: The engine has been created and is cached for future access. 3. Taken: The engine has been transferred to a consumer, and the container is now empty. This also refactors the `engine` computed property into an `acquireEngine()` method. In Swift and Objective-C, properties should generally be idempotent and avoid side effects. Since the engine property triggers the lazy instantiation and execution of a `FlutterEngine` (a pretty heavy operation), exposing it as a method better communicates the potential cost / side effects. This also adds a bit more context to the docs that this class is an internal compatibility measure introduced as part of `UISceneDelegate` migration to avoid breaking the old AppDelegate-based plugin registration API.
d17fa8b to
c46ef46
Compare
flutter/flutter@2844af6...3d0e822 2026-04-21 [email protected] Reland "Unpin google_mobile_ads" (flutter/flutter#180838) 2026-04-21 [email protected] fix: correct LicenseRegistry docs to reference NOTICES instead of LICENSE (flutter/flutter#184572) 2026-04-21 [email protected] Roll Skia from f8637ade3d92 to a234f0ed7245 (2 revisions) (flutter/flutter#185334) 2026-04-21 [email protected] Roll Skia from 3b338913f623 to f8637ade3d92 (9 revisions) (flutter/flutter#185331) 2026-04-21 [email protected] Fix non-minimal relative imports in flutter_tools (flutter/flutter#183971) 2026-04-21 [email protected] Reapply "Unpin sdk package dependencies" (flutter/flutter#185268) 2026-04-21 [email protected] Remove unused private header (flutter/flutter#185260) 2026-04-20 [email protected] [iOS] Improve LaunchEngine implementation/API/docs (flutter/flutter#185200) 2026-04-20 [email protected] [ios][pv] Reland platform view hitTest approach (again) (flutter/flutter#185126) 2026-04-20 [email protected] Roll Skia from 75c2791c6274 to 3b338913f623 (3 revisions) (flutter/flutter#185304) 2026-04-20 [email protected] ignore avoid_type_to_string lint rule (flutter/flutter#184765) 2026-04-20 [email protected] Fix race condition in modifying release manifest. (flutter/flutter#185185) 2026-04-20 [email protected] In the dev/bots/analyze.dart script, obtain the relevant set of paths from Git instead of crawling the filesystem (flutter/flutter#185058) 2026-04-20 [email protected] [wimp] Implement images for wimp. (flutter/flutter#183913) 2026-04-20 [email protected] add the next batch for VPAT assessment (flutter/flutter#185053) 2026-04-20 [email protected] Roll Packages from c2e3d1f to 01c505f (21 revisions) (flutter/flutter#185287) 2026-04-20 [email protected] Avoid use of direct string injection in GitHub Workflow "run" steps. (flutter/flutter#185301) 2026-04-20 [email protected] Regenerate pubspec.lock (flutter/flutter#185290) 2026-04-20 [email protected] Report an error if the git ls-tree command fails in the content_aware_hash script (flutter/flutter#185170) 2026-04-20 [email protected] Roll Skia from d8415c5d7b91 to 75c2791c6274 (40 revisions) (flutter/flutter#185284) 2026-04-20 [email protected] Move widget_preview_scaffold into pub workspace (flutter/flutter#185176) 2026-04-20 [email protected] [record_use] Run build hooks and link hooks in separate targets (flutter/flutter#184880) 2026-04-20 [email protected] feat: add reloadIsRestart to handle hot reload as a restart for web #179448 (flutter/flutter#183233) 2026-04-20 [email protected] Roll Fuchsia Linux SDK from aDbXQm6WA0wFCAUp-... to LPa7NLiXEZP2A7IwZ... (flutter/flutter#185269) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#11548) flutter/flutter@2844af6...3d0e822 2026-04-21 [email protected] Reland "Unpin google_mobile_ads" (flutter/flutter#180838) 2026-04-21 [email protected] fix: correct LicenseRegistry docs to reference NOTICES instead of LICENSE (flutter/flutter#184572) 2026-04-21 [email protected] Roll Skia from f8637ade3d92 to a234f0ed7245 (2 revisions) (flutter/flutter#185334) 2026-04-21 [email protected] Roll Skia from 3b338913f623 to f8637ade3d92 (9 revisions) (flutter/flutter#185331) 2026-04-21 [email protected] Fix non-minimal relative imports in flutter_tools (flutter/flutter#183971) 2026-04-21 [email protected] Reapply "Unpin sdk package dependencies" (flutter/flutter#185268) 2026-04-21 [email protected] Remove unused private header (flutter/flutter#185260) 2026-04-20 [email protected] [iOS] Improve LaunchEngine implementation/API/docs (flutter/flutter#185200) 2026-04-20 [email protected] [ios][pv] Reland platform view hitTest approach (again) (flutter/flutter#185126) 2026-04-20 [email protected] Roll Skia from 75c2791c6274 to 3b338913f623 (3 revisions) (flutter/flutter#185304) 2026-04-20 [email protected] ignore avoid_type_to_string lint rule (flutter/flutter#184765) 2026-04-20 [email protected] Fix race condition in modifying release manifest. (flutter/flutter#185185) 2026-04-20 [email protected] In the dev/bots/analyze.dart script, obtain the relevant set of paths from Git instead of crawling the filesystem (flutter/flutter#185058) 2026-04-20 [email protected] [wimp] Implement images for wimp. (flutter/flutter#183913) 2026-04-20 [email protected] add the next batch for VPAT assessment (flutter/flutter#185053) 2026-04-20 [email protected] Roll Packages from c2e3d1f to 01c505f (21 revisions) (flutter/flutter#185287) 2026-04-20 [email protected] Avoid use of direct string injection in GitHub Workflow "run" steps. (flutter/flutter#185301) 2026-04-20 [email protected] Regenerate pubspec.lock (flutter/flutter#185290) 2026-04-20 [email protected] Report an error if the git ls-tree command fails in the content_aware_hash script (flutter/flutter#185170) 2026-04-20 [email protected] Roll Skia from d8415c5d7b91 to 75c2791c6274 (40 revisions) (flutter/flutter#185284) 2026-04-20 [email protected] Move widget_preview_scaffold into pub workspace (flutter/flutter#185176) 2026-04-20 [email protected] [record_use] Run build hooks and link hooks in separate targets (flutter/flutter#184880) 2026-04-20 [email protected] feat: add reloadIsRestart to handle hot reload as a restart for web #179448 (flutter/flutter#183233) 2026-04-20 [email protected] Roll Fuchsia Linux SDK from aDbXQm6WA0wFCAUp-... to LPa7NLiXEZP2A7IwZ... (flutter/flutter#185269) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Refactors the internal state management of
LaunchEngineto more idiomatic Swift, makes the possible states clear, and makes invalid states impossible: e.g.didTakeEngine == truebut_engine != nil.LaunchEngine has three valid states:
.uninitialized: Initial state. The engine has not yet been created..created: The engine has been created and is cached for future access..taken: The engine has been transferred to a consumer, and the container is now empty.This also refactors the
enginecomputed property into anacquireEngine()method. In Swift and Objective-C, properties should generally be idempotent and avoid side effects. Since the engine property triggers the lazy instantiation and execution of aFlutterEngine(a pretty heavy operation), exposing it as a method better communicates the potential cost / side effects.Also adds a bit more context to the docs that this class is an internal compatibility measure introduced as part of
UISceneDelegatemigration to avoid breaking the old AppDelegate-based plugin registration API.I'll send a followup that tags this
@MainActorbut that should probably be its own patch.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.