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.

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

Implement dummy stubs for all functions defined in the WebIDL.

Stub out WebGLSampler, WebGLSync, WebGLTransformFeedback, WebGLVertexArrayObject.

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.

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

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

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

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

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

WebGLSampler, WebGLSync, WebGLTransformFeedback, WebGLVertexArrayObject. Refresh patch after content/canvas/src -> dom/canvas move.

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

(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. :\

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

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

https://tbpl.mozilla.org/?tree=Try&rev=4073745f7429 error C2220: warning treated as error - no 'object' file generated :-/

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.

Rolled up previous patches into one. Carry r+ from :jgilbert.

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 :-)

> > + 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.

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.

(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 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 :-)

nevermind the review tool stupidly reposting my old review comments!

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.

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

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

> > @@ +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 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.

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 this is killing WebGLVertexArray interface object. Is that web compatible?

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

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

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.

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

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

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

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.

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.

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.

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