Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Remove dlCanvasRecorder from flutter::PictureRecorder #38127

Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Dec 7, 2022

Fixes: flutter/flutter#116670

The Paragraph code was the last known use of the DisplayListCanvasRecorder that was held by the PictureRecorder/Canvas objects. Now that the text code talks directly to the DisplayListBuilder, this vestigial object can be removed in favor of allocating a DisplayListBuilder directly.

This is a deletion of code paths that are no longer used so no new unit tests are needed as long as it passes existing tests.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@jason-simmons
Copy link
Member

The old SkCanvas-based rendering path was needed in order to support Libtxt, which was still available as a backup option.

But we're planning to delete Libtxt soon, so that isn't really an issue anymore.

@flar
Copy link
Contributor Author

flar commented Dec 7, 2022

The old SkCanvas-based rendering path was needed in order to support Libtxt, which was still available as a backup option.

But we're planning to delete Libtxt soon, so that isn't really an issue anymore.

Oh, are you saying that we might have had a case where the paint(builder) code would return false?

Should I hold off on this until we can guarantee that? I suppose it is logically not going to happen for now, but is it fully impossible (yet)?

@jason-simmons
Copy link
Member

That code path would be used if the user selected Libtxt instead of SkParagraph.

That isn't an officially supported option, but you can force it by patching the engine or by setting a hidden engine flag.

I wanted to keep this option around until the current release branch was created. But now that it's done I've been planning to remove everything related to Libtxt soon (probably at the start of next year).
We could wait until then to remove the SkCanvas recorder. But I'd also be fine with just doing it now.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 7, 2022
@auto-submit auto-submit bot merged commit 6aa4ccd into flutter:main Dec 7, 2022
flar added a commit that referenced this pull request Dec 8, 2022
auto-submit bot pushed a commit that referenced this pull request Dec 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 9, 2022
…116802)

* bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120)

* dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754)

* 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122)

* 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963)

* 5caef8585 Full implementation of text-input-test (flutter/engine#37986)

* 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865)

* 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080)

* 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078)

* 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118)

* 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127)

* dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112)

* 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135)

* 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115)

* 84abf21d4 Remove autoninja. (flutter/engine#38136)

* 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133)

* 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139)

* 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (#38127)" (flutter/engine#38137)

* b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141)

* 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126)

* 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145)

* 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146)

* 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123)

* 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151)

* 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152)

* cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016)

* aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157)

* 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134)

* 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147)

* 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#116802)

* bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120)

* dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754)

* 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122)

* 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963)

* 5caef8585 Full implementation of text-input-test (flutter/engine#37986)

* 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865)

* 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080)

* 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078)

* 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118)

* 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127)

* dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112)

* 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135)

* 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115)

* 84abf21d4 Remove autoninja. (flutter/engine#38136)

* 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133)

* 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139)

* 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (flutter#38127)" (flutter/engine#38137)

* b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141)

* 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126)

* 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145)

* 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146)

* 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123)

* 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151)

* 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152)

* cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016)

* aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157)

* 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134)

* 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147)

* 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate DisplayListCanvasRecorder from flutter::PictureRecorder
2 participants