908841 - WebGL2 Add more natively supported extensions

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Add native support of followed extension in WebGL 2:
 - OES_element_index_uint
 - OES_standard_derivatives
 - OES_texture_float
 - OES_texture_float_linear
 - WEBGL_depth_texture

Comment 1

[Guillaume Abadie]

Attachment: patch step 1 - revision 1: Move WebGL extension mechanism in WebGLContextExtensions.cpp

Comment 2

[Guillaume Abadie]

Attachment: patch step 2 - revision 1: New string conversion for WebGL extensions

The objective is to have:
 static const char* WebGLContext::GetExtensionString(WebGLExtensionID ext)

for following patches.

Comment 3

[Guillaume Abadie]

Attachment: patch step 3 revision 1: Change the WebGL 2 validation system.

Comment 4

[Guillaume Abadie]

Attachment: patch step 4 - revision 1: Add WebGL extension as natively supported in WebGL 2.0

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 794909 [details] [diff] [review]
patch step 2 - revision 1: New string conversion for WebGL extensions

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

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +158,4 @@
>      {
> +        if (CompareWebGLExtensionName(name, "MOZ_WEBGL_lose_context")) {
> +            ext = WEBGL_lose_context;
> +        }

Why do we still have these? Why aren't they caught by the for loop above?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 794913 [details] [diff] [review]
patch step 3 revision 1: Change the WebGL 2 validation system.

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

::: content/canvas/src/WebGL2Context.cpp
@@ +71,5 @@
> +        GLFeature::transform_feedback
> +    };
> +
> +    // check WebGL extensions that are supposed to be natively supported
> +    for (size_t i = 0; i < size_t(MOZ_ARRAY_LENGTH(sExtensionNativelySupportedArr)); i++)

Does MOZ_ARRAY_LENGTH really not return a size_t?

Comment 7

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 794909 [details] [diff] [review]
> patch step 2 - revision 1: New string conversion for WebGL extensions
> 
> Review of attachment 794909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextExtensions.cpp
> @@ +158,4 @@
> >      {
> > +        if (CompareWebGLExtensionName(name, "MOZ_WEBGL_lose_context")) {
> > +            ext = WEBGL_lose_context;
> > +        }
> 
> Why do we still have these? Why aren't they caught by the for loop above?
Because if it were, we would have to put those strings in the const array, and so having different indices for same extensions to keep the 1:1 matching with the enumeration. So I did like that instead, and because we are not going to add MOZ_ prefix anymore thanks by the draft extension flag like Chrome does, it is okay to just accept them here for compatibility.

Comment 8

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 794913 [details] [diff] [review]
> patch step 3 revision 1: Change the WebGL 2 validation system.
> 
> Review of attachment 794913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGL2Context.cpp
> @@ +71,5 @@
> > +        GLFeature::transform_feedback
> > +    };
> > +
> > +    // check WebGL extensions that are supposed to be natively supported
> > +    for (size_t i = 0; i < size_t(MOZ_ARRAY_LENGTH(sExtensionNativelySupportedArr)); i++)
> 
> Does MOZ_ARRAY_LENGTH really not return a size_t?
Oups... Good catch! Fixed.

Comment 9

[Guillaume Abadie]

Attachment: patch step 4 - revision 2: Add WebGL extension as natively supported in WebGL 2.0

I have forgotten to qrefresh, again... So this patch just add constants in the WebGL 2's WebIDL.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #7)
> (In reply to Jeff Gilbert [:jgilbert] from comment #5)
> > Comment on attachment 794909 [details] [diff] [review]
> > patch step 2 - revision 1: New string conversion for WebGL extensions
> > 
> > Review of attachment 794909 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/canvas/src/WebGLContextExtensions.cpp
> > @@ +158,4 @@
> > >      {
> > > +        if (CompareWebGLExtensionName(name, "MOZ_WEBGL_lose_context")) {
> > > +            ext = WEBGL_lose_context;
> > > +        }
> > 
> > Why do we still have these? Why aren't they caught by the for loop above?
> Because if it were, we would have to put those strings in the const array,
> and so having different indices for same extensions to keep the 1:1 matching
> with the enumeration. So I did like that instead, and because we are not
> going to add MOZ_ prefix anymore thanks by the draft extension flag like
> Chrome does, it is okay to just accept them here for compatibility.

Oh, ok. It'd be nice to leave a comment that says 'check for vendor-prefixed extensions for backwards compatibility', or something.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 794909 [details] [diff] [review]
patch step 2 - revision 1: New string conversion for WebGL extensions

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

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +158,4 @@
>      {
> +        if (CompareWebGLExtensionName(name, "MOZ_WEBGL_lose_context")) {
> +            ext = WEBGL_lose_context;
> +        }

Let's leave a comment that these are here so we can still support the deprecated vendor-prefixed aliases.

Comment 12

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> Comment on attachment 794909 [details] [diff] [review]
> patch step 2 - revision 1: New string conversion for WebGL extensions
> 
> Review of attachment 794909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextExtensions.cpp
> @@ +158,4 @@
> >      {
> > +        if (CompareWebGLExtensionName(name, "MOZ_WEBGL_lose_context")) {
> > +            ext = WEBGL_lose_context;
> > +        }
> 
> Let's leave a comment that these are here so we can still support the
> deprecated vendor-prefixed aliases.

Fixed! Thanks!

Comment 13

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce3466b3c49
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad33575aad4
https://hg.mozilla.org/integration/mozilla-inbound/rev/57b672779178
https://hg.mozilla.org/integration/mozilla-inbound/rev/6156c0dc50bf

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ce3466b3c49
https://hg.mozilla.org/mozilla-central/rev/4ad33575aad4
https://hg.mozilla.org/mozilla-central/rev/57b672779178
https://hg.mozilla.org/mozilla-central/rev/6156c0dc50bf

Comment 15

[Kelsey Gilbert [:jgilbert]]

I don't see what docs this needs.