Make FlutterEngineGroup support dart entrypoint args #29096

ColdPaleLight · 2021-10-09T07:50:27Z

This PR try to add a new parameter named initialArguments to pass the startup arguments from platform to Flutter app

fixes: flutter/flutter#91547 flutter/flutter#23786 flutter/flutter#34170

design doc: https://docs.google.com/document/d/1ufsbXdhCm1qTAY2eXsRrVrfxED5WCZgqQ-Tm2LzScoE/edit

sample: https://github.com/ColdPaleLight/multiple_flutters/

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.

flutter-dashboard · 2021-10-09T07:50:29Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

ColdPaleLight · 2021-10-09T15:16:24Z

(Remove "need tests" label because unit tests has been added)

gaaclarke · 2021-10-11T17:22:52Z

Why can't you use platform channels for this?

chinmaygarde

The underlying issue warrants further discussion of the use-case and alternatives (in a design doc perhaps) before jumping into an implementation. This data blob is persistent across the lifecycle of the engine and cannot be reclaimed. It has the possibility of growing as the application grows more complex.

dnfield · 2021-10-19T04:20:34Z

lib/ui/platform_dispatcher.dart

+  dynamic getInitialArguments({required List<String> rawArgs}) {
+    if (rawArgs.isEmpty) return null;
+    return json.decode(rawArgs[0]);
+  }


We should avoid using dynamic, and avoid using or assuming this is JSON.

1 We should avoid using dynamic
Done. Using Object? now

2 and avoid using or assuming this is JSON.
Done.
The current solution cannot avoid using json, but some changes can be made here.
We can change the dart entrypoint args passed by the platform into the following format

['--initial-arguments','{\"foo\":\"bar\"}']

Then we change the code to the following

Object? getInitialArguments({required List<String> rawArgs}) { if (rawArgs.isEmpty) return null; final int initialArgumentsIndex = rawArgs.indexOf('--initial-arguments') + 1; if (initialArgumentsIndex <= 0 || initialArgumentsIndex == rawArgs.length) return null; return json.decode(rawArgs[initialArgumentsIndex]); }

dnfield · 2021-10-19T04:21:23Z

shell/common/fixtures/shell_test.dart

+void canRecieveEmptyArguments(List<String> args) {
+  final dynamic initialArguments = PlatformDispatcher.instance.getInitialArguments(rawArgs: args);
+  notifyNativeWhenEngineRun(args.isEmpty && initialArguments == null);
+}


nit: newline at EOF

dnfield

One high level concern: the args get stuck in memory forever even with this patch.

If we do this for debug to support restart it's probably acceptable, but we should not be doing this for profile/release where restart isn't possible.

ColdPaleLight · 2021-10-19T07:00:10Z

One high level concern: the args get stuck in memory forever even with this patch.

If we do this for debug to support restart it's probably acceptable, but we should not be doing this for profile/release where restart isn't possible.

I have considered this problem, and currently I have two solutions:

Use the macro FLUTTER_RELEASE to do conditional compilation, do not store arguments in last_entry_point_args_ in engine in release mode.
Use DartVM::IsRunningPrecompiledCode(). If it is AOT mode, do not store arguments in last_entry_point_args_ in engine.

Hi, @dnfield , do you think these two options are feasible? If feasible, which one do you prefer?

dnfield · 2021-10-19T07:09:01Z

I'd say just guard it with conditional compilation, but probably on DEBUG rather than RELEASE, and make sure both branches are covered by tests :)

ColdPaleLight · 2021-10-19T07:34:01Z

I'd say just guard it with conditional compilation, but probably on DEBUG rather than RELEASE, and make sure both branches are covered by tests :)

Ok, I'll do that :)

…make FlutterEngine

gaaclarke · 2021-11-16T01:40:36Z

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

+ * must be decorated with `@pragma(vm:entry-point)` to ensure themethod is not tree-shaken by the
+ * Dart compiler.
+ */
+@property(nonatomic, copy) NSString* entrypoint;


These properties are nullable right? You should specify that @property(nonatomic, copy, nullable)

…s::dart_entrypoint_args both be set

gaaclarke

I read through everything again. This looks close. The only important thing I noticed is that one test that I think should make sure the arguments match on the ui thread with the ones that were supplied.

shell/common/shell_unittests.cc

gaaclarke · 2021-11-18T00:09:19Z

shell/platform/android/io/flutter/embedding/engine/FlutterEngineGroup.java

+    private Context context;
+    private DartEntrypoint dartEntrypoint;
+    private String initialRoute;
+    private List<String> dartEntrypointArgs;


Nullable annotations would be helpful here too.

gaaclarke · 2021-11-18T00:09:54Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

  }

  private native void nativeRunBundleAndSnapshotFromLibrary(
      long nativeShellHolderId,
      @NonNull String bundlePath,
      @Nullable String entrypointFunctionName,
      @Nullable String pathToEntrypointFunction,
-      @NonNull AssetManager manager);
+      @NonNull AssetManager manager,
+      @NonNull List<String> entrypointArgs);


Shouldn't this be nullable?

gaaclarke · 2021-11-18T00:13:45Z

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

@@ -42,16 +54,32 @@ - (FlutterEngine*)makeEngineWithEntrypoint:(nullable NSString*)entrypoint
 - (FlutterEngine*)makeEngineWithEntrypoint:(nullable NSString*)entrypoint
                                libraryURI:(nullable NSString*)libraryURI
                              initialRoute:(nullable NSString*)initialRoute {
-  NSString* engineName = [NSString stringWithFormat:@"%@.%d", self.name, ++_enginesCreatedCount];
+  FlutterEngineGroupOptions* options = [FlutterEngineGroupOptions new];


There are new analyzer rules coming, r/[FlutterEngineGroupOptions new]/[[FlutterEngineGroupOptions alloc] init]

dnfield

Please add a TODO on settings.h to remove the duplicated entry, and link it to the existing issue that has context.

This LGTM once it LGT @gaaclarke

Chinmay says his concerns have been addressed

ColdPaleLight · 2021-11-18T02:28:35Z

Please add a TODO on settings.h to remove the duplicated entry, and link it to the existing issue that has context.

Done

gaaclarke

LGTM, thanks @ColdPaleLight =)

…/engine#29096)

flutter-dashboard bot added platform-android platform-ios labels Oct 9, 2021

flutter-dashboard bot added the needs tests label Oct 9, 2021

google-cla bot added the cla: yes label Oct 9, 2021

ColdPaleLight force-pushed the engine_group_with_initial_arguments branch from 97c7562 to 8a29f14 Compare October 9, 2021 12:49

ColdPaleLight removed the needs tests label Oct 9, 2021

ColdPaleLight changed the title ~~[WIP] Make FlutterEngineGroup support initialArguments~~ Make FlutterEngineGroup support initialArguments Oct 9, 2021

ColdPaleLight requested review from gaaclarke and xster October 9, 2021 15:52

chinmaygarde previously requested changes Oct 12, 2021

View reviewed changes

ColdPaleLight force-pushed the engine_group_with_initial_arguments branch 4 times, most recently from e684b21 to e893583 Compare October 15, 2021 08:51

ColdPaleLight requested a review from chinmaygarde October 15, 2021 10:13

ColdPaleLight force-pushed the engine_group_with_initial_arguments branch from 834be03 to 58bb313 Compare October 18, 2021 17:27

dnfield reviewed Oct 19, 2021

View reviewed changes

ColdPaleLight added 5 commits October 19, 2021 17:25

Make FlutterEngineGroup support initialArguments

9de9409

Fix unit tests of FlutterEngineGroupComponentTest

cf3fdac

Fix unit tests for FlutterEngine.spawn in iOS

d793a65

Add unit tests in shell_unittests.cc

42b12cf

Add unit tests in FlutterEngineGroupComponentTest

e52adb8

ColdPaleLight and others added 4 commits November 12, 2021 21:50

[iOS] Add FlutterEngineGroupOptions to avoid too many arguments when …

8d83669

…make FlutterEngine

Update gen_objcdoc

1d6d00b

Merge branch 'master' into engine_group_with_initial_arguments

d88181e

Format code

6b8897e

CaseyHillers changed the base branch from master to main November 15, 2021 18:14

fluttergithubbot force-pushed the master branch from b1a8771 to 2962099 Compare November 15, 2021 18:30

gaaclarke reviewed Nov 16, 2021

View reviewed changes

ColdPaleLight added 2 commits November 16, 2021 10:32

[iOS] Add nullable to properties of FlutterEngineGroupOptions

505497d

Throw an assert if Runconfiguration::dart_entrypoint_args and Setting…

6a5d40f

…s::dart_entrypoint_args both be set

ColdPaleLight requested a review from gaaclarke November 16, 2021 03:18

ColdPaleLight changed the base branch from master to main November 16, 2021 04:34

gaaclarke reviewed Nov 18, 2021

View reviewed changes

dnfield approved these changes Nov 18, 2021

View reviewed changes

Tweak the code and add the TODO comment

3df8fc1

ColdPaleLight requested a review from gaaclarke November 18, 2021 02:32

gaaclarke approved these changes Nov 18, 2021

View reviewed changes

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 18, 2021

ColdPaleLight added 5 commits November 19, 2021 02:32

Merge branch 'flutter:main' into engine_group_with_initial_arguments

1ab03be

Fix memory leak of FlutterEngineGroupOptions

131bd4e

Merge branch 'flutter:main' into engine_group_with_initial_arguments

61a8cee

Merge branch 'flutter:main' into engine_group_with_initial_arguments

f3d83ac

Merge branch 'flutter:main' into engine_group_with_initial_arguments

8fdf017

fluttergithubbot merged commit 9e49ffb into flutter:main Nov 19, 2021

This was referenced Nov 19, 2021

Ability to send structured data to Flutter app at startup flutter/flutter#23786

Closed

Create a generic 'synchronous' engine data initialization mechanism flutter/flutter#34170

Closed

engine-flutter-autoroll mentioned this pull request Nov 19, 2021

Roll Engine from d3dcd41c9490 to 64b13b8eff25 (29 revisions) flutter/flutter#93941

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 19, 2021

9e49ffb Make FlutterEngineGroup support dart entrypoint args (flutter…

6d22e85

…/engine#29096)

ColdPaleLight added the customer: alibaba label Jan 6, 2022

Make FlutterEngineGroup support dart entrypoint args #29096

Make FlutterEngineGroup support dart entrypoint args #29096

Uh oh!

Conversation

ColdPaleLight commented Oct 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 9, 2021

Uh oh!

ColdPaleLight commented Oct 9, 2021

Uh oh!

gaaclarke commented Oct 11, 2021

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight commented Oct 19, 2021

Uh oh!

dnfield commented Oct 19, 2021

Uh oh!

ColdPaleLight commented Oct 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

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!

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight commented Nov 18, 2021

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ColdPaleLight commented Oct 9, 2021 •

edited

Loading

ColdPaleLight Oct 19, 2021 •

edited

Loading