Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate SemanticsObject and FlutterSemanticsScrollView to ARC #52824

Merged
merged 2 commits into from
May 15, 2024

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented May 14, 2024

On top of the SemanticsObject refactor #52729 migrate SemanticsObject and FlutterSemanticsScrollView to ARC.

Part of flutter/flutter#137801.

@jmagman jmagman marked this pull request as ready for review May 14, 2024 21:23
@jmagman jmagman self-assigned this May 14, 2024
@@ -296,14 +289,9 @@ - (void)dealloc {
for (SemanticsObject* child in _children) {
child.parent = nil;
}
[_children removeAllObjects];
[_children release];
[_childrenInHitTestOrder release];

_parent = nil;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion in the previous PR it's not clear we can. Instead maybe we can add a comment for these two lines explaining why they are necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion in the previous PR

To link it back: #52729 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.

[[[SemanticsObjectContainer alloc] initWithSemanticsObject:self
bridge:self.bridge] autorelease];
self.container = [[SemanticsObjectContainer alloc] initWithSemanticsObject:self
bridge:[self bridge]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: self.bridge

@@ -794,7 +779,6 @@ @implementation FlutterSemanticsObject {

// Method declared as unavailable in the interface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely not related to this pr - can we just remove this init since it's already marked as unavailable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -296,14 +289,9 @@ - (void)dealloc {
for (SemanticsObject* child in _children) {
child.parent = nil;
}
[_children removeAllObjects];
[_children release];
[_childrenInHitTestOrder release];

_parent = nil;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion in the previous PR it's not clear we can. Instead maybe we can add a comment for these two lines explaining why they are necessary.

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label May 15, 2024
@auto-submit auto-submit bot merged commit 57d6b29 into flutter:main May 15, 2024
29 checks passed
@jmagman jmagman deleted the arc-semantics-object branch May 15, 2024 04:41
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2024
auto-submit bot pushed a commit that referenced this pull request May 15, 2024
1. Migrate TextInputSemanticsObject to ARC
2. Rename the Objective-C-only `accessibility_text_entry.*` files to `TextInputSemanticsObject.*`
3. Move `FlutterInactiveTextInput` interface from the header file into the implementation, the only place it's used.

Blocked on `SemanticsObject` ARC migration #52824

Part of flutter/flutter#137801.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 15, 2024
…148428)

flutter/engine@bf1c6da...41b86b5

2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from 475559da107d to b9a304eb05d2 (1 revision) (flutter/engine#52857)
2024-05-15 magder@google.com Migrate TextInputSemanticsObject to ARC (flutter/engine#52785)
2024-05-15 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from NbCi2ETfLHPLLB-JV... to Ftw7GopnudHydGS1y... (flutter/engine#52853)
2024-05-15 skia-flutter-autoroll@skia.org Roll Dart SDK from 7d9de0c0b231 to f773d45634ed (2 revisions) (flutter/engine#52854)
2024-05-15 zanderso@users.noreply.github.com Roll buildroot (flutter/engine#52826)
2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from 4a7419d662d5 to 475559da107d (5 revisions) (flutter/engine#52848)
2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from a9ba7411d4b2 to 4a7419d662d5 (1 revision) (flutter/engine#52844)
2024-05-15 skia-flutter-autoroll@skia.org Roll Dart SDK from 1eec9de2f9c6 to 7d9de0c0b231 (1 revision) (flutter/engine#52841)
2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from b3aadd56d187 to a9ba7411d4b2 (1 revision) (flutter/engine#52839)
2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from c0caf10486ce to b3aadd56d187 (3 revisions) (flutter/engine#52837)
2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from fad584324d85 to c0caf10486ce (1 revision) (flutter/engine#52835)
2024-05-15 magder@google.com Migrate SemanticsObject and FlutterSemanticsScrollView to ARC (flutter/engine#52824)
2024-05-15 skia-flutter-autoroll@skia.org Roll Dart SDK from 0b77fbab8cf5 to 1eec9de2f9c6 (1 revision) (flutter/engine#52833)
2024-05-15 skia-flutter-autoroll@skia.org Roll Skia from d0d87c26b489 to fad584324d85 (1 revision) (flutter/engine#52831)
2024-05-15 matanlurey@users.noreply.github.com Re-add `MipFilter::kBase`, but keep `kNearest` as the default. (flutter/engine#52779)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from NbCi2ETfLHPL to Ftw7GopnudHy

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 bdero@google.com,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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
3 participants