Implemented FlutterEngineGroup and Spawn API. #22975

gaaclarke · 2020-12-10T00:24:31Z

Description

This implements the top level API for eventually will be engines sharing components. Currently they just share threads. Improvements will incrementally follow.

Related Issues

fixes flutter/flutter#72010

Tests

I added the following tests:

Shell::Spawn unit test
FlutterEngineGroup spawn test

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

gaaclarke · 2020-12-10T21:07:10Z

Sorry, prematurely put this up for review, i'll implement FlutterEngineGroup here too before review.

gaaclarke · 2020-12-11T19:37:50Z

This is hung up until flutter/flutter#72107 is resolved.

gaaclarke · 2020-12-11T19:51:24Z

This is ready for review, it can't merge probably until the aforementioned unit test fixes have landed.

shell/common/shell.cc

shell/common/shell.h

shell/common/shell_unittests.cc

shell/platform/darwin/ios/framework/Headers/FlutterEngineGroup.h

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

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

shell/common/shell_unittests.cc

shell/platform/darwin/ios/framework/Headers/FlutterEngineGroup.h

xster · 2020-12-14T17:33:59Z

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

+                                                      project:_dartProject.get()
+                                       allowHeadlessExecution:_allowHeadlessExecution];
+
+  flutter::Settings settings = _shell->GetSettings();


Can we extract parts of this into a private method to share between here and createShell? To minimize the risk of someone updating one and not the other as much as possible?

Yep, I did that in setupShell:withObservatoryPublication: and SetEntryPoint. There is very little happening here, mostly boilerplate around lambdas.

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

xster · 2020-12-14T17:43:32Z

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

+  FlutterEngine* spawnee = [group makeEngineWithEntrypoint:nil libraryURI:nil];
+  XCTAssertNotNil(spawnee);
+}
+


Can we test sequences of adds and deletes and make sure it's tracking correctly?

If there is a sequence that would exercise new code let me know. I think deleting the last engine is the only interesting one that uses the code in a different way.

xster · 2020-12-14T17:44:36Z

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

+  [engine run];
+  FlutterEngine* spawn = [engine spawnWithEntrypoint:nil libraryURI:nil];
+  XCTAssertNotNil(spawn);
+}


check that calling spawn accidentally before run doesn't crash or do something too weird

It should crash which is typical for other methods that require an engine to have been run first.

xster · 2020-12-14T17:45:07Z

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

+ * Creates one FlutterEngine from another, sharing components between them.
+ * This results in a faster creation time and a smaller memory footprint engine.
+ *
+ * @see http://flutter.dev/go/multiple-engines


Lets make these all code comments

I don't think that is appropriate. Docstrings are the proper place for documenting what a method does.

It's not documentation though. i.e. we don't want our users to actually read the doc in a year. It's good context for engineers and maintainers to figure out the project scope and technical decisions.

You talking about the link to the design doc? I've removed that.

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

xster · 2020-12-14T23:19:32Z

tools/gen_objcdoc.sh

 FlutterCallbackInformation.html
 FlutterDartProject.html
 FlutterEngine.html
+FlutterEngineGroup.html


ha good catch. I always forget about this one.

xster · 2020-12-14T23:20:30Z

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

+ * Creates one FlutterEngine from another, sharing components between them.
+ * This results in a faster creation time and a smaller memory footprint engine.
+ *
+ * @see http://flutter.dev/go/multiple-engines


It's not documentation though. i.e. we don't want our users to actually read the doc in a year. It's good context for engineers and maintainers to figure out the project scope and technical decisions.

xster

LGTM

…#22975)

flutter-dashboard bot added the platform-ios label Dec 10, 2020

google-cla bot added the cla: yes label Dec 10, 2020

gaaclarke force-pushed the lfe-api branch 6 times, most recently from 2745d0a to be38ab2 Compare December 10, 2020 21:03

gaaclarke marked this pull request as ready for review December 10, 2020 21:03

gaaclarke requested a review from xster December 10, 2020 21:04

gaaclarke marked this pull request as draft December 10, 2020 21:06

gaaclarke force-pushed the lfe-api branch 2 times, most recently from 14d06cb to 3c594ce Compare December 10, 2020 21:55

Implemented FlutterEngineGroup and Spawn API.

922f141

gaaclarke force-pushed the lfe-api branch from 3c594ce to 922f141 Compare December 11, 2020 19:33

gaaclarke marked this pull request as ready for review December 11, 2020 19:50

xster reviewed Dec 11, 2020

View reviewed changes

gaaclarke added 3 commits December 11, 2020 14:37

xiao feedback 1

a3091d4

xiao feedback 2

0923668

xiao feedback 3 - added libaryuri support

e5dd242

gaaclarke requested a review from xster December 11, 2020 23:12

gaaclarke added 2 commits December 11, 2020 16:30

added FlutterEngineGroup to expected output docs

f3916b0

sorted gen objcdoc correctly

030b1f4

xster reviewed Dec 14, 2020

View reviewed changes

gaaclarke added 2 commits December 14, 2020 10:47

xiao feedback 4

d56e93a

xiao feedback 5

255e504

gaaclarke requested a review from xster December 14, 2020 23:13

xster reviewed Dec 14, 2020

View reviewed changes

xster approved these changes Dec 14, 2020

View reviewed changes

gaaclarke merged commit f37c8c5 into flutter:master Dec 15, 2020

engine-flutter-autoroll mentioned this pull request Dec 15, 2020

Roll Engine from 1749dbcc1b09 to 9643c9d1108a (6 revisions) flutter/flutter#72328

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020

f37c8c5 Implemented FlutterEngineGroup and Spawn API. (flutter/engine…

91d4b8d

…#22975)

engine-flutter-autoroll mentioned this pull request Dec 15, 2020

Roll Engine from 1749dbcc1b09 to e2cb6beca397 (7 revisions) flutter/flutter#72331

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020

f37c8c5 Implemented FlutterEngineGroup and Spawn API. (flutter/engine…

c27085f

…#22975)

engine-flutter-autoroll mentioned this pull request Dec 15, 2020

Roll Engine from 1749dbcc1b09 to 7810b5e18e52 (8 revisions) flutter/flutter#72332

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020

f37c8c5 Implemented FlutterEngineGroup and Spawn API. (flutter/engine…

a92b626

…#22975)

engine-flutter-autoroll mentioned this pull request Dec 15, 2020

Roll Engine from 1749dbcc1b09 to c59f94dfa116 (9 revisions) flutter/flutter#72338

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020

f37c8c5 Implemented FlutterEngineGroup and Spawn API. (flutter/engine…

8cbe93f

…#22975)

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021

Implemented FlutterEngineGroup and Spawn API. (flutter#22975)

ff455c4

xster mentioned this pull request Jan 15, 2021

FlutterEngineGroup for Android #23675

Merged

13 tasks

Implemented FlutterEngineGroup and Spawn API. #22975

Implemented FlutterEngineGroup and Spawn API. #22975

Uh oh!

Conversation

gaaclarke commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

gaaclarke commented Dec 10, 2020

Uh oh!

gaaclarke commented Dec 11, 2020

Uh oh!

gaaclarke commented Dec 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaaclarke commented Dec 10, 2020 •

edited

Loading