Migrate PlatformViewIOS to ARC #55672

cbracken · 2024-10-05T00:44:59Z

Migrates PlatformViewIOS from manual reference counting to ARC. Eliminates use of scoped_nsobject, scoped_nsprotocol, and WeakNSObject.

Since this is the last non-ARC file in flutter_framework_source, this also eliminates the flutter_framework_source target, then also renames the flutter_framework_source_arc target to flutter_framework_source since... it's ALL ARC.

No semantic changes, therefore no semantic changes to tests.

Issue: flutter/flutter#137801

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I'm hopeful this is the last time anyone needs to come up with some ARCane pun on ARC to jam into the checkboxes.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cbracken · 2024-10-05T00:49:30Z

shell/platform/darwin/ios/platform_view_ios.h

  // Since the `ios_surface_` is created on the platform thread but
  // used on the raster thread we need to protect it with a mutex.
  std::mutex ios_surface_mutex_;
  std::unique_ptr<IOSSurface> ios_surface_;
  std::shared_ptr<IOSContext> ios_context_;
  const std::shared_ptr<PlatformViewsController>& platform_views_controller_;
  AccessibilityBridgeManager accessibility_bridge_;
-  fml::scoped_nsprotocol<FlutterTextInputPlugin*> text_input_plugin_;


This... wasn't used anywhere at all.

jmagman · 2024-10-05T00:52:56Z

shell/platform/darwin/ios/BUILD.gn

-  ]
-}
-
-source_set("flutter_framework_source") {


I'm jumping for joy over here!

shell/platform/darwin/ios/platform_view_ios.h

jmagman · 2024-10-05T02:20:13Z

shell/platform/darwin/ios/platform_view_ios.mm

-  auto ca_layer = fml::scoped_nsobject<CALayer>{[[flutter_view layer] retain]};
+  FML_DCHECK(owner_controller_.isViewLoaded) << "FlutterViewController's view should be loaded "
+                                                "before attaching to PlatformViewIOS.";
+  FlutterView* flutter_view = static_cast<FlutterView*>(owner_controller_.view);


Is this static_cast needed?

Yes, unfortunately since owner_controller_.view returns a UIView*.

shell/platform/darwin/ios/platform_view_ios.h

chinmaygarde · 2024-10-21T17:53:52Z

Is this blocked?

cbracken · 2024-10-21T18:23:55Z

This is queued up behind #55669, which should land today.

cbracken · 2024-10-26T18:17:33Z

Alright this is ready for review whenever people are ready to give it a look. We're finally at the end (compile/linker-wise), folks!

jmagman

It's so beautiful.

stuartmorgan-g

LGTM once the unrelated TODO is restored.

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

Migrates PlatformViewIOS from manual reference counting to ARC. Eliminates use of scoped_nsobject, scoped_nsprotocol, and WeakNSObject. Since this is the last non-ARC file in `flutter_framework_source`, this also eliminates the `flutter_framework_source` target, then also renames the `flutter_framework_source_arc` target to `flutter_framework_source` since... it's ALL ARC. No semantic changes, therefore no changes to tests. Issue: flutter/flutter#137801

…157751) flutter/engine@70671ba...ed587dc 2024-10-28 [email protected] Roll Dart SDK from 69b50768d733 to c9180e9de9e8 (1 revision) (flutter/engine#56180) 2024-10-28 [email protected] [Impeller] fix initial layout for loadOp load and incorrect usage of host visible textures. (flutter/engine#56148) 2024-10-28 [email protected] Roll Skia from 21035cd95b68 to bdd225968dab (1 revision) (flutter/engine#56178) 2024-10-28 [email protected] iOS/macOS: migrate darwin/common to ARC (flutter/engine#56155) 2024-10-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Pin local_engine mac builds to arm64 (#56172)" (flutter/engine#56179) 2024-10-28 [email protected] Migrate PlatformViewIOS to ARC (flutter/engine#55672) 2024-10-28 [email protected] Roll Skia from 35ad4e89212f to 21035cd95b68 (1 revision) (flutter/engine#56176) 2024-10-28 [email protected] Roll buildroot to pick up revert of debugging gen_snapshot prints (flutter/engine#56175) 2024-10-28 [email protected] Pin local_engine mac builds to arm64 (flutter/engine#56172) 2024-10-28 [email protected] Switch some mac_unopt tests from intel to arm hosts (flutter/engine#55882) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

Migrates PlatformViewIOS from manual reference counting to ARC. Eliminates use of scoped_nsobject, scoped_nsprotocol, and WeakNSObject. Since this is the last non-ARC file in `flutter_framework_source`, this also eliminates the `flutter_framework_source` target, then also renames the `flutter_framework_source_arc` target to `flutter_framework_source` since... it's ALL ARC. No semantic changes, therefore no semantic changes to tests. Issue: flutter#137801 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-ios label Oct 5, 2024

cbracken force-pushed the arc-platformviewios branch from f0c9427 to 79ad78a Compare October 5, 2024 00:48

cbracken commented Oct 5, 2024

View reviewed changes

jmagman reviewed Oct 5, 2024

View reviewed changes

shell/platform/darwin/ios/platform_view_ios.h Outdated Show resolved Hide resolved

cbracken force-pushed the arc-platformviewios branch from 79ad78a to 7946f64 Compare October 26, 2024 18:10

jmagman approved these changes Oct 28, 2024

View reviewed changes

stuartmorgan-g approved these changes Oct 28, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Show resolved Hide resolved

cbracken added 3 commits October 28, 2024 11:05

review: add cf_audited_transfer attr

548cc94

Re-add TODO

9abdbf3

cbracken force-pushed the arc-platformviewios branch from 58a36b8 to 9abdbf3 Compare October 28, 2024 18:05

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2024

auto-submit bot merged commit 8c74f3f into flutter:main Oct 28, 2024
30 checks passed

cbracken deleted the arc-platformviewios branch October 28, 2024 19:34

engine-flutter-autoroll mentioned this pull request Oct 28, 2024

Roll Flutter Engine from 70671baabbba to 0b9f56b6d3cf (7 revisions) flutter/flutter#157749

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2024

8c74f3f85 Migrate PlatformViewIOS to ARC (flutter/engine#55672)

65bb165

engine-flutter-autoroll mentioned this pull request Oct 28, 2024

Roll Flutter Engine from 70671baabbba to ed587dcf61fd (10 revisions) flutter/flutter#157751

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2024

8c74f3f85 Migrate PlatformViewIOS to ARC (flutter/engine#55672)

7b34d3b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate PlatformViewIOS to ARC #55672

Migrate PlatformViewIOS to ARC #55672

Uh oh!

cbracken commented Oct 5, 2024

Uh oh!

cbracken Oct 5, 2024

Uh oh!

jmagman Oct 5, 2024

Uh oh!

Uh oh!

jmagman Oct 5, 2024

Uh oh!

cbracken Oct 5, 2024

Uh oh!

Uh oh!

chinmaygarde commented Oct 21, 2024

Uh oh!

cbracken commented Oct 21, 2024

Uh oh!

cbracken commented Oct 26, 2024 •

edited

Loading

Uh oh!

jmagman left a comment

Uh oh!

stuartmorgan-g left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Migrate PlatformViewIOS to ARC #55672

Migrate PlatformViewIOS to ARC #55672

Uh oh!

Conversation

cbracken commented Oct 5, 2024

Pre-launch Checklist

Uh oh!

cbracken Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chinmaygarde commented Oct 21, 2024

Uh oh!

cbracken commented Oct 21, 2024

Uh oh!

cbracken commented Oct 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken commented Oct 26, 2024 •

edited

Loading