Migrate PlatformViewIOS to ARC by cbracken · Pull Request #55672 · flutter/engine

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

  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

flutter/engine@70671ba...ed587dc 2024-10-28 skia-flutter-autoroll@skia.org Roll Dart SDK from 69b50768d733 to c9180e9de9e8 (1 revision) (flutter/engine#56180) 2024-10-28 jonahwilliams@google.com [Impeller] fix initial layout for loadOp load and incorrect usage of host visible textures. (flutter/engine#56148) 2024-10-28 skia-flutter-autoroll@skia.org Roll Skia from 21035cd95b68 to bdd225968dab (1 revision) (flutter/engine#56178) 2024-10-28 chris@bracken.jp 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 chris@bracken.jp Migrate PlatformViewIOS to ARC (flutter/engine#55672) 2024-10-28 skia-flutter-autoroll@skia.org Roll Skia from 35ad4e89212f to 21035cd95b68 (1 revision) (flutter/engine#56176) 2024-10-28 aam@google.com Roll buildroot to pick up revert of debugging gen_snapshot prints (flutter/engine#56175) 2024-10-28 zanderso@users.noreply.github.com Pin local_engine mac builds to arm64 (flutter/engine#56172) 2024-10-28 zanderso@users.noreply.github.com 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 codefu@google.com,zra@google.com 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 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

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 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

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

8c74f3f 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

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

7b34d3b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate PlatformViewIOS to ARC#55672

Migrate PlatformViewIOS to ARC#55672
auto-submit[bot] merged 3 commits intoflutter:mainfrom
cbracken:arc-platformviewios

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbracken commented Oct 26, 2024 •

edited

Loading