Fold the calls for FlutterMain into the FlutterEngine constructor #12069
Conversation
| * <p> | ||
| * The first {@code FlutterEngine} instance constructed per process will also load the Flutter | ||
| * native library and start a Dart VM. | ||
| * |
There was a problem hiding this comment.
I think you want another
tag here.
| * A new {@code FlutterEngine} does come with all default system channels attached. | ||
| * <p> | ||
| * The first {@code FlutterEngine} instance constructed per process will also load the Flutter | ||
| * native library and start a Dart VM. |
There was a problem hiding this comment.
Can you describe the time and memory impact of this here?
There was a problem hiding this comment.
Let's loop back to this after I write the website page for this this week-ish. I'd like to keep all these numbers in a centralized place that's easy to update if they change.
| * and {@link FlutterMain#ensureInitializationComplete(io.flutter.view.Context, String[])}. | ||
| */ | ||
| public FlutterEngine(@NonNull Context context) { | ||
| FlutterMain.startInitialization(context); |
There was a problem hiding this comment.
Do these methods need to stay in FlutterMain if we're making this change? I'm not sure where a better place is yet...maybe statically in FlutterEngine, or maybe even instance methods in FlutterEngine, or possibly somewhere else. But I'd like to avoid instances of the new and improved FlutterEngine getting stuck with static dependencies on the unfortunate FlutterMain...
There was a problem hiding this comment.
I don't have a strong opinion. They probably shouldn't be in the FlutterEngine since that code would interact with an entirely different lifecycle as the FlutterEngine so it wouldn't be the right encapsulation. Perhaps a new io.flutter.embedding.something.FlutterLoader. Though looking at the content of FlutterMain, the encapsulation is reasonable so if we do move it, it would just be a file move.
| * Once started, a {@link DartExecutor} cannot be stopped. The associated Dart code will execute | ||
| * until it completes, or until the {@link io.flutter.embedding.engine.FlutterEngine} that owns | ||
| * this {@link DartExecutor} is destroyed. | ||
| * this {@link DartExecutor} is destroyed via {@link io.flutter.embedding.engine.FlutterEngine#destroy()}. |
There was a problem hiding this comment.
We should not comment on dependencies in reverse. DartExecutor does not require that it be used exclusively from a FlutterEngine, nor is it DartExecutor's business when and where a FlutterEngine destroys the DartExecutor.
|
|
||
| /** | ||
| * Starts initialization of the native system. | ||
| * |
There was a problem hiding this comment.
Missing
tags here.
|
|
||
| /** | ||
| * Blocks until initialization of the native system has completed. | ||
| * |
There was a problem hiding this comment.
Missing
tags here.
| @RunWith(AndroidJUnit4.class) | ||
| public class LaunchTest { | ||
| @Test | ||
| public void testEngineLaunch() throws Throwable { |
There was a problem hiding this comment.
Please name tests based on what is being verified. E.g., engineLaunchesNormally instead of testEngineLaunch. We know that everything in here is a test, as indicated by @Test, so "test" is superfluous. With regard to "engine launch", we might choose to test a dozen different details. This test is specifically verifying a normal launch, right?
There was a problem hiding this comment.
sure. Renamed smokeTestEngineLaunch
| import static org.junit.Assert.fail; | ||
|
|
||
| @RunWith(AndroidJUnit4.class) | ||
| public class LaunchTest { |
There was a problem hiding this comment.
Is this an end-to-end test suite, or a component test suite? If it's a component test then we said we'd name this with a ComponentTest suffix, right? Also, this strikes me more as part of an EngineComponentTest. Launching is one of the behaviors of the engine, as a component, right?
There was a problem hiding this comment.
I renamed this EngineLaunchE2ETest.
| ); | ||
| CompletableFuture<Boolean> statusReceived = new CompletableFuture<>(); | ||
|
|
||
| engine.get().getDartExecutor().setMessageHandler( |
There was a problem hiding this comment.
Can you add more comments to the lines in this test? The ones above are pretty good, but this test is doing some nuanced things. I'd prefer that we be safe and comment each step to describe what we're doing and why.
xster
force-pushed
the
fold-flutter-main
branch
from
ebfde16 to
62d0a4f
Compare
|
Any more thoughts on this one? |
|
Discussion offline, try to create a lightweight dependency injector to make it possible to test the FlutterEngine without a real JNI backing while doing this refactor. Also move the logic to a FlutterLoader in the new embedding package and reverse shim back into the FlutterMain APIs. |
xster
force-pushed
the
fold-flutter-main
branch
from
62d0a4f to
c80e083
Compare
|
Ok, altered this PR a bit more. Now there's a FlutterInjector lightweight dependency injector at the top level. Added some tests. FlutterMain is hollowed out and loading and loading related utility classes are moved to io.flutter.embedding.engine.loader. Let the other existing tests use the injector mechanism to override static. |
|
Took over the PR here: |
3 participants
Fixes flutter/flutter#32946
Fixes flutter/flutter#33145
Fixes flutter/flutter#37195
Partially alleviates flutter/flutter#32987
Remove the call mainly because the extra granularity is error-prone boilerplate (flutter/flutter#39675, flutter/flutter#39384, flutter/flutter#39108, flutter/flutter#30332, flutter/flutter#28439) and doesn't really add any degree of control.
On a low end Android device, this means merging in the 40ms call to load the native library with the already 290ms call to create the VM. Controlling when that 40ms call happens isn't useful.
Redundant calls will still work, as will the old embedding. This just allows new code using the new embedding to omit the FlutterMain calls.
Add an AndroidJUnit in-process test in the scenario_app project. It makes sure the real engine boots on a device. Didn't hook into firebase in this PR yet. One step at a time.
Also added some supplemental docs while going through this.