Skip to content

Conversation

@JoeOsborn
Copy link
Contributor

Fixes type error in #22943 when creating a context with explicit swap control.

@JoeOsborn
Copy link
Contributor Author

I added a test that fails without the patch and passes with the patch.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I guess this feature simply never worked? Or maybe it worked at some point in history and bitrotted? (@juj WDYT?)

Thanks for working on this!

@@ -0,0 +1,27 @@
// Copyright 2016 The Emscripten Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2024

@sbc100
Copy link
Collaborator

sbc100 commented Nov 21, 2024

It looks like this code was added back in 1b27891.

It looks like in that original patch this code would have worked because offscreenCanvases was DOM ID -> OffscreenCanvas mappings of <canvas> elements that have their rendering control transferred to offscreen. i.e. the values in the map were canvas elements.

It looks like the breaking change was in 736565e where the values in offscreenCanvases changed to be custom JS object. So I guess this combination of settings was been broken since 2019?

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

What do you think @juj?

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JoeOsborn !

@kripken kripken enabled auto-merge (squash) November 21, 2024 18:21
@parameterized({
'offscreencanvas': (['-sOFFSCREENCANVAS_SUPPORT'],),
'offscreenframebuffer': (['-sOFFSCREEN_FRAMEBUFFER', '-DUSE_OFFSCREEN_FRAMEBUFFER'],),
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flake8 doesn't like this }). It should be indented to match the @parameterized above.

Sorry for all the back a forth, I think this will be good to go after this fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I fixed this now)

@JoeOsborn
Copy link
Contributor Author

Thank you!

@kripken
Copy link
Member

kripken commented Nov 21, 2024

Hmm, the code size failures here seem unrelated. @sbc100 was there a recent update there?

@sbc100
Copy link
Collaborator

sbc100 commented Nov 21, 2024

Hmm, the code size failures here seem unrelated. @sbc100 was there a recent update there?

Yup, I just updated the expectation. Rebasing should fix those failures.

@kripken kripken merged commit b3edd4c into emscripten-core:main Nov 22, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants