-
Notifications
You must be signed in to change notification settings - Fork 6k
Make FlutterEngineGroup support initialRoute #28884
Changes from 7 commits
ae313dd
67b206b
194acc9
651631d
405cfde
8d95bd5
0891de3
ab80c4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -480,6 +480,7 @@ Shell::~Shell() { | |
|
|
||
| std::unique_ptr<Shell> Shell::Spawn( | ||
| RunConfiguration run_configuration, | ||
| const std::string& initial_route, | ||
| const CreateCallback<PlatformView>& on_create_platform_view, | ||
| const CreateCallback<Rasterizer>& on_create_rasterizer) const { | ||
| FML_DCHECK(task_runners_.IsValid()); | ||
|
|
@@ -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)]( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here for std::move. Since this is executing on a separate thread, just explicitly make a copy.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| Engine::Delegate& delegate, | ||
| const PointerDataDispatcherMaker& dispatcher_maker, DartVM& vm, | ||
| fml::RefPtr<const DartSnapshot> isolate_snapshot, | ||
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/initial_route/initial_route= I wish the linter worked for that, doesn't seem to be.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| }, | ||
| is_gpu_disabled)); | ||
| return result; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2695,6 +2695,8 @@ TEST_F(ShellTest, Spawn) { | |
| ASSERT_TRUE(second_configuration.IsValid()); | ||
| second_configuration.SetEntrypoint("testCanLaunchSecondaryIsolate"); | ||
|
|
||
| const std::string initial_route("/foo"); | ||
|
|
||
| fml::AutoResetWaitableEvent main_latch; | ||
| std::string last_entry_point; | ||
| // Fulfill native function for the first Shell's entrypoint. | ||
|
|
@@ -2719,10 +2721,11 @@ TEST_F(ShellTest, Spawn) { | |
|
|
||
| PostSync( | ||
| shell->GetTaskRunners().GetPlatformTaskRunner(), | ||
| [this, &spawner = shell, &second_configuration, &second_latch]() { | ||
| [this, &spawner = shell, &second_configuration, &second_latch, | ||
| &initial_route]() { | ||
| MockPlatformViewDelegate platform_view_delegate; | ||
| auto spawn = spawner->Spawn( | ||
| std::move(second_configuration), | ||
| std::move(second_configuration), initial_route, | ||
| [&platform_view_delegate](Shell& shell) { | ||
| auto result = std::make_unique<MockPlatformView>( | ||
| platform_view_delegate, shell.GetTaskRunners()); | ||
|
|
@@ -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()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/"/foo"/initial_route
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| // TODO(74520): Remove conditional once isolate groups are | ||
| // supported by JIT. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done