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: | P2 | 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
Kenneth Russell
2021-03-04 12:22:35 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. Created attachment 427717 [details]
Patch
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]. 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; (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. (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". |