1002302 - WebGL2 - Stub WebGL 2.0 WebIDL and functions in WebGL2Context

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

Stub out the WebIDL from
https://www.khronos.org/registry/webgl/specs/latest/2.0/ so we have a
baseline to incrementally submit patches against.

Comment 1

[u480271]

Attachment: Patch 1 - WebGL2 - Make WebIDL bind to WebGL2Context instead of WebGLContext.

For WebGL 2 context, bind to WebGL2Context object instead of WebGLContext.

Comment 2

[u480271]

Attachment: Patch 2 - WebGL2 - Stub WebGL 2.0 WebIDL and functions in WebGL2Context.

Implement dummy stubs for all functions defined in the WebIDL.

Comment 3

[u480271]

Attachment: Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL 2.0

Stub out WebGLSampler, WebGLSync, WebGLTransformFeedback,
WebGLVertexArrayObject.

Comment 4

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=d1143223ee13

Comment 5

[(no longer active)]

Please send an intent to implement email to dev-platform as per <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>.  Also, remember that WebIDL changes require review from a DOM peer.

Comment 6

[u480271]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #5)
> Please send an intent to implement email to dev-platform as per
> <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>.  Also, remember that
> WebIDL changes require review from a DOM peer.

Ehsan, who would you suggest as to DOM peer to ask for review?

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #5)
> > Please send an intent to implement email to dev-platform as per
> > <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>.  Also, remember that
> > WebIDL changes require review from a DOM peer.
> 
> Ehsan, who would you suggest as to DOM peer to ask for review?

bz/smaug/jst I guess.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8416299 [details] [diff] [review]
Patch 2 - WebGL2 - Stub WebGL 2.0 WebIDL and functions in WebGL2Context.

Review of attachment 8416299 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGL2Context.h
@@ +83,5 @@
> +    void TexSubImage3D(GLenum target, GLint level,
> +                       GLint xoffset, GLint yoffset, GLint zoffset,
> +                       GLsizei depth,
> +                       GLenum format, GLenum type, ElementType& elt, ErrorResult& rv)
> +    {}

MOZ_CRASH

::: dom/webidl/WebGL2RenderingContext.webidl
@@ +451,5 @@
>      WebGLQuery? getQuery(GLenum target, GLenum pname);
>      any getQueryObject(WebGLQuery? queryObject, GLenum pname);
>      [WebGLHandlesContextLoss] GLboolean isQuery(WebGLQuery? queryObject);
>  
> +    // WebGL 2.0 spec signature follows

Why is this here? Why don't we use this above?

@@ +503,3 @@
>      void bindBufferBase(GLenum target, GLuint index, WebGLBuffer? buffer);
> +    void bindBufferRange(GLenum target, GLuint index, WebGLBuffer? buffer, GLintptr offset, GLsizeiptr size);
> +    //

Spare slashes.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8416300 [details] [diff] [review]
Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL 2.0

Review of attachment 8416300 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGL2Sync.h
@@ +23,5 @@
> +    friend class WebGL2Context;
> +
> +public:
> +
> +    WebGL2Sync(WebGLContext *context);

Stars to the left, please!

Comment 10

[Guillaume Abadie]

Comment on attachment 8416300 [details] [diff] [review]
Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL 2.0

Review of attachment 8416300 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGL2Sampler.h
@@ +14,5 @@
> +#include "mozilla/LinkedList.h"
> +
> +namespace mozilla {
> +
> +class WebGL2Sampler MOZ_FINAL

Why WebGL2Sampler and not WebGLSampler? See WebGLQuery is a WebGL 2.0 (prototype?) object but doesn't have the 2...

Also:
class WebGL1Context : public WebGLContext
class WebGL2Context : public WebGLContext

So it has a sense to remove the 2 from the WebGL 2.0's objects' names...

::: content/canvas/src/WebGL2VertexArrayObject.h
@@ +14,5 @@
> +#include "mozilla/LinkedList.h"
> +
> +namespace mozilla {
> +
> +class WebGL2VertexArrayObject MOZ_FINAL

WebGLVertexArray already exists. Also the "Object" name doesn't have its place here since all other WebGL's classes doesn't have it.

Comment 11

[u480271]

(In reply to Guillaume Abadie from comment #10)
> Comment on attachment 8416300 [details] [diff] [review]
> Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL 2.0
> 
> Review of attachment 8416300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGL2Sampler.h
> > +class WebGL2Sampler MOZ_FINAL
> 
> Why WebGL2Sampler and not WebGLSampler? See WebGLQuery is a WebGL 2.0
> (prototype?) object but doesn't have the 2...

Because I wanted to make it clear in the C++ code that this is a WebGL2 only class. Via the binding conf, I keep the name binding as WebGLSampler to match the spec.

> Also:
> class WebGL1Context : public WebGLContext
> class WebGL2Context : public WebGLContext
> 
> So it has a sense to remove the 2 from the WebGL 2.0's objects' names...

In patches to follow and in a discussion I posted internally, it is planned that WebGLContext becomes a place for shared implementations for WebGL1 and WebGL2 (and future versions). Version specific code (and parameter) checking will go into WebGL1Context or WebGL2Context.

> ::: content/canvas/src/WebGL2VertexArrayObject.h
> @@ +14,5 @@
> > +#include "mozilla/LinkedList.h"
> > +
> > +namespace mozilla {
> > +
> > +class WebGL2VertexArrayObject MOZ_FINAL
> 
> WebGLVertexArray already exists. Also the "Object" name doesn't have its
> place here since all other WebGL's classes doesn't have it.

Because https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.6 calls it WebGLVertexArrayObject.

While we may be able to share the implementation of VAOs, I'm not certain about what we have to do to expose names to DOM. WebGL1 extension OES_vertex_array_object names it WebGLVertexArrayObjectOES and WebGL2 names it WebGLVertexArrayObject. We may have to have these "shim" objects to expose the correct names to JS. Jeff?

Comment 12

[u480271]

Attachment: Patch 1 - WebGL2 - Make WebIDL bind to WebGL2Context instead of WebGLContext.

Refresh patch after content/canvas/src -> dom/canvas move.

Comment 13

[u480271]

Attachment: Patch 2 - WebGL2 - Stub WebGL 2.0 WebIDL and functions in WebGL2Context.

Refresh patch after content/canvas/src -> dom/canvas move.

Comment 14

[u480271]

Attachment: Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL2

WebGLSampler, WebGLSync, WebGLTransformFeedback,
WebGLVertexArrayObject.

Refresh patch after content/canvas/src -> dom/canvas move.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8467493 [details] [diff] [review]
Patch 2 - WebGL2 - Stub WebGL 2.0 WebIDL and functions in WebGL2Context.

Review of attachment 8467493 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGL2ContextQueries.cpp
@@ +10,5 @@
> +using namespace mozilla::dom;
> +
> +// -------------------------------------------------------------------------
> +// Query Objects
> +// TODO(djg): Implemented in WebGLContext

Shouldn't we just not have this file then?

::: dom/webidl/WebGL2RenderingContext.webidl
@@ +422,5 @@
> +    // TODO(djg): Implemented in WebGLContext
> +    void vertexAttribDivisor(GLuint index, GLuint divisor);
> +    void drawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei instanceCount);
> +    void drawElementsInstanced(GLenum mode, GLsizei count, GLenum type, GLintptr offset, GLsizei instanceCount);
> +    //

What's with this extra empty comment line?

@@ +435,2 @@
>      void drawBuffers(sequence<GLenum> buffers);
> +    //

What's with this extra empty comment line?

@@ +452,5 @@
>      any getQueryObject(WebGLQuery? queryObject, GLenum pname);
>      [WebGLHandlesContextLoss] GLboolean isQuery(WebGLQuery? queryObject);
>  
> +    // WebGL 2.0 spec signature follows
> +    /* WebGLQuery? createQuery();

Why is this commented?

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8467495 [details] [diff] [review]
Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL2

Review of attachment 8467495 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Bindings.conf
@@ +1562,5 @@
>  },
>  
> +'WebGLSampler': {
> +    'nativeType': 'mozilla::WebGL2Sampler',
> +    'headerFile': 'WebGL2Sampler.h'

So I'm thinking that these shouldn't have '2's in their names.

Similar to how we have WebGL{1,2}Context being based off WebGLContext, I think we should think of WebGLSamplers as objects that are not implicitly part of WebGL2, but are merely exposed by it.

::: dom/canvas/WebGL2VertexArrayObject.h
@@ +24,5 @@
> +{
> +    friend class WebGLContext;
> +
> +public:
> +

I don't like these blank lines immediately after public/protected/private labels, but I can live with leaving them.

@@ +32,5 @@
> +    WebGLContext* GetParentObject() const;
> +
> +    // -------------------------------------------------------------------------
> +    // IMPLEMENT NS
> +    virtual JSObject* WrapObject(JSContext *cx) MOZ_OVERRIDE;

Stars to left please.

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #11)
> 
> Because https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.6 calls
> it WebGLVertexArrayObject.
> 
> While we may be able to share the implementation of VAOs, I'm not certain
> about what we have to do to expose names to DOM. WebGL1 extension
> OES_vertex_array_object names it WebGLVertexArrayObjectOES and WebGL2 names
> it WebGLVertexArrayObject. We may have to have these "shim" objects to
> expose the correct names to JS. Jeff?

Yeah, we technically need to have them named as indicated.

Behind the scenes, they can be the same, but JS has to think they're different. :\

Comment 18

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 8416299 [details] [diff] [review]
> Patch 2 - WebGL2 - Stub WebGL 2.0 WebIDL and functions in WebGL2Context.
> 
> Review of attachment 8416299 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/webidl/WebGL2RenderingContext.webidl
> @@ +451,5 @@
> >      WebGLQuery? getQuery(GLenum target, GLenum pname);
> >      any getQueryObject(WebGLQuery? queryObject, GLenum pname);
> >      [WebGLHandlesContextLoss] GLboolean isQuery(WebGLQuery? queryObject);
> >  
> > +    // WebGL 2.0 spec signature follows
> 
> Why is this here? Why don't we use this above?

So Guillaume had implemented some of the interfaces in previous work. In some cases the names at slightly different and this was here as a reminder to me.  I'll clean it up.

> @@ +503,3 @@
> >      void bindBufferBase(GLenum target, GLuint index, WebGLBuffer? buffer);
> > +    void bindBufferRange(GLenum target, GLuint index, WebGLBuffer? buffer, GLintptr offset, GLsizeiptr size);
> > +    //
> 
> Spare slashes.

Oh those go with the comment you label "Why is this here?" above.

Comment 19

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> Comment on attachment 8467495 [details] [diff] [review]
> Patch 3 - WebGL2 - Stub out new WebGLObjects for WebGL2
> 
> Review of attachment 8467495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ +1562,5 @@
> >  },
> >  
> > +'WebGLSampler': {
> > +    'nativeType': 'mozilla::WebGL2Sampler',
> > +    'headerFile': 'WebGL2Sampler.h'
> 
> So I'm thinking that these shouldn't have '2's in their names.
> 
> Similar to how we have WebGL{1,2}Context being based off WebGLContext, I
> think we should think of WebGLSamplers as objects that are not implicitly
> part of WebGL2, but are merely exposed by it.

I'm not fussed either way. My thinking was to name it with WebGL2 prefix until we get to a point where we want to share code between different version. At that point we've "push it up" the hierarchy.

This mirrors my thoughts on WebGLContext, WebGL1Context, and WebGL2Context (as shared in emails).

Given the focus on WebGL2, I guess this means that WebGL1 won't be seeing many new extensions. Looking at the current spec+extensions for potential overlap, we have VAOs and Query objects. Since we're unlikely to get sync, samplers, etc. I'm happy to drop the number.

Comment 20

[u480271]

Attachment: Patch 4 - Rename WebGL2Sampler to WebGLSampler

Comment 21

[u480271]

Attachment: Patch 5 - Rename WebGL2Sync to WebGLSync

Comment 22

[u480271]

Attachment: Patch 6 - Rename WebGL2TransformFeedback to WebGLTransformFeedback

Comment 23

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=4073745f7429

error C2220: warning treated as error - no 'object' file generated :-/

Comment 24

[u480271]

Attachment: WebGL2 - Refactor VAOs to allow sharing of code between WebGL 1 and WebGL 2.

Refactor Vertex Array code to allow sharing between WebGL 1 and WebGL
2. To make this easier I removed the split into fake and GL VAOs and
placed that code into the WebGLVertexArray class. I created two shim
classes that match the WebIDL binding names. These classes are there
just to appease the DOM binding code.

Comment 25

[u480271]

Attachment: WebGL2 - Stub WebGL 2.0 WebIDL, WebGL 2.0 Objects, and functions in WebGL2Context.

Rolled up previous patches into one.

Carry r+ from :jgilbert.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8489317 [details] [diff] [review]
WebGL2 - Refactor VAOs to allow sharing of code between WebGL 1 and WebGL 2.

Review of attachment 8489317 [details] [diff] [review]:
-----------------------------------------------------------------

Having some questions / we might need another round depending on the answers.

::: dom/canvas/WebGLVertexArray.cpp
@@ +18,5 @@
>      , WebGLContextBoundObject(context)
>  {
>      SetIsDOMBinding();
> +
> +    if (context->gl->IsSupported(gl::GLFeature::vertex_array_object)) {

After much digging I realized that the reason for this check here is that the split between "fake" and "GL" vertex array objects was moved to here, inside the now-unified WebGLVertexArray class, right? That deserves a comment.

@@ +36,5 @@
> +WebGLVertexArray::BindVertexArray()
> +{
> +    /* Bind to dummy value to signal that this vertex array has
> +       ever been bound */
> +    BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);

I'm not a fan of taking an existing OpenGL concept such as here the GL_VERTEX_ARRAY_BINDING enum constant, and assigning it new meaning that's local to us here. Normally, when a programmer reads our code and sees GL_VERTEX_ARRAY_BINDING they can know right away that we're just querying OpenGL state for the current VAO binding; that quick-reading habit would be broken here.

We used to have mHasEverBeenBound boolean members for that.

@@ +40,5 @@
> +    BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);
> +
> +    if (mGLName) {
> +        mContext->mBoundVertexArray = this;
> +        mContext->gl->fBindVertexArray(mGLName);

The old code unconditionally called glBindVertexArray(mGLName). The new code only calls it if mGLName is nonzero. Do we for some reason never need to go back to the zero binding?

@@ +66,5 @@
> +
> +        mContext->BindBuffer(LOCAL_GL_ARRAY_BUFFER, vd.buf);
> +
> +        gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized, vd.stride,
> +                                 reinterpret_cast<const GLvoid*>(vd.byteOffset));

Casting to [const] void* is only a static_cast.

@@ +79,5 @@
> +    for (size_t i = mAttribs.Length(); i < prevVertexArray->mAttribs.Length(); ++i) {
> +        const WebGLVertexAttribData& vd = prevVertexArray->mAttribs[i];
> +
> +        if (vd.enabled) {
> +            gl->fDisableVertexAttribArray(i);

Is it true for some reason that the attrib set for prevVertexArray has empty intersection with the attrib set in *this?
 - if that's true, please add an explanatory comment
 - if that's not true, then don't we have a bug here, where we're disabling attribs that were enabled just above?

::: dom/canvas/WebGLVertexArray.h
@@ +76,5 @@
>      friend class WebGLContext;
>  };
>  
> +// DOM binding shims - these classes just override WrapObject method
> +class WebGLVertexArrayObjectOES : public WebGLVertexArray

Was Bindings.conf not happy with two different interfaces having the same nativeType?

If that was the problem, should that be filed as a bug against the bindings?

::: dom/canvas/moz.build
@@ +136,5 @@
>  ]
>  
>  CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']
>  CXXFLAGS += CONFIG['TK_CFLAGS']
> +CXXFLAGS += [ '-O0 -g' ]

This unintentionally committed hunk must be removed :-)

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

> > +        gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized, vd.stride,
> > +                                 reinterpret_cast<const GLvoid*>(vd.byteOffset));
> 
> Casting to [const] void* is only a static_cast.

Sorry, didn't realize we were casting integer to pointer. Nevermind.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Bug 1067436 is an assert failure that is caused by an earlier version of the "Refactor VAOs to allow sharing of code between WebGL 1 and WebGL 2" patch.

I tried applying the new version, but it doesn't apply cleanly --- all hunks are rejected.

Comment 29

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #26)
> ::: dom/canvas/WebGLVertexArray.cpp
> @@ +18,5 @@
> >      , WebGLContextBoundObject(context)
> >  {
> >      SetIsDOMBinding();
> > +
> > +    if (context->gl->IsSupported(gl::GLFeature::vertex_array_object)) {
> 
> After much digging I realized that the reason for this check here is that
> the split between "fake" and "GL" vertex array objects was moved to here,
> inside the now-unified WebGLVertexArray class, right? That deserves a
> comment.

Yes that is correct. I'll add a comment to explain that.

> @@ +36,5 @@
> > +WebGLVertexArray::BindVertexArray()
> > +{
> > +    /* Bind to dummy value to signal that this vertex array has
> > +       ever been bound */
> > +    BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);
> 
> I'm not a fan of taking an existing OpenGL concept such as here the
> GL_VERTEX_ARRAY_BINDING enum constant, and assigning it new meaning that's
> local to us here. Normally, when a programmer reads our code and sees
> GL_VERTEX_ARRAY_BINDING they can know right away that we're just querying
> OpenGL state for the current VAO binding; that quick-reading habit would be
> broken here.
> 
> We used to have mHasEverBeenBound boolean members for that.

So, this is where Jeff and you and I differ.  I dislike having "derived" state, such as mHasEverBeenBound, that can be computed from other state in the object, mTarget != 0, more than I dislike that GL_VERTEX_ARRAY_BINDING is being used to in the way I do here. I'm used to playing fast-n-loose like this.

But I'm not precious about it. I'm not passionate to fight for it. Walter's strong typed enum patch has effectively added "mHasEverBeenBound" back, although I explained my objection to that with Jeff in Toronto.
 
> @@ +40,5 @@
> > +    BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);
> > +
> > +    if (mGLName) {
> > +        mContext->mBoundVertexArray = this;
> > +        mContext->gl->fBindVertexArray(mGLName);
> 
> The old code unconditionally called glBindVertexArray(mGLName). The new code
> only calls it if mGLName is nonzero. Do we for some reason never need to go
> back to the zero binding?

mGLName != 0 means that we have real OpenGL supported VAOs. If mGLName == 0 then we have to "fake" it.

> @@ +66,5 @@
> > +
> > +        mContext->BindBuffer(LOCAL_GL_ARRAY_BUFFER, vd.buf);
> > +
> > +        gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized, vd.stride,
> > +                                 reinterpret_cast<const GLvoid*>(vd.byteOffset));
> 
> Casting to [const] void* is only a static_cast.

byteOffset is an integer, not a pointer. This code was just moved from another file.

> @@ +79,5 @@
> > +    for (size_t i = mAttribs.Length(); i < prevVertexArray->mAttribs.Length(); ++i) {
> > +        const WebGLVertexAttribData& vd = prevVertexArray->mAttribs[i];
> > +
> > +        if (vd.enabled) {
> > +            gl->fDisableVertexAttribArray(i);
> 
> Is it true for some reason that the attrib set for prevVertexArray has empty
> intersection with the attrib set in *this?
>  - if that's true, please add an explanatory comment
>  - if that's not true, then don't we have a bug here, where we're disabling
> attribs that were enabled just above?

I didn't write this code, I moved it.

I can look into answering your questions if you think that is in scope of this review. Actually, I think Walter may have written the fake binding code. Walter?
> 
> ::: dom/canvas/WebGLVertexArray.h
> @@ +76,5 @@
> >      friend class WebGLContext;
> >  };
> >  
> > +// DOM binding shims - these classes just override WrapObject method
> > +class WebGLVertexArrayObjectOES : public WebGLVertexArray
> 
> Was Bindings.conf not happy with two different interfaces having the same
> nativeType?

Correct. I tried to setup Binding.conf to make WebGLVertexArrayObject and WebGLVertexArrayObjectOES have the same native class. The generated bindings had issues with "Wrap()". Adding these shim classes was quick to implement (10 minutes).

> If that was the problem, should that be filed as a bug against the bindings?

I don't understand with enough clarity how JS wrapping of native objects work to answer whether it's a bug and just a harsh reality.

> >  CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']
> >  CXXFLAGS += CONFIG['TK_CFLAGS']
> > +CXXFLAGS += [ '-O0 -g' ]
> 
> This unintentionally committed hunk must be removed :-)

Yes. That should be there. Removed.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8489317 [details] [diff] [review]
WebGL2 - Refactor VAOs to allow sharing of code between WebGL 1 and WebGL 2.

Review of attachment 8489317 [details] [diff] [review]:
-----------------------------------------------------------------

OK, thanks for answering my questions. Based on your answers, I'm comfortable r+ing this patch, with the above-discussed nits (that we agree on) addressed. I'm OK with leaving the more aesthetic considerations for later. Landing the patches on this bug is a huge priority as it unlocks us being able to land all the other WebGL2 patches in parallel.

::: dom/canvas/WebGLVertexArray.cpp
@@ +18,5 @@
>      , WebGLContextBoundObject(context)
>  {
>      SetIsDOMBinding();
> +
> +    if (context->gl->IsSupported(gl::GLFeature::vertex_array_object)) {

After much digging I realized that the reason for this check here is that the split between "fake" and "GL" vertex array objects was moved to here, inside the now-unified WebGLVertexArray class, right? That deserves a comment.

@@ +36,5 @@
> +WebGLVertexArray::BindVertexArray()
> +{
> +    /* Bind to dummy value to signal that this vertex array has
> +       ever been bound */
> +    BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);

I'm not a fan of taking an existing OpenGL concept such as here the GL_VERTEX_ARRAY_BINDING enum constant, and assigning it new meaning that's local to us here. Normally, when a programmer reads our code and sees GL_VERTEX_ARRAY_BINDING they can know right away that we're just querying OpenGL state for the current VAO binding; that quick-reading habit would be broken here.

We used to have mHasEverBeenBound boolean members for that.

@@ +40,5 @@
> +    BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);
> +
> +    if (mGLName) {
> +        mContext->mBoundVertexArray = this;
> +        mContext->gl->fBindVertexArray(mGLName);

The old code unconditionally called glBindVertexArray(mGLName). The new code only calls it if mGLName is nonzero. Do we for some reason never need to go back to the zero binding?

@@ +66,5 @@
> +
> +        mContext->BindBuffer(LOCAL_GL_ARRAY_BUFFER, vd.buf);
> +
> +        gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized, vd.stride,
> +                                 reinterpret_cast<const GLvoid*>(vd.byteOffset));

Casting to [const] void* is only a static_cast.

@@ +79,5 @@
> +    for (size_t i = mAttribs.Length(); i < prevVertexArray->mAttribs.Length(); ++i) {
> +        const WebGLVertexAttribData& vd = prevVertexArray->mAttribs[i];
> +
> +        if (vd.enabled) {
> +            gl->fDisableVertexAttribArray(i);

Is it true for some reason that the attrib set for prevVertexArray has empty intersection with the attrib set in *this?
 - if that's true, please add an explanatory comment
 - if that's not true, then don't we have a bug here, where we're disabling attribs that were enabled just above?

::: dom/canvas/WebGLVertexArray.h
@@ +76,5 @@
>      friend class WebGLContext;
>  };
>  
> +// DOM binding shims - these classes just override WrapObject method
> +class WebGLVertexArrayObjectOES : public WebGLVertexArray

Was Bindings.conf not happy with two different interfaces having the same nativeType?

If that was the problem, should that be filed as a bug against the bindings?

::: dom/canvas/moz.build
@@ +136,5 @@
>  ]
>  
>  CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']
>  CXXFLAGS += CONFIG['TK_CFLAGS']
> +CXXFLAGS += [ '-O0 -g' ]

This unintentionally committed hunk must be removed :-)

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

nevermind the review tool stupidly reposting my old review comments!

Comment 32

[Kelsey Gilbert [:jgilbert]]

Yeah, I'm split on explicit (state duplication/dependent states) vs. implicit (unintuitive). I would prefer to err towards intuitiveness rather than trying to exhaustively eliminate dependent states. Either is fine here, I think.

Comment 33

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=c05d7f513ea1

Comment 34

[u480271]

Attachment: WebGL2 WebIDL and DOM changes

Changes to WebIDL and DOM need review by DOM peer. :bjacob said that you could do this review. If not, please suggest appropriate peer.

Comment 35

[u480271]

Attachment: WebGL2 WebIDL and DOM changes

Comment 36

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=1e3ba38d33d7

Comment 37

[u480271]

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/c8aec5458731
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/e0af7fa50758
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/371dafadb902

Comment 38

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=371dafadb902

Comment 39

[(no longer active)]

Comment on attachment 8489926 [details] [diff] [review]
WebGL2 WebIDL and DOM changes

I'm not a DOM peer, you can find the list of the peers here: <https://wiki.mozilla.org/Modules/Core#Document_Object_Model>

Comment 40

[Walter Litwinczyk [:walter]]

> > @@ +79,5 @@
> > > +    for (size_t i = mAttribs.Length(); i < prevVertexArray->mAttribs.Length(); ++i) {
> > > +        const WebGLVertexAttribData& vd = prevVertexArray->mAttribs[i];
> > > +
> > > +        if (vd.enabled) {
> > > +            gl->fDisableVertexAttribArray(i);
> > 
> > Is it true for some reason that the attrib set for prevVertexArray has empty
> > intersection with the attrib set in *this?
> >  - if that's true, please add an explanatory comment
> >  - if that's not true, then don't we have a bug here, where we're disabling
> > attribs that were enabled just above?
> 
> I didn't write this code, I moved it.
> 
> I can look into answering your questions if you think that is in scope of
> this review. Actually, I think Walter may have written the fake binding
> code. Walter?

I believe it's wrong, the original code was:

>void
>WebGLVertexArrayFake::BindVertexArrayImpl()
>{
>    // Go through and re-bind all buffers and setup all
>    // vertex attribute pointers
>    gl::GLContext* gl = mContext->gl;
>
>    WebGLRefPtr<WebGLBuffer> prevBuffer = mContext->mBoundArrayBuffer;
>    mContext->BindBuffer(LOCAL_GL_ELEMENT_ARRAY_BUFFER, mElementArrayBuffer);
>
>    for (size_t i = 0; i < mAttribs.Length(); ++i) {
>        const WebGLVertexAttribData& vd = mAttribs[i];
>
>        mContext->BindBuffer(LOCAL_GL_ARRAY_BUFFER, vd.buf);
>
>        gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized,
>                                 vd.stride, reinterpret_cast<void*>(vd.byteOffset));
>
>        if (vd.enabled) {
>            gl->fEnableVertexAttribArray(i);
>        } else {
>            gl->fDisableVertexAttribArray(i);
>        }
>    }
>
>    mContext->BindBuffer(LOCAL_GL_ARRAY_BUFFER, prevBuffer);
>}

The original loop already disables the attribute if it's not enabled. I believe that length(mAttribs[]) == max number of attributes, so it will properly disable/enable all vertex attributes appropriately.

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8489926 [details] [diff] [review]
WebGL2 WebIDL and DOM changes

Review of attachment 8489926 [details] [diff] [review]:
-----------------------------------------------------------------

Smaug, would you have time to look at this today?

Nevermind that TIMEOUT_IGNORED is out of range as a JS number --- we're already aware of the issue and plan to change this to int64 and the value -1 while we get the WG to decide what to do about it.

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

There still are mochitest-gl failures on Android slaves, namely in the OES_vertex_array_object mochitest. https://tbpl.mozilla.org/php/getParsedLog.php?id=48203780&tree=Try&full=1

Looking into it (reproduces for me on NVIDIA/linux).

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

So this is killing WebGLVertexArray interface object. Is that web compatible?

Comment 44

[Benoit Jacob [:bjacob] (mostly away)]

It is a known, intentional incompatible change. Interfaces defined in WebGL extensions have been switched to [NoInterfaceObject]. Also, this should have been exposed as WebGLVertexArrayOES in the first place.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8489926 [details] [diff] [review]
WebGL2 WebIDL and DOM changes

Ok, I was told the possibly breaking change to WebGLVertexArray was done on purpose.


Some of the const have /* TODO: Depends on mapBufferRange */, but
some of the consts which have /* TODO: Depends on mapBufferRange */ aren't
commented out. Total mystery to the reader of the code.
Please be consistent, and follow https://www.khronos.org/registry/webgl/specs/latest/2.0/webgl2.idl better and comment them all out.

mostly rs+

Comment 46

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Jacob [:bjacob] from comment #42)
> There still are mochitest-gl failures on Android slaves, namely in the
> OES_vertex_array_object mochitest.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=48203780&tree=Try&full=1
> 
> Looking into it (reproduces for me on NVIDIA/linux).

So actually, what I ran into was something else --- I actually can't reproduce this failure locally. It's OK to mark the test as failing for now, though, as the code seems to work, only fails on Android slaves, and that extension isn't vital there.

Comment 47

[Benoit Jacob [:bjacob] (mostly away)]

Dan: note that the patch on bug 1067436 is something that I needed to avoid asserting; it is definitely a correctness fix; maybe it is related to these android failures. It seems to be an issue with the present VAO patch on this bug.

Comment 48

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: prevent-infinite-recursion

In addition to the patch mentioned on comment 47, there is also this patch that I needed in order to get things to work ---- without it, we just infinitely recurse.

These two patches together have a fair chance of fixing the Android failures...

Comment 49

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=10538ffac15c

Comment 50

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=20bae6c9c265

Comment 51

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=0f3b1294e7c6

Comment 52

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=42c4afa260d5

Comment 53

[u480271]

(In reply to Dan Glastonbury :djg :kamidphish from comment #52)
> https://tbpl.mozilla.org/?tree=Try&rev=42c4afa260d5

If this doesn't change the result of Android 4.0 test, then I'm going to have to admit that I have no idea how the mochitests are run. failing_android.txt doesn't appear to control anything and logging added to test_webgl_conformance_test_suite.html didn't appear in the logs. I really want to land these patches to unblock :bjacob, and I'm just wasting hours spinning my wheels while android slowly compile and then slowing tests.

Comment 54

[u480271]

Comment on attachment 8490479 [details] [diff] [review]
prevent-infinite-recursion

Review of attachment 8490479 [details] [diff] [review]:
-----------------------------------------------------------------

I've added this to the patchset I'm trying to land as Bug 1002302.

Comment 55

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=9ba2a3f366dd

Comment 56

[u480271]

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/84a33931c623
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/6ca63f4c16fb
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/1990a2c88e12
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/7e6c0d4cd583

Comment 57

[u480271]

Attachment: WebGL2 - Mark oes-vertex-array-object.html failing on android

Comment 58

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=0535805cfc83

Comment 59

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8490651 [details] [diff] [review]
WebGL2 - Mark oes-vertex-array-object.html failing on android

Review of attachment 8490651 [details] [diff] [review]:
-----------------------------------------------------------------

(I should probably have included a header in _mochitest.ini)
_mochitest.ini is a generated file.
You want to add an entry for this[1] in mochitest-errata.ini, and then run `python generate-wrappers-and-manifest.py` from dom/canvas/test/webgl-conformance/.
This will update _mochitest.ini, and just submit a patch with all these changes.

[1]:
[_wrappers/test_conformance__extensions__oes-vertex-array-object.html]
skip-if = os == 'android'

Comment 60

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: interface-test

I found this in Dan's try push which I'm trying to land, and apparently this must be reviewed by a DOM peer.

You already r+'d the removal of this interface.

Comment 61

[Benoit Jacob [:bjacob] (mostly away)]

The remaining Mac OSX mochitest-2 orange is a failure of gl-bind-attrib-location-test.html. I cannot reproduce it on any of the machines that I tried --- not even on a Mac. At this point I believe that this is a driver bug, and I'm going to mark this test as failing on mac slaves.

Comment 62

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fail-test-on-mac

Comment 63

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=14ccbde840b2 shows an orange on Android mochitest-gl but it says "Should fail" so that looks like an unexpected-pass.

Comment 64

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: failing-tests

Comment 65

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=a0ccd12b6256

Comment 66

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=3b27b4b6920c

Comment 67

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc603905597

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0fc603905597

Comment 69

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.