Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3630458
Impl
dkwingsmt Jun 30, 2023
d0a43cd
Impl
dkwingsmt Jun 30, 2023
3d86303
Fix doc
dkwingsmt Jul 5, 2023
1a69ee9
Move get_window to cc
dkwingsmt Jul 5, 2023
a293b8e
Fix doc
dkwingsmt Jul 6, 2023
7699ee8
Better doc
dkwingsmt Jul 6, 2023
1c57271
Merge remote-tracking branch 'origin/main' into multiview-window-metrics
dkwingsmt Jul 6, 2023
85adee8
Add view update test
dkwingsmt Jul 6, 2023
ad0a0d6
Extractg get_window
dkwingsmt Jul 6, 2023
d397d55
Fix test
dkwingsmt Jul 6, 2023
57e705e
Back to current
dkwingsmt Jul 7, 2023
1dfe95d
Merge branch 'rename-default-to-implicit' into multiview-window-metrics
dkwingsmt Jul 7, 2023
13764d7
More default->implicit
dkwingsmt Jul 7, 2023
e49b875
Merge remote-tracking branch 'origin/main' into multiview-window-metrics
dkwingsmt Jul 7, 2023
a71e5b5
Update FlutterPluginRegistrarMacOS.h
dkwingsmt Jul 7, 2023
6517132
Format
dkwingsmt Jul 7, 2023
14f5094
Merge remote-tracking branch 'dkwingsmt/multiview-window-metrics' int…
dkwingsmt Jul 7, 2023
bc67089
Merge remote-tracking branch 'origin/main' into rename-default-to-imp…
dkwingsmt Jul 7, 2023
2aaabb8
Merge remote-tracking branch 'dkwingsmt/rename-default-to-implicit' i…
dkwingsmt Jul 7, 2023
19a5e69
Merge branch 'rename-default-to-implicit' into multiview-window-metrics
dkwingsmt Jul 7, 2023
19f24b2
Update parameter docs
dkwingsmt Jul 7, 2023
73790ca
Fix format for fuchsia
dkwingsmt Jul 7, 2023
4b57af3
Address feedback
dkwingsmt Jul 7, 2023
f3cb395
Add test IgnoresMetricsUpdateToInvalidView
dkwingsmt Jul 7, 2023
03e4b53
Merge remote-tracking branch 'origin/main' into multiview-window-metrics
dkwingsmt Jul 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/ui/window/platform_configuration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,15 @@ void PlatformConfiguration::ReportTimings(std::vector<int64_t> timings) {
}));
}

Window* PlatformConfiguration::get_window(int window_id) {
auto found = windows_.find(window_id);
if (found != windows_.end()) {
return found->second.get();
} else {
return nullptr;
}
}

void PlatformConfiguration::CompletePlatformMessageEmptyResponse(
int response_id) {
if (!response_id) {
Expand Down
5 changes: 3 additions & 2 deletions lib/ui/window/platform_configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -413,9 +413,10 @@ class PlatformConfiguration final {
///
/// @param[in] window_id The id of the window to find and return.
///
/// @return a pointer to the Window.
/// @return a pointer to the Window. Nullptr if the ID is not registered
/// yet.
Copy link
Member

Choose a reason for hiding this comment

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

Do you remember what our plan is for Canvas::drawShadow? It uses the implicit view's device pixel ratio:

SkScalar dpr = static_cast<float>(UIDartState::Current()
->platform_configuration()
->get_window(0)
->viewport_metrics()
.device_pixel_ratio);

If the implicit view is disabled, this will crash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, I totally forgot. I'll open an issue to discuss this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

///
Window* get_window(int window_id) { return windows_[window_id].get(); }
Window* get_window(int window_id);

//----------------------------------------------------------------------------
/// @brief Responds to a previous platform message to the engine from the
Expand Down
53 changes: 47 additions & 6 deletions lib/ui/window/platform_configuration_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
namespace flutter {
namespace testing {

TEST_F(ShellTest, PlatformConfigurationInitialization) {
class PlatformConfigurationTest : public ShellTest {};

TEST_F(PlatformConfigurationTest, Initialization) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();

auto nativeValidateConfiguration = [message_latch](
Expand Down Expand Up @@ -63,7 +65,7 @@ TEST_F(ShellTest, PlatformConfigurationInitialization) {
DestroyShell(std::move(shell), task_runners);
}

TEST_F(ShellTest, PlatformConfigurationWindowMetricsUpdate) {
TEST_F(PlatformConfigurationTest, WindowMetricsUpdate) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();

auto nativeValidateConfiguration = [message_latch](
Expand Down Expand Up @@ -113,7 +115,46 @@ TEST_F(ShellTest, PlatformConfigurationWindowMetricsUpdate) {
DestroyShell(std::move(shell), task_runners);
}

TEST_F(ShellTest, PlatformConfigurationOnErrorHandlesError) {
TEST_F(PlatformConfigurationTest, RegularWindowIsUnavailableAtStartup) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();

auto nativeValidateConfiguration =
[message_latch](Dart_NativeArguments args) {
PlatformConfiguration* configuration =
UIDartState::Current()->platform_configuration();

ASSERT_EQ(configuration->get_window(1), nullptr);
ASSERT_EQ(configuration->get_window(2), nullptr);

message_latch->Signal();
};

Settings settings = CreateSettingsForFixture();
TaskRunners task_runners("test", // label
GetCurrentTaskRunner(), // platform
CreateNewThread(), // raster
CreateNewThread(), // ui
CreateNewThread() // io
);

AddNativeCallback("ValidateConfiguration",
CREATE_NATIVE_ENTRY(nativeValidateConfiguration));

std::unique_ptr<Shell> shell = CreateShell(settings, task_runners);

ASSERT_TRUE(shell->IsSetup());
auto run_configuration = RunConfiguration::InferFromSettings(settings);
run_configuration.SetEntrypoint("validateConfiguration");

shell->RunEngine(std::move(run_configuration), [&](auto result) {
ASSERT_EQ(result, Engine::RunStatus::Success);
});

message_latch->Wait();
DestroyShell(std::move(shell), task_runners);
}

TEST_F(PlatformConfigurationTest, OnErrorHandlesError) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
bool did_throw = false;

Expand Down Expand Up @@ -159,7 +200,7 @@ TEST_F(ShellTest, PlatformConfigurationOnErrorHandlesError) {
DestroyShell(std::move(shell), task_runners);
}

TEST_F(ShellTest, PlatformConfigurationOnErrorDoesNotHandleError) {
TEST_F(PlatformConfigurationTest, OnErrorDoesNotHandleError) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
std::string ex;
std::string st;
Expand Down Expand Up @@ -211,7 +252,7 @@ TEST_F(ShellTest, PlatformConfigurationOnErrorDoesNotHandleError) {
DestroyShell(std::move(shell), task_runners);
}

TEST_F(ShellTest, PlatformConfigurationOnErrorThrows) {
TEST_F(PlatformConfigurationTest, OnErrorThrows) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
std::vector<std::string> errors;
size_t throw_count = 0;
Expand Down Expand Up @@ -266,7 +307,7 @@ TEST_F(ShellTest, PlatformConfigurationOnErrorThrows) {
DestroyShell(std::move(shell), task_runners);
}

TEST_F(ShellTest, PlatformConfigurationSetDartPerformanceMode) {
TEST_F(PlatformConfigurationTest, SetDartPerformanceMode) {
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
auto finish = [message_latch](Dart_NativeArguments args) {
// call needs to happen on the UI thread.
Expand Down
19 changes: 13 additions & 6 deletions runtime/runtime_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

namespace flutter {

const uint64_t kFlutterDefaultViewId = 0llu;
constexpr uint64_t kFlutterImplicitViewId = 0ll;

RuntimeController::RuntimeController(RuntimeDelegate& p_client,
const TaskRunners& task_runners)
Expand Down Expand Up @@ -115,7 +115,10 @@ std::unique_ptr<RuntimeController> RuntimeController::Clone() const {
}

bool RuntimeController::FlushRuntimeStateToIsolate() {
return SetViewportMetrics(platform_data_.viewport_metrics) &&
// TODO(dkwingsmt): Needs a view ID here (or platform_data should probably
// have multiple view metrics).
return SetViewportMetrics(kFlutterImplicitViewId,
platform_data_.viewport_metrics) &&
SetLocales(platform_data_.locale_data) &&
SetSemanticsEnabled(platform_data_.semantics_enabled) &&
SetAccessibilityFeatures(
Expand All @@ -125,13 +128,17 @@ bool RuntimeController::FlushRuntimeStateToIsolate() {
SetDisplays(platform_data_.displays);
}

bool RuntimeController::SetViewportMetrics(const ViewportMetrics& metrics) {
bool RuntimeController::SetViewportMetrics(int64_t view_id,
const ViewportMetrics& metrics) {
TRACE_EVENT0("flutter", "SetViewportMetrics");
platform_data_.viewport_metrics = metrics;

if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) {
platform_configuration->get_window(0)->UpdateWindowMetrics(metrics);
return true;
Window* window = platform_configuration->get_window(view_id);
if (window) {
window->UpdateWindowMetrics(metrics);
return true;
}
}

return false;
Expand Down Expand Up @@ -320,7 +327,7 @@ void RuntimeController::ScheduleFrame() {
// |PlatformConfigurationClient|
void RuntimeController::Render(Scene* scene) {
// TODO(dkwingsmt): Currently only supports a single window.
int64_t view_id = kFlutterDefaultViewId;
int64_t view_id = kFlutterImplicitViewId;
auto window =
UIDartState::Current()->platform_configuration()->get_window(view_id);
if (window == nullptr) {
Expand Down
2 changes: 1 addition & 1 deletion runtime/runtime_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class RuntimeController : public PlatformConfigurationClient {
///
/// @return If the window metrics were forwarded to the running isolate.
///
bool SetViewportMetrics(const ViewportMetrics& metrics);
bool SetViewportMetrics(int64_t view_id, const ViewportMetrics& metrics);

//----------------------------------------------------------------------------
/// @brief Forward the specified display metrics to the running isolate.
Expand Down
5 changes: 3 additions & 2 deletions shell/common/engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,9 @@ tonic::DartErrorHandleType Engine::GetUIIsolateLastError() {
return runtime_controller_->GetLastError();
}

void Engine::SetViewportMetrics(const ViewportMetrics& metrics) {
runtime_controller_->SetViewportMetrics(metrics);
void Engine::SetViewportMetrics(int64_t view_id,
const ViewportMetrics& metrics) {
runtime_controller_->SetViewportMetrics(view_id, metrics);
ScheduleFrame();
}

Expand Down
2 changes: 1 addition & 1 deletion shell/common/engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate {
///
/// @param[in] metrics The metrics
///
void SetViewportMetrics(const ViewportMetrics& metrics);
void SetViewportMetrics(int64_t view_id, const ViewportMetrics& metrics);

//----------------------------------------------------------------------------
/// @brief Updates the display metrics for the currently running Flutter
Expand Down
5 changes: 4 additions & 1 deletion shell/common/engine_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
namespace flutter {

namespace {
constexpr int64_t kDefaultViewId = 0ll;

class MockDelegate : public Engine::Delegate {
public:
MOCK_METHOD2(OnEngineUpdateSemantics,
Expand Down Expand Up @@ -330,7 +332,8 @@ TEST_F(EngineTest, SpawnResetsViewportMetrics) {
const double kViewHeight = 1024;
old_viewport_metrics.physical_width = kViewWidth;
old_viewport_metrics.physical_height = kViewHeight;
mock_runtime_controller->SetViewportMetrics(old_viewport_metrics);
mock_runtime_controller->SetViewportMetrics(kDefaultViewId,
old_viewport_metrics);
Copy link
Member

Choose a reason for hiding this comment

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

This test uses PlatformData.viewport_metrics to check the metrics, which appears to be a single-view assumption. Should we update PlatformData as part of this change? I'm fine with punting this until later, but I'd consider adding a TODO with a tracking issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I added a TODO at RuntimeController::FlushRuntimeStateToIsolate. I can add one more here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I don't think an extra one is needed here. PlatformData.viewport_metrics is definitely something we need to change in the future, which will for sure affect this test. The TODO I mentioned above should be enough.

Copy link
Member

Choose a reason for hiding this comment

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

Could you put a TODO on PlatformData::viewport_metrics as well? Skipping the TODO here sounds good though 👍

auto engine = std::make_unique<Engine>(
/*delegate=*/delegate_,
/*dispatcher_maker=*/dispatcher_maker_,
Expand Down
5 changes: 3 additions & 2 deletions shell/common/platform_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ void PlatformView::SetAccessibilityFeatures(int32_t flags) {
delegate_.OnPlatformViewSetAccessibilityFeatures(flags);
}

void PlatformView::SetViewportMetrics(const ViewportMetrics& metrics) {
delegate_.OnPlatformViewSetViewportMetrics(metrics);
void PlatformView::SetViewportMetrics(int64_t view_id,
const ViewportMetrics& metrics) {
delegate_.OnPlatformViewSetViewportMetrics(view_id, metrics);
}

void PlatformView::NotifyCreated() {
Expand Down
3 changes: 2 additions & 1 deletion shell/common/platform_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class PlatformView {
/// @param[in] metrics The updated viewport metrics.
///
virtual void OnPlatformViewSetViewportMetrics(
int64_t view_id,
const ViewportMetrics& metrics) = 0;

//--------------------------------------------------------------------------
Expand Down Expand Up @@ -474,7 +475,7 @@ class PlatformView {
///
/// @param[in] metrics The updated viewport metrics.
///
void SetViewportMetrics(const ViewportMetrics& metrics);
void SetViewportMetrics(int64_t view_id, const ViewportMetrics& metrics);

//----------------------------------------------------------------------------
/// @brief Used by embedders to notify the shell that a platform view
Expand Down
7 changes: 4 additions & 3 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,8 @@ void Shell::OnPlatformViewScheduleFrame() {
}

// |PlatformView::Delegate|
void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {
void Shell::OnPlatformViewSetViewportMetrics(int64_t view_id,
const ViewportMetrics& metrics) {
FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

Expand All @@ -978,9 +979,9 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {
});

task_runners_.GetUITaskRunner()->PostTask(
[engine = engine_->GetWeakPtr(), metrics]() {
[engine = engine_->GetWeakPtr(), metrics, view_id]() {
Copy link
Member

Choose a reason for hiding this comment

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

Consider swapping captures to match argument order:

Suggested change
[engine = engine_->GetWeakPtr(), metrics, view_id]() {
[engine = engine_->GetWeakPtr(), view_id, metrics]() {

I don't feel strongly about this.

if (engine) {
engine->SetViewportMetrics(metrics);
engine->SetViewportMetrics(view_id, metrics);
}
});

Expand Down
1 change: 1 addition & 0 deletions shell/common/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ class Shell final : public PlatformView::Delegate,

// |PlatformView::Delegate|
void OnPlatformViewSetViewportMetrics(
int64_t view_id,
const ViewportMetrics& metrics) override;

// |PlatformView::Delegate|
Expand Down
6 changes: 4 additions & 2 deletions shell/common/shell_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
namespace flutter {
namespace testing {

constexpr int64_t kDefaultViewId = 0;

ShellTest::ShellTest()
: thread_host_("io.flutter.test." + GetCurrentTestName() + ".",
ThreadHost::Type::Platform | ThreadHost::Type::IO |
Expand Down Expand Up @@ -144,7 +146,7 @@ void ShellTest::SetViewportMetrics(Shell* shell, double width, double height) {
shell->GetTaskRunners().GetUITaskRunner()->PostTask(
[&latch, engine = shell->weak_engine_, viewport_metrics]() {
if (engine) {
engine->SetViewportMetrics(viewport_metrics);
engine->SetViewportMetrics(kDefaultViewId, viewport_metrics);
const auto frame_begin_time = fml::TimePoint::Now();
const auto frame_end_time =
frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0);
Expand Down Expand Up @@ -186,7 +188,7 @@ void ShellTest::PumpOneFrame(Shell* shell,
fml::AutoResetWaitableEvent latch;
shell->GetTaskRunners().GetUITaskRunner()->PostTask(
[&latch, engine = shell->weak_engine_, viewport_metrics]() {
engine->SetViewportMetrics(viewport_metrics);
engine->SetViewportMetrics(kDefaultViewId, viewport_metrics);
const auto frame_begin_time = fml::TimePoint::Now();
const auto frame_end_time =
frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0);
Expand Down
Loading