Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

jmagman · 2024-05-02T23:37:22Z

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate FlutterView, FlutterPlatformViews, and FlutterOverlayView from MRC to ARC.

jmagman · 2024-05-02T23:37:42Z

shell/platform/darwin/ios/BUILD.gn

@@ -87,6 +92,9 @@ source_set("flutter_framework_source_arc") {
    "framework/Source/FlutterUndoManagerDelegate.h",
    "framework/Source/FlutterUndoManagerPlugin.h",
    "framework/Source/FlutterUndoManagerPlugin.mm",
+    "framework/Source/FlutterView.h",
+    "framework/Source/FlutterView.mm",
+    "framework/Source/FlutterViewResponder.h",


FlutterViewResponder.h wasn't included anywhere?

jmagman · 2024-05-02T23:44:55Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -210,16 +210,19 @@ class FlutterPlatformViewsController {

  fml::WeakPtr<flutter::FlutterPlatformViewsController> GetWeakPtr();

-  void SetFlutterView(UIView* flutter_view);
+  void SetFlutterView(UIView* flutter_view) __attribute__((cf_audited_transfer));


See cf_audited_transfer discussion #52226 (comment)

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

jmagman · 2024-05-08T20:15:53Z

testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj

@@ -99,6 +100,7 @@
 		0AC232E924BA71D300A85907 /* Source */ = {
 			isa = PBXGroup;
 			children = (
+				F76A3A892BE48F2F00A654F1 /* FlutterPlatformViewsTest.mm */,


Just a convenience to add it here, the test dylib made from BUILD.gn already built it.

I found this while migrating `FlutterPlatformViews_Internal.mm` to ARC #52535. I'll land this first. ```objc if (_backdropFilterView != visualEffectView) { _backdropFilterView = [visualEffectView retain]; } ``` should instead be something like: ```objc if (_backdropFilterView != visualEffectView) { id oldBackdropFilterView = _backdropFilterView; _backdropFilterView = [visualEffectView retain]; [oldBackdropFilterView release]; } ``` But that's already what the built-in MRC `nonatomic, retain` property setter does, so use that instead. Added a test that passes on this PR and fails on main.

jmagman · 2024-05-10T03:51:20Z

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

- (void)dealloc {
-  [_flutterAccessibilityContainer release];
-  [super dealloc];
+  return self.flutterAccessibilityContainer;


@tacck can you double-check me here re: #52484 ?

I think it's LGTM!

jmagman · 2024-05-10T03:53:05Z

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

-  UIView* _embeddedView;
-}
+@interface FlutterTouchInterceptingView ()
+@property(nonatomic, weak, readonly) UIView* embeddedView;


Not retained:

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

Line 993 in ea83156

_embeddedView = embeddedView;

jmagman · 2024-05-10T03:54:05Z

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

-@property(nonatomic) bool touchedEndedWithoutBlocking;
+@property(nonatomic) BOOL touchedEndedWithoutBlocking;
+
+@property(nonatomic, readonly) UIGestureRecognizer* forwardingRecognizer;


Swapping from scoped_nsobject to a property.

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

Lines 1073 to 1075 in ea83156

@implementation DelayingGestureRecognizer {

fml::scoped_nsobject<UIGestureRecognizer> _forwardingRecognizer;

}

jmagman · 2024-05-10T03:54:59Z

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

-}
+@interface FlutterTouchInterceptingView ()
+@property(nonatomic, weak, readonly) UIView* embeddedView;
+@property(nonatomic, readonly) DelayingGestureRecognizer* delayingRecognizer;


Swapping from scoped_nsobject to a property.

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

Line 981 in ea83156

fml::scoped_nsobject<DelayingGestureRecognizer> _delayingRecognizer;

jmagman · 2024-05-10T03:57:00Z

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

-@property(retain, nonatomic) NSArray<PlatformViewFilter*>* filters;
-@property(retain, nonatomic) NSMutableArray<UIVisualEffectView*>* backdropFilterSubviews;
+@property(nonatomic, copy) NSArray<PlatformViewFilter*>* filters;
+@property(nonatomic) NSMutableArray<UIVisualEffectView*>* backdropFilterSubviews;


Mutable collection, strong

jmagman · 2024-05-10T03:57:03Z

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


 // The pool contains the views that are available to use.
 // The number of items in the pool must not excceds `capacity`.
-@property(retain, nonatomic) NSMutableSet<FlutterClippingMaskView*>* pool;
+@property(nonatomic) NSMutableSet<FlutterClippingMaskView*>* pool;


Mutable collection, strong

jmagman · 2024-05-10T03:57:49Z

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

+FLUTTER_ASSERT_ARC
+
+@interface FlutterView ()
+@property(nonatomic, weak) id<FlutterViewEngineDelegate> delegate;


Not retained

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

Line 72 in ea83156

_delegate = delegate;

jmagman · 2024-05-10T04:00:02Z

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

@@ -417,7 +416,7 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,

 long FlutterPlatformViewsController::FindFirstResponderPlatformViewId() {
  for (auto const& [id, root_view] : root_views_) {
-    if ((UIView*)(root_view.get()).flt_hasFirstResponderInViewHierarchySubtree) {
+    if (((UIView*)root_view.get()).flt_hasFirstResponderInViewHierarchySubtree) {


error: cast of 'BOOL' (aka 'bool') to 'UIView *' is disallowed with ARC

../../../flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm:419:9: error: cast of 'BOOL' (aka 'bool') to 'UIView *' is disallowed with ARC 419 | if ((UIView*)(root_view.get()).flt_hasFirstResponderInViewHierarchySubtree) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.

This was somehow working before?

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

Lines 2771 to 2775 in ea83156

XCTAssertTrue(textField.isFirstResponder);

XCTAssertTrue(textField.flt_hasFirstResponderInViewHierarchySubtree);

[textField resignFirstResponder];

XCTAssertFalse(textField.isFirstResponder);

XCTAssertFalse(textField.flt_hasFirstResponderInViewHierarchySubtree);

#33093

hellohuanlin · 2024-05-10T18:18:20Z

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

@@ -417,7 +416,7 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,

 long FlutterPlatformViewsController::FindFirstResponderPlatformViewId() {
  for (auto const& [id, root_view] : root_views_) {
-    if ((UIView*)(root_view.get()).flt_hasFirstResponderInViewHierarchySubtree) {
+    if (((UIView*)root_view.get()).flt_hasFirstResponderInViewHierarchySubtree) {


…ew to ARC (flutter/engine#52535)

…148147) flutter/engine@d4f705c...ba8e0d3 2024-05-10 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Clip before applying ColorFilter so it doesn't filter beyond child bounds (flutter/engine#52704) 2024-05-10 magder@google.com Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC (flutter/engine#52535) 2024-05-10 matanlurey@users.noreply.github.com Infer `--rbe` based on the existence of `//flutter/build/rbe` (flutter/engine#52700) 2024-05-10 jonahwilliams@google.com [Impeller] Disable AHB swapchain. (flutter/engine#52713) 2024-05-10 skia-flutter-autoroll@skia.org Roll Skia from c7cd1e9690d1 to 11d892ce49b6 (25 revisions) (flutter/engine#52712) 2024-05-10 chinmaygarde@google.com [Impeller] Document how to do basic rendering in Impeller. (flutter/engine#52703) 2024-05-10 30870216+gaaclarke@users.noreply.github.com [impeller] adds experimental canvas docstring (flutter/engine#52710) 2024-05-10 daniel.l@hpcnt.com Roll third_party/freetype2 from 3bea27612 to af4c2d86d (2 revisions) (flutter/engine#52689) 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 rmistry@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

github-actions bot added the platform-ios label May 2, 2024

jmagman commented May 2, 2024

View reviewed changes

jmagman force-pushed the flutter-view-arc branch from c61ec7d to a567cf9 Compare May 7, 2024 01:17

jmagman mentioned this pull request May 7, 2024

Fix UIVisualEffectView leak in platform view filter #52591

Merged

jmagman commented May 8, 2024

View reviewed changes

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

jmagman commented May 8, 2024

View reviewed changes

jmagman force-pushed the flutter-view-arc branch from 6ac5e98 to a87814b Compare May 8, 2024 23:15

jmagman mentioned this pull request May 9, 2024

[iOS] Fix App crash when use WebView with iOS VoiceOver #52484

Merged

8 tasks

jmagman added 3 commits May 9, 2024 20:44

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC

4885b63

clang-tidy

f089c3b

Rebase with flutter#52484

baa72bc

jmagman force-pushed the flutter-view-arc branch from a87814b to baa72bc Compare May 10, 2024 03:50

jmagman commented May 10, 2024

View reviewed changes

jmagman marked this pull request as ready for review May 10, 2024 17:00

jmagman requested review from stuartmorgan and hellohuanlin May 10, 2024 17:00

hellohuanlin approved these changes May 10, 2024

View reviewed changes

jmagman merged commit f553caa into flutter:main May 10, 2024
26 checks passed

jmagman deleted the flutter-view-arc branch May 10, 2024 19:07

engine-flutter-autoroll mentioned this pull request May 10, 2024

Roll Flutter Engine from d4f705ccb695 to f553caaa220b (7 revisions) flutter/flutter#148145

Closed

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

f553caaa2 Migrate FlutterView, FlutterPlatformViews, FlutterOverlayVi…

0a55d75

…ew to ARC (flutter/engine#52535)

engine-flutter-autoroll mentioned this pull request May 10, 2024

Roll Flutter Engine from d4f705ccb695 to ba8e0d3e2f23 (8 revisions) flutter/flutter#148147

Merged

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

f553caaa2 Migrate FlutterView, FlutterPlatformViews, FlutterOverlayVi…

d9f76f2

…ew to ARC (flutter/engine#52535)

jmagman mentioned this pull request Jul 26, 2024

Crash [FlutterView isWideGamutSupported] objc_msgSend flutter/flutter#152226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

jmagman commented May 2, 2024

jmagman May 2, 2024

jmagman May 2, 2024

jmagman May 8, 2024

jmagman May 10, 2024

tacck May 10, 2024

jmagman May 10, 2024

jmagman May 10, 2024

jmagman May 10, 2024

jmagman May 10, 2024

jmagman May 10, 2024

jmagman May 10, 2024

jmagman May 10, 2024 •

edited

Loading

hellohuanlin May 10, 2024

hellohuanlin May 10, 2024

	@implementation DelayingGestureRecognizer {
	fml::scoped_nsobject<UIGestureRecognizer> _forwardingRecognizer;
	}

	XCTAssertTrue(textField.isFirstResponder);
	XCTAssertTrue(textField.flt_hasFirstResponderInViewHierarchySubtree);
	[textField resignFirstResponder];
	XCTAssertFalse(textField.isFirstResponder);
	XCTAssertFalse(textField.flt_hasFirstResponderInViewHierarchySubtree);

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

Conversation

jmagman commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman May 10, 2024 •

edited

Loading