Bug 222758

Summary:

WebKit must treat 'webgl' and 'webgl2' as distinct context types

Product:

WebKit

Reporter:

Kenneth Russell <kbr>

Component:

WebGL

Assignee:

Kenneth Russell <kbr>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, changseok, darin, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, jdarpinian, kbr, kkinnunen, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=28786

Bug Depends on:

Bug Blocks:

222812, 225887

Attachments:

Description	Flags
Patch	none

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	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

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

<rdar://problem/75327781>

Kenneth Russell

Comment 3 2021-05-04 17:54:09 PDT

Created attachment 427717 [details] Patch

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.