This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Check that TransformLayer has a finite matrix #8585
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
To catch issues like flutter/flutter#30586 We'll also upload a test in the framework that will trigger this DCHECK if flutter#8467 were reverted.
| TransformLayer::~TransformLayer() = default; | ||
|
|
||
| void TransformLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) { | ||
| // Checks (in some degree) that SkMatrix transform_ is valid and initialized. |
Member
There was a problem hiding this comment.
Can the TransformLayer constructor initialize the transform_ to the identity matrix?
Another option would be setting a flag when set_transform is called and checking that flag here.
Contributor
Author
There was a problem hiding this comment.
I'll add a flag, together with this isFinite check. The reason that we still need this isFinite check is that someone could call set_transform with an uninitialized SkMatrix.
flow/layers/transform_layer.cc
Outdated
| // | ||
| // We have to write this flaky test because there is no reliable way to test | ||
| // whether a variable is initialized or not in C++. | ||
| FML_DCHECK(transform_.isFinite()); |
Member
There was a problem hiding this comment.
The unopt variant is not used in any serious manner when exercising flow. Having uninitialized stuff in the matrix is quite serious. Maybe just terminate the process here.
Contributor
Author
There was a problem hiding this comment.
I've changed FML_DCHECK to FML_CHECK.
liyuqian
force-pushed
the
check_transform_matrix
branch
from
085d11f to
c1caefd
Compare
Contributor
Author
|
@chinmaygarde @jason-simmons : how does the new patch look to you? |
9 tasks
Member
|
Instead of the |
chinmaygarde
approved these changes
Apr 17, 2019
Member
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
lgtm with the nit about initializing the matrix.
liyuqian
force-pushed
the
check_transform_matrix
branch
from
741cfff to
c592f71
Compare
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 17, 2019
This was referenced Apr 17, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 17, 2019
engine-flutter-autoroll
added a commit
to flutter/flutter
that referenced
this pull request
Apr 17, 2019
flutter/engine@388124f...fdd4272 git log 388124f..fdd4272 --no-merges --oneline fdd4272 Roll src/third_party/skia 70ed05e53ad2..652b007a4cbb (1 commits) (flutter/engine#8601) 3b01610 Check that TransformLayer has a finite matrix (flutter/engine#8585) 4805d72 Implement StandardMethodCodec for C++ shells (flutter/engine#8598) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
liyuqian
added a commit
to liyuqian/flutter
that referenced
this pull request
Apr 18, 2019
As we've introduced offset to the Opacity layer, we have to override `applyTransform` to make Leader/FollowerLayer work correctly. Fixes flutter#30587 Together with flutter/engine#8585, this test will also exercise test against flutter#30586.
liyuqian
added a commit
to flutter/flutter
that referenced
this pull request
Apr 29, 2019
## Description As we've introduced offset to the Opacity layer, we have to override `applyTransform` to make Leader/FollowerLayer work correctly. ## Related Issues Fixes #30587 Together with flutter/engine#8585, this test will also exercise test against #30586. ## Tests I added the following tests: * text field selection toolbar renders correctly inside opacity
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
To catch issues like flutter/flutter#30586
We'll also upload a test in the framework that will trigger this DCHECK
if #8467 were reverted.