Migrate FlutterViewController to ARC #55669

cbracken · 2024-10-04T21:36:52Z

Migrates FlutterViewController from manual reference counting to ARC. Eliminates use of scoped_nsobject and scoped_nsprotocol, and migrates ivars to property syntax where possible.

No semantic changes, therefore no 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 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.
We're about one patch away from vanquishing our ARCh enemy... MRC.
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.

flutter-dashboard · 2024-10-04T21:36:55Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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

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

cbracken · 2024-10-04T22:39:54Z

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

-
 - (fml::WeakNSObject<FlutterViewController>)getWeakNSObject {
  return _weakFactory->GetWeakNSObject();
 }


Still needed by FlutterEngine, which calls this, and puts it into a field in a C++ class in platform_view_ios.h, which I'll migrate in the next patch.

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

jmagman

LGTM!

stuartmorgan-g

LGTM

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

stuartmorgan-g · 2024-10-05T10:31:12Z

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

+    self.engineNeedsLaunch = NO;
+  } else if (self.engine.viewController == self) {
+    [self.engine attachView];
  }


(Definitely not in scope for this PR, but it sure feels like this method and the last method could use some refactoring into engine methods given that they mostly walls of self.engine calls...)

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

stuartmorgan-g · 2024-10-05T10:54:28Z

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

 // keyboard.
+//
+// We define separate superPresses* overrides to avoid implicitly capturing self in the blocks
+// passed to the presses* methods below.


There's an interesting (but maybe theoretical) question here: if handling a press both caused self to be released, but also claimed not to have handled the event (I'm not sure why that would happen, but I suspect you could write Dart code that would do it if you really wanted to), isn't the right thing to still forward the press to super?

If so, we actually would want the implicit capture. Since these are fire-once callbacks, capture isn't necessarily a problem. I did look at the usage though, and I can imagine a case where for some reason we never get the callback for some press due to an engine bug, and suddenly we have a leak that would be extremely hard to find.

This conversion makes sense in the context of the PR since the old code wasn't a strong ref, but since this structure and comment is making the non-retaining nature much more explicit I wanted to flag it as something the team may want to think about later.

stuartmorgan-g · 2024-10-07T14:31:02Z

test-exempt: code refactor with no semantic change

hellohuanlin · 2024-10-07T18:00:27Z

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

+// We keep a separate reference to this and create it ahead of time because we want to be able to
+// set up a shell along with its platform view before the view has to appear.
+@property(nonatomic, strong) FlutterView* flutterView;
+@property(nonatomic, strong) void (^flutterViewRenderedCallback)(void);


nit: copy (tho same behavior as strong in ARC)

hellohuanlin · 2024-10-07T18:01:03Z

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

 @property(nonatomic, assign) BOOL isHomeIndicatorHidden;
 @property(nonatomic, assign) BOOL isPresentingViewControllerAnimating;

+@property(nonatomic, strong) NSMutableSet<NSNumber*>* ongoingTouches;


should this be copy?

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

stuartmorgan-g

Changes LGTM with one nit.

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

cbracken · 2024-10-23T00:00:24Z

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

  scrollView.contentOffset = CGPointMake(kScrollViewContentSize, kScrollViewContentSize);

-  [self.view addSubview:self.scrollView];
+  [self.view addSubview:scrollView];


And that's really the most important part of creating the scrollView – the adding it as a subview.

jmagman · 2024-10-25T20:51:12Z

I'm going to convert this to a draft, @cbracken can you mark it as ready when you'd like us to review?

flutter-dashboard · 2024-10-25T20:53:02Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

…tually

Commit crime... then immediately cover my tracks so it looks like the scrollView is set. I mean it is... it's just never added to the view hierarchy, which is really the most important part.

Previously we held a scoped_nsobject<FlutterEngine> as a strong reference ivar. This restores that behaviour.

cbracken · 2024-10-26T00:22:44Z

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

-  fml::ScopedBlock<void (^)(void)> _flutterViewRenderedCallback;
-  UIInterfaceOrientationMask _orientationPreferences;
-  UIStatusBarStyle _statusBarStyle;
+  FlutterEngine* _engine;


Notice that previously we held _engine as a strong ref (the fml::scoped_nsobject above) but that the header declares it as a weak property:

engine/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h

Line 236 in 33f9064

@property(weak, nonatomic, readonly) FlutterEngine* engine;

Meanwhile, the engine owns FlutterViewController weakly:

engine/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

Line 157 in 33f9064

fml::WeakNSObject<FlutterViewController> _viewController;

To preserve existing semantics, I'm adding back a strong ref here. We need the getter too so that the compiler doesn't complain that the ivar should be declared as __weak FlutterEngine* _engine.

We should probably update the header to be a strong property, but it's a public header and I don't think that ARC migration is the right spot to change semantics in our public APIs.

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken requested review from jmagman and hellohuanlin October 4, 2024 22:34

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken commented Oct 4, 2024

View reviewed changes

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

cbracken requested a review from stuartmorgan-g October 4, 2024 23:23

jmagman reviewed Oct 4, 2024

View reviewed changes

jmagman approved these changes Oct 5, 2024

View reviewed changes

stuartmorgan-g approved these changes Oct 5, 2024

View reviewed changes

hellohuanlin reviewed Oct 7, 2024

View reviewed changes

stuartmorgan-g approved these changes Oct 18, 2024

View reviewed changes

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

cbracken mentioned this pull request Oct 21, 2024

Migrate PlatformViewIOS to ARC #55672

Merged

9 tasks

cbracken commented Oct 23, 2024

View reviewed changes

github-actions bot added affects: desktop platform-macos labels Oct 23, 2024

jmagman marked this pull request as draft October 25, 2024 20:50

cbracken added 3 commits October 25, 2024 17:06

iOS: Migrate FlutterViewController to ARC

7f9c045

Migrate _engine to property

60ea3e5

Migrate _flutterView to property

7e46ed2

cbracken added 18 commits October 25, 2024 17:06

ivar stuff

134e6d3

Moar property syntax

03ec71b

Add engine non-nil asserts

258e7e6

Set explicit type for my own sanity

5c9923b

One more property

445202b

err YES, not true. NO, not false.

bd4af67

Let's maybe assert ARC

e4d6308

retain to strong

e7360f4

Fix self capture

be400a2

Revert to previous setup local then assign

25a6263

Remove NSAsserts on self.engine -- though we probably do want em even…

496e4f5

…tually

review feedback: comments and todos

f757a08

review feedback: moar strongSelf

f3c808e

Extract keyboardAnimationCallback helper

c492fcc

Extract onFirstFrameRendered

861d6a0

Format

be71895

review feedback: method name

18781e1

Hangs head in shame

49800bb

Commit crime... then immediately cover my tracks so it looks like the scrollView is set. I mean it is... it's just never added to the view hierarchy, which is really the most important part.

cbracken marked this pull request as ready for review October 26, 2024 00:09

Weak engine is strong

4f9cf85

Previously we held a scoped_nsobject<FlutterEngine> as a strong reference ivar. This restores that behaviour.

cbracken commented Oct 26, 2024

View reviewed changes

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

auto-submit bot merged commit 3162aaa into flutter:main Oct 26, 2024
30 checks passed

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

Roll Flutter Engine from c9b8ac96f6ce to 3162aaad0086 (3 revisions) flutter/flutter#157677

Closed

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

3162aaad0 Migrate FlutterViewController to ARC (flutter/engine#55669)

b8aa4ce

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

Roll Flutter Engine from c9b8ac96f6ce to 3162aaad0086 (3 revisions) flutter/flutter#157681

Merged

cbracken deleted the arc-viewcontroller branch November 28, 2024 16:31

Migrate FlutterViewController to ARC #55669

Migrate FlutterViewController to ARC #55669

Uh oh!

Conversation

cbracken commented Oct 4, 2024

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 4, 2024

Uh oh!

Uh oh!

Uh oh!

cbracken Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

stuartmorgan-g Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Oct 7, 2024

Uh oh!

hellohuanlin Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman commented Oct 25, 2024

Uh oh!

flutter-dashboard bot commented Oct 25, 2024

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cbracken Oct 26, 2024 •

edited

Loading