Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing #38981

Merged
merged 12 commits into from Jan 25, 2023
9 changes: 9 additions & 0 deletions shell/platform/darwin/macos/framework/Headers/FlutterEngine.h
Expand Up @@ -67,6 +67,15 @@ FLUTTER_DARWIN_EXPORT
*
* The default view always has ID kFlutterDefaultViewId, and is the view
* operated by the APIs that do not have a view ID specified.
*
* Setting this field from nil to a non-nil view controller also updates
* the view controller's engine and ID.
*
* Setting this field to non-nil to nil will terminate the engine if
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
* allowHeadlessExecution is NO.
*
* Setting this field from non-nil to a different non-nil is prohibited and
* will throw an assertion error.
*/
@property(nonatomic, nullable, weak) FlutterViewController* viewController;

Expand Down
Expand Up @@ -31,9 +31,19 @@ FLUTTER_DARWIN_EXPORT

/**
* The Flutter engine associated with this view controller.
*
* The engine is strongly referenced by the FlutterViewController, and weakly
* vice versa.
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
*/
@property(nonatomic, nonnull, readonly) FlutterEngine* engine;

/**
* The identifier for this view controller.
*
* The ID is assigned when the view controller is added to FlutterEngine.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe expand on this, like:

Suggested change
* The ID is assigned when the view controller is added to FlutterEngine.
* The ID is assigned when the view controller is assigned to the
* FlutterEngine.viewController property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mechanism is a little more complicated than this, so I decided to explain everything in FlutterViewController's doc. Can you take a look?

*/
@property(nonatomic, readonly) uint64_t id;

/**
* The style of mouse tracking to use for the view. Defaults to
* FlutterMouseTrackingModeInKeyWindow.
Expand All @@ -43,6 +53,12 @@ FLUTTER_DARWIN_EXPORT
/**
* Initializes a controller that will run the given project.
*
* In this initializer, this controller creates an engine, and is attached to
* this engine as the default controller. In this way, this controller can not
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
* be set to other engines. This initializer is suitable for the first Flutter
* view controller of the app. To use the controller for an existing engine,
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
* use initWithEngine:nibName:bundle: instead.
*
* @param project The project to run in this view controller. If nil, a default `FlutterDartProject`
* will be used.
*/
Expand All @@ -56,7 +72,8 @@ FLUTTER_DARWIN_EXPORT
/**
* Initializes this FlutterViewController with the specified `FlutterEngine`.
*
* The initialized viewcontroller will attach itself to the engine as part of this process.
* This initializer is suitable for both the first Flutter view controller and
* the following ones of the app.
*
* @param engine The `FlutterEngine` instance to attach to. Cannot be nil.
* @param nibName The NIB name to initialize this controller with.
Expand Down
Expand Up @@ -51,26 +51,22 @@ @implementation AccessibilityBridgeTestEngine
namespace {

// Returns an engine configured for the text fixture resource configuration.
FlutterEngine* CreateTestEngine() {
FlutterViewController* CreateTestViewController() {
NSString* fixtures = @(testing::GetFixturesPath());
FlutterDartProject* project = [[FlutterDartProject alloc]
initWithAssetsPath:fixtures
ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
return [[AccessibilityBridgeTestEngine alloc] initWithName:@"test"
project:project
allowHeadlessExecution:true];
FlutterEngine* engine = [[AccessibilityBridgeTestEngine alloc] initWithName:@"test"
project:project
allowHeadlessExecution:true];
return [[FlutterViewController alloc] initWithEngine:engine nibName:nil bundle:nil];
}
} // namespace

TEST(AccessibilityBridgeMacTest, sendsAccessibilityCreateNotificationToWindowOfFlutterView) {
FlutterEngine* engine = CreateTestEngine();
NSString* fixtures = @(testing::GetFixturesPath());
FlutterDartProject* project = [[FlutterDartProject alloc]
initWithAssetsPath:fixtures
ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];
FlutterViewController* viewController = CreateTestViewController();
FlutterEngine* engine = viewController.engine;
[viewController loadView];
[engine setViewController:viewController];

NSWindow* expectedTarget = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600)
styleMask:NSBorderlessWindowMask
Expand Down Expand Up @@ -122,14 +118,9 @@ @implementation AccessibilityBridgeTestEngine
}

TEST(AccessibilityBridgeMacTest, doesNotSendAccessibilityCreateNotificationWhenHeadless) {
FlutterEngine* engine = CreateTestEngine();
NSString* fixtures = @(testing::GetFixturesPath());
FlutterDartProject* project = [[FlutterDartProject alloc]
initWithAssetsPath:fixtures
ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];
FlutterViewController* viewController = CreateTestViewController();
FlutterEngine* engine = viewController.engine;
[viewController loadView];
[engine setViewController:viewController];
// Setting up bridge so that the AccessibilityBridgeMacDelegateSpy
// can query semantics information from.
engine.semanticsEnabled = YES;
Expand Down Expand Up @@ -173,15 +164,9 @@ @implementation AccessibilityBridgeTestEngine
}

TEST(AccessibilityBridgeMacTest, doesNotSendAccessibilityCreateNotificationWhenNoWindow) {
FlutterEngine* engine = CreateTestEngine();
// Create a view controller without attaching it to a window.
NSString* fixtures = @(testing::GetFixturesPath());
FlutterDartProject* project = [[FlutterDartProject alloc]
initWithAssetsPath:fixtures
ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];
FlutterViewController* viewController = CreateTestViewController();
FlutterEngine* engine = viewController.engine;
[viewController loadView];
[engine setViewController:viewController];

// Setting up bridge so that the AccessibilityBridgeMacDelegateSpy
// can query semantics information from.
Expand Down
55 changes: 31 additions & 24 deletions shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
Expand Up @@ -80,6 +80,8 @@ @interface FlutterEngine () <FlutterBinaryMessenger>
*/
@property(nonatomic, strong) NSMutableArray<NSNumber*>* isResponseValid;

- (nullable FlutterViewController*)viewControllerForId:(uint64_t)viewId;

/**
* Sends the list of user-preferred locales to the Flutter engine.
*/
Expand Down Expand Up @@ -213,8 +215,6 @@ @implementation FlutterEngine {
// when the engine is destroyed.
std::unique_ptr<flutter::FlutterCompositor> _macOSCompositor;

FlutterViewEngineProvider* _viewProvider;

// FlutterCompositor is copied and used in embedder.cc.
FlutterCompositor _compositor;

Expand Down Expand Up @@ -248,7 +248,6 @@ - (instancetype)initWithName:(NSString*)labelPrefix
_currentMessengerConnection = 1;
_allowHeadlessExecution = allowHeadlessExecution;
_semanticsEnabled = NO;
_viewProvider = [[FlutterViewEngineProvider alloc] initWithEngine:self];
_isResponseValid = [[NSMutableArray alloc] initWithCapacity:1];
[_isResponseValid addObject:@YES];

Expand Down Expand Up @@ -411,29 +410,28 @@ - (void)loadAOTData:(NSString*)assetsDir {
}

- (void)setViewController:(FlutterViewController*)controller {
if (_viewController != controller) {
if (_viewController == controller) {
// From nil to nil, or from non-nil to the same controller.
return;
}
if (_viewController == nil && controller != nil) {
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
_viewController = controller;

if (_semanticsEnabled && _bridge) {
_bridge->UpdateDefaultViewController(_viewController);
}

if (!controller && !_allowHeadlessExecution) {
[_viewController attachToEngine:self withId:kFlutterDefaultViewId];
} else if (_viewController != nil && controller == nil) {
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
[_viewController detachFromEngine];
_viewController = nil;
if (!_allowHeadlessExecution) {
[self shutDownEngine];
}
} else {
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
NSLog(@"Failed to set view controller to the engine: "
@"Replacing the view controller of the engine is not supported.");
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
}
}

- (FlutterCompositor*)createFlutterCompositor {
// TODO(richardjcai): Add support for creating a FlutterCompositor
// with a nil _viewController for headless engines.
// https://github.com/flutter/flutter/issues/71606
if (!_viewController) {
return nil;
}

_macOSCompositor =
std::make_unique<flutter::FlutterCompositor>(_viewProvider, _platformViewController);
_macOSCompositor = std::make_unique<flutter::FlutterCompositor>(
[[FlutterViewEngineProvider alloc] initWithEngine:self], _platformViewController);

_compositor = {};
_compositor.struct_size = sizeof(FlutterCompositor);
Expand Down Expand Up @@ -539,10 +537,10 @@ - (nonnull NSString*)executableName {
}

- (void)updateWindowMetrics {
if (!_engine || !_viewController.viewLoaded) {
if (!_engine || !self.viewController.viewLoaded) {
return;
}
NSView* view = _viewController.flutterView;
NSView* view = self.viewController.flutterView;
CGRect scaledBounds = [view convertRectToBacking:view.bounds];
CGSize scaledSize = scaledBounds.size;
double pixelRatio = view.bounds.size.width == 0 ? 1 : scaledSize.width / view.bounds.size.width;
Expand Down Expand Up @@ -603,6 +601,17 @@ - (FlutterPlatformViewController*)platformViewController {

#pragma mark - Private methods

- (FlutterViewController*)viewControllerForId:(uint64_t)viewId {
// TODO(dkwingsmt): The engine only supports single-view, therefore it
// only processes the default ID. After the engine supports multiple views,
// this method should be able to return the view for any IDs.
NSAssert(viewId == kFlutterDefaultViewId, @"Unexpected view ID %llu", viewId);
if (viewId == kFlutterDefaultViewId) {
return _viewController;
}
return nil;
}

- (void)sendUserLocales {
if (!self.running) {
return;
Expand Down Expand Up @@ -679,9 +688,7 @@ - (void)shutDownEngine {
return;
}

if (_viewController && _viewController.flutterView) {
[_viewController.flutterView shutdown];
}
[self.viewController.flutterView shutdown];
loic-sharma marked this conversation as resolved.
Show resolved Hide resolved

FlutterEngineResult result = _embedderAPI.Deinitialize(_engine);
if (result != kSuccess) {
Expand Down