Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC #51633

jmagman · 2024-03-22T22:55:58Z

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

Migrate some files from MRC to ARC. These files do not themselves import any MRC files, only ARC-ified files like FlutterMetalLayer.h, making them leaf nodes in the dependency graph of MRC files. Just doing a few at a time to make the dependency graph manageable, and to easily revert if this causes retain cycles or other memory management issues.

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

jmagman · 2024-04-04T00:18:09Z

shell/platform/darwin/ios/rendering_api_selection.mm

  if (@available(iOS METAL_IOS_VERSION_BASELINE, *)) {
    auto device = MTLCreateSystemDefaultDevice();
    ios_version_supports_metal = [device supportsFeatureSet:MTLFeatureSet_iOS_GPUFamily1_v3];
-    [device release];


This kicked off a yak shave to remove this code by removing SHELL_ENABLE_METAL

https://github.com/flutter/engine/pull/51636/files#diff-1db60a8a74c1cfb40c970541dd1f7f0f6301853a2ba17451f5117f154036d732L27

I'll let that merge and then rebase this on top.

stuartmorgan · 2024-04-09T10:58:07Z

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

@@ -2,9 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterUIPressProxy.h"
+
+#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"


Why this order change? Does the engine not follow the suggested style guide header order where framework imports are before non-associated-header project imports?

There seem to be many places in the engine that don't follow the guidelines. In this case I believe I should have made it:

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterUIPressProxy.h" #import <UIKit/UIKit.h> #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"

stuartmorgan · 2024-04-09T11:00:19Z

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

 #import <UIKit/UIKit.h>

-#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterUIPressProxy.h"
+FLUTTER_ASSERT_ARC

 @interface FlutterUIPressProxy ()
 @property(nonatomic, readonly) UIPress* press;


Were these properties accidentally weak before and it just hadn't happened to break anything?

I guess? The usages look like this:

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

Lines 1885 to 1889 in fef8499

[self handlePressEvent:[[[FlutterUIPressProxy alloc] initWithPress:press

withEvent:event] autorelease]

nextAction:^() {

[super pressesBegan:[NSSet setWithObject:press] withEvent:event];

}];

so maybe press and event are being retained in the nextAction block? I don't see any obvious bugs about it...

Interesting - yeah seems like this should previously have been explicitly strong, and maybe worked because (I assume) it's retained in that map we create to pointer events being sent to the framework until we get a response? (Didn't look but IIRC we hold onto them there)

stuartmorgan · 2024-04-09T11:02:33Z

shell/platform/darwin/ios/ios_context_metal_impeller.mm

-      fml::scoped_nsobject<FlutterDarwinExternalTextureMetal>{
-          [[darwin_context_metal_impeller_ createExternalTextureWithIdentifier:texture_id
-                                                                       texture:texture] retain]});
+      fml::scoped_nsobject<FlutterDarwinExternalTextureMetal>{[darwin_context_metal_impeller_


Do we still need the scoped_nsobject here?

Ideally the answer should be no, but since scoped_nsobject is ARC-friendly, we should be safe to do those in a followup patch if preferred.

ios_context_->CreateExternalTexture is only called from mrc platform_view_ios:

engine/shell/platform/darwin/ios/platform_view_ios.mm

Lines 139 to 140 in a9a4496

RegisterTexture(ios_context_->CreateExternalTexture(

texture_id, fml::scoped_nsobject<NSObject<FlutterTexture>>{[texture retain]}));

I'm going to hold off on removing scoped_nsobject from this signature until I can remove it there as well. Do you foresee any issues with that?

Should be fine to keep scoped_nsobjects since we have the multi-mode compliant version.

stuartmorgan · 2024-04-09T11:02:56Z

shell/platform/darwin/ios/ios_context_metal_skia.mm

@@ -52,8 +54,7 @@
    fml::scoped_nsobject<NSObject<FlutterTexture>> texture) {
  return std::make_unique<IOSExternalTextureMetal>(
      fml::scoped_nsobject<FlutterDarwinExternalTextureMetal>{
-          [[darwin_context_metal_ createExternalTextureWithIdentifier:texture_id
-                                                              texture:texture] retain]});
+          [darwin_context_metal_ createExternalTextureWithIdentifier:texture_id texture:texture]});


Same question here.

stuartmorgan · 2024-04-09T11:04:18Z

shell/platform/darwin/ios/ios_external_texture_metal.mm

@@ -5,6 +5,8 @@
 #import "flutter/shell/platform/darwin/ios/ios_external_texture_metal.h"
 #include "flow/layers/layer.h"

+FLUTTER_ASSERT_ARC
+
 namespace flutter {

 IOSExternalTextureMetal::IOSExternalTextureMetal(


Can we convert this scoped_nsobject to a simple Obj-C object property now?

cbracken

lgtm. Whether or not you prefer to deal with scoped_nsojbect in this patch or a followup, either way seems fine to me.

cbracken · 2024-04-15T17:03:32Z

shell/platform/darwin/ios/ios_context_metal_impeller.mm

-      fml::scoped_nsobject<FlutterDarwinExternalTextureMetal>{
-          [[darwin_context_metal_impeller_ createExternalTextureWithIdentifier:texture_id
-                                                                       texture:texture] retain]});
+      fml::scoped_nsobject<FlutterDarwinExternalTextureMetal>{[darwin_context_metal_impeller_


Ideally the answer should be no, but since scoped_nsobject is ARC-friendly, we should be safe to do those in a followup patch if preferred.

cbracken · 2024-04-15T17:08:04Z

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

 #import <UIKit/UIKit.h>

-#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterUIPressProxy.h"
+FLUTTER_ASSERT_ARC

 @interface FlutterUIPressProxy ()
 @property(nonatomic, readonly) UIPress* press;


Interesting - yeah seems like this should previously have been explicitly strong, and maybe worked because (I assume) it's retained in that map we create to pointer events being sent to the framework until we get a response? (Didn't look but IIRC we hold onto them there)

jmagman · 2024-04-15T22:03:41Z

shell/platform/darwin/ios/ios_context_metal_impeller.h

@@ -25,8 +25,6 @@ class IOSContextMetalImpeller final : public IOSContext {

  ~IOSContextMetalImpeller();

-  fml::scoped_nsobject<FlutterDarwinContextMetalSkia> GetDarwinContext() const;


"Hmm, why is IOSContextMetalImpeller returning FlutterDarwinContextMetalSkia?"
It's dead code.

flutter-dashboard · 2024-04-16T03:58:43Z

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

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.

stuartmorgan · 2024-04-16T16:36:54Z

shell/platform/darwin/ios/rendering_api_selection.mm

 #include "flutter/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.h"

+FLUTTER_ASSERT_ARC
+
 namespace flutter {

 bool ShouldUseMetalRenderer() {
  bool ios_version_supports_metal = false;
  if (@available(iOS METAL_IOS_VERSION_BASELINE, *)) {
    auto device = MTLCreateSystemDefaultDevice();


Optional nit: I don't think this meets the style guide guidance on auto, since I couldn't tell what the type was; I would give this an explicit type instead.

auto-submit · 2024-04-16T18:16:05Z

auto label is removed for flutter/engine/51633, due to - The status or check suite Mac mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…#52148) 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`

…lection, and a few other files to ARC (flutter/engine#51633)

…146857) flutter/engine@4d69c0c...e7d8c62 2024-04-16 magder@google.com Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC (flutter/engine#52148) 2024-04-16 skia-flutter-autoroll@skia.org 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 skia-flutter-autoroll@skia.org Roll Skia from d506a3d526f7 to b83b6bf7174f (2 revisions) (flutter/engine#52173) 2024-04-16 magder@google.com Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC (flutter/engine#51633) 2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from 98dbba281a84 to d506a3d526f7 (2 revisions) (flutter/engine#52170) 2024-04-16 skia-flutter-autoroll@skia.org Roll Dart SDK from f2464b2892a1 to 57d7cba5bc3d (1 revision) (flutter/engine#52168) 2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from 300741074b41 to 98dbba281a84 (11 revisions) (flutter/engine#52167) 2024-04-16 30870216+gaaclarke@users.noreply.github.com [Impeller] removes advanced plus blending (flutter/engine#52163) 2024-04-16 jonahwilliams@google.com [Impeller] remove most temporary allocation during polyline generation. (flutter/engine#52131) 2024-04-16 jason-simmons@users.noreply.github.com 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 jsimmons@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

…lutter#146857) flutter/engine@4d69c0c...e7d8c62 2024-04-16 magder@google.com Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC (flutter/engine#52148) 2024-04-16 skia-flutter-autoroll@skia.org 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 skia-flutter-autoroll@skia.org Roll Skia from d506a3d526f7 to b83b6bf7174f (2 revisions) (flutter/engine#52173) 2024-04-16 magder@google.com Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC (flutter/engine#51633) 2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from 98dbba281a84 to d506a3d526f7 (2 revisions) (flutter/engine#52170) 2024-04-16 skia-flutter-autoroll@skia.org Roll Dart SDK from f2464b2892a1 to 57d7cba5bc3d (1 revision) (flutter/engine#52168) 2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from 300741074b41 to 98dbba281a84 (11 revisions) (flutter/engine#52167) 2024-04-16 30870216+gaaclarke@users.noreply.github.com [Impeller] removes advanced plus blending (flutter/engine#52163) 2024-04-16 jonahwilliams@google.com [Impeller] remove most temporary allocation during polyline generation. (flutter/engine#52131) 2024-04-16 jason-simmons@users.noreply.github.com 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 jsimmons@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

jmagman self-assigned this Mar 22, 2024

github-actions bot added the platform-ios label Mar 22, 2024

jmagman commented Apr 4, 2024

View reviewed changes

jmagman force-pushed the migrate-simple-arc branch from 398e54a to 7a8c5e9 Compare April 9, 2024 00:57

jmagman marked this pull request as ready for review April 9, 2024 04:21

jmagman force-pushed the migrate-simple-arc branch from ba7005b to cef18e6 Compare April 9, 2024 04:23

jmagman requested review from stuartmorgan and cbracken April 9, 2024 04:23

stuartmorgan reviewed Apr 9, 2024

View reviewed changes

jmagman force-pushed the migrate-simple-arc branch 2 times, most recently from 59c56fd to 608bf8c Compare April 13, 2024 03:42

cbracken approved these changes Apr 15, 2024

View reviewed changes

jmagman force-pushed the migrate-simple-arc branch from 608bf8c to 224f6d2 Compare April 15, 2024 22:02

jmagman commented Apr 15, 2024

View reviewed changes

jmagman mentioned this pull request Apr 15, 2024

Migrate ios_surface files to ARC #52139

Merged

jmagman force-pushed the migrate-simple-arc branch from 224f6d2 to dfd937d Compare April 16, 2024 03:57

jmagman changed the title ~~Migrate a few manually reference counted iOS files that do not import any other MRC files to ARC~~ Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC Apr 16, 2024

jmagman mentioned this pull request Apr 16, 2024

Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC #52148

Merged

stuartmorgan reviewed Apr 16, 2024

View reviewed changes

jmagman added 5 commits April 16, 2024 10:43

Migrate a few "leaf import" MRC iOS files to ARC

d8d6ef6

FlutterUIPressProxy

86cb270

context

cedec0c

import order

03f9b71

Remove GetDarwinContext

d4899bb

jmagman force-pushed the migrate-simple-arc branch from dfd937d to d4899bb Compare April 16, 2024 17:45

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

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 16, 2024

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

auto-submit bot merged commit 0e9ba75 into flutter:main Apr 16, 2024
32 checks passed

jmagman deleted the migrate-simple-arc branch April 16, 2024 21:46

engine-flutter-autoroll mentioned this pull request Apr 16, 2024

Roll Flutter Engine from 4d69c0c95483 to 599c3855c27a (8 revisions) flutter/flutter#146855

Closed

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

0e9ba7585 Migrate FlutterUIPressProxy, ios_context*, rendering_api_se…

788b898

…lection, and a few other files to ARC (flutter/engine#51633)

engine-flutter-autoroll mentioned this pull request Apr 17, 2024

Roll Flutter Engine from 4d69c0c95483 to e7d8c62d291a (11 revisions) flutter/flutter#146857

Merged

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

0e9ba7585 Migrate FlutterUIPressProxy, ios_context*, rendering_api_se…

8d9331c

…lection, and a few other files to ARC (flutter/engine#51633)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC #51633

Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC #51633

jmagman commented Mar 22, 2024 •

edited

Loading

jmagman Apr 4, 2024 •

edited

Loading

stuartmorgan Apr 9, 2024

jmagman Apr 11, 2024

stuartmorgan Apr 9, 2024

jmagman Apr 11, 2024

cbracken Apr 15, 2024

stuartmorgan Apr 9, 2024

cbracken Apr 15, 2024

jmagman Apr 15, 2024

stuartmorgan Apr 16, 2024

stuartmorgan Apr 9, 2024

stuartmorgan Apr 9, 2024

cbracken left a comment

cbracken Apr 15, 2024

cbracken Apr 15, 2024

jmagman Apr 15, 2024

flutter-dashboard bot commented Apr 16, 2024

stuartmorgan Apr 16, 2024

jmagman Apr 16, 2024

auto-submit bot commented Apr 16, 2024

	[self handlePressEvent:[[[FlutterUIPressProxy alloc] initWithPress:press
	withEvent:event] autorelease]
	nextAction:^() {
	[super pressesBegan:[NSSet setWithObject:press] withEvent:event];
	}];

	RegisterTexture(ios_context_->CreateExternalTexture(
	texture_id, fml::scoped_nsobject<NSObject<FlutterTexture>>{[texture retain]}));

		@@ -25,8 +25,6 @@ class IOSContextMetalImpeller final : public IOSContext {

		~IOSContextMetalImpeller();

		fml::scoped_nsobject<FlutterDarwinContextMetalSkia> GetDarwinContext() const;

Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC #51633

Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, and a few other files to ARC #51633

Conversation

jmagman commented Mar 22, 2024 • edited Loading

Pre-launch Checklist

jmagman Apr 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

cbracken 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

flutter-dashboard bot commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auto-submit bot commented Apr 16, 2024

jmagman commented Mar 22, 2024 •

edited

Loading

jmagman Apr 4, 2024 •

edited

Loading