Refactor Semantics in preparation for ARC migration #52729

jmagman · 2024-05-11T00:59:10Z

Split the too-large Semantics MRC to ARC migration into two PRs: this one, which refactors, and the next which will actually do the migration.

Use properties instead of their backing ivars (except in the usual init, dealloc, getters/setters)
For Foundation collections prefer copy over strong.
Dot notation for properties
Change privateSetParent: to instead use a readwrite property in SemanticsObject ().
Switch the semanticsObject property from weak to retain to get the synthesized property (keeping it as weak is a compilation error in MRC) but I'll swap it back to a weak in the ARC migration PR coming next.
SemanticsObjectTest fails on my machine and passes on CI. Switched the cleaner CGRectEqualToRect (and related) checks to instead assert x, y, width, height so we can see the value when it fails:

((CGSizeEqualToSize( scrollView.contentSize, CGSizeMake((w + scrollExtentMax) * effectivelyScale, h * effectivelyScale))) is true) failed

becomes:

((scrollView.contentSize.height) equal to (h * effectivelyScale)) failed: ("33.3333333333") is not equal to ("33.333336")

Use XCTAssertEqualWithAccuracy now that I can see it's a floating point precision issue.

jmagman · 2024-05-11T01:21:39Z

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

@@ -231,7 +231,7 @@ constexpr float kScrollExtentMaxForInf = 1000;
 bridge:(fml::WeakPtr<flutter::AccessibilityBridgeIos>)bridge
 NS_DESIGNATED_INITIALIZER;

-@property(nonatomic, weak) SemanticsObject* semanticsObject;
+@property(nonatomic, assign) SemanticsObject* semanticsObject;


Made this assign to get the property synthesis. I'm going to change it back to weak when I do the ARC migration, which allows weak property synthesis. This header isn't public.

oh wow i didn't know they have weak support in MRC.

Made this assign to get the property synthesis
So weak in MRC doesn't synthesize the getter/setter/ivar?

jmagman · 2024-05-11T01:22:17Z

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

 /** Should only be called in conjunction with setting child/parent relationship. */
- (void)privateSetParent:(SemanticsObject*)parent;
+@property(nonatomic, assign, readwrite) SemanticsObject* parent;


Swap privateSetParent: to a readwrite property.

nit: readwrite is already the default

this will change to weak in arc right?

Yes, I will make it weak in the ARC migration PR.

I was making the readwrite explicit since it's now overriding a readonly in the header:

@property(nonatomic, assign, readonly) SemanticsObject* parent;

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

jmagman · 2024-05-11T01:25:29Z

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

 }

 @end

 @implementation SemanticsObjectContainer {
- SemanticsObject* _semanticsObject;


This backing ivar isn't when I swap from weak to assign. I included it here because when I migrate to ARC this isn't needed since the weak ARC property is auto-synthesized.

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

stuartmorgan

I love to see all this modernization! LGTM!

Just some extremely optional comments.

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

stuartmorgan · 2024-05-13T23:24:45Z

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

 [_children release];
 [_childrenInHitTestOrder release];

 _parent = nil;
- _container.get().semanticsObject = nil;


Just to make sure I'm reading the other code correctly: this is safe to remove because semantics object was changed to weak at some point, right, and thus we don't need to explicitly clear it any more?

That's how I read it too, I don't think weak property would need to be set to nil in MRC?

I see this comment where it was introduced:
https://github.com/flutter/engine/pull/7244/files#r259211775

But I don't see why it would be needed...

stuartmorgan · 2024-05-13T23:25:56Z

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

 }
 [_children removeAllObjects];
- [_childrenInHitTestOrder removeAllObjects];
 [_children release];
 [_childrenInHitTestOrder release];

 _parent = nil;


Is this here for the same reason as the awful _inDealloc workaround? In theory it shouldn't be necessary.

The _inDealloc workaround was done in #27786, which didn't include setting _parent to nil.

Setting _parent to nil was done in #4602 and I think it was for this guard:

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

Lines 709 to 714 in b4b798d

if ([self parent] == nil) {

// This can happen when we have released the accessibility tree but iOS is

// still holding onto our objects. iOS can take some time before it

// realizes that the tree has changed.

return nil;

}

I don't like this but it seems like iOS is, or at least was, digging around the accessibilityContainer type methods during -[super dealloc] which was causing crashes. I'd rather not touch that for this refactor.

stuartmorgan · 2024-05-13T23:28:34Z

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

 }

 return self;
 }

 - (void)dealloc {
 for (SemanticsObject* child in _children) {
- [child privateSetParent:nil];
+ child.parent = nil;
 }
 [_children removeAllObjects];


Can we remove this too? Hopefully we don't have code that is relying on the contents of an array that came from a since-deallocated object changing out from under it...

I'll remove it in the ARC migration.

hellohuanlin · 2024-05-14T00:19:06Z

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

@@ -231,7 +231,7 @@ constexpr float kScrollExtentMaxForInf = 1000;
 bridge:(fml::WeakPtr<flutter::AccessibilityBridgeIos>)bridge
 NS_DESIGNATED_INITIALIZER;

-@property(nonatomic, weak) SemanticsObject* semanticsObject;
+@property(nonatomic, assign) SemanticsObject* semanticsObject;


oh wow i didn't know they have weak support in MRC.

Made this assign to get the property synthesis
So weak in MRC doesn't synthesize the getter/setter/ivar?

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

hellohuanlin · 2024-05-14T00:22:55Z

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

 /** Should only be called in conjunction with setting child/parent relationship. */
- (void)privateSetParent:(SemanticsObject*)parent;
+@property(nonatomic, assign, readwrite) SemanticsObject* parent;


nit: readwrite is already the default

hellohuanlin · 2024-05-14T00:24:38Z

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

 /** Should only be called in conjunction with setting child/parent relationship. */
- (void)privateSetParent:(SemanticsObject*)parent;
+@property(nonatomic, assign, readwrite) SemanticsObject* parent;


this will change to weak in arc right?

…r/engine#52729)

…148338) flutter/engine@7bf8657...08b44d9 2024-05-14 [email protected] [Impeller] migrated one test over from aiks to dl (flutter/engine#52786) 2024-05-14 [email protected] [Impeller] Create framebuffer blend vertices based on the snapshot's texture size instead of coverage (flutter/engine#52790) 2024-05-14 [email protected] Refactor Semantics in preparation for ARC migration (flutter/engine#52729) 2024-05-14 [email protected] Rolls in buildroot with stack protection flag (flutter/engine#52774) 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],[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

On top of the `SemanticsObject` refactor #52729 migrate `SemanticsObject` and `FlutterSemanticsScrollView` to ARC. Part of flutter/flutter#137801.

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

jmagman self-assigned this May 11, 2024

Refactor Semantics in preparation of ARC migration

0700783

jmagman force-pushed the semantics-refactor branch from aa3da13 to 0700783 Compare May 11, 2024 01:19

jmagman commented May 11, 2024

View reviewed changes

release

627e040

jmagman changed the title ~~Refactor Semantics in preparation of ARC migration~~ Refactor Semantics in preparation for ARC migration May 11, 2024

autorelease

aa33ded

jmagman marked this pull request as ready for review May 13, 2024 20:47

jmagman requested review from stuartmorgan and hellohuanlin May 13, 2024 20:48

jmagman mentioned this pull request May 13, 2024

Migrate TextInputSemanticsObject to ARC #52785

Merged

stuartmorgan approved these changes May 13, 2024

View reviewed changes

hellohuanlin reviewed May 14, 2024

View reviewed changes

review edits

4927970

jmagman force-pushed the semantics-refactor branch from a134df0 to 4927970 Compare May 14, 2024 07:35

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2024

auto-submit bot merged commit 86c55dd into flutter:main May 14, 2024
29 checks passed

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

Roll Flutter Engine from 7bf865774d06 to 86c55dde6963 (2 revisions) flutter/flutter#148336

Closed

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

86c55dde6 Refactor Semantics in preparation for ARC migration (flutte…

6692ff8

…r/engine#52729)

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

Roll Flutter Engine from 7bf865774d06 to 08b44d906fab (4 revisions) flutter/flutter#148338

Merged

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

86c55dde6 Refactor Semantics in preparation for ARC migration (flutte…

682e646

…r/engine#52729)

jmagman mentioned this pull request May 14, 2024

Migrate SemanticsObject and FlutterSemanticsScrollView to ARC #52824

Merged

jmagman deleted the semantics-refactor branch May 15, 2024 03:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Semantics in preparation for ARC migration #52729

Refactor Semantics in preparation for ARC migration #52729

jmagman commented May 11, 2024 •

edited

jmagman May 11, 2024

hellohuanlin May 14, 2024

jmagman May 11, 2024

hellohuanlin May 14, 2024

hellohuanlin May 14, 2024

jmagman May 14, 2024

jmagman May 11, 2024

stuartmorgan left a comment

stuartmorgan May 13, 2024

jmagman May 14, 2024

stuartmorgan May 13, 2024

jmagman May 14, 2024

stuartmorgan May 13, 2024

jmagman May 14, 2024

hellohuanlin May 14, 2024

hellohuanlin May 14, 2024

hellohuanlin May 14, 2024

	if ([self parent] == nil) {
	// This can happen when we have released the accessibility tree but iOS is
	// still holding onto our objects. iOS can take some time before it
	// realizes that the tree has changed.
	return nil;
	}

Refactor Semantics in preparation for ARC migration #52729

Refactor Semantics in preparation for ARC migration #52729

Conversation

jmagman commented May 11, 2024 • edited

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

stuartmorgan left a 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 commented May 11, 2024 •

edited