Add WebGLContext2 C++ class inheriting WebGLContextBase See https://wiki.mozilla.org/Platform/GFX/WebGL2

Add WebGLContext2 class Enable with about:config flag (webgl.enable-ensecure-webgl2) Add virtual func WebGLContextBase::IsWebGL2 Add WebGL2 IDL inheriting WebGL 1 IDL Edit HTMLCanvasElement::GetContextHelper to allow experimental-webgl2 gl.getParameter(gl.VERSION) return "WebGL 2"

Comment on attachment 774398 [details] [diff] [review] patch revision 2 Review of attachment 774398 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGL2Context.cpp @@ +27,5 @@ > +// ----------------------------------------------------------- STATIC FUNCTIONS > + > +bool WebGL2Context::IsSupported() > +{ > + return Preferences::GetBool("webgl.enable-ensecure-webgl2", false); Let's use 'webgl.enable-webgl2'. [1] @@ +30,5 @@ > +{ > + return Preferences::GetBool("webgl.enable-ensecure-webgl2", false); > +} > + > +WebGL2Context * WebGL2Context::Create() Star sticks to the left, within reason. ::: content/canvas/src/WebGL2Context.h @@ +2,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef WebGL2Context_H_ If we're matching the file name, the '_H_' should be lowercase. (mfbt style) @@ +25,5 @@ > + // ------------------------------------------------------- STATIC FUNCTIONS > + > + static bool IsSupported(); > + > + static WebGL2Context * Create(); If we have a static constructor (which I support), the ctor should not be public. Also, stars to the left. ::: content/canvas/src/WebGLContextGL.cpp @@ +2113,5 @@ > case LOCAL_GL_RENDERER: > return StringValue(cx, "Mozilla", rv); > case LOCAL_GL_VERSION: > + if (IsWebGL2()) { > + return StringValue(cx, "WebGL 2.0", rv); This would be better as combining the return paths, and just passing along which `const char*` to use in `StringValue()`. ::: content/canvas/src/WebGLContextNotSupported.cpp @@ +12,5 @@ > + > +WebGL2Context * WebGL2Context::Create() > +{ > + return nullptr; > +} What is this file and class used/needed for? ::: dom/bindings/Bindings.conf @@ +1268,5 @@ > 'headerFile': 'WebGLRenderbuffer.h' > }, > > 'WebGLRenderingContext': { > + 'nativeType': 'mozilla::WebGLContext', Why have these reverted back to WebGLContext from WebGLNContext? @@ +1277,5 @@ > +}, > + > +'WebGLRenderingContext2': { > + 'nativeType': 'mozilla::WebGLContext', > + 'headerFile': 'WebGLContext.h', These look the same as those for 'WebGLRenderingContext'? ::: modules/libpref/src/init/all.js @@ +4003,5 @@ > pref("webgl.lose-context-on-heap-minimize", false); > pref("webgl.can-lose-context-in-foreground", true); > pref("webgl.max-warnings-per-context", 32); > pref("webgl.enable-draft-extensions", false); > +pref("webgl.enable-ensecure-webgl2", false); [1]

(In reply to Jeff Gilbert [:jgilbert] from comment #3) > Comment on attachment 774398 [details] [diff] [review] > patch revision 2 > > Review of attachment 774398 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGL2Context.cpp > @@ +27,5 @@ > > +// ----------------------------------------------------------- STATIC FUNCTIONS > > + > > +bool WebGL2Context::IsSupported() > > +{ > > + return Preferences::GetBool("webgl.enable-ensecure-webgl2", false); > > Let's use 'webgl.enable-webgl2'. [1] Has discussed offline, will use webgl.enable-prototype-webgl2 > > @@ +30,5 @@ > > +{ > > + return Preferences::GetBool("webgl.enable-ensecure-webgl2", false); > > +} > > + > > +WebGL2Context * WebGL2Context::Create() > > Star sticks to the left, within reason. > > ::: content/canvas/src/WebGL2Context.h > @@ +2,5 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +#ifndef WebGL2Context_H_ > > If we're matching the file name, the '_H_' should be lowercase. (mfbt style) > > @@ +25,5 @@ > > + // ------------------------------------------------------- STATIC FUNCTIONS > > + > > + static bool IsSupported(); > > + > > + static WebGL2Context * Create(); > > If we have a static constructor (which I support), the ctor should not be > public. Also, stars to the left. > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +2113,5 @@ > > case LOCAL_GL_RENDERER: > > return StringValue(cx, "Mozilla", rv); > > case LOCAL_GL_VERSION: > > + if (IsWebGL2()) { > > + return StringValue(cx, "WebGL 2.0", rv); > > This would be better as combining the return paths, and just passing along > which `const char*` to use in `StringValue()`. > > ::: content/canvas/src/WebGLContextNotSupported.cpp > @@ +12,5 @@ > > + > > +WebGL2Context * WebGL2Context::Create() > > +{ > > + return nullptr; > > +} > > What is this file and class used/needed for? Ok, somewhere else, to create the WebGL 2 context, I just call WebGL2Context::Create(). But the thing is the whole WebGL code might be removed from the compilation for some platforms, and compile this file instead. Therefore, to prevent the linking time error Symbol "WebGL2Context::Create()" not found, I added it here. > > ::: dom/bindings/Bindings.conf > @@ +1268,5 @@ > > 'headerFile': 'WebGLRenderbuffer.h' > > }, > > > > 'WebGLRenderingContext': { > > + 'nativeType': 'mozilla::WebGLContext', > > Why have these reverted back to WebGLContext from WebGLNContext? Yeah, I know, But I have to do this way because of an WebIDL compilor update last week that has generated a compile time error. Before, it has worked with WebGLRenderingContext binded on WebGL1Context, and WebGLRenderingContext2 binded on WebGL@Context, but not anymore. [1] > > @@ +1277,5 @@ > > +}, > > + > > +'WebGLRenderingContext2': { > > + 'nativeType': 'mozilla::WebGLContext', > > + 'headerFile': 'WebGLContext.h', > > These look the same as those for 'WebGLRenderingContext'? [1] > > ::: modules/libpref/src/init/all.js > @@ +4003,5 @@ > > pref("webgl.lose-context-on-heap-minimize", false); > > pref("webgl.can-lose-context-in-foreground", true); > > pref("webgl.max-warnings-per-context", 32); > > pref("webgl.enable-draft-extensions", false); > > +pref("webgl.enable-ensecure-webgl2", false); > > [1] As discussed offline, will add a #ifdef RELEASE_BUILD to disable webgl 2 on releases

Comment on attachment 776105 [details] [diff] [review] patch revision 3 Review of attachment 776105 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGL2Context.cpp @@ +28,5 @@ > + > +// ----------------------------------------------------------------------------- > +// STATIC FUNCTIONS > + > +bool WebGL2Context::IsSupported() Standard practice for member function definitions is: Type Class::MemberFunc(...) { ... } @@ +39,5 @@ > +} > + > +WebGL2Context* WebGL2Context::Create() > +{ > + return new WebGL2Context(); Let's be explicit here and do: #ifdef RELEASE_BUILD return nullptr; #endif ::: content/canvas/src/WebGLContextGL.cpp @@ +2113,5 @@ > case LOCAL_GL_RENDERER: > return StringValue(cx, "Mozilla", rv); > case LOCAL_GL_VERSION: > + if (IsWebGL2()) { > + return StringValue(cx, "WebGL 2.0", rv); I still think this branch should just select either "WebGL 2.0" or "WebGL 1.0", not actually return the StringValue. You didn't respond to my nit about this, so I'm going to mention it again. :) ::: content/canvas/src/WebGLContextNotSupported.cpp @@ +11,5 @@ > DUMMY(NS_NewCanvasRenderingContextWebGL, nsIDOMWebGLRenderingContext) > + > +WebGL2Context * WebGL2Context::Create() > +{ > + return nullptr; I think this is a messier way to do what we can already do with ifdefs, since it requires messing about within the build system. ::: content/html/content/src/HTMLCanvasElement.cpp @@ +684,5 @@ > ctx.forget(aContext); > return NS_OK; > } > + else if (WebGL2Context::IsSupported() && > + aContextId.EqualsLiteral("experimental-webgl2")) This is surely not the best place for this, though I'm not certain who to ask. Maybe dzbarsky knows? ::: dom/bindings/Bindings.conf @@ +1286,5 @@ > 'headerFile': 'WebGLRenderbuffer.h' > }, > > 'WebGLRenderingContext': { > + 'nativeType': 'mozilla::WebGLContext', Yeah, let's ask dzbarsky. ::: modules/libpref/src/init/all.js @@ +4018,5 @@ > pref("webgl.can-lose-context-in-foreground", true); > pref("webgl.max-warnings-per-context", 32); > pref("webgl.enable-draft-extensions", false); > +#ifndef RELEASE_BUILD > +pref("webgl.enable-prototype-webgl2", false); It's actually probably best to not mention this flag, for now. Just remove it from all.js until we're more ready to have people discover it.

Can you give me a patch to apply and the revision it's based off of that I could play around with?

(In reply to David Zbarsky (:dzbarsky) from comment #7) > Can you give me a patch to apply and the revision it's based off of that I > could play around with? My actual working revision is : changeset: 138455:5e191a26d909 tag: tip parent: 138454:e86f02d4738e parent: 138447:ff0a372e3170 user: Ed Morley <emorley@mozilla.com> date: Mon Jul 15 13:01:56 2013 +0100 summary: Merge latest green birch changeset and mozilla-central apply this patch first : https://bug890049.bugzilla.mozilla.org/attachment.cgi?id=776372 and then the attachment 776105 [details] [diff] [review]

(In reply to Jeff Gilbert [:jgilbert] from comment #6) > Comment on attachment 776105 [details] [diff] [review] > patch revision 3 > > Review of attachment 776105 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGL2Context.cpp > @@ +28,5 @@ > > + > > +// ----------------------------------------------------------------------------- > > +// STATIC FUNCTIONS > > + > > +bool WebGL2Context::IsSupported() > > Standard practice for member function definitions is: > > Type > Class::MemberFunc(...) > { > ... > } > > @@ +39,5 @@ > > +} > > + > > +WebGL2Context* WebGL2Context::Create() > > +{ > > + return new WebGL2Context(); > > Let's be explicit here and do: > #ifdef RELEASE_BUILD > return nullptr; > #endif > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +2113,5 @@ > > case LOCAL_GL_RENDERER: > > return StringValue(cx, "Mozilla", rv); > > case LOCAL_GL_VERSION: > > + if (IsWebGL2()) { > > + return StringValue(cx, "WebGL 2.0", rv); > > I still think this branch should just select either "WebGL 2.0" or "WebGL > 1.0", not actually return the StringValue. You didn't respond to my nit > about this, so I'm going to mention it again. :) Oups, Sorry. > > ::: content/canvas/src/WebGLContextNotSupported.cpp > @@ +11,5 @@ > > DUMMY(NS_NewCanvasRenderingContextWebGL, nsIDOMWebGLRenderingContext) > > + > > +WebGL2Context * WebGL2Context::Create() > > +{ > > + return nullptr; > > I think this is a messier way to do what we can already do with ifdefs, > since it requires messing about within the build system. I'm just doing the same as WebGL 1, if you would like to change that, my mind is that should be in another bug. > > ::: content/html/content/src/HTMLCanvasElement.cpp > @@ +684,5 @@ > > ctx.forget(aContext); > > return NS_OK; > > } > > + else if (WebGL2Context::IsSupported() && > > + aContextId.EqualsLiteral("experimental-webgl2")) > > This is surely not the best place for this, though I'm not certain who to > ask. Maybe dzbarsky knows? Benoit Jacob has explicitly said me to add it here temporally ... > > ::: dom/bindings/Bindings.conf > @@ +1286,5 @@ > > 'headerFile': 'WebGLRenderbuffer.h' > > }, > > > > 'WebGLRenderingContext': { > > + 'nativeType': 'mozilla::WebGLContext', > > Yeah, let's ask dzbarsky. > > ::: modules/libpref/src/init/all.js > @@ +4018,5 @@ > > pref("webgl.can-lose-context-in-foreground", true); > > pref("webgl.max-warnings-per-context", 32); > > pref("webgl.enable-draft-extensions", false); > > +#ifndef RELEASE_BUILD > > +pref("webgl.enable-prototype-webgl2", false); > > It's actually probably best to not mention this flag, for now. Just remove > it from all.js until we're more ready to have people discover it. May be #ifdef RELEASE_BUILD is enough to not use the webgl2 prototype ...

Can we change the WebGL2Context creation in another patch ? Because has the preference flag said, this is a prototype ... And I would rather land the 5 following patch remaining instead.

Comment on attachment 776105 [details] [diff] [review] patch revision 3 Review of attachment 776105 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +1286,5 @@ > 'headerFile': 'WebGLRenderbuffer.h' > }, > > 'WebGLRenderingContext': { > + 'nativeType': 'mozilla::WebGLContext', You want WebGLRenderingContext's nativetype to be WebGLContext and WebGLRenderingContext2's nativetype to be WebGL2Context. http://pastebin.mozilla.org/2640997

Comment on attachment 776397 [details] [diff] [review] patch revision 4 Review of attachment 776397 [details] [diff] [review]: ----------------------------------------------------------------- Switch to WebGL2RenderingContext unless there's a reason not to match what we did with WebGL2Context. ::: content/canvas/src/WebGLContextGL.cpp @@ +2118,5 @@ > + > + if (IsWebGL2()) { > + version = "WebGL 2.0"; > + } > + else { `} else {` ::: dom/webidl/WebGLRenderingContext2.webidl @@ +5,5 @@ > + * > + * This IDL depend on WebGLRenderingContext.webidl > + */ > + > +interface WebGLRenderingContext2 : WebGLRenderingContext { Why did we not switch to WebGL2RenderingContext here as well?

(In reply to Guillaume Abadie from comment #10) > Created attachment 776397 [details] [diff] [review] > patch revision 4 > > Can we change the WebGL2Context creation in another patch ? Because has the > preference flag said, this is a prototype ... And I would rather land the 5 > following patch remaining instead. In general, no. It's not a good idea to take new code which we immediately plan on getting rid of. Review is review, regardless of how annoying it is, or how many patches depend on something. The only alternative is technical debt.

Comment on attachment 776760 [details] [diff] [review] patch revision 5 Review of attachment 776760 [details] [diff] [review]: ----------------------------------------------------------------- Noticed some nits while is was compiling some code. ::: content/canvas/src/WebGL2Context.cpp @@ +41,5 @@ > + > +WebGL2Context* > +WebGL2Context::Create() > +{ > +#ifdef RELEASE_BUILD I think this should never be hit. ::: content/canvas/src/WebGLContextGL.cpp @@ +2113,5 @@ > case LOCAL_GL_RENDERER: > return StringValue(cx, "Mozilla", rv); > case LOCAL_GL_VERSION: > + { > + const char* version = 0; nullptr probably, or even don't initialize. @@ +2121,5 @@ > + } else { > + version = "WebGL 1.0"; > + } > + > + MOZ_ASSERT(version != 0); can't really be null, seems like this could be removed. ::: content/html/content/src/HTMLCanvasElement.cpp @@ +690,5 @@ > + Telemetry::Accumulate(Telemetry::CANVAS_WEBGL_USED, 1); > + nsRefPtr<WebGL2Context> ctx = WebGL2Context::Create(); > + > + if (ctx == nullptr) { > + return NS_ERROR_NOT_IMPLEMENTED; Why is that not implemented, shouldn't the IsSupported test check for that? ::: dom/webidl/WebGL2RenderingContext.webidl @@ +2,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * This IDL depend on WebGLRenderingContext.webidl depends

(In reply to Tom Schuster [:evilpie] from comment #17) > Comment on attachment 776760 [details] [diff] [review] > patch revision 5 > > Review of attachment 776760 [details] [diff] [review]: > ----------------------------------------------------------------- > > Noticed some nits while is was compiling some code. > > ::: content/canvas/src/WebGL2Context.cpp > @@ +41,5 @@ > > + > > +WebGL2Context* > > +WebGL2Context::Create() > > +{ > > +#ifdef RELEASE_BUILD > > I think this should never be hit. I disagree, we don't want WebGL 2 prototype context in releases because it is not secure at all. And the WebGL2Context creation is still experimental, and will be surly changed. Therefore we keep as many #ifdef RELEASE_BUILD as possible to prevent futur changes > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +2113,5 @@ > > case LOCAL_GL_RENDERER: > > return StringValue(cx, "Mozilla", rv); > > case LOCAL_GL_VERSION: > > + { > > + const char* version = 0; > > nullptr probably, or even don't initialize. Its true nullptr would be the most appropriate for C++11, but = 0 is also legal regarding C/C++ specification. > > @@ +2121,5 @@ > > + } else { > > + version = "WebGL 1.0"; > > + } > > + > > + MOZ_ASSERT(version != 0); > > can't really be null, seems like this could be removed. True, but I prefere have a code to much strong for now and preventing futuf changes. Cost nothing keep. > > ::: content/html/content/src/HTMLCanvasElement.cpp > @@ +690,5 @@ > > + Telemetry::Accumulate(Telemetry::CANVAS_WEBGL_USED, 1); > > + nsRefPtr<WebGL2Context> ctx = WebGL2Context::Create(); > > + > > + if (ctx == nullptr) { > > + return NS_ERROR_NOT_IMPLEMENTED; > > Why is that not implemented, shouldn't the IsSupported test check for that? Because WebGL2Context::Create can return null if all the WebGL2 code is removed at the compilation. > > ::: dom/webidl/WebGL2RenderingContext.webidl > @@ +2,5 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > > + * You can obtain one at http://mozilla.org/MPL/2.0/. > > + * > > + * This IDL depend on WebGLRenderingContext.webidl > > depends Oups ... =)