[ios] add setting flag to enable embedder api. #40025

cyanglaz · 2023-03-02T18:59:12Z

First patch for iOS Embedder api migration.
The iOS Embedder API implementation is going to be hidden behind this flag during the migration.
This PR introduced:

A command line flag enable-embedder-api, which can also be used for Android when we want to migrate it to embedder API.
An info.plist flag FLTEnableIOSEmbedderAPI, which is used on iOS.
Both flag acts the same on iOS and either one of them being true should run iOS embedder with the embedder API.
This flag defaults to false.

First step of flutter/flutter#112232

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.

jmagman

Minor nits. Looks like the FlutterEngineTest are crashing though.

shell/common/switches.h

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

jmagman · 2023-03-02T20:14:15Z

shell/common/switches.h

    "requested sampling value, MSAA will be disabled.")
+DEF_SWITCH(EnableIOSEmbedderAPI,
+           "enable-embedder-api",
+           "Enable the embedder api. Defaults to false. Ignored if the "


Alternatively you could say "iOS only" for now, up to you though.

My first version was actually "iOS only" but wanted to keep it future proof for android. I will revert back to "iOS only", it's less confusing.

cyanglaz · 2023-03-03T18:22:40Z

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


 - (void)testCreate {
-  id project = OCMClassMock([FlutterDartProject class]);
+  FlutterDartProject* project = [[FlutterDartProject alloc] init];


@jmagman The test failure was due to the mock project doesn't have the setting ivar mocked. For these tests, I either removed the project completely or used a real project object. There was no reason to mock the project at the first place.

draft draft fix fix test fix test revuew fix tests

jmagman · 2023-03-03T19:59:32Z

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

+- (BOOL)enableEmbedderAPI {
+  return _enableEmbedderAPI;
+}


This didn't get synthesized for you?

They should. The getters were from the old implementation. I forgot to remove this and the below getters.
Will remove now.

jmagman · 2023-03-03T19:59:39Z

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

+#pragma mark - Embedder API properties
+
+// Function pointers for interacting with the embedder.h API.
+@property(nonatomic, assign) BOOL enableEmbedderAPI;


Suggested change

@property(nonatomic, assign) BOOL enableEmbedderAPI;

@property(nonatomic, assign, readonly) BOOL enableEmbedderAPI;

probably also wanna remove assign if do readonly

I think you still want to keep assign (as opposed to retain or copy) for the primitive? I don't think the readwrite/readonly impacts that.

iirc assign/retain/copy etc only affect how the setters are synthesized. readonly won't synthesize the setter at all in the first place.

I looked at it again. It is in the .mm file. So I will remove the readonly and keep the assign.

hellohuanlin · 2023-03-03T20:45:14Z

Both flag acts the same on iOS and either one of them being true should run iOS embedder with the embedder API.

Can you clarify why do we need both flags and when to use which one?

jmagman · 2023-03-03T20:48:08Z

Both flag acts the same on iOS and either one of them being true should run iOS embedder with the embedder API.

Can you clarify why do we need both flags and when to use which one?

The tool flag would be for a single flutter run invocation, and it gets passed on as a launch argument to the app, read by the engine. The Info.plist bool makes it persistent for the app across runs, so you can open it in Xcode and have it be passed into the engine without actually setting a launch argument in the scheme.
We do the same pattern for other experimental features.

cyanglaz · 2023-03-03T20:54:05Z

Can you clarify why do we need both flags and when to use which one?

The info.plist is for running from XCode, the commandline flag is for running from terminal without the need to edit the info.plist.

hellohuanlin · 2023-03-03T20:54:05Z

common/settings.h

+  /// Enable embedder api on the embedder.
+  ///
+  /// This is ignored on the platforms that use the embedder_api by default.
+  bool enable_embedder_api = false;


Maybe more self-descriptive naming like enable_embedder_api_for_ios (or enable_embedder_api_for_mobile if wanna re-use for android)

I tried to keep it generic so it could be reused by android.
I also wanted to keep it the same name as the command line flag. And I would like the command line flag to be short for easier typing when testing ;p

…ine#40025)

flutter-dashboard bot added the platform-ios label Mar 2, 2023

cyanglaz requested a review from jmagman March 2, 2023 19:05

cyanglaz marked this pull request as ready for review March 2, 2023 19:06

cyanglaz force-pushed the embedder_api_patch1_flag branch from e2f883d to 8942090 Compare March 2, 2023 19:07

jmagman reviewed Mar 2, 2023

View reviewed changes

cyanglaz force-pushed the embedder_api_patch1_flag branch from 2d99212 to b2f8505 Compare March 2, 2023 23:02

cyanglaz commented Mar 3, 2023

View reviewed changes

cyanglaz requested a review from jmagman March 3, 2023 18:59

add setting flag

ec2664f

draft draft fix fix test fix test revuew fix tests

cyanglaz mentioned this pull request Mar 3, 2023

[flutter_tool] add enable-embedder-api flag flutter/flutter#121895

Merged

8 tasks

cyanglaz requested a review from hellohuanlin March 3, 2023 19:31

jmagman approved these changes Mar 3, 2023

View reviewed changes

hellohuanlin reviewed Mar 3, 2023

View reviewed changes

review

99a48ca

cyanglaz force-pushed the embedder_api_patch1_flag branch from b624b14 to 99a48ca Compare March 3, 2023 20:56

Chris Yang added 2 commits March 3, 2023 13:02

update comment

91a0985

remove readonly

cf9a0cf

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

auto-submit bot merged commit 85d0e15 into flutter:main Mar 3, 2023

engine-flutter-autoroll mentioned this pull request Mar 4, 2023

Roll Flutter Engine from 34a828142134 to 3070e61574c0 (3 revisions) flutter/flutter#121919

Merged

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

85d0e152f [ios] add setting flag to enable embedder api. (flutter/eng…

677fdc0

…ine#40025)

	@property(nonatomic, assign) BOOL enableEmbedderAPI;
	@property(nonatomic, assign, readonly) BOOL enableEmbedderAPI;

[ios] add setting flag to enable embedder api. #40025

[ios] add setting flag to enable embedder api. #40025

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

hellohuanlin commented Mar 3, 2023

Uh oh!

jmagman commented Mar 3, 2023

Uh oh!

cyanglaz commented Mar 3, 2023

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 Mar 2, 2023 •

edited

Loading