The extension draft is here: http://www.khronos.org/registry/webgl/extensions/OES_vertex_array_object/ An overview of how WebGL extensions are added is here: https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions

Started looking into bug and working on it.

Mark, still looking into this? Let us know if you have any questions on where/how to get started.

Hi Vladimir, am still looking into this. Changed my developer laptop to a mac, am setting up the mozilla build environment on it. I would greatly appreciate any information and advice on the requirements for this task and/or any technical details that would help with implementing the required WebGL component. Thanks in advance :)

Hello all! I'm really interested in the implementation of WebGL and I'd like to start on it by looking into this. Mark: If you're still working on it I'll leave it alone but if not I'll take it on.

Hi Peter! We just completely re-did the way that WebGL extensions are implemented. Consequently, documentation is currently outdated. I'll update this bug as soon as it's updated.

Done! https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions is now up to date again, and describes the new way to expose WebGL extensions.

Hi Peter, I have almost set up my mozilla dev environment in my new mac, and will start working on this soon. Will forward you the task if i am getting delayed or take more time :)

Note, if there is demand for more WebGL mentored bugs, always feel free to let me and jgilbert know, we're happy to file more. There is work that needs to be done.

Note: bug 738867 is another webgl extension that would be a good first bug/first extension.

Vlad: Jon Buckley and/or :dwarfcrank is already looking into this one.

I found a demo for this extension: http://media.tojicode.com/webgl-samples/OES_vertex_array_object.html More informations: http://blog.tojicode.com/2012/10/oesvertexarrayobject-extension.html

This blogpost about running the Nebula3 game engine on the web mentions this extension, http://flohofwoe.blogspot.de/2013/01/a-drakensang-online-map-viewer-in.html He says that it would greatly reduce the number of GL calls in the demo he links to there, sounds like it should remove some of the frame stutter.

Hey James, Thanks for the patches! It's a good start, but unfortunately not complete :) - It needs an implementation of the WebGLExtensionVertexArrayObjectOES class - It's missing a bunch of integration points for VAOs in the WebGL code -- basically, all the security checks that track what the currently bound vertex attrib arrays are. This might just be a matter of making sure that the contents of mAttribBuffers is kept up to date with the VAO (maybe just making a copy of it when the VAO is created would be enough, and updating it with any binding change?) - (Ideally, but optinally) it could use a manual implementation of VAOs when the underlying GL extension isn't available. This is something that we could emulate and still get a performance win, because we'd avoid the need to make a bunch of calls from JS for attrib setup. This should probably be done as a followup patch to the initial one that implements/enables the VAO extension. Also, it looks like you attached two separate patches that should be combined into one... not sure how you're generating your diffs, but worst case you can just cat them together into one file. Thanks for taking this on! You can find a bunch of us on irc.mozilla.org #gfx as well, if you have any questions that you'd like more real-time answers to.

Comment on attachment 735756 [details] [diff] [review] WIP implementation of OES_vertex_array_object Review of attachment 735756 [details] [diff] [review]: ----------------------------------------------------------------- This is going in the right direction, but you'll also need entries in GLContextSymbols.h, as well as the actual functions in GLContext.h. See what was done in the bug for adding ARB_sync support: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=739421&attachment=612737

Comment on attachment 735755 [details] [diff] [review] WIP implementation of OES_vertex_array_object Review of attachment 735755 [details] [diff] [review]: ----------------------------------------------------------------- Alright, we're on the right track, but I think we're missing an important file from this patch. ::: content/canvas/src/Makefile.in @@ +53,5 @@ > WebGLExtensionLoseContext.cpp \ > WebGLExtensionStandardDerivatives.cpp \ > WebGLExtensionTextureFilterAnisotropic.cpp \ > WebGLExtensionTextureFloat.cpp \ > + WebGLExtensionVertexArrayObjectOES.cpp \ This file wasn't included in this patch. Did you forget to `hg add` it? Also, drop the 'OES' from the end here, as well. ::: content/canvas/src/WebGLContext.cpp @@ +950,5 @@ > case OES_texture_float: > return gl->IsExtensionSupported(gl->IsGLES2() ? GLContext::OES_texture_float > : GLContext::ARB_texture_float); > + case OES_vertex_array_object: > + return gl->IsExtensionSupported(GLContext::OES_vertex_array_object); You should also check for the desktop GL extension: http://www.opengl.org/registry/specs/ARB/vertex_array_object.txt ::: content/canvas/src/WebGLExtensions.h @@ +128,5 @@ > > DECL_WEBGL_EXTENSION_GOOP > }; > > +class WebGLExtensionVertexArrayObjectOES Just 'WebGLExtensionVertexArrayObject', without the 'OES' suffix. @@ +138,5 @@ > + > + WebGLExtensionVertexArrayObjectOES createVertexArrayOES(); > + void deleteVertexArrayOES(WebGLExtensionVertexArrayObjectOES arrayObject); > + GLboolean isVertexArrayOES(WebGLExtensionVertexArrayObjectOES arrayObject); > + void bindVertexArrayOES(WebGLExtensionVertexArrayObjectOES arrayObject); These functions all take (or return) `WebGLVertexArrayObjectOES`, not `WebGLExtensionVertexArrayObjectOES`.

To be clear, I really want two different patches: One that just adds ARB/OES_vertex_array_object support to GLContext, and a second which does the actual webgl implementation.

(In reply to Jeff Gilbert [:jgilbert] from comment #18) > To be clear, I really want two different patches: > One that just adds ARB/OES_vertex_array_object support to GLContext, and a > second which does the actual webgl implementation. To be double-clear, the way you're splitting it right now is exactly what I want.

Forgot to add the (rather light) implementation I have so far.

Jeff, I will keep the patches separate going forward.

Comment on attachment 736309 [details] [diff] [review] Updated based on Jeff's review Review of attachment 736309 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +61,5 @@ > "GL_ARB_texture_float", > "GL_EXT_unpack_subimage", > "GL_OES_standard_derivatives", > + "GL_OES_vertex_array_object", > + "GL_ARB_vertex_array_object", These need to go at the end of the list. This needs to match the enum in GLContext.h, as they are parallel structures. @@ +534,5 @@ > mSymbols.fEGLImageTargetRenderbufferStorage = nullptr; > } > } > + > + if (IsExtensionSupported(OES_vertex_array_object)) { Needs to be checking for the desktop version, as well. @@ +539,5 @@ > + SymLoadStruct vaoSymbols[] = { > + { (PRFuncPtr*) &mSymbols.fIsVertexArray, { "IsVertexArray", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fGenVertexArrays, { "GenVertexArrays", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fBindVertexArray, { "BindVertexArray", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fDeleteVertexArrays, { "DeleteVertexAraays", nullptr } }, Should be "DeleteVertexArrays". Also all of these functions we're querying for should be suffixed by "OES". (Check the GL/GLES spec) You also need to include the changes in GLContextSymbols.h. Again, see the ARB_sync patch. This patch here should be basically the same thing, except for a different extension.

Comment on attachment 736310 [details] [diff] [review] Updated based on Jeff's review Review of attachment 736310 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/Makefile.in @@ +53,5 @@ > WebGLExtensionLoseContext.cpp \ > WebGLExtensionStandardDerivatives.cpp \ > WebGLExtensionTextureFilterAnisotropic.cpp \ > WebGLExtensionTextureFloat.cpp \ > + WebGLExtensionVertexArrayObjectOES.cpp \ This filename shouldn't have OES at the end. Also you need to include this file in this patch. If you're using HG, use `hg add content/canvas/src/WebGLExtensionVertexArrayObject.cpp`.

Comment on attachment 738080 [details] [diff] [review] WIP gfx/gl update for OES/ARB vertex array object Review of attachment 738080 [details] [diff] [review]: ----------------------------------------------------------------- These file are almost good, but I still don't see the changes we need done to GLDefs.h and GLContextSymbols.h. Without changes to GLContextSymbols.h, this won't even compile. Please include the changes to these two extra files in the next review request. ::: gfx/gl/GLContext.cpp @@ +536,5 @@ > } > + > + if (IsExtensionSupported(OES_vertex_array_object)) { > + SymLoadStruct vaoSymbols[] = { > + { (PRFuncPtr*) &mSymbols.fIsVertexArray, { "IsVertexArrayOES", nullptr } }, We don't need to split these two extensions. They're the same, they just have different symbol names that we want to coalesce into one standard set. This line should be: { (PRFuncPtr*) &mSymbols.fIsVertexArray, { "IsVertexArray", "IsVertexArrayOES", nullptr } }, This way it first tries the un-suffixed version, then the suffixed version, then fails if neither is found. This will cover both the OES and ARB extensions. @@ +546,5 @@ > + > + if (!LoadSymbols(&vaoSymbols[0], trygl, prefix)) { > + NS_ERROR("GL supports OES_vertex_array_object without supplying its functions."); > + > + MarkExtensionUnsupported(OES_vertex_array_object); Just unconditionally mark both as unsupported, as we combine these two branches. @@ +552,5 @@ > + mSymbols.fGenVertexArrays = nullptr; > + mSymbols.fBindVertexArray = nullptr; > + mSymbols.fDeleteVertexArrays = nullptr; > + } > + } else if (IsExtensionSupported(ARB_vertex_array_object)) { Don't split the handling of the ARB and OES extension. See other comments. ::: gfx/gl/GLContext.h @@ +2866,5 @@ > mSymbols.fEGLImageTargetRenderbufferStorage(target, image); > AFTER_GL_CALL; > } > > + void GLAPIENTRY fBindVertexArray(GLuint array) Take out 'GLAPIENTRY'. It should just be ancient cruft which needs to be removed.

Comment on attachment 738081 [details] [diff] [review] WIP update to content/canvas/ add WebGLExtensionVertexArrayObject Review of attachment 738081 [details] [diff] [review]: ----------------------------------------------------------------- Great, we're getting there. ::: content/canvas/src/WebGLExtensionVertexArrayObject.cpp @@ +17,5 @@ > +WebGLExtensionVertexArrayObjectOES::~WebGLExtensionVertexArrayObjectOES() > +{ > +} > + > +WebGLVertexArrayObject WebGLExtensionVertexArrayObjectOES::CreateVertexArrayOES() You will need to create a new WebGLVertexArrayObject here. (along with creating its class definition in its own pair of header/source files, similar to WebGLRenderbuffer, and other first-class webgl objects) @@ +26,5 @@ > + > + return array; > +} > + > +void WebGLExtensionVertexArrayObjectOES::DeleteVertexArrayOES(WebGLVertexArrayObject array) For the rest of these, you'll need to accept a pointer to the `array` object, and do the `mContext->gl->` calls based on the data it holds. Basically, the `WebGLVertexArrayObject` object will be a wrapper around the `GLuint` handle used internally by normal OpenGL. ::: content/canvas/src/WebGLExtensions.h @@ +135,5 @@ > +public: > + WebGLExtensionVertexArrayObject(WebGLContext*); > + virtual ~WebGLExtensionVertexArrayObject(); > + > + WebGLVertexArrayObject CreateVertexArrayOES(); // WebGLVertexArrayObject should be a typedef for GLuint This should return an `already_AddRefed<WebGLVertexArrayObject>`. @@ +137,5 @@ > + virtual ~WebGLExtensionVertexArrayObject(); > + > + WebGLVertexArrayObject CreateVertexArrayOES(); // WebGLVertexArrayObject should be a typedef for GLuint > + void DeleteVertexArrayOES(WebGLVertexArrayObject array); > + GLboolean IsVertexArrayOES(WebGLVertexArrayObject array); This should just return `bool`, not `GLboolean`. @@ +138,5 @@ > + > + WebGLVertexArrayObject CreateVertexArrayOES(); // WebGLVertexArrayObject should be a typedef for GLuint > + void DeleteVertexArrayOES(WebGLVertexArrayObject array); > + GLboolean IsVertexArrayOES(WebGLVertexArrayObject array); > + void BindVertexArrayOES(WebGLVertexArrayObject array); These three need the type of `array` to be `WebGLVertexArrayObject*`. ::: content/canvas/src/WebGLObjectModel.h @@ +19,5 @@ > typedef uint32_t WebGLuint; > typedef float WebGLfloat; > typedef float WebGLclampf; > typedef bool WebGLboolean; > +typedef unsigned int WebGLVertexArrayObject; WebGLVertexArrayObject needs to be a full object type, like WebGLTexture.

Also, check WebGLContext::CreateRenderbuffer and WebGLContext::DeleteRenderbuffer (etc) for a taste of how the implementation of CreateVertexArray (etc) should work.

Just a drive-by question. Why this typedef? +typedef unsigned int WebGLVertexArrayObject; Is this a OpenGL unsigned integer name like for other OpenGL objects? If yes, then 1) why does this use its own typedef whereas for textures/buffers/etc we just use GLuint like the OpenGL API itself does; and 2) if this is a generic OpenGL thing then why does this have WebGL in its name?

Thanks :bjacob; It should be a GLuint and I'm in the process of following through with Jeff's comments.

Comment on attachment 752332 [details] [diff] [review] WIP update to content/canvas/ add WebGLExtensionVertexArrayObject Just another WIP update to show progress on the patch. I think it's just missing the JS wrappers but would like someone to take a peek at it and make sure I've got the implementation going in the right direction.

Comment on attachment 752332 [details] [diff] [review] WIP update to content/canvas/ add WebGLExtensionVertexArrayObject Review of attachment 752332 [details] [diff] [review]: ----------------------------------------------------------------- This is close. ::: content/canvas/src/WebGLContextGL.cpp @@ +216,5 @@ > + > + if (!ValidateObjectAllowDeletedOrNull("bindVertexArrayObject", array)) > + return; > + > + if (array and array->IsDeleted()) `and`? Too much python? :P @@ +220,5 @@ > + if (array and array->IsDeleted()) > + return; > + > + if (array) > + array->SetHasEverBeenBound(true); I would prefer to do this after we bind, not before. @@ +3032,5 @@ > + if (!IsContextStable()) > + return false; > + > + return ValidateObjectAllowDeleted("isVertexArrayObject", array) && > + !array->IsDeleted() && Indent continued lines to match the beginning of `ValidateObje...`. @@ +4263,5 @@ > +WebGLContext::CreateVertexArray() > +{ > + if (!IsContextStable()) > + return nullptr; > + nsRefPtr<WebGLVertexArrayObject> globj = new WebGLVertexArrayObject(this); Newline after the return. ::: content/canvas/src/WebGLExtensionVertexArrayObject.cpp @@ +26,5 @@ > + > +void WebGLExtensionVertexArrayObject::DeleteVertexArray(WebGLVertexArrayObject* array) > +{ > + if (!array) > + return; Handle this in WebGLContext. @@ +35,5 @@ > + > +bool WebGLExtensionVertexArrayObject::IsVertexArray(WebGLVertexArrayObject* array) > +{ > + if (!array) > + return false; Handle this in WebGLContext. @@ +49,5 @@ > + > + if (!array->HasEverBeenBound()) { > + // mContext->MakeContextCurrent(); > + mContext->BindVertexArray(array); > + array->SetHasEverBeenBound(true); These functions should be very minimal pass-throughs to the functions in WebGLContext. ::: content/canvas/src/WebGLVertexArrayObject.h @@ +44,5 @@ > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebGLVertexArrayObject) > + > +protected: > + WebGLuint mGLName; > + bool mHasEverBeenBound; These need to be initialized in the constructor's initializer list.

The linker still cannot find the missing JS Wrap symbols which will need fixing, but I've gone through and cleaned things up.

Comment on attachment 752791 [details] [diff] [review] WIP update following jgilbert's review Review of attachment 752791 [details] [diff] [review]: ----------------------------------------------------------------- Almost! ::: content/canvas/src/WebGLContextGL.cpp @@ +210,5 @@ > > void > +WebGLContext::BindVertexArray(WebGLVertexArrayObject *array) > +{ > + if (!array) We need to allow null here, since that's how we unbind VAOs without binding a new one. @@ +228,5 @@ > + WebGLuint vertexarrayobjectname = array ? array->GLName() : 0; > + gl->fBindVertexArray(vertexarrayobjectname); > + > + mBoundVertexArrayObject = array; > + array->SetHasEverBeenBound(true); Don't call against `array` unless it's non-null. @@ +1102,5 @@ > + > + if (!IsContextStable()) > + return; > + > + if (!ValidateObjectAllowDeletedOrNull("deleteVertexArrayObject", array)) `array` can't be null here. @@ +1105,5 @@ > + > + if (!ValidateObjectAllowDeletedOrNull("deleteVertexArrayObject", array)) > + return; > + > + if (!array || array->IsDeleted()) `array` can't be null here.

review ping!

Comment on attachment 753779 [details] [diff] [review] WIP update to canvas following jgilbert's review Review of attachment 753779 [details] [diff] [review]: ----------------------------------------------------------------- Ok, bad news. There's actually a decent amount of work still to do. This patch is almost correct for just adding raw support to WebGL, but we still need to tie it to all the state tracking we do. I detailed what needs to happen in the comment attached to `BindVertexArray`. ::: content/canvas/src/WebGLContextGL.cpp @@ +208,5 @@ > mBoundRenderbuffer = wrb; > } > > void > +WebGLContext::BindVertexArray(WebGLVertexArrayObject *array) This is actually going to have to be more involved than I thought. When we switch VAOs, we would need to update `mBoundArrayBuffer` and `mBoundElementArrayBuffer`, based on the new vao. (think of vao 0 as the 'default vao') We would also need to update all our info about vertex attrib arrays. Instead, what we should do, is put all this info on the `WebGLVertexArrayObject` object, and indirect into the currently bound one when we need such info. The list of state that VAOs store is enumerated in the extension. @@ +217,5 @@ > + if (!ValidateObjectAllowDeletedOrNull("bindVertexArrayObject", array)) > + return; > + > + if (array && array->IsDeleted()) > + return; Deleted should emit an INVALID_OPERATION. @@ +226,5 @@ > + WebGLuint vertexarrayobjectname = array ? array->GLName() : 0; > + gl->fBindVertexArray(vertexarrayobjectname); > + > + mBoundVertexArrayObject = array; > + array->SetHasEverBeenBound(true); This is the only line that should be inside this `if`. All other lines should be run unconditionally. @@ +3028,5 @@ > +{ > + if (!array) > + return false; > + > + if (!IsContextStable()) `IsContextStable` should always come first.

I had a feeling it couldn't quite be that simple. I'll take a peek at this ASAP.

Comment on attachment 752337 [details] [diff] [review] WIP update to gl to implement OES_vertex_array_object Let's at least take this patch.

Comment on attachment 752337 [details] [diff] [review] WIP update to gl to implement OES_vertex_array_object https://hg.mozilla.org/integration/mozilla-inbound/rev/5a43a331c2cc To make life easier for those checking in on your behalf, please make sure that future patches follow the guidelines below. Thanks!

And by link below, I meant this of course :P https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Why [leave open] ?

(In reply to Benoit Jacob [:bjacob] from comment #47) > Why [leave open] ? Because the patch we landed only adds support for this extension to GLContext, not WebGLContext.

Alright, so let me first provide some context: Guillaume Abadie is going to work on starting our WebGL 2 implementation, and our first priority there is the subset of WebGL 2 (i.e. of ES 3.0) that is already specced as WebGL 1 extensions. That makes VAO's a particularly high priority. Given that, Guillaume had a look at the work already done here today, and it seems like a lot of great work is already done, in the patch already reviewed above (not to mention the GLContext part that already landed), and just some works remains to be done to fix remaining issues. So: James, if you already happen to have the rest done, that's great, otherwise, no worries, Guillaume can give you a hand. That could take the form of landing your patch as is, together with a separate fixup patch that he would write soon, so that credit is given where credit is due. I hope that sounds fair & a good use of the great work already done here.

(In reply to Benoit Jacob [:bjacob] from comment #49) > Alright, so let me first provide some context: Guillaume Abadie is going to > work on starting our WebGL 2 implementation, and our first priority there is > the subset of WebGL 2 (i.e. of ES 3.0) that is already specced as WebGL 1 > extensions. That makes VAO's a particularly high priority. > > Given that, Guillaume had a look at the work already done here today, and it > seems like a lot of great work is already done, in the patch already > reviewed above (not to mention the GLContext part that already landed), and > just some works remains to be done to fix remaining issues. > > So: James, if you already happen to have the rest done, that's great, > otherwise, no worries, Guillaume can give you a hand. That could take the > form of landing your patch as is, together with a separate fixup patch that > he would write soon, so that credit is given where credit is due. I hope > that sounds fair & a good use of the great work already done here. I'd be happy t work on it and finish it with Guillaume if that is alright.

james' patch rebased on top of WebGL_draw_buffers, and build system changes.

renaming: WebGLVertexArrayObject.h/.cpp -> WebGLVertexArray.h/.cpp renaming: WebGLContext::mBoundElementArrayBuffer -> WebGLVertexArray::mBoundElementArrayBuffer renaming: WebGLContext::mAttribBuffers -> WebGLVertexArray::mAttribBuffers add: WebGLContext::mDefaultVertexArray add: WebGLExtensionVertexArray::IsSupported(this); add: .cpp files in moz.config add: JS bindings add: WebIDL add: APPLE_vertex_array_object support add: #define LOCAL_GL_VERTEX_ARRAY_BINDING add: guaranteed without regressions on WebGL conformance 1.0.1 and 1.0.2 add: all tests passed on Push to Try https://tbpl.mozilla.org/?tree=Try&rev=a92be182914b bug fix: GetVertexAttrib had wrong return with LOCAL_GL_VERTEX_ATTRIB_ARRAY_NORMALIZED bug fix: GLContext was looking for symbole glBindVertexArrays instead of glBindVertexArray bug fix: on OES_vertex_array_object's WebGL Conformance test : isVertexArrayOES() is not specified

Hm, actually attachment 753779 [details] [diff] [review] and the rebased version (attachment 768427 [details] [diff] [review]) are very far from even compiling, so let's give up on the idea of keeping separate patches --- sorry. Let's do a single combined patch.

here is the single unified patch

Comment on attachment 768477 [details] [diff] [review] single cobined patch Review of attachment 768477 [details] [diff] [review]: ----------------------------------------------------------------- Use MOZ_ASSERT instead of NS_ASSERTION to enforce internal c++ code contracts (crashes debug builds). No need to MOZ_ASSERT that RefPtr's are non-null before dereferencing them: they already check that e.g. in operator->(). ::: content/canvas/src/WebGLExtensionVertexArray.cpp @@ +57,5 @@ > + if (gl->IsExtensionSupported(gl::GLContext::APPLE_vertex_array_object)) { > + return true; > + } > + > + return false; the two last if's and this return can be simplified into just: return a || b; ::: content/canvas/src/WebGLVertexArray.cpp @@ +10,5 @@ > +#include "nsContentUtils.h" > + > +using namespace mozilla; > + > +JSObject* trailing \w @@ +42,5 @@ > + if (index >= WebGLuint(mContext->mGLMaxVertexAttribs)) { > + if (index == WebGLuint(-1)) { > + mContext->ErrorInvalidValue("%s: index -1 is invalid. That probably comes from a getAttribLocation() call, " > + "where this return value -1 means that the passed name didn't correspond to an active attribute in " > + "the specified program.", info); You can un-indent the two previous lines a bit (code moved from elsewhere). ::: gfx/gl/GLContext.h @@ +1026,5 @@ > OES_element_index_uint, > OES_vertex_array_object, > ARB_vertex_array_object, > ARB_draw_buffers, > + APPLE_vertex_array_object, Move this up. Current order doesn't match!

Comment on attachment 768512 [details] [diff] [review] patch for landing Review of attachment 768512 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContext.cpp @@ +266,5 @@ > + where the gl context is lost, so we unref its mBoundElementArrayBuffer > + manualy > + */ > + mDefaultVertexArray->mBoundElementArrayBuffer = nullptr; > + mDefaultVertexArray->mAttribBuffers.Clear(); I would much rather just mDefaultVertexArray = nullptr; , please explain precisely why we couldn't do this. ::: content/canvas/src/WebGLContextGL.cpp @@ +255,5 @@ > + gl->fBindVertexArray(array->GLName()); > + > + array->SetHasEverBeenBound(true); > + > + mBoundVertexArray = array; Too many blank lines above. ::: content/canvas/src/WebGLExtensionVertexArray.cpp @@ +14,5 @@ > +WebGLExtensionVertexArray::WebGLExtensionVertexArray(WebGLContext* context) > + : WebGLExtensionBase(context) > +{ > + MOZ_ASSERT(IsSupported(context), "should not construct WebGLExtensionVertexArray :" > + "OES_vertex_array_object unsuported."); Un-indent @@ +43,5 @@ > +} > + > +bool WebGLExtensionVertexArray::IsSupported(const WebGLContext* context) > +{ > + gl::GLContext * gl = context->GL(); One space too many. @@ +50,5 @@ > + return gl->IsExtensionSupported(gl::GLContext::OES_vertex_array_object); > + } > + > + return (gl->IsExtensionSupported(gl::GLContext::ARB_vertex_array_object) || > + gl->IsExtensionSupported(gl::GLContext::APPLE_vertex_array_object)); Redundant (...) ::: content/canvas/test/webgl/conformance/extensions/oes-vertex-array-object.html @@ +148,5 @@ > ext.bindVertexArrayOES(null); > shouldBeTrue("ext.isVertexArrayOES(vao)"); > > + /* > + * Issue found in the conformance test. The public webgl mailing list has been notified about it. The tests have already been fixed upstream. ::: gfx/gl/GLContext.cpp @@ +576,5 @@ > + } > + } > + else if (IsExtensionSupported(APPLE_vertex_array_object)) { > + /* > + * separated call for LoadSymbols with APPLE_vertex_array_object because of separate call to .... to work around a driver bug: the IsVertexArray symbol (without suffix) can be present but unusable.

Pushed with this identity (crediting both authors) and commit msg: # User Guillaume Abadie <gabadie@mozilla.com> and James King <james@agentultra.com> bug 738869 - implement OES_vertex_array_object webgl extension - r=bjacob https://hg.mozilla.org/integration/mozilla-inbound/rev/88b65229c78c Thanks James & Guillaume!

For the record, WebGL tests are only m1 on desktop. On Android, they're in m-gl. (We don't have them on B2G :C)

https://hg.mozilla.org/mozilla-central/rev/88b65229c78c https://hg.mozilla.org/mozilla-central/rev/c5ccdfa3a392