Consolidate functionality, fix test

Author: gspencergoog

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

@@ -7,7 +7,7 @@
 #import <AppKit/AppKit.h>
 
 #include "flutter/fml/logging.h"
-#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_internal.h"
+#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_Internal.h"
 #include "flutter/shell/platform/embedder/embedder.h"
 
 @interface FlutterAppDelegate ()
@@ -19,9 +19,7 @@ - (NSString*)applicationName;
 
 @end
 
-@implementation FlutterAppDelegate {
-  FlutterEngineTerminationHandler* _terminationHandler;
-}
+@implementation FlutterAppDelegate
 
 - (instancetype)init {
   if (self = [super init]) {
@@ -58,22 +56,4 @@ - (NSString*)applicationName {
   return applicationName;
 }
 
-- (void)setTerminationRequestHandler:(FlutterEngineTerminationHandler*)handler {
-  _terminationHandler = handler;
-}
-
-- (void)requestApplicationTermination:(NSApplication*)application
-                             exitType:(FlutterAppExitType)type {
-  if (![application isKindOfClass:[FlutterApplication class]]) {
-    [application terminate:application];
-    return;
-  };
-  FlutterApplication* flutterApp = static_cast<FlutterApplication*>(application);
-  if (_terminationHandler) {
-    [_terminationHandler requestApplicationTermination:flutterApp exitType:type result:nil];
-  } else {
-    [flutterApp terminateApplication:application];
-  }
-}
-
 @end

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

@@ -10,15 +10,13 @@
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
 
 @interface FlutterAppDelegate ()
-/**
- * Allows the engine to add a handler for termination requests
- */
-- (void)setTerminationRequestHandler:(FlutterEngineTerminationHandler*)handler;
 
 /**
- * Allows the delegate to respond to an attempt to terminate the application.
+ * Holds a weak reference to the termination handler owned by the engine.
+ * Called by the |FlutterApplication| when termination is requested by the OS.
  */
-- (void)requestApplicationTermination:(NSApplication*)application exitType:(FlutterAppExitType)type;
+@property(readwrite, weak) FlutterEngineTerminationHandler* terminationHandler;
+
 @end
 
 #endif  // FLUTTER_FLUTTERAPPDELEGATE_INTERNAL_H_

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

@@ -6,7 +6,8 @@
 
 #include "flutter/shell/platform/embedder/embedder.h"
 #import "shell/platform/darwin/macos/framework/Headers/FlutterAppDelegate.h"
-#import "shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_internal.h"
+#import "shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_Internal.h"
+#import "shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
 
 // An NSApplication subclass that implements overrides necessary for some
 // Flutter features, like application lifecycle handling.
@@ -21,18 +22,17 @@ + (NSApplication*)sharedApplication {
   // +sharedApplication initializes the global NSApp, so if we're delivering
   // something other than a FlutterApplication, warn the developer once.
 #ifndef FLUTTER_RELEASE
-  static dispatch_once_t onceToken = 0;
-  dispatch_once(&onceToken, ^{
-    if (![NSApp isKindOfClass:[FlutterApplication class]]) {
-      NSLog(@"NSApp should be of type %s, not %s. "
-             "Some application lifecycle requests (e.g. ServicesBinding.exitApplication) "
-             "and notifications will be unavailable.\n"
-             "Modify the application's NSPrincipleClass to be %s"
-             "in the Info.plist to fix this.",
-            [[self className] UTF8String], [[NSApp className] UTF8String],
-            [[self className] UTF8String]);
-    }
-  });
+  static bool notified = false;
+  if (!notified && ![NSApp isKindOfClass:[FlutterApplication class]]) {
+    NSLog(@"NSApp should be of type %s, not %s. "
+           "Some application lifecycle requests (e.g. ServicesBinding.exitApplication) "
+           "and notifications will be unavailable.\n"
+           "Modify the application's NSPrincipleClass to be %s"
+           "in the Info.plist to fix this.",
+          [[self className] UTF8String], [[NSApp className] UTF8String],
+          [[self className] UTF8String]);
+    notified = true;
+  }
 #endif  // !FLUTTER_RELEASE
   return app;
 }
@@ -62,9 +62,13 @@ + (NSApplication*)sharedApplication {
 // it if it is OK to terminate. When that method channel call returns with a
 // result, the application either terminates or continues running.
 - (void)terminate:(id)sender {
-  FlutterAppDelegate* appDelegate = static_cast<FlutterAppDelegate*>([NSApp delegate]);
-
-  [appDelegate requestApplicationTermination:self exitType:kFlutterAppExitTypeCancelable];
+  FlutterEngineTerminationHandler* terminationHandler = [static_cast<FlutterAppDelegate*>([NSApp delegate]) terminationHandler];
+  if (terminationHandler) {
+    [terminationHandler requestApplicationTermination:self exitType:kFlutterAppExitTypeCancelable result:nil];
+  } else {
+    // If there's no termination handler, then just terminate.
+    [super terminate:sender];
+  }
   // Return, don't exit. The application delegate is responsible for exiting on
   // its own by calling |-terminateApplication|.
 }

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

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"
+#include "FlutterCodecs.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
 
 #include <algorithm>
@@ -14,7 +15,7 @@
 
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterAppDelegate.h"
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterApplication.h"
-#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_internal.h"
+#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_Internal.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject_Internal.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterMenuPlugin.h"
@@ -160,19 +161,28 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result;
 
 @implementation FlutterEngineTerminationHandler {
   FlutterEngine* _engine;
+  FlutterTerminationCallback _terminator;
 }
 
-- (instancetype)initWithEngine:(FlutterEngine*)engine {
+- (instancetype)initWithEngine:(FlutterEngine*)engine
+                    terminator:(FlutterTerminationCallback)terminator {
   self = [super init];
   _engine = engine;
+  _terminator = terminator ? terminator : ^(id sender) {
+    // Default to actually terminating the application. The terminator exists to
+    // allow tests to override it so that an actual exit doesn't occur.
+    [[FlutterApplication sharedApplication] terminateApplication:sender];
+  };
+  FlutterAppDelegate* appDelegate =
+      (FlutterAppDelegate*)[[FlutterApplication sharedApplication] delegate];
+  appDelegate.terminationHandler = self;
   return self;
 }
 
 // This is called by the method call handler in the engine when the application
 // requests termination itself.
 - (void)requestAppExit:(NSDictionary<NSString*, id>*)arguments result:(FlutterResult)result {
   NSString* type = arguments[@"type"];
-
   // Ignore the "exitCode" value in the arguments because AppKit doesn't have
   // any good way to set the process exit code other than calling exit(), and
   // that bypasses all of the native applicationShouldExit shutdown events,
@@ -193,26 +203,26 @@ - (void)requestApplicationTermination:(id)sender
                                result:(nullable FlutterResult)result {
   switch (type) {
     case kFlutterAppExitTypeCancelable: {
-      [_engine
-          sendOnChannel:kFlutterPlatformChannel
-                message:[[FlutterJSONMethodCodec sharedInstance]
-                            encodeMethodCall:[FlutterMethodCall
-                                                 methodCallWithMethodName:@"System.requestAppExit"
-                                                                arguments:@{}]]
-            binaryReply:^(NSData* _Nullable reply) {
-              NSDictionary* replyArgs =
-                  [[FlutterJSONMethodCodec sharedInstance] decodeEnvelope:reply];
-              if ([replyArgs[@"response"] isEqual:@"exit"]) {
-                [[FlutterApplication sharedApplication] terminateApplication:sender];
-              }
-              if (result != nil) {
-                result(replyArgs);
-              }
-            }];
+      FlutterJSONMethodCodec* codec = [FlutterJSONMethodCodec sharedInstance];
+      FlutterMethodCall* methodCall =
+          [FlutterMethodCall methodCallWithMethodName:@"System.requestAppExit" arguments:nil];
+      [_engine sendOnChannel:kFlutterPlatformChannel
+                     message:[codec encodeMethodCall:methodCall]
+                 binaryReply:^(NSData* _Nullable reply) {
+                   NSDictionary* replyArgs = [codec decodeEnvelope:reply];
+                   if ([replyArgs[@"response"] isEqual:@"exit"]) {
+                     NSAssert(_terminator, @"terminator shouldn't be nil");
+                     _terminator(sender);
+                   }
+                   if (result != nil) {
+                     result(replyArgs);
+                   }
+                 }];
       break;
     }
     case kFlutterAppExitTypeRequired:
-      [[FlutterApplication sharedApplication] terminateApplication:sender];
+      NSAssert(_terminator, @"terminator shouldn't be nil");
+      _terminator(sender);
       break;
   }
 }
@@ -339,9 +349,6 @@ @implementation FlutterEngine {
   // A method channel for miscellaneous platform functionality.
   FlutterMethodChannel* _platformChannel;
 
-  // A delegate used to receive application termination events.
-  FlutterEngineTerminationHandler* _terminateHelper;
-
   int _nextViewId;
 }
 
@@ -362,10 +369,8 @@ - (instancetype)initWithName:(NSString*)labelPrefix
   _semanticsEnabled = NO;
   _isResponseValid = [[NSMutableArray alloc] initWithCapacity:1];
   [_isResponseValid addObject:@YES];
-  _terminateHelper = [[FlutterEngineTerminationHandler alloc] initWithEngine:self];
-  FlutterAppDelegate* appDelegate =
-      (FlutterAppDelegate*)[[FlutterApplication sharedApplication] delegate];
-  [appDelegate setTerminationRequestHandler:_terminateHelper];
+  _terminationHandler = [[FlutterEngineTerminationHandler alloc] initWithEngine:self
+                                                                     terminator:nil];
   // kFlutterDefaultViewId is reserved for the default view.
   // All IDs above it are for regular views.
   _nextViewId = kFlutterDefaultViewId + 1;
@@ -958,7 +963,7 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
   } else if ([call.method isEqualToString:@"Clipboard.hasStrings"]) {
     result(@{@"value" : @([self clipboardHasStrings])});
   } else if ([call.method isEqualToString:@"System.exitApplication"]) {
-    [_terminateHelper requestAppExit:call.arguments result:result];
+    [[self terminationHandler] requestAppExit:call.arguments result:result];
   } else {
     result(FlutterMethodNotImplemented);
   }

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

@@ -2,8 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "OCMock/OCMArg.h"
+#include "OCMock/OCMock.h"
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
+#include "gtest/gtest.h"
 
 #include <functional>
 #include <thread>
@@ -407,7 +410,7 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
   ASSERT_TRUE(latch_called);
 }
 
-TEST(FlutterEngineTest, Compositor) {
+TEST_F(FlutterEngineTest, Compositor) {
   NSString* fixtures = @(flutter::testing::GetFixturesPath());
   FlutterDartProject* project = [[FlutterDartProject alloc]
       initWithAssetsPath:fixtures
@@ -446,7 +449,7 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
   [engine shutDownEngine];
 }  // namespace flutter::testing
 
-TEST(FlutterEngineTest, DartEntrypointArguments) {
+TEST_F(FlutterEngineTest, DartEntrypointArguments) {
   NSString* fixtures = @(flutter::testing::GetFixturesPath());
   FlutterDartProject* project = [[FlutterDartProject alloc]
       initWithAssetsPath:fixtures
@@ -541,7 +544,7 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
   EXPECT_EQ(record, 21);
 }
 
-TEST(FlutterEngineTest, HasStringsWhenPasteboardEmpty) {
+TEST_F(FlutterEngineTest, HasStringsWhenPasteboardEmpty) {
   id engineMock = CreateMockFlutterEngine(nil);
 
   // Call hasStrings and expect it to be false.
@@ -559,7 +562,7 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
   EXPECT_FALSE(valueAfterClear);
 }
 
-TEST(FlutterEngineTest, HasStringsWhenPasteboardFull) {
+TEST_F(FlutterEngineTest, HasStringsWhenPasteboardFull) {
   id engineMock = CreateMockFlutterEngine(@"some string");
 
   // Call hasStrings and expect it to be true.
@@ -619,7 +622,7 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
   }
 }
 
-TEST(EngineTest, ThreadSynchronizerNotBlockingRasterThreadAfterShutdown) {
+TEST_F(FlutterEngineTest, ThreadSynchronizerNotBlockingRasterThreadAfterShutdown) {
   FlutterThreadSynchronizer* threadSynchronizer = [[FlutterThreadSynchronizer alloc] init];
   [threadSynchronizer shutdown];
 
@@ -695,17 +698,54 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
   EXPECT_EQ(viewController1.id, 0ull);
 }
 
-TEST(FlutterEngineTerminationHandlerTest, HandlesTerminationRequest) {
+TEST_F(FlutterEngineTest, HandlesTerminationRequest) {
   id engineMock = CreateMockFlutterEngine(nil);
-
-  __block FlutterAppExitResponse calledAfterTerminate = kFlutterAppExitResponseCancel;
+  __block NSString* nextResponse = @"exit";
+  __block BOOL triedToTerminate = FALSE;
+  FlutterEngineTerminationHandler* terminationHandler =
+      [[FlutterEngineTerminationHandler alloc] initWithEngine:engineMock
+                                                   terminator:^(id sender) {
+                                                     triedToTerminate = TRUE;
+                                                     // Don't actually terminate, of course.
+                                                   }];
+  OCMStub([engineMock terminationHandler]).andReturn(terminationHandler);
+  id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
+  OCMStub(  // NOLINT(google-objc-avoid-throwing-exception)
+      [engineMock binaryMessenger])
+      .andReturn(binaryMessengerMock);
+  OCMStub([engineMock sendOnChannel:@"flutter/platform"
+                            message:[OCMArg any]
+                        binaryReply:[OCMArg any]])
+      .andDo((^(NSInvocation* invocation) {
+        [invocation retainArguments];
+        FlutterBinaryReply callback;
+        [invocation getArgument:&callback atIndex:4];
+        NSDictionary* responseDict = @{@"response" : nextResponse};
+        NSData* returnedMessage =
+            [[FlutterJSONMethodCodec sharedInstance] encodeSuccessEnvelope:responseDict];
+        callback(returnedMessage);
+      }));
+  __block NSString* calledAfterTerminate = @"";
   FlutterResult appExitResult = ^(id result) {
-    NSLog(@"Response: %@", result);
+    NSDictionary* resultDict = result;
+    calledAfterTerminate = resultDict[@"response"];
   };
-  FlutterMethodCall* methodCallAfterClear =
-      [FlutterMethodCall methodCallWithMethodName:@"System.exitApplication" arguments:nil];
-  [engineMock handleMethodCall:methodCallAfterClear result:appExitResult];
-  EXPECT_EQ(kFlutterAppExitResponseCancel, calledAfterTerminate);
+  FlutterMethodCall* methodExitApplication =
+      [FlutterMethodCall methodCallWithMethodName:@"System.exitApplication"
+                                        arguments:@{@"type" : @"cancelable"}];
+
+  [engineMock handleMethodCall:methodExitApplication result:appExitResult];
+
+  triedToTerminate = FALSE;
+  nextResponse = @"exit";
+  EXPECT_STREQ([calledAfterTerminate UTF8String], "exit");
+  EXPECT_TRUE(triedToTerminate);
+
+  triedToTerminate = FALSE;
+  nextResponse = @"cancel";
+  [engineMock handleMethodCall:methodExitApplication result:appExitResult];
+  EXPECT_STREQ([calledAfterTerminate UTF8String], "cancel");
+  EXPECT_FALSE(triedToTerminate);
 }
 
 }  // namespace flutter::testing