[ios] making objective-C smart pointers support ARC #47612

cyanglaz · 2023-11-02T18:33:38Z

Moving the implementation from https://codereview.chromium.org/1855483004 into the code base, including:

scoped_nsobject, scoped_nsprotocol, scoped_block will support both mrc and arc
Added parent class scoped_typeref for shared code between scoped_block and scoped_nsobject
moving OwnershipPolicy to its own file

The implementation of the smart pointers are almost identical to https://codereview.chromium.org/1855483004 besides some syntax preference differences between chromium and flutter.

This PR also migrated VsyncWaiterIosTest.mm to ARC with scoped_nsobject

fixes flutter/flutter#137802

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-03T18:28:32Z

Hmm I'm not sure why ci is not happy with excluding ../../../flutter/fml/platform/darwin/scoped_nsobject_unittests_arc.mm from license

stuartmorgan-g

LGTM!

stuartmorgan-g · 2023-11-06T11:50:25Z

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

-    fml::scoped_nsobject<FlutterOverlayView> overlay_view;
-    fml::scoped_nsobject<FlutterOverlayView> overlay_view_wrapper;
+    fml::scoped_nsobject<UIView> overlay_view;
+    fml::scoped_nsobject<UIView> overlay_view_wrapper;


Why did these change?

The newer version added explicit to the move constructors. Since the impact is low (only PlatformViews.mm need to be updated). I think we should adopt the explicit from chromium

objc smart pointers support arc fix license add unittests gn format format migrate VsyncWaiterIosTest.mm to arc try build.gn fix try build.gn fix exclude tests from license file fix license

fake init to supress warning

cyanglaz · 2023-11-06T20:36:17Z

I went through the history of scoped_nsobject and scoped_block and found that there were some custom updates we made to our own version that we probably want to keep. I will update the PR with those changes and request another review

cyanglaz · 2023-11-06T22:33:51Z

The last commit contains changes in:

be90369
d61bf8e
a37560a
f3d7a76

The change in f3d7a76 is modified to make the method protected instead, because it is used in subclass in the new version.

@stuartmorgan Please take a look the last commit, want to make sure im not missing something.

cyanglaz · 2023-11-06T22:40:01Z

Hmm I'm not sure why ci is not happy with excluding ../../../flutter/fml/platform/darwin/scoped_nsobject_unittests_arc.mm from license

@Hixie Do you know what I should do to fix this?

stuartmorgan-g · 2023-11-07T15:41:46Z

fml/platform/darwin/scoped_typeref.h

  // ScopedTypeRef<>::release() is like std::unique_ptr<>::release.  It is NOT
  // a wrapper for Release().  To force a ScopedTypeRef<> object to call
  // Release(), use ScopedTypeRef<>::reset().
-  T release() __attribute((ns_returns_not_retained)) WARN_UNUSED_RESULT {


If want to use [[nodiscard]] instead of the macro, presumably we should remove the part of this PR that adds the macro to our codebase and replace any other usage as well?

stuartmorgan-g · 2023-11-07T15:46:10Z

Hmm I'm not sure why ci is not happy with excluding ../../../flutter/fml/platform/darwin/scoped_nsobject_unittests_arc.mm from license

@Hixie Do you know what I should do to fix this?

I believe the problem here is that the script doesn't recognize this as a file that's not part of the build; see https://github.com/flutter/engine/blob/main/tools/licenses/lib/paths.dart (and in particular the regexes at the end of that file). Can you rename this scoped_nsobject_arc_unittests.mm?

cyanglaz · 2023-11-07T17:37:31Z

@stuartmorgan Fixed per your last reviews

stuartmorgan-g

LGTM

…r/engine#47612)

…138034) flutter/engine@1b20752...f8961d2 2023-11-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Add support for specialization constants redux." (flutter/engine#47762) 2023-11-07 [email protected] [Impeller] added tests for matrices (flutter/engine#47754) 2023-11-07 [email protected] Roll Skia from 62fc1374cc5d to 030e21befbc9 (2 revisions) (flutter/engine#47756) 2023-11-07 [email protected] [ios] making objective-C smart pointers support ARC (flutter/engine#47612) 2023-11-07 [email protected] Don't use Skia BUILD.gn files (flutter/engine#47677) 2023-11-07 [email protected] [Impeller] Add support for specialization constants redux. (flutter/engine#47678) 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

…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.

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

cyanglaz requested a review from stuartmorgan-g November 2, 2023 19:08

cyanglaz force-pushed the arc branch from 678e04d to 51f8706 Compare November 2, 2023 22:02

cyanglaz marked this pull request as ready for review November 3, 2023 01:23

cyanglaz force-pushed the arc branch from b0299dd to b98b8f5 Compare November 3, 2023 01:23

stuartmorgan-g approved these changes Nov 6, 2023

View reviewed changes

Chris Yang added 3 commits November 6, 2023 11:46

draft

e22606a

objc smart pointers support arc fix license add unittests gn format format migrate VsyncWaiterIosTest.mm to arc try build.gn fix try build.gn fix exclude tests from license file fix license

pick up change in https://codereview.chromium.org/2485463002/

68da898

include utility

cfccb75

fake init to supress warning

cyanglaz force-pushed the arc branch from 3fa9f52 to cfccb75 Compare November 6, 2023 19:46

cyanglaz changed the title ~~[darwin] making objective-C smart pointers support ARC~~ [ios] making objective-C smart pointers support ARC Nov 6, 2023

adopt custom changes in flutter

05a3a97

stuartmorgan-g reviewed Nov 7, 2023

View reviewed changes

review

d76831c

stuartmorgan-g suggested changes Nov 7, 2023

View reviewed changes

stuartmorgan-g approved these changes Nov 7, 2023

View reviewed changes

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

auto-submit bot merged commit cf3473b into flutter:main Nov 7, 2023

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

Roll Flutter Engine from 1b20752e2a63 to f8961d203039 (6 revisions) flutter/flutter#138034

Merged

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

cf3473b59 [ios] making objective-C smart pointers support ARC (flutte…

7510551

…r/engine#47612)

cyanglaz deleted the arc branch November 8, 2023 15:25

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

jmagman mentioned this pull request Apr 9, 2024

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

Merged

jmagman mentioned this pull request Apr 9, 2024

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] making objective-C smart pointers support ARC #47612

[ios] making objective-C smart pointers support ARC #47612

cyanglaz commented Nov 2, 2023 •

edited

Loading

cyanglaz commented Nov 3, 2023 •

edited

Loading

stuartmorgan-g left a comment

stuartmorgan-g Nov 6, 2023

cyanglaz Nov 6, 2023

stuartmorgan-g Nov 6, 2023

cyanglaz commented Nov 6, 2023

cyanglaz commented Nov 6, 2023 •

edited

Loading

cyanglaz commented Nov 6, 2023

stuartmorgan-g Nov 7, 2023

stuartmorgan-g commented Nov 7, 2023

cyanglaz commented Nov 7, 2023

stuartmorgan-g left a comment

[ios] making objective-C smart pointers support ARC #47612

[ios] making objective-C smart pointers support ARC #47612

Conversation

cyanglaz commented Nov 2, 2023 • edited Loading

Pre-launch Checklist

cyanglaz commented Nov 3, 2023 • edited Loading

stuartmorgan-g left a comment

Choose a reason for hiding this comment

stuartmorgan-g Nov 6, 2023

Choose a reason for hiding this comment

cyanglaz Nov 6, 2023

Choose a reason for hiding this comment

stuartmorgan-g Nov 6, 2023

Choose a reason for hiding this comment

cyanglaz commented Nov 6, 2023

cyanglaz commented Nov 6, 2023 • edited Loading

cyanglaz commented Nov 6, 2023

stuartmorgan-g Nov 7, 2023

Choose a reason for hiding this comment

stuartmorgan-g commented Nov 7, 2023

cyanglaz commented Nov 7, 2023

stuartmorgan-g left a comment

Choose a reason for hiding this comment

cyanglaz commented Nov 2, 2023 •

edited

Loading

cyanglaz commented Nov 3, 2023 •

edited

Loading

cyanglaz commented Nov 6, 2023 •

edited

Loading