[Ios] move flutter_test_ios_mrc unittests to arc #48162

cyanglaz · 2023-11-16T23:54:52Z

Move all tests in flutter_test_ios_mrc to arc

Part of flutter/flutter#137801

Changes mostly involves trivial mrc to arc changes. non-trivial changes are commented inline.

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.

cyanglaz · 2023-11-16T23:59:28Z

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

@@ -261,7 +261,7 @@ class FlutterPlatformViewsController {
                   const std::shared_ptr<IOSContext>& ios_context,
                   std::unique_ptr<SurfaceFrame> frame);

-  void OnMethodCall(FlutterMethodCall* call, FlutterResult& result);
+  void OnMethodCall(FlutterMethodCall* call, FlutterResult result);


FlutterResult& result in ARC doesn't work. And I don't think having a pointer to the objc block as the parameter made sense at first place? The objc block is already a pointer.
I might be wrong.

Agreed, this looks like it was a mistake. Notably, the PR that introduced this pattern started its description with "Note that this is my first time writing ObjC, don't assume I know what I'm doing", and there was no discussion of this in the review, which is very strong evidence that there wasn't actually some good, well-though-out reason for this very unusual pattern that neither of us is seeing :)

While you are here, do you want to fix all of the private methods below that do the same thing?

cyanglaz · 2023-11-17T00:00:32Z

If I'm not able to land this before end of tomorrow, please feel free to adopt it :)

cyanglaz · 2023-11-17T18:41:04Z

shell/platform/darwin/ios/BUILD.gn

@@ -74,6 +74,9 @@ source_set("flutter_framework_source") {
  deps = []

  sources = [
+    # iOS embedder is migrating to ARC.


I'm still a little nervous about sending the announcement since we haven't done any prod code migration to be sure we can truly start migrate. There could be a (small) chance that we'd have to fix some more stuff before the migration can really start.

I added this comment here so people would probably notice when they are trying to add new mrc files.

stuartmorgan

LGTM with nits

stuartmorgan · 2023-11-17T18:58:47Z

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

@@ -261,7 +261,7 @@ class FlutterPlatformViewsController {
                   const std::shared_ptr<IOSContext>& ios_context,
                   std::unique_ptr<SurfaceFrame> frame);

-  void OnMethodCall(FlutterMethodCall* call, FlutterResult& result);
+  void OnMethodCall(FlutterMethodCall* call, FlutterResult result);


Agreed, this looks like it was a mistake. Notably, the PR that introduced this pattern started its description with "Note that this is my first time writing ObjC, don't assume I know what I'm doing", and there was no discussion of this in the review, which is very strong evidence that there wasn't actually some good, well-though-out reason for this very unusual pattern that neither of us is seeing :)

stuartmorgan · 2023-11-17T18:59:10Z

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

@@ -261,7 +261,7 @@ class FlutterPlatformViewsController {
                   const std::shared_ptr<IOSContext>& ios_context,
                   std::unique_ptr<SurfaceFrame> frame);

-  void OnMethodCall(FlutterMethodCall* call, FlutterResult& result);
+  void OnMethodCall(FlutterMethodCall* call, FlutterResult result);


While you are here, do you want to fix all of the private methods below that do the same thing?

stuartmorgan · 2023-11-17T19:05:00Z

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

 @class FlutterPlatformViewsTestMockPlatformView;
-static FlutterPlatformViewsTestMockPlatformView* gMockPlatformView = nil;
+__weak static FlutterPlatformViewsTestMockPlatformView* gMockPlatformView = nil;


Ew :(

(Not the change, just the pattern itself of using a global static to track deallocation of an individual object; no changed need in this PR.)

stuartmorgan · 2023-11-17T19:09:00Z

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

@@ -15,9 +15,9 @@
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
 #import "flutter/shell/platform/darwin/ios/platform_view_ios.h"

-FLUTTER_ASSERT_NOT_ARC
+FLUTTER_ASSERT_ARC


Nit: Let's put a newline after this.

stuartmorgan · 2023-11-17T19:09:51Z

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

Should we FLUTTER_ASSERT_ARC this file? Seems like it's probably a good idea to do in all our ARC files during the transition.

stuartmorgan · 2023-11-17T19:10:05Z

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

@@ -14,9 +14,8 @@
 #import "flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h"
 #import "flutter/shell/platform/darwin/ios/platform_view_ios.h"



FLUTTER_ASSERT_ARC

stuartmorgan · 2023-11-17T19:11:06Z

shell/platform/darwin/ios/platform_message_handler_ios_test.mm

@@ -12,9 +12,6 @@
 #import "flutter/lib/ui/window/platform_message.h"
 #import "flutter/lib/ui/window/platform_message_response.h"
 #import "flutter/shell/common/thread_host.h"
-#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"
-
-FLUTTER_ASSERT_NOT_ARC



FLUTTER_ASSERT_ARC? Even though nothing had to change, it could get code added in the future that assumes ARC.

…ngine#48162)

…138643) flutter/engine@141a01c...e010f17 2023-11-17 skia-flutter-autoroll@skia.org Roll Skia from 8e9e168418a0 to dd7a26ead897 (1 revision) (flutter/engine#48178) 2023-11-17 ychris@google.com [Ios] move flutter_test_ios_mrc unittests to arc (flutter/engine#48162) 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 jonahwilliams@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

…rScreenAndSceneIfLoaded to ARC (#51984) Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `FlutterRestorationPlugin`, `FlutterTextureRegistryRelay`, and `UIViewController+FlutterScreenAndSceneIfLoaded` from MRC to ARC. These files do not themselves import any MRC files, making them leaf nodes in the dependency graph of MRC files. 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.

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `FlutterEmbedderKeyResponder` from MRC to ARC. Clean up some header imports which made this seem like it depended on not-yet-migrated MRC files. Part of flutter/flutter#137801.

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate`FlutterCallbackCache` and `FlutterKeyboardManager` from MRC to ARC. These files do not themselves import any MRC files, making them leaf nodes in the dependency graph of MRC files. 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.

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `FlutterDartVMServicePublisher` from MRC to ARC. I validated `flutter attach` works on this engine PR, so the VM service URL is being advertised and the tool is discovering it. Part of flutter/flutter#137801.

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `vsync_waiter_ios` from MRC to ARC. Part of flutter/flutter#137801. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…nd a few other files to ARC (#51633) 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. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `FlutterUndoManagerPlugin` from MRC to ARC. 1. Refactor so the plugin and its tests don't need to understand the details of `FlutterViewController` or `FlutterEngine` (its delegate). This decouples the plugin, and means it doesn't depend on any MRC classes. 2. Change the delegate so conforming only requires the objects the undo plugin actually needs. Part of flutter/flutter#137801.

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `ios_surface` classes from MRC to ARC. Decorate C functions that take or return Objective-C objects or structs containing Objective-C objects with [`cf_audited_transfer`](https://clang.llvm.org/docs/AutomaticReferenceCounting.html#auditing-of-c-retainable-pointer-interfaces). Part of flutter/flutter#137801.

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `PlatformMessageHandlerIos` from MRC to ARC. Clean up the `#include`s. Part of flutter/flutter#137801.

…52535) Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162. Migrate `FlutterView`, `FlutterPlatformViews`, and `FlutterOverlayView` from MRC to ARC. Part of flutter/flutter#137801.

Chris Yang added 3 commits November 16, 2023 10:09

draft

383a993

move mrc ios tests to arc

8a6dc12

update license

4a831e3

cyanglaz requested a review from stuartmorgan November 16, 2023 23:54

cyanglaz marked this pull request as ready for review November 16, 2023 23:55

github-actions bot added the platform-ios label Nov 16, 2023

cyanglaz commented Nov 16, 2023

View reviewed changes

cyanglaz commented Nov 17, 2023

View reviewed changes

stuartmorgan approved these changes Nov 17, 2023

View reviewed changes

reivew

32284e2

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 17, 2023

auto-submit bot merged commit 1427b9c into flutter:main Nov 17, 2023
26 checks passed

cyanglaz deleted the ios_unittest_arc branch November 17, 2023 20:39

engine-flutter-autoroll mentioned this pull request Nov 17, 2023

Roll Flutter Engine from 141a01c5c70b to e010f17eeb10 (2 revisions) flutter/flutter#138643

Merged

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

1427b9c09 [Ios] move flutter_test_ios_mrc unittests to arc (flutter/e…

2be4481

…ngine#48162)

jmagman mentioned this pull request Feb 16, 2024

Round SkRects in ClipRectContainsPlatformViewBoundingRect #50387

Closed

8 tasks

jmagman mentioned this pull request Mar 13, 2024

Support accessibility back gesture to pop route from view controller #51241

Merged

8 tasks

jmagman mentioned this pull request Mar 22, 2024

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

Merged

8 tasks

This was referenced Apr 9, 2024

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

Merged

Migrate FlutterRestorationPlugin, FlutterTextureRegistryRelay, FlutterScreenAndSceneIfLoaded to ARC #51984

Merged

This was referenced Apr 11, 2024

Migrate FlutterEmbedderKeyResponder to ARC #52048

Merged

Migrate FlutterDartVMServicePublisher to ARC #52081

Merged

jmagman mentioned this pull request Apr 13, 2024

Migrate vsync_waiter_ios to ARC #52104

Merged

8 tasks

jmagman mentioned this pull request Apr 15, 2024

Migrate ios_surface files to ARC #52139

Merged

This was referenced Apr 18, 2024

Migrate PlatformMessageHandlerIos to ARC #52226

Merged

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

Merged

jmagman mentioned this pull request May 2, 2024

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ios] move flutter_test_ios_mrc unittests to arc #48162

[Ios] move flutter_test_ios_mrc unittests to arc #48162

cyanglaz commented Nov 16, 2023

cyanglaz Nov 16, 2023

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

cyanglaz commented Nov 17, 2023

cyanglaz Nov 17, 2023

stuartmorgan left a comment

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

stuartmorgan Nov 17, 2023

		@@ -14,9 +14,8 @@
		#import "flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h"
		#import "flutter/shell/platform/darwin/ios/platform_view_ios.h"

[Ios] move flutter_test_ios_mrc unittests to arc #48162

[Ios] move flutter_test_ios_mrc unittests to arc #48162

Conversation

cyanglaz commented Nov 16, 2023

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Nov 17, 2023

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