[macOS] Consolidate view management (#52254)

This PR improves view management logic of the macOS `FlutterEngine`
class.
* View operation assertions are now centralized in
`registerViewController:` and `deregisterViewControllerForIdentifier:`.
* `addViewController` now directly calls `.viewController =` on implicit
views, so that it matches its verbatim description.
* The doc for `addViewController` correctly reflects the fact that it
doesn't support multiple views yet.

Additionally, a useless (for now) member variable is removed.

## 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].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

Author: dkwingsmt

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

@@ -456,9 +456,6 @@ @implementation FlutterEngine {
 
   FlutterThreadSynchronizer* _threadSynchronizer;
 
-  // The next available view ID.
-  int _nextviewIdentifier;
-
   // Whether the application is currently the active application.
   BOOL _active;
 
@@ -515,8 +512,6 @@ - (instancetype)initWithName:(NSString*)labelPrefix
   _binaryMessenger = [[FlutterBinaryMessengerRelay alloc] initWithParent:self];
   _isResponseValid = [[NSMutableArray alloc] initWithCapacity:1];
   [_isResponseValid addObject:@YES];
-  // kFlutterImplicitViewId is reserved for the implicit view.
-  _nextviewIdentifier = kFlutterImplicitViewId + 1;
 
   _embedderAPI.struct_size = sizeof(FlutterEngineProcTable);
   FlutterEngineGetProcAddresses(&_embedderAPI);
@@ -736,15 +731,25 @@ - (void)loadAOTData:(NSString*)assetsDir {
 - (void)registerViewController:(FlutterViewController*)controller
                  forIdentifier:(FlutterViewIdentifier)viewIdentifier {
   NSAssert(controller != nil, @"The controller must not be nil.");
-  NSAssert(![controller attached],
-           @"The incoming view controller is already attached to an engine.");
+  NSAssert(controller.engine == nil,
+           @"The FlutterViewController is unexpectedly attached to "
+           @"engine %@ before initialization.",
+           controller.engine);
   NSAssert([_viewControllers objectForKey:@(viewIdentifier)] == nil,
            @"The requested view ID is occupied.");
+  [_viewControllers setObject:controller forKey:@(viewIdentifier)];
   [controller setUpWithEngine:self
                viewIdentifier:viewIdentifier
            threadSynchronizer:_threadSynchronizer];
   NSAssert(controller.viewIdentifier == viewIdentifier, @"Failed to assign view ID.");
-  [_viewControllers setObject:controller forKey:@(viewIdentifier)];
+  // Verify that the controller's property are updated accordingly. Failing the
+  // assertions is likely because either the FlutterViewController or the
+  // FlutterEngine is mocked. Please subclass these classes instead.
+  NSAssert(controller.attached, @"The FlutterViewController should switch to the attached mode "
+                                @"after it is added to a FlutterEngine.");
+  NSAssert(controller.engine == self,
+           @"The FlutterViewController was added to %@, but its engine unexpectedly became %@.",
+           self, controller.engine);
 
   if (controller.viewLoaded) {
     [self viewControllerViewDidLoad:controller];
@@ -779,11 +784,17 @@ - (void)viewControllerViewDidLoad:(FlutterViewController*)viewController {
 }
 
 - (void)deregisterViewControllerForIdentifier:(FlutterViewIdentifier)viewIdentifier {
-  FlutterViewController* oldController = [self viewControllerForIdentifier:viewIdentifier];
-  if (oldController != nil) {
-    [oldController detachFromEngine];
-    [_viewControllers removeObjectForKey:@(viewIdentifier)];
+  FlutterViewController* controller = [self viewControllerForIdentifier:viewIdentifier];
+  // The controller can be nil. The engine stores only a weak ref, and this
+  // method could have been called from the controller's dealloc.
+  if (controller != nil) {
+    [controller detachFromEngine];
+    NSAssert(!controller.attached,
+             @"The FlutterViewController unexpectedly stays attached after being removed. "
+             @"In unit tests, this is likely because either the FlutterViewController or "
+             @"the FlutterEngine is mocked. Please subclass these classes instead.");
   }
+  [_viewControllers removeObjectForKey:@(viewIdentifier)];
   @synchronized(_vsyncWaiters) {
     [_vsyncWaiters removeObjectForKey:@(viewIdentifier)];
   }
@@ -877,26 +888,14 @@ - (FlutterCompositor*)createFlutterCompositor {
 #pragma mark - Framework-internal methods
 
 - (void)addViewController:(FlutterViewController*)controller {
-  NSAssert(controller.engine == nil,
-           @"The FlutterViewController is unexpectedly attached to "
-           @"engine %@ before initialization.",
-           controller.engine);
-  [self registerViewController:controller forIdentifier:kFlutterImplicitViewId];
-  NSAssert(controller.attached,
-           @"The FlutterViewController unexpectedly stays unattached after being added. "
-           @"In unit tests, this is likely because either the FlutterViewController or "
-           @"the FlutterEngine is mocked. Please subclass these classes instead.");
-  NSAssert(controller.engine == self,
-           @"The FlutterViewController #%lld has unexpected engine %@ after being added, "
-           @"instead of %@. "
-           @"In unit tests, this is likely because either the FlutterViewController or "
-           @"the FlutterEngine is mocked. Please subclass these classes instead.",
-           controller.viewIdentifier, controller.engine, self);
+  // FlutterEngine can only handle the implicit view for now. Adding more views
+  // throws an assertion.
+  NSAssert(self.viewController == nil,
+           @"The engine already has a view controller for the implicit view.");
+  self.viewController = controller;
 }
 
 - (void)removeViewController:(nonnull FlutterViewController*)viewController {
-  NSAssert([viewController attached] && viewController.engine == self,
-           @"The given view controller is not associated with this engine.");
   [self deregisterViewControllerForIdentifier:viewController.viewIdentifier];
   [self shutDownIfNeeded];
 }

shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h

@@ -125,9 +125,10 @@ typedef NS_ENUM(NSInteger, FlutterAppExitResponse) {
 /**
  * Attach a view controller to the engine as its default controller.
  *
- * Practically, since FlutterEngine can only be attached with one controller,
- * the given controller, if successfully attached, will always have the default
- * view ID kFlutterImplicitViewId.
+ * Since FlutterEngine can only handle the implicit view for now, the given
+ * controller will always be assigned to the implicit view, if there isn't an
+ * implicit view yet. If the engine already has an implicit view, this call
+ * throws an assertion.
  *
  * The engine holds a weak reference to the attached view controller.
  *
@@ -146,9 +147,6 @@ typedef NS_ENUM(NSInteger, FlutterAppExitResponse) {
  *
  * If the view controller is not associated with this engine, this call throws an
  * assertion.
- *
- * Practically, since FlutterEngine can only be attached with one controller for
- * now, the given controller must be the current view controller.
  */
 - (void)removeViewController:(FlutterViewController*)viewController;