This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Avoid sending a 0 DPR to framework #21389
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arbreng
reviewed
Sep 24, 2020
shell/common/shell.cc
Outdated
| @@ -816,6 +816,12 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) { | |||
| FML_DCHECK(is_setup_); | |||
| FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); | |||
|
|
|||
| if (metrics.device_pixel_ratio <= 0) { | |||
There was a problem hiding this comment.
I think this should also check the width and height for positivity, or a 2nd check could handle that. Otherwise, there seems to be nothing in the engine guarding against negative or 0 dimensions
A new test case below can also verify the width/height behavior
There was a problem hiding this comment.
IIRC a zero sized viewport just disengages the animator and is a valid case. That is also the initial viewport size.
There was a problem hiding this comment.
Done, and just updated the same test case.
There was a problem hiding this comment.
It is the initial size, but I'm not seeing where it disengages the animator.
What would it mean to actually set the viewport to zero width or height?
There was a problem hiding this comment.
Hmm, you're right. I thought Shell::OnPlatformViewSetViewportMetrics used to pause the animator when the size was zero. That doesn't not seem to be the case.
There was a problem hiding this comment.
The Engine skips rendering the generated LayerTree here: https://github.com/flutter/engine/blob/master/shell/common/engine.cc#L483
Perhaps there should be a DLOG there, or here in the shell
There was a problem hiding this comment.
Also note that LayerTree has another DCHECK for non-zero DPR: https://github.com/flutter/engine/blob/master/flow/layers/layer_tree.cc#L20
There was a problem hiding this comment.
That one's a CHECK. That'll just crash the whole process in release mode. It's probably .. ok to crash there rather than later.
arbreng
approved these changes
Sep 24, 2020
arbreng
reviewed
Sep 24, 2020
chinmaygarde
approved these changes
Sep 24, 2020
dnfield
added
waiting for tree to go green
|
I'm merging this PR since engine is green (the top line: https://ci.chromium.org/p/flutter/g/engine/console |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 25, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 25, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 25, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 26, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 27, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 27, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 27, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 27, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 27, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 27, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 28, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 28, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 28, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 28, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 28, 2020
dnfield
pushed a commit
to flutter/flutter
that referenced
this pull request
Sep 28, 2020
* a1db2b3 Roll Fuchsia Linux SDK from NeYDIjo58... to BFLXvCMVi... (flutter/engine#21403) * faeff0a Roll Dart SDK from eb8e6232da02 to 13b3f2d7b6ea (3 revisions) (flutter/engine#21407) * 7dfcde1 [Fix] Replaces call to deprecated method Name.name. (flutter/engine#21241) * 48ab5d1 Roll Skia from 1748c6a3b8c8 to 3b88c0772e89 (1 revision) (flutter/engine#21410) * aa8d5d4 Avoid sending a 0 DPR to framework (flutter/engine#21389) * 67fdd7e Embedder API Support for display settings (flutter/engine#21355) * 1fc87c0 Roll Skia from 3b88c0772e89 to d7ab45027877 (1 revision) (flutter/engine#21411) * de5f2b4 Revert "Revert "Adds fuchsia node roles to accessibility bridge updates. (#20385)" (#20936)" (flutter/engine#21367) * c9b40c6 Remove ASCII art from mDNS error log (flutter/engine#21397) * fed0531 Roll Fuchsia Mac SDK from xnB_uJM8T... to _e0onA6gY... (flutter/engine#21414) * d877b83 [web] enable ios safari screenshot tests (flutter/engine#21226) * a8f52e5 Roll Skia from d7ab45027877 to aeae3a58e3da (6 revisions) (flutter/engine#21415) * 8275609 Roll Skia from aeae3a58e3da to 7bd60430299f (1 revision) (flutter/engine#21417) * 65c1122 Roll Dart SDK from 13b3f2d7b6ea to 4fb134a228c7 (2 revisions) (flutter/engine#21419) * d735f2c Roll Fuchsia Linux SDK from BFLXvCMVi... to XcAUWQUZm... (flutter/engine#21420) * f1961e5 Roll Skia from 7bd60430299f to 68861e391313 (14 revisions) (flutter/engine#21421) * 08cf725 Fix getNextFrame (flutter/engine#21422) * db99912 Support dragging native platform views (flutter/engine#21396) * df83e8f Roll Skia from 68861e391313 to a05d27b170ee (1 revision) (flutter/engine#21424) * fc7d0fc [web] Respond with null for unimplemented method channels (flutter/engine#21423) * 02b8567 Roll Skia from a05d27b170ee to 5e1545fa00c8 (1 revision) (flutter/engine#21425) * 6e54e68 Roll Dart SDK from 4fb134a228c7 to db7eb2549480 (1 revision) (flutter/engine#21426) * b0cd7d1 Roll Dart SDK from db7eb2549480 to 200e8da5072a (1 revision) (flutter/engine#21427) * 0de5c0c Roll Fuchsia Linux SDK from XcAUWQUZm... to 0nW5DAxcC... (flutter/engine#21430) * 854943d [macOS] Set the display refresh rate (flutter/engine#21095) * 11aecf4 Roll Skia from 5e1545fa00c8 to 766eeb2ac325 (1 revision) (flutter/engine#21431) * b8e2b3f Roll Skia from 766eeb2ac325 to 5648572f4a94 (1 revision) (flutter/engine#21433) * 33d0bbb Roll Fuchsia Mac SDK from _e0onA6gY... to SUSVNJcX5... (flutter/engine#21434) * 51049d2 Roll Skia from 5648572f4a94 to eabce08bb2f2 (1 revision) (flutter/engine#21435) * a491533 Roll Dart SDK from 200e8da5072a to c938793e2d6f (1 revision) (flutter/engine#21437) * 06398b8 Roll Fuchsia Linux SDK from 0nW5DAxcC... to xdxm8rU8b... (flutter/engine#21439) * 4282bbc Roll Dart SDK from c938793e2d6f to fe83360d3a7c (1 revision) (flutter/engine#21440) * eba7b8e Roll Fuchsia Mac SDK from SUSVNJcX5... to v5Ko06GkT... (flutter/engine#21441) * 249bcf7 Roll Fuchsia Linux SDK from xdxm8rU8b... to ej-CkfSra... (flutter/engine#21443) * 4422ede Roll Fuchsia Mac SDK from v5Ko06GkT... to k_lSjZxIH... (flutter/engine#21445) * 35fa4bb Roll Dart SDK from fe83360d3a7c to 44e4f3958019 (1 revision) (flutter/engine#21446) * a9b5b13 Roll Fuchsia Linux SDK from ej-CkfSra... to HNNs4gfuM... (flutter/engine#21447) * 4f7ff21 Roll Skia from eabce08bb2f2 to ad6aeace6eee (2 revisions) (flutter/engine#21448) * f72613d Roll Dart SDK from 44e4f3958019 to e2a4eaba73b8 (1 revision) (flutter/engine#21451) * 2917a65 [ios] Remove unused is_valid_ from IOS Metal Context (flutter/engine#21432) * b591674 Roll Dart SDK from e2a4eaba73b8 to 13deada5b267 (1 revision) (flutter/engine#21453) * a07e0e0 Roll Fuchsia Mac SDK from k_lSjZxIH... to qyoO7f9Sk... (flutter/engine#21454) * cf1fbf2 Apply dpr transform to fuchsia accessibility bridge (flutter/engine#21364) * 9db9a57 Revert "Apply dpr transform to fuchsia accessibility bridge (#21364)" (flutter/engine#21458) * 8d165fa Revert multiple display support for embedder API (flutter/engine#21456)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fuchsia has been observed to sometimes send a 0 DPR to the framework. We had FML_DCHECKs that might have caught this, but it was a timing/race related issue. Removing the DCHECKs so we can test that if someone does manager to do this at runtime it won't send the bad value to the framework.
Related Issues
https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=60299#c5
Tests
I added the following tests:
Test that setting the DPR to zero doesn't trigger onMetricsChanged for Window.