Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC #52148

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC #52148

merged 2 commits into from
Apr 16, 2024

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented Apr 16, 2024

Clean up headers in FlutterChannelKeyResponder and FlutterSpellCheckPlugin. Migrate to ARC.

Move FlutterSpellCheckResult interface into the .mm since it's only used there.

Part of flutter/flutter#137801.
Blocked by #51633 FlutterUIPressProxy

@jmagman jmagman self-assigned this Apr 16, 2024
jmagman
jmagman commented Apr 16, 2024
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm
- (void)dealloc {
[_suggestions release];
[super dealloc];
return @{
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid the NSMutableDictionary

@jmagman jmagman force-pushed the channel-key-responder-arc branch 2 times, most recently from e116695 to 5066c4c Compare April 16, 2024 05:53
@jmagman jmagman marked this pull request as ready for review April 16, 2024 06:58
stuartmorgan-g
stuartmorgan-g approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM. Two quick suggested improvements to lines you are touching anyway, but both are preexisting and minor so your call.

shell/platform/darwin/ios/framework/Source/FlutterChannelKeyResponder.mm Outdated
@@ -139,7 +135,7 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
NSString* characters = getEventCharacters(press.key.characters, press.key.keyCode);
NSString* charactersIgnoringModifiers =
getEventCharacters(press.key.charactersIgnoringModifiers, press.key.keyCode);
NSMutableDictionary* keyMessage = [[@{
NSMutableDictionary* keyMessage = [@{
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just make this an NSDictionary and remove the mutableCopy; sendMessage: doesn't require a mutable dictionary. I looked at the history; there used to be conditional additional key/value setting logic, which is why it was mutable, but now that's just cruft.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm Outdated
@@ -88,13 +102,12 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
}
} while (nextSpellSuggestion != nil && nextOffset < text.length);

NSMutableArray* methodChannelResult = [[[NSMutableArray alloc] init] autorelease];
NSMutableArray* methodChannelResult = [[NSMutableArray alloc] init];
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: it would be nice to make this an initWithCapacity: to avoid pointless re-allocations in the loop, since we know the length will be the same as allSpellSuggestions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jmagman jmagman added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Apr 16, 2024
@jmagman jmagman force-pushed the channel-key-responder-arc branch from 3d7715d to 9502111 Compare April 16, 2024 22:03
@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 16, 2024
@auto-submit auto-submit bot merged commit e7d8c62 into flutter:main Apr 16, 2024
29 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 17, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 17, 2024
@engine-flutter-autoroll
e7d8c62d2 Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlu…
8047371 
…gin to ARC (flutter/engine#52148)
@jmagman jmagman deleted the channel-key-responder-arc branch April 17, 2024 01:17
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 17, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 4d69c0c95483 to e7d8c62d291a (11 revisions) (#…
ab98a0a 
…146857)

flutter/engine@4d69c0c...e7d8c62

2024-04-16 [email protected] Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC (flutter/engine#52148)
2024-04-16 [email protected] Roll Skia from b83b6bf7174f to e335a0a11aa0 (1 revision) (flutter/engine#52175)
2024-04-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] remove most temporary allocation during polyline generation. (#52131)" (flutter/engine#52177)
2024-04-16 [email protected] Roll Skia from d506a3d526f7 to b83b6bf7174f (2 revisions) (flutter/engine#52173)
2024-04-16 [email protected] Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC (flutter/engine#51633)
2024-04-16 [email protected] Roll Skia from 98dbba281a84 to d506a3d526f7 (2 revisions) (flutter/engine#52170)
2024-04-16 [email protected] Roll Dart SDK from f2464b2892a1 to 57d7cba5bc3d (1 revision) (flutter/engine#52168)
2024-04-16 [email protected] Roll Skia from 300741074b41 to 98dbba281a84 (11 revisions) (flutter/engine#52167)
2024-04-16 [email protected] [Impeller] removes advanced plus blending (flutter/engine#52163)
2024-04-16 [email protected] [Impeller] remove most temporary allocation during polyline generation. (flutter/engine#52131)
2024-04-16 [email protected] Roll reclient, libpng, and zlib (flutter/engine#52072)

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
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
@engine-flutter-autoroll @gilnobrega
Roll Flutter Engine from 4d69c0c95483 to e7d8c62d291a (11 revisions) (f…
c85724b 
…lutter#146857)

flutter/engine@4d69c0c...e7d8c62

2024-04-16 [email protected] Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC (flutter/engine#52148)
2024-04-16 [email protected] Roll Skia from b83b6bf7174f to e335a0a11aa0 (1 revision) (flutter/engine#52175)
2024-04-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] remove most temporary allocation during polyline generation. (flutter#52131)" (flutter/engine#52177)
2024-04-16 [email protected] Roll Skia from d506a3d526f7 to b83b6bf7174f (2 revisions) (flutter/engine#52173)
2024-04-16 [email protected] Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC (flutter/engine#51633)
2024-04-16 [email protected] Roll Skia from 98dbba281a84 to d506a3d526f7 (2 revisions) (flutter/engine#52170)
2024-04-16 [email protected] Roll Dart SDK from f2464b2892a1 to 57d7cba5bc3d (1 revision) (flutter/engine#52168)
2024-04-16 [email protected] Roll Skia from 300741074b41 to 98dbba281a84 (11 revisions) (flutter/engine#52167)
2024-04-16 [email protected] [Impeller] removes advanced plus blending (flutter/engine#52163)
2024-04-16 [email protected] [Impeller] remove most temporary allocation during polyline generation. (flutter/engine#52131)
2024-04-16 [email protected] Roll reclient, libpng, and zlib (flutter/engine#52072)

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

@hellohuanlin hellohuanlin hellohuanlin approved these changes

Assignees

@jmagman jmagman

Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmagman @stuartmorgan-g @hellohuanlin