Repository navigation
iOS: Remove dead code/unused imports from FlutterPlatformViewsController - #196084
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 refactors the FlutterPlatformViewsController to change the return type of submitFrame:withIosContext: from BOOL to void, removing unused frame encoding state tracking. It also cleans up unused properties like rect in PendingRRectClip and optimizes header imports across several files. Feedback on the tests suggests using independent tracking variables for different test scenarios to prevent potential test pollution and false positives.
| [submitted](const flutter::SurfaceFrame& surface_frame) { | ||
| *submitted = true; | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
This is just testing exactly the same thing but more directly.
cbracken
changed the title
4d89d70 to
0715036
Compare
…troller `PendingRRectClip` had a `rect` property that was never read. It was also the only C++ engine type on that class. This deletes it. `-submitFrame:withIosContext:` returned whether every overlay frame encoded successfully, the result was never read. The only caller is in `IOSExternalViewEmbedder`. `OverlayLayer::did_submit_last_frame` was only there to provide that return value and similarly, was never read. This updates `submitFrame` to return void and we delete the flag. This also updates the test that checked the return value to assert via the submit callback, which is more important regardless. When encode fails, we don't submit the frame, if it succeeds, we submit it. This also fixes some imports/includes: * Drop `FlutterPlatformViews_Internal.h` in `FlutterView.h`, since all we needed was a forward-declaration of FlutterPlatformViewsController * Same in `ios_surface.h` which only needed `IOSContext`, which we can get directly from `ios_context.h` * A few files had been picking up QuartzCore and `FlutterMacros.h` transitively. We now import them directly when needed. This is just pre-factoring/cleanups prior to starting work on platform view refactoring for the embedder API. There are no changes to behaviour. Issue: flutter#112232
0715036 to
bfeb863
Compare
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Even though we weren't checking the return result in production code, was the assertion in the tests providing some value? I didn't dig through what goes into determining that return result but it seems a bit of a sad thing to throw out unless it's completely useless.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Giving this approval because the code looks good. My question isn't really a big deal and I trust chris to address it if we do in fact want to keep it around.
gaaclarke
added
the
test-exempt
Good question. The key thing the test was verifying is that we only submit on success, and not on failure. It was testing via this value that was meant to indicate whether or not the frame submitted, but the return value was actually a bit of a mess and had two different meanings depending on the scenario:
TL;DR the return value looks like it was intended as an indication of whether or not the frame submitted but it wasn't actually that and we ignored it everywhere anyway; it was only used in the test. That said, we didn't need it since we can just test whether the submit callback fired directly by supplying our own in the test, which is how I updated the it. |
flutter/flutter@09175cd...e36b370 2026-11-07 [email protected] Roll Fuchsia Linux SDK from KIc0XZbhLmvDpTluZ... to Sf2yvdTiFXAKkfLkD... (flutter/flutter#196160) 2026-11-07 [email protected] Roll pub packages (flutter/flutter#196155) 2026-11-07 [email protected] [iOS a11y] Use a real `UIScrollView` as a11y scrollable container (flutter/flutter#195731) 2026-11-07 [email protected] Add ParagraphStyle.hyphenCharacter to dart:ui (flutter/flutter#195345) 2026-11-07 [email protected] Roll pub packages (flutter/flutter#196143) 2026-11-06 [email protected] [Linux] Fix layer position when compositing without glBlitFramebuffer (flutter/flutter#196083) 2026-11-06 [email protected] Add support for riscv64 desktop linux engine (flutter/flutter#178712) 2026-11-06 [email protected] Add ResizeImagePolicy.cover (flutter/flutter#194665) 2026-11-06 [email protected] Roll Skia from f533d4034900 to b11315d0a096 (6 revisions) (flutter/flutter#196138) 2026-11-06 [email protected] iOS: Remove dead code/unused imports from FlutterPlatformViewsController (flutter/flutter#196084) 2026-11-06 [email protected] Replace pull_request_target workflows with pull_request + workflow_run (flutter/flutter#195536) 2026-11-06 [email protected] [master] Update Flutter DEPS to Dart 3b174b7217fe09b21da836ea623932b5f041b091 (flutter/flutter#196004) 2026-11-06 [email protected] Fix android_hardware_smoke_test retry mechanisms (flutter/flutter#195708) 2026-11-06 [email protected] Roll Skia from 452b9c1e5db6 to f533d4034900 (1 revision) (flutter/flutter#196129) 2026-11-06 [email protected] [flutter_tools] Use `ErrorHandlingFileSystem.deleteIfExists` for `IOSCoreDeviceControl` temp directory cleanup (flutter/flutter#195913) 2026-11-06 [email protected] Adds guideline tests to flutter create samples (flutter/flutter#195827) 2026-11-06 [email protected] refactor(flutter_tools): unify OptionDescriptor identity and addTo (flutter/flutter#196076) 2026-11-06 [email protected] Roll pub packages (flutter/flutter#196114) 2026-11-06 [email protected] Roll Skia from 5ce64e63a0e7 to 452b9c1e5db6 (5 revisions) (flutter/flutter#196111) 2026-11-06 [email protected] Add PlatformDispatcher.onAlwaysUse24HourFormatChanged callback (flutter/flutter#194670) 2026-11-06 [email protected] Roll Packages from 0478162 to 63881f9 (10 revisions) (flutter/flutter#196108) 2026-11-06 [email protected] Roll Skia from 793b8ac73d4b to 5ce64e63a0e7 (4 revisions) (flutter/flutter#196107) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[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
PendingRRectCliphad arectproperty that was never read. It was also the only C++ engine type on that class. This deletes it.-submitFrame:withIosContext:returned whether every overlay frame encoded successfully, the result was never read. The only caller is inIOSExternalViewEmbedder.OverlayLayer::did_submit_last_framewas only there to provide that return value and similarly, was never read.This updates
submitFrameto return void and we delete the flag. This also updates the test that checked the return value to assert via the submit callback, which is more important regardless. When encode fails, we don't submit the frame, if it succeeds, we submit it.This also fixes some imports/includes:
FlutterPlatformViews_Internal.hinFlutterView.h, since all we needed was a forward-declaration of FlutterPlatformViewsControllerios_surface.hwhich only neededIOSContext, which we can get directly fromios_context.hFlutterMacros.htransitively. We now import them directly when needed.This is just pre-factoring/cleanups prior to starting work on platform view refactoring for the embedder API. There are no changes to behaviour.
Issue: #112232
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.