GLContext extension (group) queries renaming

RESOLVED FIXED in mozilla26

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Guillaume Abadie, Assigned: Guillaume Abadie)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

4 years ago
Switch
  IsExtensionSupported(ARB_draw_buffers) 
by:
  IsSupported(GLExtension::ARB_draw_buffers)

And
  IsExtensionSupported(XXX_draw_buffers)
by:
  IsSupported(GLFeature::draw_buffers)
(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.
(Assignee)

Comment 2

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 905161
(Assignee)

Comment 3

4 years ago
Created attachment 792271 [details] [diff] [review]
patch step 1 - revision 1: Add mozilla::GLFeature
Attachment #792271 - Flags: review?(jgilbert)
(Assignee)

Comment 4

4 years ago
Created attachment 792273 [details] [diff] [review]
patch step 2 - revision 2: Remane occlusion queries
Attachment #792273 - Flags: review?(jgilbert)
(Assignee)

Comment 5

4 years ago
Created attachment 792274 [details] [diff] [review]
patch step 3 - revision 1: Let use GLFeatures instead off extension group queries
Attachment #792274 - Flags: review?(jgilbert)
(Assignee)

Comment 6

4 years ago
Created attachment 792275 [details] [diff] [review]
patch step 4 - revision 1: Remove extension group queries
Attachment #792275 - Flags: review?(jgilbert)
(Assignee)

Comment 7

4 years ago
Created attachment 792291 [details] [diff] [review]
patch step 5: rename GLContextExtensionGroupQueries.cpp to GLContextFeatures.cpp
Attachment #792291 - Flags: review?(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)
Attachment #792271 - Flags: review?(jgilbert) → review+
Attachment #792273 - Flags: review?(jgilbert) → review+
Attachment #792274 - Flags: review?(jgilbert) → review+
Attachment #792275 - Flags: review?(jgilbert) → review+
Attachment #792291 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 9

4 years ago
(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!
(Assignee)

Comment 10

4 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Updated

4 years ago
Blocks: 908841

Updated

4 years ago
Depends on: 908449
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.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 13

4 years ago
(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.
(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.
Flags: needinfo?(ryanvm)

Updated

4 years ago
No longer depends on: 908449
You need to log in before you can comment on or make changes to this bug.