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

Added a default entrypoint variable to match android syntax. #12370

Merged
merged 5 commits into from
Sep 27, 2019

Conversation

gaaclarke
Copy link
Member

relevant issue: flutter/flutter#33097

* library that contains the app's main() function. If this is FlutterDefaultDartEntrypoint (or
* nil) it will default to `main()`. If it is not the app's main() function, that function must
* be decorated with `@pragma(vm:entry-point)` to ensure the method is not tree-shaken by the Dart
* compiler.
* @return YES if the call succeeds in creating and running a Flutter Engine instance; NO otherwise.
*/
- (BOOL)runWithEntrypoint:(nullable NSString*)entrypoint;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with how things are packaged at the binary level in iOS, but do we not need a configurable bundle path along with the function name? The entrypoint on Android has both pieces in a data structure called DartEntrypoint...

Copy link
Member Author

Choose a reason for hiding this comment

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

- (instancetype)initWithName:(NSString*)labelPrefix project:(nullable FlutterDartProject*)project;
. You specify the bundle path when you create the FlutterEngine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that's separated? Is the goal of this change to match Android? If so, shouldn't that involve pairing the entrypoint with the bundle path? The entrypoint only exists in a specific bundle, so if we think it's legitimate to lazily choose the entrypoint method, then it seems like you should be able to lazily choose the bundle path, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a legitimate point. What you are proposing is a breaking change though. The idea with this PR is it gets us closer to Android without breaking. We can see what @xster thinks. I'd say that could be a different PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can see what @xster says, but seems like we could introduce a new constructor and a new dart entrypoint setter method, then soft deprecate the existing ones. That wouldn't be a breaking change, but it would move us forward for the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

That DartEntrypoint contains 2 fields: an entrypoint function and a bundle path:

https://github.com/flutter/engine/blob/master/shell/platform/android/io/flutter/embedding/engine/dart/DartExecutor.java#L277

Copy link
Member

@xster xster Sep 23, 2019

Choose a reason for hiding this comment

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

ya, we're in kind of a pickle. The Android engine constructor doesn't ask for anything and everything's provided by the dart executor's execute.

The iOS engine asks for half of the stuff during init and the other half during run.

I think the ideal solution is probably:

  • Deprecate the initWithName:(NSString*)labelPrefix project:(FlutterDartProject*)project init. If you give it a project, we'll just ignore it.
  • Let the launchEngine:(NSString*)entrypoint libraryURI:(NSString*)libraryOrNil also have a withProject:(FlutterDartProject*)project to fulfill this API.
  • Let there also be a launchDefaultEngine or some such so you can just [[[FlutterEngine alloc] initWithName:@"blah"] launchDefaultEngine] so that the most default engine can be launched without params or nils like Android.

It looks like _dartProject is only ever used in the launchEngine method in the FlutterEngine so moving it implementation wise shouldn't alter too much.

The break would be ok (but still needs announcing) since the only valid use of that API is add-to-app currently and we never shipped add-to-app yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'll look into making this a bit deeper. If you want the API's to be closer though, shouldn't we introduce a class called FlutterDartEntrypoint in objc and make a method on FlutterEngine runWithDartEntrypoint:?

Copy link
Member

Choose a reason for hiding this comment

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

Depends on how ambitious you want to get. If you want to go big, you can do flutter/flutter#33099 at the same time :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unable to add the bundle to the run arguments. This would negatively affect our app launch time for full flutter apps since the engine is created and launch is only called after viewWillAppear:

There are still a few things I can do to make working with the FlutterEngine nicer though. I'll do everything but move the bundle to run/launch arguments.

* must be decorated with `@pragma(vm:entry-point)` to ensure the method is not
* tree-shaken by the Dart compiler.
* library that contains the app's main() function. If this is FlutterDefaultDartEntrypoint (or
* nil) it will default to `main()`. If it is not the app's main() function, that function must
Copy link

Choose a reason for hiding this comment

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

q: is it main()or main? The Android embedding seems to use just main

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that the literal value on Android is just the name of the function, no (). For the purpose of documentation it might be useful to represent it as "main()" for clarity.

@gaaclarke gaaclarke added the Work in progress (WIP) Not ready (yet) for review! label Sep 25, 2019
@gaaclarke
Copy link
Member Author

I've implemented the 3rd part of @xster's feedback, allow people to create and run FlutterEngine's with default values, [[[FlutterEngine alloc] initWithName:@"blah"] launchDefaultEngine] works.

I attempted to move the NSBundle/FlutterDartProject to run arguments but that will adversely affect startup time as mentioned in my previous comment. I suspect that Android will run into this as well.

I also attempted to deprecate FlutterDartProject, but that became a bit more involved and doesn't need to happen here.

@gaaclarke gaaclarke removed the Work in progress (WIP) Not ready (yet) for review! label Sep 25, 2019
* must be decorated with `@pragma(vm:entry-point)` to ensure the method is not
* tree-shaken by the Dart compiler.
* library that contains the app's main() function. If this is FlutterDefaultDartEntrypoint (or
* nil) it will default to `main()`. If it is not the app's main() function, that function must
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to allow nil here? I explicitly didn't allow that on the Android side. Instead I offered a factory method for DartEntrypoint.createDefault() to make it easy to resolve, but still explicitly selecting the entrypoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like the interface and I don't like the use of nil as placeholders for future calculations either. In this PR I've tried to make it so no one ever has to mention a nil to create or run an Engine. Truly eliminating the use of nil will require some excavation, a breaking change and some philosophy debates. I've avoided that to make some concrete progress on the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

@gaaclarke gaaclarke merged commit 92d30c0 into flutter:master Sep 27, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 27, 2019
git@github.com:flutter/engine.git/compare/ce6ab8ce25fa...a206557

git log ce6ab8c..a206557 --no-merges --oneline
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia ec85f407bfee..b83cc76a5e3a (15 commits) (flutter/engine#12472)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Added a default entrypoint variable to match android syntax. (flutter/engine#12370)
2019-09-26 tauu@h2overclock.de [web_ui] add missing dispose handler for MethodCalls to flutter/platform_view (flutter/engine#12226)
2019-09-26 tauu@h2overclock.de [web_ui] PersistedPlatformView attribute update handling to enable resizing (flutter/engine#12227)
2019-09-26 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from z3PaM... to HoRV8... (flutter/engine#12471)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
git@github.com:flutter/engine.git/compare/ce6ab8ce25fa...a206557

git log ce6ab8c..a206557 --no-merges --oneline
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia ec85f407bfee..b83cc76a5e3a (15 commits) (flutter/engine#12472)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Added a default entrypoint variable to match android syntax. (flutter/engine#12370)
2019-09-26 tauu@h2overclock.de [web_ui] add missing dispose handler for MethodCalls to flutter/platform_view (flutter/engine#12226)
2019-09-26 tauu@h2overclock.de [web_ui] PersistedPlatformView attribute update handling to enable resizing (flutter/engine#12227)
2019-09-26 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from z3PaM... to HoRV8... (flutter/engine#12471)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 30, 2019
git@github.com:flutter/engine.git/compare/5b952f286fc0...f0c7740

git log 5b952f2..f0c7740 --no-merges --oneline
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia e242d0ea937c..89e1f600ec78 (3 commits) (flutter/engine#12696)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ncc-K... to KH7Tv... (flutter/engine#12694)
2019-09-30 50856934+nturgut@users.noreply.github.com Refactoring text_editing.dart (flutter/engine#12479)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia abc851eb3dcc..e242d0ea937c (2 commits) (flutter/engine#12693)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0e7500021402..abc851eb3dcc (3 commits) (flutter/engine#12692)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7aeabcfa6a73..0e7500021402 (1 commits) (flutter/engine#12691)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from eUBos... to sWs8N... (flutter/engine#12690)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from VACNQ... to ncc-K... (flutter/engine#12689)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from OEG20... to eUBos... (flutter/engine#12688)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 6fMRC... to VACNQ... (flutter/engine#12687)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia be971636cf9b..7aeabcfa6a73 (3 commits) (flutter/engine#12686)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia 2e856451654d..be971636cf9b (1 commits) (flutter/engine#12685)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from lHiKM... to OEG20... (flutter/engine#12683)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from z3oH6... to 6fMRC... (flutter/engine#12682)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from nVAcH... to lHiKM... (flutter/engine#12680)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from Kiq-H... to z3oH6... (flutter/engine#12679)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia bd84330ba277..2e856451654d (1 commits) (flutter/engine#12661)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8ab1530cd3cd..bd84330ba277 (1 commits) (flutter/engine#12639)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from qx53U... to nVAcH... (flutter/engine#12620)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from oob8T... to Kiq-H... (flutter/engine#12616)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28ad6f869822..8ab1530cd3cd (5 commits) (flutter/engine#12614)
2019-09-27 liyuqian@google.com Revert "[fuchsia] Wire up OpacityLayer to Scenic (#11322)" (flutter/engine#12610)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Started asserting the FlutterEngine is running before communicating over channels. (flutter/engine#12469)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Split out the logic to handle status bar touches into its own function (flutter/engine#12587)
2019-09-27 bkonyi@google.com Roll src/third_party/dart c3c31b74fd..132bee48d0 (16 commits)
2019-09-27 iska.kaushik@gmail.com [flutter_runner] Refactor thread_application pair to ActiveApplication (flutter/engine#12573)
2019-09-27 gw280@google.com Reword confusing messaging surrounding unhandled exception in flutter_runner on Fuchsia (flutter/engine#12428)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b23d66e10a98..28ad6f869822 (10 commits) (flutter/engine#12580)
2019-09-27 iska.kaushik@gmail.com Revert "Update linux toolchain for fuchsia" (flutter/engine#12578)
2019-09-27 jonahwilliams@google.com Add support for JIT release mode (flutter/engine#12446)
2019-09-27 iska.kaushik@gmail.com Update linux toolchain for fuchsia (flutter/engine#12572)
2019-09-27 iska.kaushik@gmail.com Remove references to topaz (flutter/engine#12565)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 7901c508c8..c3c31b74fd (4 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3787f51c65c3..b23d66e10a98 (1 commits) (flutter/engine#12559)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from HoRV8... to oob8T... (flutter/engine#12538)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from YDv3O... to qx53U... (flutter/engine#12520)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 296743a86281..3787f51c65c3 (4 commits) (flutter/engine#12512)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 403c4af720..7901c508c8 (7 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b83cc76a5e3a..296743a86281 (1 commits) (flutter/engine#12491)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 327bc451f8..403c4af720 (9 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from HfPKR... to zpVtV... (flutter/engine#12473)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia ec85f407bfee..b83cc76a5e3a (15 commits) (flutter/engine#12472)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Added a default entrypoint variable to match android syntax. (flutter/engine#12370)
2019-09-26 tauu@h2overclock.de [web_ui] add missing dispose handler for MethodCalls to flutter/platform_view (flutter/engine#12226)
2019-09-26 tauu@h2overclock.de [web_ui] PersistedPlatformView attribute update handling to enable resizing (flutter/engine#12227)
...
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
git@github.com:flutter/engine.git/compare/5b952f286fc0...f0c7740

git log 5b952f2..f0c7740 --no-merges --oneline
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia e242d0ea937c..89e1f600ec78 (3 commits) (flutter/engine#12696)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ncc-K... to KH7Tv... (flutter/engine#12694)
2019-09-30 50856934+nturgut@users.noreply.github.com Refactoring text_editing.dart (flutter/engine#12479)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia abc851eb3dcc..e242d0ea937c (2 commits) (flutter/engine#12693)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0e7500021402..abc851eb3dcc (3 commits) (flutter/engine#12692)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7aeabcfa6a73..0e7500021402 (1 commits) (flutter/engine#12691)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from eUBos... to sWs8N... (flutter/engine#12690)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from VACNQ... to ncc-K... (flutter/engine#12689)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from OEG20... to eUBos... (flutter/engine#12688)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 6fMRC... to VACNQ... (flutter/engine#12687)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia be971636cf9b..7aeabcfa6a73 (3 commits) (flutter/engine#12686)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia 2e856451654d..be971636cf9b (1 commits) (flutter/engine#12685)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from lHiKM... to OEG20... (flutter/engine#12683)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from z3oH6... to 6fMRC... (flutter/engine#12682)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from nVAcH... to lHiKM... (flutter/engine#12680)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from Kiq-H... to z3oH6... (flutter/engine#12679)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia bd84330ba277..2e856451654d (1 commits) (flutter/engine#12661)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8ab1530cd3cd..bd84330ba277 (1 commits) (flutter/engine#12639)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from qx53U... to nVAcH... (flutter/engine#12620)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from oob8T... to Kiq-H... (flutter/engine#12616)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28ad6f869822..8ab1530cd3cd (5 commits) (flutter/engine#12614)
2019-09-27 liyuqian@google.com Revert "[fuchsia] Wire up OpacityLayer to Scenic (flutter#11322)" (flutter/engine#12610)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Started asserting the FlutterEngine is running before communicating over channels. (flutter/engine#12469)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Split out the logic to handle status bar touches into its own function (flutter/engine#12587)
2019-09-27 bkonyi@google.com Roll src/third_party/dart c3c31b74fd..132bee48d0 (16 commits)
2019-09-27 iska.kaushik@gmail.com [flutter_runner] Refactor thread_application pair to ActiveApplication (flutter/engine#12573)
2019-09-27 gw280@google.com Reword confusing messaging surrounding unhandled exception in flutter_runner on Fuchsia (flutter/engine#12428)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b23d66e10a98..28ad6f869822 (10 commits) (flutter/engine#12580)
2019-09-27 iska.kaushik@gmail.com Revert "Update linux toolchain for fuchsia" (flutter/engine#12578)
2019-09-27 jonahwilliams@google.com Add support for JIT release mode (flutter/engine#12446)
2019-09-27 iska.kaushik@gmail.com Update linux toolchain for fuchsia (flutter/engine#12572)
2019-09-27 iska.kaushik@gmail.com Remove references to topaz (flutter/engine#12565)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 7901c508c8..c3c31b74fd (4 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3787f51c65c3..b23d66e10a98 (1 commits) (flutter/engine#12559)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from HoRV8... to oob8T... (flutter/engine#12538)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from YDv3O... to qx53U... (flutter/engine#12520)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 296743a86281..3787f51c65c3 (4 commits) (flutter/engine#12512)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 403c4af720..7901c508c8 (7 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b83cc76a5e3a..296743a86281 (1 commits) (flutter/engine#12491)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 327bc451f8..403c4af720 (9 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from HfPKR... to zpVtV... (flutter/engine#12473)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia ec85f407bfee..b83cc76a5e3a (15 commits) (flutter/engine#12472)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Added a default entrypoint variable to match android syntax. (flutter/engine#12370)
2019-09-26 tauu@h2overclock.de [web_ui] add missing dispose handler for MethodCalls to flutter/platform_view (flutter/engine#12226)
2019-09-26 tauu@h2overclock.de [web_ui] PersistedPlatformView attribute update handling to enable resizing (flutter/engine#12227)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants