RESOLVED FIXED 222758
WebKit must treat 'webgl' and 'webgl2' as distinct context types
https://bugs.webkit.org/show_bug.cgi?id=222758
Summary WebKit must treat 'webgl' and 'webgl2' as distinct context types
Kenneth Russell
Reported 2021-03-04 12:22:35 PST
The wording isn't completely obvious in the specification: https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext but the 'webgl' and 'webgl2' context types must be treated as distinct from each other. If 'webgl' has been fetched for a canvas, then attempting to fetch 'webgl2' must return null. If 'webgl2' has been fetched for a canvas, then attempting to fetch 'webgl' must return null. This is the behavior in Firefox and Chrome. This Emscripten bug covers it: https://github.com/emscripten-core/emscripten/pull/13497 Will add a WebGL conformance test for this at the same time as fixing it in WebKit.
Attachments
Patch (10.07 KB, patch)
2021-05-04 17:54 PDT, Kenneth Russell
no flags
Kenneth Russell
Comment 1 2021-03-10 13:48:02 PST
Haven't started this yet - unassigning until I can, in case someone else can pick it up. Should be a small fix, but important.
Radar WebKit Bug Importer
Comment 2 2021-03-11 12:23:13 PST
Kenneth Russell
Comment 3 2021-05-04 17:54:09 PDT
EWS
Comment 4 2021-05-04 18:23:24 PDT
Committed r276999 (237319@main): <https://commits.webkit.org/237319@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427717 [details].
Darin Adler
Comment 5 2021-05-04 19:57:00 PDT
Comment on attachment 427717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427717&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:266 > + if (version == WebGLVersion::WebGL1 && !m_context->isWebGL1()) > + return Optional<RenderingContext> { WTF::nullopt }; > + if (version != WebGLVersion::WebGL1 && m_context->isWebGL1()) > + return Optional<RenderingContext> { WTF::nullopt }; I would suggest writing this instead: if ((version == WebGLVersion::WebGL1) != m_context->isWebGL1()) return Optional<RenderingContext> { }; > Source/WebCore/html/HTMLCanvasElement.cpp:498 > + if (type == WebGLVersion::WebGL1 && !m_context->isWebGL1()) > + return nullptr; > + > + if (type != WebGLVersion::WebGL1 && m_context->isWebGL1()) > + return nullptr; Same here: if ((type == WebGLVersion::WebGL1) != m_context->isWebGL1()) return nullptr;
Kenneth Russell
Comment 6 2021-05-10 14:35:17 PDT
(In reply to Darin Adler from comment #5) > Comment on attachment 427717 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427717&action=review > > > Source/WebCore/html/HTMLCanvasElement.cpp:266 > > + if (version == WebGLVersion::WebGL1 && !m_context->isWebGL1()) > > + return Optional<RenderingContext> { WTF::nullopt }; > > + if (version != WebGLVersion::WebGL1 && m_context->isWebGL1()) > > + return Optional<RenderingContext> { WTF::nullopt }; > > I would suggest writing this instead: > > if ((version == WebGLVersion::WebGL1) != m_context->isWebGL1()) > return Optional<RenderingContext> { }; > > > Source/WebCore/html/HTMLCanvasElement.cpp:498 > > + if (type == WebGLVersion::WebGL1 && !m_context->isWebGL1()) > > + return nullptr; > > + > > + if (type != WebGLVersion::WebGL1 && m_context->isWebGL1()) > > + return nullptr; > > Same here: > > if ((type == WebGLVersion::WebGL1) != m_context->isWebGL1()) > return nullptr; Thanks for the suggestions. I considered these as well as using the xor operator but thought the code would be more readable as written. Happy to update it if you feel strongly.
Darin Adler
Comment 7 2021-05-10 14:52:50 PDT
(In reply to Kenneth Russell from comment #6) > Thanks for the suggestions. I considered these as well as using the xor > operator but thought the code would be more readable as written. Happy to > update it if you feel strongly. I think using xor is super-unreadable, oblique tricky-programmer Boolean logic. But using != seems *more* direct to me than the multiple if statements and likely *more* readable. Could use names if you feel that makes a big difference: bool versionIsWebGL1 = version == WebGLVersion::WebGL1; if (versionIsWebGL1 != m_context->isWebGL1()) return Optional<RenderingContext> { }; Or the boolean name could be requestingWebGL1. The point of the code, is "if we have the wrong type", which is best coded as "type != the needed type".
Note You need to log in before you can comment on or make changes to this bug.