Make FlutterEngineGroup support initialRoute #28884

ColdPaleLight · 2021-09-27T12:15:59Z

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.

gaaclarke · 2021-09-28T20:56:51Z

The way initial route typically works is through the navigation platform channel: https://github.com/gaaclarke/engine/blob/0e76836ea8409e87b553341bcb16f98028faba90/shell/common/engine.cc#L361:L361

Why are creating c++ plumbing to handle initial route instead of using the navigation platform channel?

ColdPaleLight · 2021-09-28T21:12:45Z

The way initial route typically works is through the navigation platform channel: https://github.com/gaaclarke/engine/blob/0e76836ea8409e87b553341bcb16f98028faba90/shell/common/engine.cc#L361:L361

Why are creating c++ plumbing to handle initial route instead of using the navigation platform channel?

Because I want to make sure that the initialRoute has been set up before Shell::RunEngine

engine/shell/common/shell.cc

Line 515 in 20ec34e

result->RunEngine(std::move(run_configuration));

ColdPaleLight · 2021-09-28T21:30:59Z

The initialization and running of the heavyweight engine are separate. So after it is initialized and before running, you have the opportunity to set up the initialRoute through the navigation platform channel.

However, the initialization and running of the lightweight engine are all done in Shell::Spawn, and there is no good time to set the initialRoute through the navigation platform channel.

gaaclarke · 2021-09-28T21:41:17Z

Because I want to make sure that the initialRoute has been set up before Shell::RunEngine

Ahh I see.

Here is where the message is sent:
https://github.com/gaaclarke/engine/blob/9f99d99628ee4a658f4fff206f588592278ed6b0/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm#L468:L468

The flow for typical usage is:

[FlutterEngine runWithEntrypoint:libraryURI:initialRoute:]
  [FlutterEngine createShell:libraryURI:initialRoute:]
    flutter::Shell::Create()
    [FlutterEngine setupShell:withObservatoryPublication:]
      [FlutterEngine setupChannels]
        <send initial route message>
  [FlutterEngine launchEngine:libraryURI:]
    flutter::Shell::RunEngine()

Because spawn is create+launch together you had to insert the ability to specify it in spawn to sandwich it between create and launch. I think that sounds good, I'll look at this again.

gaaclarke

This looks good, another great contribution @ColdPaleLight, thanks. I just have a few nitpicks.

gaaclarke · 2021-09-28T21:43:38Z

shell/common/engine.cc

@@ -133,6 +134,7 @@ std::unique_ptr<Engine> Engine::Spawn(
      settings_.isolate_shutdown_callback,   // isolate shutdown callback
      settings_.persistent_isolate_data      // persistent isolate data
  );
+  result->initial_route_ = std::move(initial_route);


I don't think you want an std::move here. This has to initiate a copy anyways, I think it is just needlessly invalidating the reference.

gaaclarke · 2021-09-28T21:45:58Z

shell/common/shell.cc

@@ -488,7 +489,8 @@ std::unique_ptr<Shell> Shell::Spawn(
        PlatformData{}, task_runners_, rasterizer_->GetRasterThreadMerger(),
        GetSettings(), vm_, vm_->GetVMData()->GetIsolateSnapshot(),
        on_create_platform_view, on_create_rasterizer,
-        [engine = this->engine_.get()](
+        [engine = this->engine_.get(),
+         initial_route = std::move(initial_route)](


Same here for std::move. Since this is executing on a separate thread, just explicitly make a copy.

gaaclarke · 2021-09-28T21:46:25Z

shell/common/shell.cc

@@ -501,7 +503,8 @@ std::unique_ptr<Shell> Shell::Spawn(
          return engine->Spawn(/*delegate=*/delegate,
                               /*dispatcher_maker=*/dispatcher_maker,
                               /*settings=*/settings,
-                               /*animator=*/std::move(animator));
+                               /*animator=*/std::move(animator),
+                               /*initial_route*/ initial_route);


s/initial_route/initial_route=

I wish the linter worked for that, doesn't seem to be.

gaaclarke · 2021-09-28T21:47:24Z

shell/common/shell_unittests.cc

@@ -2740,6 +2743,7 @@ TEST_F(ShellTest, Spawn) {
                   // Check second shell ran the second entrypoint.
                   ASSERT_EQ("testCanLaunchSecondaryIsolate",
                             spawn->GetEngine()->GetLastEntrypoint());
+                   ASSERT_EQ("/foo", spawn->GetEngine()->InitialRoute());


s/"/foo"/initial_route

gaaclarke · 2021-09-28T21:53:24Z

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

@@ -1027,8 +1028,13 @@ - (FlutterEngine*)spawnWithEntrypoint:(/*nullable*/ NSString*)entrypoint
  flutter::Shell::CreateCallback<flutter::Rasterizer> on_create_rasterizer =
      [](flutter::Shell& shell) { return std::make_unique<flutter::Rasterizer>(shell); };

-  std::unique_ptr<flutter::Shell> shell =
-      _shell->Spawn(std::move(configuration), on_create_platform_view, on_create_rasterizer);
+  std::string initial_route;


The naming convention for objc is camel case. It's confusing having initialRoute and initial_route in the same context. I'm not sure if we have an established convention for this but how about initialRoute and cppInitialRoute?

gaaclarke

LGTM, thanks!

fluttergithubbot · 2021-09-29T17:48:05Z

This pull request is not suitable for automatic merging in its current state.

…28884)

Make FlutterEngineGroup support initialRoute

ae313dd

flutter-dashboard bot added platform-android platform-ios labels Sep 27, 2021

google-cla bot added the cla: yes label Sep 27, 2021

ColdPaleLight added 3 commits September 28, 2021 18:47

Add unit tests

67b206b

Add documentation comments

194acc9

Tweak unit tests

651631d

ColdPaleLight changed the title ~~[WIP] Make FlutterEngineGroup support initialRoute~~ Make FlutterEngineGroup support initialRoute Sep 28, 2021

ColdPaleLight added 3 commits September 28, 2021 23:02

Fix crash in iOS unit tests

405cfde

Tweak the code

8d95bd5

Correct the typos

0891de3

gaaclarke suggested changes Sep 28, 2021

View reviewed changes

Tweak the code according comments of review

ab80c4d

ColdPaleLight requested a review from gaaclarke September 29, 2021 06:59

gaaclarke approved these changes Sep 29, 2021

View reviewed changes

gaaclarke 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 Sep 29, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 29, 2021

gaaclarke merged commit 7be0c36 into flutter:master Sep 29, 2021

engine-flutter-autoroll mentioned this pull request Sep 29, 2021

Roll Engine from bd250bdd8178 to e2775928ec64 (16 revisions) flutter/flutter#90980

Merged

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

7be0c36 Make FlutterEngineGroup support initialRoute (flutter/engine#…

b21247e

…28884)

akbiggs pushed a commit to akbiggs/engine that referenced this pull request Oct 1, 2021

Make FlutterEngineGroup support initialRoute (flutter#28884)

86cc1e4

dnfield pushed a commit to dnfield/engine that referenced this pull request Oct 6, 2021

Make FlutterEngineGroup support initialRoute (flutter#28884)

1caf2b4

ColdPaleLight added the customer: alibaba label Jan 6, 2022

Make FlutterEngineGroup support initialRoute #28884

Make FlutterEngineGroup support initialRoute #28884

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

gaaclarke commented Sep 28, 2021

Uh oh!

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

Uh oh!

Uh oh!

ColdPaleLight commented Sep 28, 2021

Uh oh!

gaaclarke commented Sep 28, 2021

Uh oh!

gaaclarke left a comment

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!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Sep 29, 2021

Uh oh!

Uh oh!

ColdPaleLight commented Sep 27, 2021 •

edited

Loading

ColdPaleLight commented Sep 28, 2021 •

edited

Loading