[reland] Unify canvas creation and Surface code in Skwasm and CanvasKit#179473
Conversation
github-actions
bot
added
engine
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a significant and well-executed refactoring of the web engine's rendering layer. Unifying the Surface and Rasterizer implementations and introducing the CanvasProvider abstraction greatly improves the architecture's modularity and maintainability. It's great to see that this work also brings proper WebGL context loss handling to the Skwasm backend. The addition of new tests for context loss is also a very valuable contribution. I have a couple of comments on potential issues I've found.
| final DomEventListener eventListener = createDomEventListener((DomEvent event) { | ||
| onContextLost(); | ||
| // The canvas is no longer usable. | ||
| releaseCanvas(canvas); | ||
| }); |
There was a problem hiding this comment.
The webglcontextlost event listener is missing a call to event.preventDefault(). According to the WebGL specification, if preventDefault() is not called on this event, the browser will not attempt to restore the context, and the webglcontextrestored event will not be fired. This will break WebGL context restoration for the CanvasKit renderer.
| final DomEventListener eventListener = createDomEventListener((DomEvent event) { | |
| onContextLost(); | |
| // The canvas is no longer usable. | |
| releaseCanvas(canvas); | |
| }); | |
| final DomEventListener eventListener = createDomEventListener((DomEvent event) { | |
| event.preventDefault(); | |
| onContextLost(); | |
| // The canvas is no longer usable. | |
| releaseCanvas(canvas); | |
| }); |
There was a problem hiding this comment.
We don't need to call preventDefault() because we don't want the browser to attempt to restore the context. In the case the context is lost, we recreate it ourselves with a new Canvas
engine/src/flutter/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/surface.dart
Show resolved
Hide resolved
| /// NOTE: `dom.DomOffscreenCanvas` is not fully defined in the provided `dom.dart` | ||
| /// snippet. This implementation assumes it exists and has a similar API to the | ||
| /// standard `OffscreenCanvas`. |
There was a problem hiding this comment.
I'm not sure what this comment means. If there are missing things in dom.dart we should fill the gaps.
| } | ||
|
|
||
| @override | ||
| bool get isDisposed => debugDisposed; |
There was a problem hiding this comment.
If debugDisposed is now meant to be used in production code, maybe let's rename it to _isDisposed and use the new isDisposed in tests?
Yes, it should be merged with this PR after it lands. |
harryterkelsen
force-pushed
the
unify-surface-skwasm-canvaskit
branch
from
0bb0a90 to
c04c46c
Compare
github-actions
bot
added
a: tests
github-actions
bot
added
framework
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
|
A reason for requesting a revert of flutter/flutter/179473 could |
|
Reason for revert: Potentially causing flaky skwasm framework tests: https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20web_skwasm_tests_1/9062/overview |
3 participants
This PR introduces a significant refactoring of the web engine's rendering layer by unifying the Surface and Rasterizer implementations. These components have been moved from being renderer-specific to a generic compositing directory, making the architecture more modular and easier to maintain. The rasterizers are now renderer-agnostic and are provided with renderer-specific surface factories via dependency injection. A new CanvasProvider abstraction has also been introduced to manage the lifecycle of the underlying canvas elements.
A key outcome of this work is that the Skwasm backend now correctly handles WebGL context loss events. This was achieved by refactoring SkwasmSurface to allow the Dart side to manage the OffscreenCanvas lifecycle. A communication channel between the main thread and the web worker is now used to gracefully handle context loss and recovery. This effort also included fixing several related bugs around surface sizing, resource cleanup, and callback handling in multi-surface scenarios.
To validate these changes, new testing APIs have been added to allow for the creation of renderer-agnostic surface tests. A new test file, surface_context_lost_test.dart, has been added to verify the context loss and recovery behavior across all supported renderers, ensuring the new architecture is robust and reliable.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.