Bug 222758

Summary: WebKit must treat 'webgl' and 'webgl2' as distinct context types
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: 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 Flags
Patch none

Description Kenneth Russell 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.
Comment 1 Kenneth Russell 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.
Comment 2 Radar WebKit Bug Importer 2021-03-11 12:23:13 PST
<rdar://problem/75327781>
Comment 3 Kenneth Russell 2021-05-04 17:54:09 PDT
Created attachment 427717 [details]
Patch
Comment 4 EWS 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].
Comment 5 Darin Adler 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;
Comment 6 Kenneth Russell 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.
Comment 7 Darin Adler 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".