Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delete the skp_shader_warmup_test harness if possible #106435

Closed
dnfield opened this issue Jun 22, 2022 · 4 comments · Fixed by flutter/engine#34233
Closed

Delete the skp_shader_warmup_test harness if possible #106435

dnfield opened this issue Jun 22, 2022 · 4 comments · Fixed by flutter/engine#34233
Assignees
@dnfield
Labels
a: tests "flutter test", flutter_test, or one of our tests c: tech-debt Technical debt, code quality, testing, etc. engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-fuchsia Fuchsia code specifically

Comments

@dnfield
Copy link
Contributor

dnfield commented Jun 22, 2022

This affects Fuchsia. It was added in flutter/engine#19260. It's not clear to me whether Fuchsia still needs this or not (@akbiggs may know).

Right now, it is the only part of the codebase that still needs PictureLayer after dropping the flag for display list.
@dnfield dnfield added a: tests "flutter test", flutter_test, or one of our tests engine flutter/engine repository. See also e: labels. platform-fuchsia Fuchsia code specifically P2 Important issues not at the top of the work list c: tech-debt Technical debt, code quality, testing, etc. labels Jun 22, 2022
@dnfield dnfield mentioned this issue Jun 22, 2022
Merged
@dnfield
Copy link
Contributor Author

dnfield commented Jun 22, 2022

Chatted with a couple Fuchsia folks and it seems like there's likely coverage downstream - it's not clear to me what this coverage is really protecting anyway, so just going to remove it.

@dnfield dnfield self-assigned this Jun 22, 2022
@dnfield
Copy link
Contributor Author

dnfield commented Jun 23, 2022

For

@dnfield dnfield closed this as completed Jun 23, 2022
@dnfield
Copy link
Contributor Author

dnfield commented Jun 23, 2022

For future me: the test harness was using picture layers, but the acutal impl just directly interacts with SkSurfaces it creates: https://github.com/flutter/engine/blob/e986f43abf1096f5a5af3270c134d17eb9b18dd9/shell/platform/fuchsia/flutter/engine.cc#L924-L954

If we really need a test harness for this, we should add one back that's fuchsia specific and uses the same method as Fuchsia.

@github-actions
Copy link

github-actions bot commented Jul 7, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dnfield dnfield

Labels
a: tests "flutter test", flutter_test, or one of our tests c: tech-debt Technical debt, code quality, testing, etc. engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-fuchsia Fuchsia code specifically
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Drop --enable-display-list flag, remove associated branched code dnfield/engine
1 participant
@dnfield