904330 - GLContext extension (group) queries renaming

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Guillaume Abadie]

Switch
  IsExtensionSupported(ARB_draw_buffers) 
by:
  IsSupported(GLExtension::ARB_draw_buffers)

And
  IsExtensionSupported(XXX_draw_buffers)
by:
  IsSupported(GLFeature::draw_buffers)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

(In reply to Guillaume Abadie from comment #0)
> Switch
>   IsExtensionSupported(ARB_draw_buffers) 
> by:
>   IsSupported(GLExtension::ARB_draw_buffers)

I don't see any reason to change IsExtensionSupported. Testing extensions and testing features are different things, we don't need to have them using the same interface.

Comment 2

[Guillaume Abadie]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> (In reply to Guillaume Abadie from comment #0)
> > Switch
> >   IsExtensionSupported(ARB_draw_buffers) 
> > by:
> >   IsSupported(GLExtension::ARB_draw_buffers)
> 
> I don't see any reason to change IsExtensionSupported. Testing extensions
> and testing features are different things, we don't need to have them using
> the same interface.
That was just a subjection from jgilbert to have similar mechanism for Extension a Features. But I can just do the second one, I don't really mind. The only thing is we should leave the gl namespace, because the mozilla::gl::GLContext is redundant.
 So the enumeration for features will directly be mozilla::GLFeature::foo_bar.

Comment 3

[Guillaume Abadie]

Attachment: patch step 1 - revision 1: Add mozilla::GLFeature

Comment 4

[Guillaume Abadie]

Attachment: patch step 2 - revision 2: Remane occlusion queries

Comment 5

[Guillaume Abadie]

Attachment: patch step 3 - revision 1: Let use GLFeatures instead off extension group queries

Comment 6

[Guillaume Abadie]

Attachment: patch step 4 - revision 1: Remove extension group queries

Comment 7

[Guillaume Abadie]

Attachment: patch step 5: rename GLContextExtensionGroupQueries.cpp to GLContextFeatures.cpp

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 792271 [details] [diff] [review]
patch step 1 - revision 1: Add mozilla::GLFeature

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

::: gfx/gl/GLContext.h
@@ +86,5 @@
> +/** GLFeature::Enum
> + * We don't use typed enum to keep the implicit integer conversion.
> + * This enum should be sorted by name.
> + */
> +namespace GLFeature {

This is better as mozilla::gl::Feature, or possibly GLContext::Feature. I think I like GLContext::Feature best.

@@ +561,5 @@
> +
> +// -----------------------------------------------------------------------------
> +// Feature queries
> +/*
> + * This mecahnism introduces a new way to check if a OpenGL feature is

'mechanism'

@@ +562,5 @@
> +// -----------------------------------------------------------------------------
> +// Feature queries
> +/*
> + * This mecahnism introduces a new way to check if a OpenGL feature is
> + * supported, regardless if it is supported by an extension or natively by

s/if/of whether/

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +284,3 @@
>  
> +    MOZ_ASSERT(feature < GLFeature::EnumMax,
> +               "GLContext::GetFeatureInfoInfo : unknown <extensionGroup>");

s/extensionGroup/feature/

@@ +311,2 @@
>  
>      return profileVersion && version >= profileVersion;

Note that if `profileVersion` is zero, it means that no version of the profile added support for the feature. (Or rather, that the feature is was never added to any version of the profile requested)

Comment 9

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 792271 [details] [diff] [review]
> patch step 1 - revision 1: Add mozilla::GLFeature
> 
> Review of attachment 792271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +86,5 @@
> > +/** GLFeature::Enum
> > + * We don't use typed enum to keep the implicit integer conversion.
> > + * This enum should be sorted by name.
> > + */
> > +namespace GLFeature {
> 
> This is better as mozilla::gl::Feature, or possibly GLContext::Feature. I
> think I like GLContext::Feature best.
mozilla::gl::Feature is not verygood, because with a using namespace we would have collisions. GLContext::Feature is maybe not good either, because that would mean that the code have to be in GLContext.h, where like this, the enumeration can be move into a separated .h. I'm going to put back GLFeature in mozilla::gl.
> 
> @@ +561,5 @@
> > +
> > +// -----------------------------------------------------------------------------
> > +// Feature queries
> > +/*
> > + * This mecahnism introduces a new way to check if a OpenGL feature is
> 
> 'mechanism'
Oups...
> 
> @@ +562,5 @@
> > +// -----------------------------------------------------------------------------
> > +// Feature queries
> > +/*
> > + * This mecahnism introduces a new way to check if a OpenGL feature is
> > + * supported, regardless if it is supported by an extension or natively by
> 
> s/if/of whether/
fixed!
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +284,3 @@
> >  
> > +    MOZ_ASSERT(feature < GLFeature::EnumMax,
> > +               "GLContext::GetFeatureInfoInfo : unknown <extensionGroup>");
> 
> s/extensionGroup/feature/
Oups...
> 
> @@ +311,2 @@
> >  
> >      return profileVersion && version >= profileVersion;
> 
> Note that if `profileVersion` is zero, it means that no version of the
> profile added support for the feature. (Or rather, that the feature is was
> never added to any version of the profile requested)
Ok!

Comment 10

[Guillaume Abadie]

Tbpl: https://tbpl.mozilla.org/?tree=Try&rev=4a43bd815af7

Landing on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eed6186b3e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/01b8243b60bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c6dcbd6fc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/69b03cba9f14
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e7c8687feb

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eed6186b3e1
https://hg.mozilla.org/mozilla-central/rev/01b8243b60bb
https://hg.mozilla.org/mozilla-central/rev/34c6dcbd6fc7
https://hg.mozilla.org/mozilla-central/rev/69b03cba9f14
https://hg.mozilla.org/mozilla-central/rev/d0e7c8687feb

Comment 12

[Tony Chung [:tchung]]

Can we back this patch out?   this bug has caused a regression in bug 908449, which is a b2g smoketest blocker on master/mozilla-central.

Comment 13

[Guillaume Abadie]

(In reply to Tony Chung [:tchung] from comment #12)
> Can we back this patch out?   this bug has caused a regression in bug
> 908449, which is a b2g smoketest blocker on master/mozilla-central.
Which one? This would need backout many WebGL 2 patches... And as I see, we are not sure this patch has caused the problem.

Cauted from bug 908449:
> Talked with jgilbert - he thinks bug 904330 is probably the root cause.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Tony Chung [:tchung] from comment #12)
> Can we back this patch out?   this bug has caused a regression in bug
> 908449, which is a b2g smoketest blocker on master/mozilla-central.

No. Let's just fix the regression instead.
As Guillaume says, a large number of dependencies on this bug has already landed. The churn would be more dangerous than just spot-fixing.