Closed Bug 890311 Opened 7 years ago Closed 6 years ago

WebGL2 Add WebGL2Context inheriting from WebGLContext

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

References

()

Details

Attachments

(1 file, 4 obsolete files)

Add WebGLContext2 C++ class inheriting WebGLContextBase 


See https://wiki.mozilla.org/Platform/GFX/WebGL2
Assignee: nobody → gabadie
Blocks: webgl2
Depends on: 890049
Blocks: 890379
Blocks: 891033
Attached patch patch revision 1 (obsolete) — Splinter Review
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"
Attachment #772216 - Flags: review?(jgilbert)
Blocks: 892169
Summary: Add WebGLContext2 C++ class inheriting WebGLContextBase → WebGL2 Add WebGL2Context inheriting from WebGLContext
Attached patch patch revision 2 (obsolete) — Splinter Review
Attachment #772216 - Attachment is obsolete: true
Attachment #772216 - Flags: review?(jgilbert)
Attachment #774398 - Flags: review?(jgilbert)
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]
Attachment #774398 - Flags: review?(jgilbert) → review-
(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
Attached patch patch revision 3 (obsolete) — Splinter Review
Attachment #774398 - Attachment is obsolete: true
Attachment #776105 - Flags: review?(jgilbert)
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.
Attachment #776105 - Flags: feedback?(dzbarsky)
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 ...
Attached patch patch revision 4 (obsolete) — Splinter Review
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.
Attachment #776105 - Attachment is obsolete: true
Attachment #776105 - Flags: review?(jgilbert)
Attachment #776105 - Flags: feedback?(dzbarsky)
Attachment #776397 - Flags: review?(jgilbert)
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
Attachment #776105 - Attachment is obsolete: false
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?
Attachment #776397 - Flags: review?(jgilbert) → review+
(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.
Attached patch patch revision 5Splinter Review
Attachment #776105 - Attachment is obsolete: true
Attachment #776397 - Attachment is obsolete: true
Attachment #776760 - Flags: review?(jgilbert)
Attachment #776760 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
Keywords: checkin-needed
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 ... =)
https://hg.mozilla.org/mozilla-central/rev/998f732e370c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 895855
Depends on: 896254
Blocks: 898615
Blocks: 908662
You need to log in before you can comment on or make changes to this bug.