Bug 222758 – WebKit must treat 'webgl' and 'webgl2' as distinct context types

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Kenneth Russell]

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]

<rdar://problem/75327781>

[Kenneth Russell]

Created attachment 427717 [details]
Patch

[EWS]

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 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]

(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]

(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".