[iOS] Add darwin_extension_safe flag and use UIScene api when building for extensions #43449

cyanglaz · 2023-07-06T21:25:06Z

iOS extensions forbids the usage of UIApplication.sharedApplication. This PR refactors the engine to use UISceneAPI when darwin_extension_safe flag is on. Using UISceneAPI can help avoid the usage of UIApplication.sharedApplication as much as possible.
This PR also added a new _extension_safe variant for the engine build so all the logic with the darwin_extension_safe flag is on can be tested separately.

This PR doesn't enable the engine to build for the extension even when darwin_extension_safe is true.

part of flutter/flutter#124289

There are several issues related to UIApplication life cycle and I manually tested they still work with the scene API:

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 Hixie said 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-07-12T21:38:45Z

ci/builders/mac_unopt.json

+            ]
+        },
+        {
+            "archives": [


Since intel machines are limited and the test suite are the same, only add Apple silicon machine for the ci tests.

flutter-dashboard · 2023-07-12T22:00:49Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

cyanglaz · 2023-07-17T19:39:53Z

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

-                                                                                      nibName:nil
-                                                                                       bundle:nil];
+  FlutterViewController* engineViewController =
+      [[[FlutterViewController alloc] initWithEngine:engine nibName:nil bundle:nil] autorelease];


The view controller is leaked, causing the newly added notification tests in other testing files to fail.

stuartmorgan-g · 2023-07-18T15:46:52Z

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

+  return self;
+}
+
+- (void)setupSceneLifecycleNotifications:(NSNotificationCenter*)center API_AVAILABLE(ios(13.0)) {


Nit: setUp... (since "set up" is the verb, while "setup" is the noun).

stuartmorgan-g · 2023-07-18T15:46:57Z

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

+}

-  return self;
+- (void)setupApplicationLifecycleNotifications:(NSNotificationCenter*)center {


stuartmorgan-g · 2023-07-18T15:52:24Z

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


+#if APPLICATION_EXTENSION_API_ONLY
+  if (@available(iOS 13.0, *)) {
+    [self setupSceneLifecycleNotifications:center];


Longer term can we unify these code paths (i.e., remove this ifdef), or are there important differences in the notifications?

If the former, a TODO explaining that might be useful.

There aren't any difference for single scene applications. When/if Flutter supports multi-scene, we will need both of them.

stuartmorgan-g · 2023-07-18T15:54:17Z

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


 @end

+@interface FlutterEngine (Tests)


Is redeclaring private methods in test files, rather than using test-specific headers, the standard practice for the iOS engine code?

I don't see a standard. Both methods are used in iOS engine code. However, I did see that FlutterEngineTest uses a test header, so I will move these methods to the test header.

stuartmorgan-g · 2023-07-18T15:56:06Z

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

+#if APPLICATION_EXTENSION_API_ONLY
+  OCMVerify([mockEngine sceneDidEnterBackground:[OCMArg any]]);
+#else
+  OCMVerify([mockEngine applicationDidEnterBackground:[OCMArg any]]);


Isn't the important thing to verify that the correct state (like GPU status) is updated, not the specific private method that is called? This test would both break for minor internal refactoring, and not break if someone removed critical code from these methods.

stuartmorgan-g · 2023-07-18T15:56:41Z

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

+          [engineViewController windowSceneIfViewLoaded].keyWindow.rootViewController;
+    } else {
+      FML_LOG(WARNING)
+          << "rootViewController is not available in application extension under iOS 15.0.";


s/under/prior to/

Although... is this actionable (per https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#only-log-actionable-messages-to-the-console)? It's not clear to me what I would do with this message as a developer if I got it.

Since it is breaking the pop flow, I imagined that the developer would either stop supporting iOS 15. Or redesign the UI for iOS 15 or lower.

I think this message is necessary to let the developer know why the pop button doesn't work. It would be hard to figure out if we only introduced a silence no-op.

We would probably need to update the documentation for this too. However, I think the warning message is more visible and this one won't be too spammy.

stuartmorgan-g · 2023-07-18T15:59:17Z

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

               object:nil];
 }

+- (void)setupSceneLifecycleNotifications:(NSNotificationCenter*)center API_AVAILABLE(ios(13.0)) {


stuartmorgan-g · 2023-07-18T15:59:23Z

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

+               object:nil];
+}
+
+- (void)setupApplicationLifecycleNotifications:(NSNotificationCenter*)center {


stuartmorgan-g · 2023-07-18T16:09:15Z

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

-        [self setNeedsUpdateOfSupportedInterfaceOrientations];
+        [self requestGeometryUpdateForWindowScene:windowScene];
      }
+#endif


Could we unify this code to have just populating a list of scenes into an ifdef, and then have the actual call to update the geometry be outside the ifdef, so we minimize code path branching.

chinmaygarde · 2023-07-20T20:13:22Z

The test failures are unrelated and should be fixed by a rebase.

stuartmorgan-g

Sorry for the delay getting back to this; LGTM with one bug fix.

stuartmorgan-g · 2023-07-31T14:45:10Z

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

-      }
+      NSSet<UIScene*>* scenes =
+#if APPLICATION_EXTENSION_API_ONLY
+          [NSSet setWithObject:self.windowSceneIfViewLoaded ? self.windowSceneIfViewLoaded : nil];


It's not valid to insert nil into a collection; if nothing is available the set should be empty.

Fixed to return empty NSSet

draft format draf format draf fix non extension build add extension safe builders generate extension safe output fix build config fix clang tidy complains add to tests fix tests remove test logs fox tests fix test fix memory leak in platform plugin test format review review bug fix format

…en building for extensions (flutter/engine#43449)

…131706) flutter/engine@10a1f9c...9dae7b7 2023-08-01 [email protected] Made the licenses script output all problems (flutter/engine#44223) 2023-08-01 [email protected] Roll Skia from a7a3646c2c8a to d53f7b880651 (6 revisions) (flutter/engine#44226) 2023-08-01 [email protected] Roll Clang from ebd0b8a0472b to 07c592048780 (flutter/engine#44224) 2023-08-01 [email protected] [iOS] Add darwin_extension_safe flag and use UIScene api when building for extensions (flutter/engine#43449) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#131706) flutter/engine@10a1f9c...9dae7b7 2023-08-01 [email protected] Made the licenses script output all problems (flutter/engine#44223) 2023-08-01 [email protected] Roll Skia from a7a3646c2c8a to d53f7b880651 (6 revisions) (flutter/engine#44226) 2023-08-01 [email protected] Roll Clang from ebd0b8a0472b to 07c592048780 (flutter/engine#44224) 2023-08-01 [email protected] [iOS] Add darwin_extension_safe flag and use UIScene api when building for extensions (flutter/engine#43449) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…g for extensions (flutter#43449) iOS extensions forbids the usage of UIApplication.sharedApplication. This PR refactors the engine to use UISceneAPI when darwin_extension_safe flag is on. Using UISceneAPI can help avoid the usage of `UIApplication.sharedApplication` as much as possible. This PR also added a new `_extension_safe` variant for the engine build so all the logic with the `darwin_extension_safe` flag is on can be tested separately. This PR doesn't enable the engine to build for the extension even when darwin_extension_safe is true. part of flutter/flutter#124289 There are several issues related to UIApplication life cycle and I manually tested they still work with the scene API: [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-ios label Jul 6, 2023

cyanglaz force-pushed the use_scene_api branch 3 times, most recently from 4b53097 to 8e0369f Compare July 12, 2023 18:20

cyanglaz requested a review from stuartmorgan-g July 12, 2023 21:37

cyanglaz marked this pull request as ready for review July 12, 2023 21:37

cyanglaz commented Jul 12, 2023

View reviewed changes

cyanglaz marked this pull request as draft July 12, 2023 22:00

cyanglaz marked this pull request as ready for review July 12, 2023 22:23

cyanglaz commented Jul 17, 2023

View reviewed changes

stuartmorgan-g suggested changes Jul 18, 2023

View reviewed changes

cyanglaz requested a review from stuartmorgan-g July 18, 2023 21:32

cyanglaz force-pushed the use_scene_api branch 2 times, most recently from 8d697bc to 2c7c8f4 Compare July 27, 2023 16:39

stuartmorgan-g approved these changes Jul 31, 2023

View reviewed changes

Chris Yang added 2 commits August 1, 2023 10:09

fix test

03d6b06

cyanglaz force-pushed the use_scene_api branch from 2c7c8f4 to 03d6b06 Compare August 1, 2023 17:30

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

auto-submit bot merged commit d5f6b00 into flutter:main Aug 1, 2023

engine-flutter-autoroll mentioned this pull request Aug 1, 2023

Roll Flutter Engine from 10a1f9cb74c9 to d5f6b007c79b (1 revision) flutter/flutter#131704

Closed

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

d5f6b007c [iOS] Add darwin_extension_safe flag and use UIScene api wh…

9a07ffc

…en building for extensions (flutter/engine#43449)

engine-flutter-autoroll mentioned this pull request Aug 1, 2023

Roll Flutter Engine from 10a1f9cb74c9 to 9dae7b708bda (4 revisions) flutter/flutter#131706

Merged

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

d5f6b007c [iOS] Add darwin_extension_safe flag and use UIScene api wh…

4464d3b

…en building for extensions (flutter/engine#43449)

cyanglaz deleted the use_scene_api branch August 3, 2023 22:42

[iOS] Add darwin_extension_safe flag and use UIScene api when building for extensions #43449

[iOS] Add darwin_extension_safe flag and use UIScene api when building for extensions #43449

Uh oh!

Conversation

cyanglaz commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

cyanglaz Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jul 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Jul 20, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyanglaz commented Jul 6, 2023 •

edited

Loading

cyanglaz Jul 12, 2023 •

edited

Loading