Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

Merged
merged 6 commits into from
Apr 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ - (void)testResize {
[binaryMessenger stopMocking];
}

- (bool)testSetWarnsOnOverflow {
- (void)testSetWarnsOnOverflow {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but noticed this wasn't running because the signature wasn't void.

NSString* channelName = @"flutter/test";
id binaryMessenger = OCMStrictProtocolMock(@protocol(FlutterBinaryMessenger));
id codec = OCMProtocolMock(@protocol(FlutterMethodCodec));
Expand Down
13 changes: 8 additions & 5 deletions shell/platform/darwin/ios/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,14 @@ source_set("flutter_framework_source_arc") {
public_configs = [ "//flutter:config" ]

sources = [
"framework/Source/FlutterCallbackCache.mm",
"framework/Source/FlutterCallbackCache_Internal.h",
"framework/Source/FlutterEmbedderKeyResponder.h",
"framework/Source/FlutterEmbedderKeyResponder.mm",
"framework/Source/FlutterKeyPrimaryResponder.h",
"framework/Source/FlutterKeySecondaryResponder.h",
"framework/Source/FlutterKeyboardManager.h",
"framework/Source/FlutterKeyboardManager.mm",
"framework/Source/FlutterMetalLayer.h",
"framework/Source/FlutterMetalLayer.mm",
"framework/Source/FlutterRestorationPlugin.h",
Expand All @@ -83,7 +87,10 @@ source_set("flutter_framework_source_arc") {
"IOSurface.framework",
]

deps += [ "//flutter/shell/platform/embedder:embedder_as_internal_library" ]
deps += [
"//flutter/lib/ui",
"//flutter/shell/platform/embedder:embedder_as_internal_library",
]
}

source_set("flutter_framework_source") {
Expand All @@ -98,8 +105,6 @@ source_set("flutter_framework_source") {
# New files are highly encouraged to be in ARC.
# To add new files in ARC, add them to the `flutter_framework_source_arc` target.
"framework/Source/FlutterAppDelegate.mm",
"framework/Source/FlutterCallbackCache.mm",
"framework/Source/FlutterCallbackCache_Internal.h",
"framework/Source/FlutterChannelKeyResponder.h",
"framework/Source/FlutterChannelKeyResponder.mm",
"framework/Source/FlutterDartProject.mm",
Expand All @@ -110,8 +115,6 @@ source_set("flutter_framework_source") {
"framework/Source/FlutterEngineGroup.mm",
"framework/Source/FlutterEngine_Internal.h",
"framework/Source/FlutterHeadlessDartRunner.mm",
"framework/Source/FlutterKeyboardManager.h",
"framework/Source/FlutterKeyboardManager.mm",
"framework/Source/FlutterOverlayView.h",
"framework/Source/FlutterOverlayView.mm",
"framework/Source/FlutterPlatformPlugin.h",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,9 @@
#include "flutter/fml/logging.h"
#include "flutter/lib/ui/plugins/callback_cache.h"

@implementation FlutterCallbackInformation

- (void)dealloc {
[_callbackName release];
[_callbackClassName release];
[_callbackLibraryPath release];
[super dealloc];
}
FLUTTER_ASSERT_ARC

@implementation FlutterCallbackInformation
@end

@implementation FlutterCallbackCache
Expand All @@ -25,7 +19,7 @@ + (FlutterCallbackInformation*)lookupCallbackInformation:(int64_t)handle {
if (info == nullptr) {
return nil;
}
FlutterCallbackInformation* new_info = [[[FlutterCallbackInformation alloc] init] autorelease];
FlutterCallbackInformation* new_info = [[FlutterCallbackInformation alloc] init];
new_info.callbackName = [NSString stringWithUTF8String:info->name.c_str()];
new_info.callbackClassName = [NSString stringWithUTF8String:info->class_name.c_str()];
new_info.callbackLibraryPath = [NSString stringWithUTF8String:info->library_path.c_str()];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
// found in the LICENSE file.

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.h"

#include "flutter/fml/platform/darwin/message_loop_darwin.h"
#include "flutter/fml/platform/darwin/weak_nsobject.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"

FLUTTER_ASSERT_ARC

static constexpr CFTimeInterval kDistantFuture = 1.0e10;

Expand All @@ -13,13 +16,13 @@ @interface FlutterKeyboardManager ()
/**
* The primary responders added by addPrimaryResponder.
*/
@property(nonatomic, retain, readonly)
@property(nonatomic, copy, readonly)
NSMutableArray<id<FlutterKeyPrimaryResponder>>* primaryResponders;

/**
* The secondary responders added by addSecondaryResponder.
*/
@property(nonatomic, retain, readonly)
@property(nonatomic, copy, readonly)
NSMutableArray<id<FlutterKeySecondaryResponder>>* secondaryResponders;

- (void)dispatchToSecondaryResponders:(nonnull FlutterUIPressProxy*)press
Expand All @@ -28,16 +31,13 @@ - (void)dispatchToSecondaryResponders:(nonnull FlutterUIPressProxy*)press

@end

@implementation FlutterKeyboardManager {
std::unique_ptr<fml::WeakNSObjectFactory<FlutterKeyboardManager>> _weakFactory;
}
@implementation FlutterKeyboardManager

- (nonnull instancetype)init {
self = [super init];
if (self != nil) {
_primaryResponders = [[NSMutableArray alloc] init];
_secondaryResponders = [[NSMutableArray alloc] init];
_weakFactory = std::make_unique<fml::WeakNSObjectFactory<FlutterKeyboardManager>>(self);
}
return self;
}
Expand All @@ -50,24 +50,6 @@ - (void)addSecondaryResponder:(nonnull id<FlutterKeySecondaryResponder>)responde
[_secondaryResponders addObject:responder];
}

- (void)dealloc {
// It will be destroyed and invalidate its weak pointers
// before any other members are destroyed.
_weakFactory.reset();

[_primaryResponders removeAllObjects];
[_secondaryResponders removeAllObjects];
[_primaryResponders release];
[_secondaryResponders release];
_primaryResponders = nil;
_secondaryResponders = nil;
[super dealloc];
}

- (fml::WeakNSObject<FlutterKeyboardManager>)getWeakNSObject {
return _weakFactory->GetWeakNSObject();
}

- (void)handlePress:(nonnull FlutterUIPressProxy*)press
nextAction:(nonnull void (^)())next API_AVAILABLE(ios(13.4)) {
if (@available(iOS 13.4, *)) {
Expand All @@ -89,7 +71,7 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
// encounter.
NSAssert([_primaryResponders count] >= 0, @"At least one primary responder must be added.");

__block auto weakSelf = [self getWeakNSObject];
__block __weak __typeof(self) weakSelf = self;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the WeakNSObjectFactory in favor of __weak where it's used before the block. I can swap __typeof(self) to FlutterKeyboardManager* I don't really have a preference.

__block NSUInteger unreplied = [self.primaryResponders count];
__block BOOL anyHandled = false;
FlutterAsyncKeyCallback replyCallback = ^(BOOL handled) {
Expand All @@ -98,7 +80,7 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
anyHandled = anyHandled || handled;
if (unreplied == 0) {
if (!anyHandled && weakSelf) {
[weakSelf.get() dispatchToSecondaryResponders:press complete:completeCallback];
[weakSelf dispatchToSecondaryResponders:press complete:completeCallback];
} else {
completeCallback(true, press);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,43 +23,6 @@

using namespace flutter::testing;

/// Sometimes we have to use a custom mock to avoid retain cycles in ocmock.
@interface FlutterEnginePartialMock : FlutterEngine
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code.

@property(nonatomic, strong) FlutterBasicMessageChannel* lifecycleChannel;
@property(nonatomic, weak) FlutterViewController* viewController;
@property(nonatomic, assign) BOOL didCallNotifyLowMemory;
@end

@interface FlutterEngine ()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code.

- (BOOL)createShell:(NSString*)entrypoint
libraryURI:(NSString*)libraryURI
initialRoute:(NSString*)initialRoute;
- (void)dispatchPointerDataPacket:(std::unique_ptr<flutter::PointerDataPacket>)packet;
@end

@interface FlutterEngine (TestLowMemory)
- (void)notifyLowMemory;
@end

extern NSNotificationName const FlutterViewControllerWillDealloc;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code.


/// A simple mock class for FlutterEngine.
///
/// OCMockClass can't be used for FlutterEngine sometimes because OCMock retains arguments to
/// invocations and since the init for FlutterViewController calls a method on the
/// FlutterEngine it creates a retain cycle that stops us from testing behaviors related to
/// deleting FlutterViewControllers.
@interface MockEngine : NSObject
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code.

@end

@interface FlutterKeyboardManagerUnittestsObjC : NSObject
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this is, there's no implementation.

- (bool)nextResponderShouldThrowOnPressesEnded;
- (bool)singlePrimaryResponder;
- (bool)doublePrimaryResponder;
- (bool)singleSecondaryResponder;
- (bool)emptyNextResponder;
@end

namespace {

typedef void (^KeyCallbackSetter)(FlutterUIPressProxy* press, FlutterAsyncKeyCallback callback)
Expand All @@ -68,52 +31,28 @@ typedef void (^KeyCallbackSetter)(FlutterUIPressProxy* press, FlutterAsyncKeyCal

} // namespace

// These tests were designed to run on iOS 13.4 or later.
API_AVAILABLE(ios(13.4))
Copy link
Member Author

@jmagman jmagman Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting the availability on the class FlutterKeyboardManagerTest means it won't need to be checked in setUp or decorated on the methods.

@interface FlutterKeyboardManagerTest : XCTestCase
@property(nonatomic, strong) id mockEngine;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this into a local in the one remaining spot the engine is mocked.

- (FlutterViewController*)mockOwnerWithPressesBeginOnlyNext API_AVAILABLE(ios(13.4));
@end

@implementation FlutterKeyboardManagerTest

- (void)setUp {
// All of these tests were designed to run on iOS 13.4 or later.
if (@available(iOS 13.4, *)) {
} else {
XCTSkip(@"Required API not present for test.");
}

[super setUp];
self.mockEngine = OCMClassMock([FlutterEngine class]);
}

- (void)tearDown {
// We stop mocking here to avoid retain cycles that stop
// FlutterViewControllers from deallocing.
[self.mockEngine stopMocking];
self.mockEngine = nil;
[super tearDown];
}

- (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code.

return [OCMArg checkWithBlock:^BOOL(id value) {
if (![value isKindOfClass:[FlutterUIPressProxy class]]) {
return NO;
}
FlutterUIPressProxy* press = value;
return press.key.keyCode == keyCode;
}];
}

- (id<FlutterKeyPrimaryResponder>)mockPrimaryResponder:(KeyCallbackSetter)callbackSetter
API_AVAILABLE(ios(13.4)) {
- (id<FlutterKeyPrimaryResponder>)mockPrimaryResponder:(KeyCallbackSetter)callbackSetter {
id<FlutterKeyPrimaryResponder> mock =
OCMStrictProtocolMock(@protocol(FlutterKeyPrimaryResponder));
OCMStub([mock handlePress:[OCMArg any] callback:[OCMArg any]])
.andDo((^(NSInvocation* invocation) {
FlutterUIPressProxy* press;
FlutterAsyncKeyCallback callback;
[invocation getArgument:&press atIndex:2];
[invocation getArgument:&callback atIndex:3];
__unsafe_unretained FlutterUIPressProxy* pressUnsafe;
__unsafe_unretained FlutterAsyncKeyCallback callbackUnsafe;

[invocation getArgument:&pressUnsafe atIndex:2];
[invocation getArgument:&callbackUnsafe atIndex:3];

// Retain the unretained parameters so they can
// be run in the perform block when this invocation goes out of scope.
FlutterUIPressProxy* press = pressUnsafe;
FlutterAsyncKeyCallback callback = callbackUnsafe;
CFRunLoopPerformBlock(CFRunLoopGetCurrent(),
fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode, ^() {
callbackSetter(press, callback);
Expand All @@ -122,8 +61,7 @@ - (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
return mock;
}

- (id<FlutterKeySecondaryResponder>)mockSecondaryResponder:(BoolGetter)resultGetter
API_AVAILABLE(ios(13.4)) {
- (id<FlutterKeySecondaryResponder>)mockSecondaryResponder:(BoolGetter)resultGetter {
id<FlutterKeySecondaryResponder> mock =
OCMStrictProtocolMock(@protocol(FlutterKeySecondaryResponder));
OCMStub([mock handlePress:[OCMArg any]]).andDo((^(NSInvocation* invocation) {
Expand All @@ -133,32 +71,27 @@ - (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
return mock;
}

- (FlutterViewController*)mockOwnerWithPressesBeginOnlyNext API_AVAILABLE(ios(13.4)) {
- (void)testNextResponderShouldThrowOnPressesEnded {
// The nextResponder is a strict mock and hasn't stubbed pressesEnded.
// An error will be thrown on pressesEnded.
UIResponder* nextResponder = OCMStrictClassMock([UIResponder class]);
OCMStub([nextResponder pressesBegan:[OCMArg any] withEvent:[OCMArg any]]).andDo(nil);
OCMStub([nextResponder pressesBegan:OCMOCK_ANY withEvent:OCMOCK_ANY]);

FlutterViewController* viewController =
[[FlutterViewController alloc] initWithEngine:self.mockEngine nibName:nil bundle:nil];
id mockEngine = OCMClassMock([FlutterEngine class]);
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:mockEngine
nibName:nil
bundle:nil];
FlutterViewController* owner = OCMPartialMock(viewController);
OCMStub([owner nextResponder]).andReturn(nextResponder);
return owner;
}

// Verify that the nextResponder returned from mockOwnerWithPressesBeginOnlyNext()
// throws exception when pressesEnded is called.
- (bool)testNextResponderShouldThrowOnPressesEnded API_AVAILABLE(ios(13.4)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't actually running because the return type wasn't void.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need a custom CI check on this? It seems like anything in Test.{m,mm} that starts - (anything but void)test is probably wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlutterViewController* owner = [self mockOwnerWithPressesBeginOnlyNext];
@try {
[owner.nextResponder pressesEnded:[NSSet init] withEvent:[[UIPressesEvent alloc] init]];
return false;
} @catch (...) {
return true;
}
XCTAssertThrowsSpecificNamed([owner.nextResponder pressesEnded:[[NSSet alloc] init]
withEvent:[[UIPressesEvent alloc] init]],
NSException, NSInternalInconsistencyException);

[mockEngine stopMocking];
}

- (void)testSinglePrimaryResponder API_AVAILABLE(ios(13.4)) {
- (void)testSinglePrimaryResponder {
FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];
__block BOOL primaryResponse = FALSE;
__block int callbackCount = 0;
Expand Down Expand Up @@ -190,7 +123,7 @@ - (void)testSinglePrimaryResponder API_AVAILABLE(ios(13.4)) {
XCTAssertFalse(completeHandled);
}

- (void)testDoublePrimaryResponder API_AVAILABLE(ios(13.4)) {
- (void)testDoublePrimaryResponder {
FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];

__block BOOL callback1Response = FALSE;
Expand Down Expand Up @@ -253,7 +186,7 @@ - (void)testDoublePrimaryResponder API_AVAILABLE(ios(13.4)) {
XCTAssertFalse(somethingWasHandled);
}

- (void)testSingleSecondaryResponder API_AVAILABLE(ios(13.4)) {
- (void)testSingleSecondaryResponder {
FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];

__block BOOL primaryResponse = FALSE;
Expand Down Expand Up @@ -308,7 +241,7 @@ - (void)testSingleSecondaryResponder API_AVAILABLE(ios(13.4)) {
XCTAssertFalse(completeHandled);
}

- (void)testEventsProcessedSequentially API_AVAILABLE(ios(13.4)) {
- (void)testEventsProcessedSequentially {
constexpr UIKeyboardHIDUsage keyId1 = (UIKeyboardHIDUsage)0x50;
constexpr UIKeyboardHIDUsage keyId2 = (UIKeyboardHIDUsage)0x51;
FlutterUIPressProxy* event1 = keyDownEvent(keyId1);
Expand Down