Make updating window metrics multi-view #43366

dkwingsmt · 2023-06-30T22:34:00Z

This PR adds multi-view support to various methods that updates the window metrics by adding a view_id parameter.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

loic-sharma · 2023-07-07T00:16:03Z

shell/common/engine_unittests.cc

    old_viewport_metrics.physical_height = kViewHeight;
-    mock_runtime_controller->SetViewportMetrics(old_viewport_metrics);
+    mock_runtime_controller->SetViewportMetrics(kDefaultViewId,
+                                                old_viewport_metrics);


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.

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

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.

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

shell/common/engine_unittests.cc

shell/common/platform_view.h

loic-sharma · 2023-07-07T00:20:37Z

shell/common/shell.cc


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


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.

shell/common/shell_unittests.cc

shell/platform/fuchsia/flutter/flatland_platform_view.cc

shell/testing/tester_main.cc

loic-sharma · 2023-07-07T00:45:46Z

lib/ui/window/platform_configuration.h

  ///
-  /// @return     a pointer to the Window.
+  /// @return     a pointer to the Window. Nullptr if the ID is not registered
+  ///             yet.


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

engine/lib/ui/painting/canvas.cc

Lines 624 to 628 in 1fa222f

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

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

flutter/flutter#130165

lib/ui/window/platform_configuration.cc

…o multiview-window-metrics

…licit

…nto rename-default-to-implicit

dkwingsmt · 2023-07-07T21:39:31Z

@loic-sharma I think I've addressed all of your comments. Would you like to take another look?

loic-sharma · 2023-07-08T00:04:32Z

shell/common/engine.h

-  ///             rendering viewport in texels as well as edge insets if
-  ///             present.
+  /// @brief      Updates the viewport metrics for a view. The viewport metrics
+  ///             detail the size of the rendering viewport in texels as well as


Is texels correct here? It looks like that's a thing but it seems like pixels would make more sense.

This comment was written by @/chinmaygarde. Maybe you can ask him for details? (Since I'm not really familiar with texels either...)

loic-sharma

LGTM, nice work!

auto-submit · 2023-07-08T04:09:29Z

auto label is removed for flutter/engine, pr: 43366, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…130195) flutter/engine@4ca6191...9006633 2023-07-08 [email protected] Make updating window metrics multi-view (flutter/engine#43366) 2023-07-08 [email protected] Rename default views to implicit views (flutter/engine#43364) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR adds multi-view support to various methods that updates the window metrics by adding a `view_id` parameter. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

dkwingsmt added 2 commits June 30, 2023 13:05

Impl

3630458

Impl

d0a43cd

github-actions bot added platform-android platform-fuchsia platform-ios labels Jun 30, 2023

dkwingsmt added 8 commits July 5, 2023 08:45

Fix doc

3d86303

Move get_window to cc

1a69ee9

Fix doc

a293b8e

Better doc

7699ee8

Merge remote-tracking branch 'origin/main' into multiview-window-metrics

1c57271

Add view update test

85adee8

Extractg get_window

ad0a0d6

Fix test

d397d55

dkwingsmt requested review from goderbauer and loic-sharma July 6, 2023 19:55

loic-sharma reviewed Jul 7, 2023

View reviewed changes

goderbauer reviewed Jul 7, 2023

View reviewed changes

lib/ui/window/platform_configuration.cc Show resolved Hide resolved

dkwingsmt and others added 8 commits July 7, 2023 10:41

Back to current

57e705e

Merge branch 'rename-default-to-implicit' into multiview-window-metrics

1dfe95d

More default->implicit

13764d7

Merge remote-tracking branch 'origin/main' into multiview-window-metrics

e49b875

Update FlutterPluginRegistrarMacOS.h

a71e5b5

Format

6517132

Merge remote-tracking branch 'dkwingsmt/multiview-window-metrics' int…

14f5094

…o multiview-window-metrics

Merge remote-tracking branch 'origin/main' into rename-default-to-imp…

bc67089

…licit

github-actions bot added the platform-macos label Jul 7, 2023

dkwingsmt added 4 commits July 7, 2023 11:53

Merge remote-tracking branch 'dkwingsmt/rename-default-to-implicit' i…

2aaabb8

…nto rename-default-to-implicit

Merge branch 'rename-default-to-implicit' into multiview-window-metrics

19a5e69

Update parameter docs

19f24b2

Fix format for fuchsia

73790ca

dkwingsmt added 2 commits July 7, 2023 13:32

Address feedback

4b57af3

Add test IgnoresMetricsUpdateToInvalidView

f3cb395

loic-sharma reviewed Jul 8, 2023

View reviewed changes

loic-sharma approved these changes Jul 8, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into multiview-window-metrics

03e4b53

github-actions bot removed the platform-macos label Jul 8, 2023

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 8, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 8, 2023

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 8, 2023

auto-submit bot merged commit 9006633 into flutter:main Jul 8, 2023

dkwingsmt deleted the multiview-window-metrics branch July 8, 2023 05:54

engine-flutter-autoroll mentioned this pull request Jul 8, 2023

Roll Flutter Engine from 4ca619166c4a to 9006633571bb (2 revisions) flutter/flutter#130195

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2023

900663357 Make updating window metrics multi-view (flutter/engine#43366)

bd413d0

HansMuller mentioned this pull request Jul 21, 2023

Flutter Framework Team: Notable Commits flutter/flutter#121415

Closed

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

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

Make updating window metrics multi-view #43366

Make updating window metrics multi-view #43366

Uh oh!

Conversation

dkwingsmt commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

dkwingsmt commented Jul 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dkwingsmt commented Jun 30, 2023 •

edited

Loading