Drop --enable-display-list flag, remove associated branched code #34233
Conversation
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Yes! This has caused a fair amount of confusion for me.
|
Is the description accurate? It says it is keeping PictureLayer, but the file diffs show it deleted. |
|
I'll update the description - I talked with a couple Fuchsia folks offline and it seems safe to delete that test target. |
| canvas_->drawPicture(picture->picture().get()); | ||
| } | ||
| } else if (picture->display_list()) { | ||
| if (picture->display_list()) { |
There was a problem hiding this comment.
Isn't "display_list_recorder_" always non-null now? The "if (canvas_)" branches should only have ever been used when we were creating SkPictures, but we aren't doing that any more, are we?
There was a problem hiding this comment.
Yes. I'll update canvas.cc
There was a problem hiding this comment.
it will be null after disposal though, so we still need the if guards to avoid segfaulting.
| "rasterizer_unittests.cc", | ||
| "resource_cache_limit_calculator_unittests.cc", | ||
| "shell_unittests.cc", | ||
| "skp_shader_warmup_unittests.cc", |
shell/common/shell_unittests.cc
Outdated
| recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), | ||
| SkPaint(SkColor4f::FromColor(SK_ColorRED))); | ||
| auto sk_picture = recorder.finishRecordingAsPicture(); | ||
| DisplayListCanvasRecorder recorder(SkRect::MakeXYWH(0, 0, 80, 80)); |
There was a problem hiding this comment.
File a follow-up Issue to start converting tests to DLBuilder rather than CanvasRecorder. Eventually we can consider deleting CanvasRecorder, unless we need it for plug ins or embedded views or something.
There was a problem hiding this comment.
shell/common/shell_unittests.cc
Outdated
| SkPaint(SkColor4f::FromColor(SK_ColorRED))); | ||
| return recorder.finishRecordingAsPicture(); | ||
| static sk_sp<DisplayList> MakeSizedDisplayList(int width, int height) { | ||
| DisplayListCanvasRecorder recorder(SkRect::MakeXYWH(0, 0, 80, 80)); |
There was a problem hiding this comment.
Why isn't this used in all of the above tests?
There was a problem hiding this comment.
Done, and updated other tests to use this.
|
Mostly only 1 "oops" and a couple of opportunities for deepening the cut, but feel free to file follow-on issues for the other stuff. Can we get away with no skp warmup tesst? Are those obsolete? |
|
The SKP warmup tests were added for Fuchsia by an engineer who's no longer working on Fuchsia. The engineers who are working on Fuchsia are telling me they don't think we need them. |
dnfield
added
the
autosubmit
|
Nice! |
4 participants
Drops --enable-display-list flag and related code branches
Keeps PictureLayer around for now until we can figure out what to do with the SKP deserialization test harness. @akbiggs may know if that is safe to delete - but I'm fine with doing that in a follow up PR since this one is likely enough as it is to break things.Deletes PictureLayer and a test harness that deserializes SKPs and uses PictureLayers as well.Fixes flutter/flutter#86634
Currently includes #34227
Fixes flutter/flutter#106435