902063 - GLContext complete extension group queries

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Guillaume Abadie]

There too few extensions groups for now. WebGL should fully use it instead of testing each extensions.

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

This patch bring:
 - XXX_depth_texture,
 - XXX_element_index_uint,
 - XXX_ES2_compatibility,
 - XXX_ES3_compatibility,
 - XXX_occlusion_query,
 - XXX_occlusion_query_boolean,
 - XXX_occlusion_query2,
 - XXX_packed_depth_stencil,
 - XXX_query_objects,
 - XXX_query_objects_iv_getter,
 - XXX_standard_derivatives,
 - XXX_texture_float_linear,

It also bring change the symbols loading for query objects, using XXX_query_objects, and let WebGL extensions use those extension group queries.

To others bug depending on this one will come:
- one would just enhance WebGL 2's query objects for simulating ANY_SAMPLES_PASSED depending on extension group queries,
- one other would change the WebGL 2 validation to add some missing WebGL 1 extensions.

Comment 2

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #0)
> There too few extensions groups for now. WebGL should fully use it instead
> of testing each extensions.

I disagree with this premise, assuming we have no other GLContext clients which need this functionality.

Comment 3

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> (In reply to Guillaume Abadie from comment #0)
> > There too few extensions groups for now. WebGL should fully use it instead
> > of testing each extensions.
> 
> I disagree with this premise, assuming we have no other GLContext clients
> which need this functionality.
After reflection (like we say in France, night bring us advises), I think this is the best solution because of those followed points:
1) Extension group queries already have the mechanism to simulate that an extension is/group of extensions are supported depending of the OpenGL context and profile, and the cost to add OpenGL extensions used by WebGL in this system is nothing (actually the patch is already ready for review as you can see).
2) That is a bit weird to say that a extension is supported even it's not in GL_EXTENSIONS where it's ok to say that a feature like XXX_draw_instanced is supported even if {ARB,EXT,NV}_draw_instanced are not in GL_EXTENSIONS.
3) Some that could be nice, is to check our self that OpenGL ES 3 is supported by simply checking that all extension groups having (mOpenGLESVersion && mOpenGLESVersion <= 300) are supported. Ideal for validating a WebGL 2 context!

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 786902 [details] [diff] [review]
patch revision 1

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

::: content/canvas/src/WebGLContext.cpp
@@ +973,5 @@
>      }
>  
>      switch (ext) {
>          case OES_element_index_uint:
> +            return gl->IsExtensionSupported(GLContext::XXX_element_index_uint);

I am realizing now, that IsExtensionSupported is now a very misleading name, and also "ExtensionGroup" is misleading, because this can return true on desktop GL where no such extension exists.

How about changing the terminology:
ExtensionGroup ---> Feature
IsExtensionSupported(group)  ---> IsFeatureSupported(feature)

(In a follow-up bug and patch)

::: content/canvas/src/WebGLContextValidate.cpp
@@ +999,5 @@
>           !gl->IsExtensionSupported(gl::GLContext::EXT_blend_minmax) ||
>           !gl->IsExtensionSupported(gl::GLContext::XXX_draw_instanced) ||
>           !gl->IsExtensionSupported(gl::GLContext::XXX_instanced_arrays) ||
> +         (!gl->IsExtensionSupported(gl::GLContext::XXX_occlusion_query) &&
> +          !gl->IsExtensionSupported(gl::GLContext::XXX_occlusion_query_boolean))

This huge boolean condition is too huge (the problem existed before this patch).

Can you do a follow-up bug and patch to rewrite split that code into separate if()'s ?

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +14,5 @@
>  
> +static const unsigned int kOpenGLES2CompatibilityProfileVersion = 410;
> +/**
> + * ARB_ES2_compatibility is natively supported in OpenGL 4.1.
> + */

Comments should be *above* what they refer to.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> (In reply to Guillaume Abadie from comment #0)
> > There too few extensions groups for now. WebGL should fully use it instead
> > of testing each extensions.
> 
> I disagree with this premise, assuming we have no other GLContext clients
> which need this functionality.

I looked at these "group queries" and even the ones that contain only one, or even zero, actual GL extensions, seem quite useful because they abstract/implement the logic of checking if the OpenGL profile/version implicitly offers this feature (so that no extension is needed).

Comment 6

[Guillaume Abadie]

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Comment on attachment 786902 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 786902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +973,5 @@
> >      }
> >  
> >      switch (ext) {
> >          case OES_element_index_uint:
> > +            return gl->IsExtensionSupported(GLContext::XXX_element_index_uint);
> 
> I am realizing now, that IsExtensionSupported is now a very misleading name,
> and also "ExtensionGroup" is misleading, because this can return true on
> desktop GL where no such extension exists.
> 
> How about changing the terminology:
> ExtensionGroup ---> Feature
> IsExtensionSupported(group)  ---> IsFeatureSupported(feature)
> 
> (In a follow-up bug and patch)
I definitely agree! My proposal is doing that with something like: IsSupported(Feature::FooBar)
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +999,5 @@
> >           !gl->IsExtensionSupported(gl::GLContext::EXT_blend_minmax) ||
> >           !gl->IsExtensionSupported(gl::GLContext::XXX_draw_instanced) ||
> >           !gl->IsExtensionSupported(gl::GLContext::XXX_instanced_arrays) ||
> > +         (!gl->IsExtensionSupported(gl::GLContext::XXX_occlusion_query) &&
> > +          !gl->IsExtensionSupported(gl::GLContext::XXX_occlusion_query_boolean))
> 
> This huge boolean condition is too huge (the problem existed before this
> patch).
> 
> Can you do a follow-up bug and patch to rewrite split that code into
> separate if()'s ?
I was planing to loop on all features that are marked as natively supported in OpenGL ES 3 depending on this patch...
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +14,5 @@
> >  
> > +static const unsigned int kOpenGLES2CompatibilityProfileVersion = 410;
> > +/**
> > + * ARB_ES2_compatibility is natively supported in OpenGL 4.1.
> > + */
> 
> Comments should be *above* what they refer to.
Oups...

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 786902 [details] [diff] [review]
patch revision 1

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

::: gfx/gl/GLContext.cpp
@@ +670,5 @@
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects_iv_getter);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query_boolean);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query2);

Shoot, it looks like I missed this earlier. We really don't want to break people's GLContext creation when we can just work around it. We should instead discuss making the `NS_ERROR(` above into a `MOZ_ASSERT(false, `.

::: gfx/gl/GLContext.h
@@ +2116,5 @@
> + * XXX_query_objects:
> + *  - provide all followed entry points
> + *
> + * XXX_occlusion_query2:
> + *  - depend on XXX_query_objects

'depends'

@@ +2124,5 @@
> + *  - depend on XXX_occlusion_query2
> + *  - provide ANY_SAMPLES_PASSED_CONSERVATIVE
> + */
> +public:
> +    void fDeleteQueries(GLsizei n, const GLuint* names) {

This body is for raw_fDeleteQueries, not fDeleteQueries.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +11,5 @@
>  namespace gl {
>  
>  const size_t kMAX_EXTENSION_GROUP_SIZE = 5;
>  
> +static const unsigned int kOpenGLES2CompatibilityProfileVersion = 410;

kGLCoreVersionForES2Compat

@@ +198,5 @@
> +         * (added in OpenGL ES 3.0)
> +         */
> +    },
> +    {
> +        "XXX_query_objects_iv_getter",

s/query_objects_iv_getter/get_query_object_iv/

Comment 8

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 786902 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 786902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.cpp
> @@ +670,5 @@
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects_iv_getter);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query_boolean);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query2);
> 
> Shoot, it looks like I missed this earlier. We really don't want to break
> people's GLContext creation when we can just work around it. We should
> instead discuss making the `NS_ERROR(` above into a `MOZ_ASSERT(false, `.
No. MarkExtensionGroupUnsupported returns false only if this feature should be natively supported by the given mVersion and mProfile. Or may be MarkExtensionGroupUnsupported should return false only if this is supposed to be natively supported by an OpenGL (ES) 2.0?...

http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextExtensionGroupQueries.cpp#207
> 
> ::: gfx/gl/GLContext.h
> @@ +2116,5 @@
> > + * XXX_query_objects:
> > + *  - provide all followed entry points
> > + *
> > + * XXX_occlusion_query2:
> > + *  - depend on XXX_query_objects
> 
> 'depends'
Oups...
> 
> @@ +2124,5 @@
> > + *  - depend on XXX_occlusion_query2
> > + *  - provide ANY_SAMPLES_PASSED_CONSERVATIVE
> > + */
> > +public:
> > +    void fDeleteQueries(GLsizei n, const GLuint* names) {
> 
> This body is for raw_fDeleteQueries, not fDeleteQueries.
But raw_fDeleteQueries would be private used only by fDeleteQueries anyway...
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +11,5 @@
> >  namespace gl {
> >  
> >  const size_t kMAX_EXTENSION_GROUP_SIZE = 5;
> >  
> > +static const unsigned int kOpenGLES2CompatibilityProfileVersion = 410;
> 
> kGLCoreVersionForES2Compat
Fixed.
> 
> @@ +198,5 @@
> > +         * (added in OpenGL ES 3.0)
> > +         */
> > +    },
> > +    {
> > +        "XXX_query_objects_iv_getter",
> 
> s/query_objects_iv_getter/get_query_object_iv/
I wanted to keep the "query_objects" at the beginning to match the XXX_query_objects, but I can change that. I don't mind. =)

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #8)
> (In reply to Jeff Gilbert [:jgilbert] from comment #7)
> > Comment on attachment 786902 [details] [diff] [review]
> > patch revision 1
> > 
> > Review of attachment 786902 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +2124,5 @@
> > > + *  - depend on XXX_occlusion_query2
> > > + *  - provide ANY_SAMPLES_PASSED_CONSERVATIVE
> > > + */
> > > +public:
> > > +    void fDeleteQueries(GLsizei n, const GLuint* names) {
> > 
> > This body is for raw_fDeleteQueries, not fDeleteQueries.
> But raw_fDeleteQueries would be private used only by fDeleteQueries anyway...
So delete raw_fDeleteQueries, and fold its code into fDeleteQueries. Don't keep two of them with the same code.

Comment 10

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> (In reply to Guillaume Abadie from comment #8)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #7)
> > > Comment on attachment 786902 [details] [diff] [review]
> > > patch revision 1
> > > 
> > > Review of attachment 786902 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > @@ +2124,5 @@
> > > > + *  - depend on XXX_occlusion_query2
> > > > + *  - provide ANY_SAMPLES_PASSED_CONSERVATIVE
> > > > + */
> > > > +public:
> > > > +    void fDeleteQueries(GLsizei n, const GLuint* names) {
> > > 
> > > This body is for raw_fDeleteQueries, not fDeleteQueries.
> > But raw_fDeleteQueries would be private used only by fDeleteQueries anyway...
> So delete raw_fDeleteQueries, and fold its code into fDeleteQueries. Don't
> keep two of them with the same code.
Ok.

Also for mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects); We should do in a separated patch, because we should cache features queries, and for now, MarkExtensionGroupUnsupported on mark extensions as unsupported. So if it's natively supoirted by the given mVersion and mProfile, the feature would stay supported after. That way for now, if MarkExtensionGroupUnsupported fail we fail the context creation.

Comment 11

[Guillaume Abadie]

Here it is https://bugzilla.mozilla.org/show_bug.cgi?id=904330

Comment 12

[Guillaume Abadie]

Attachment: patch revision 2 (r+ by bjacob)

Comment 13

[Guillaume Abadie]

Attachment: patch revision 3 (r+ by bjacob)

Forgot to qrefresh before. =D

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 790233 [details] [diff] [review]
patch revision 3 (r+ by bjacob)

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

We can't stop giving people GLContexts because our fancy new group system doesn't support them. We need to fix the system, and then we can use it.

::: gfx/gl/GLContext.cpp
@@ +670,5 @@
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_get_query_object_iv);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query_boolean);
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query2);

We shouldn't r+ something that could (further) break people's GLContext creation. If we can't remove this initialization failure, this bug becomes blocked on fixing feature queries such that we don't need to abort initialization.

Other existing cases of this should also be fixed, and fixing them should be fairly high priority.

::: gfx/gl/GLContext.h
@@ +2128,5 @@
> +    void fDeleteQueries(GLsizei n, const GLuint* names) {
> +        BEFORE_GL_CALL;
> +        ASSERT_SYMBOL_PRESENT(fDeleteQueries);
> +        mSymbols.fDeleteQueries(n, names);
> +        AFTER_GL_CALL;

This is missing its TRACKING_CONTEXT line.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +13,5 @@
>  const size_t kMAX_EXTENSION_GROUP_SIZE = 5;
>  
> +/**
> + * ARB_ES2_compatibility is natively supported in OpenGL 4.1.
> + */

Use single-line comments for single lines.

@@ +122,5 @@
>      },
>      {
> +        "XXX_get_query_object_iv",
> +        200, // OpenGL version
> +        0,   // OpenGL ES version

We should probably use a sentinel for VERSION_NEVER, or similar. '0' is sort of a magic var that paradoxically means never, instead of always.

It would actually likely be cleaner to have VERSION_NEVER be 10000, so we can unconditionally check against the version number, instead of special-casing for zero. Also, zero can then mean 'always had this'.

@@ +129,5 @@
> +        }
> +        /*
> +         * XXX_get_query_object_iv only provide GetQueryObjectiv provided by
> +         * ARB_occlusion_query (added by OpenGL 2.0).
> +         */

These doesn't need to be a block-format comment.

@@ +151,5 @@
> +            GLContext::Extensions_End
> +        }
> +        /*
> +         * XXX_occlusion_query depend on ARB_occlusion_query (added in OpenGL 2.0)
> +         */

Don't use block comments for just one line.
The rule of thumb is generally the third line of comments means to switch to a block comment.

@@ +155,5 @@
> +         */
> +    },
> +    {
> +        "XXX_occlusion_query_boolean",
> +        kGLCoreVersionForES3Compat,   // OpenGL version

This line doesn't really need this comment after the variable name change.

@@ +171,5 @@
> +         */
> +    },
> +    {
> +        "XXX_occlusion_query2",
> +        330, // = min(330, kGLCoreVersionForES3Compat), // OpenGL version

Why not actually set it to std::min()?

Comment 15

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 790233 [details] [diff] [review]
> patch revision 3 (r+ by bjacob)
> 
> Review of attachment 790233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We can't stop giving people GLContexts because our fancy new group system
> doesn't support them. We need to fix the system, and then we can use it.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +670,5 @@
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_get_query_object_iv);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query_boolean);
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_occlusion_query2);
> 
> We shouldn't r+ something that could (further) break people's GLContext
> creation. If we can't remove this initialization failure, this bug becomes
> blocked on fixing feature queries such that we don't need to abort
> initialization.
> 
> Other existing cases of this should also be fixed, and fixing them should be
> fairly high priority.
While GL_VERSION parser is not landed because of B2G bustage, this system doesn't block anything for now. And we just have to delay GL_VERSION parser, and the behavior stay like it was before before the fix.
> 
> ::: gfx/gl/GLContext.h
> @@ +2128,5 @@
> > +    void fDeleteQueries(GLsizei n, const GLuint* names) {
> > +        BEFORE_GL_CALL;
> > +        ASSERT_SYMBOL_PRESENT(fDeleteQueries);
> > +        mSymbols.fDeleteQueries(n, names);
> > +        AFTER_GL_CALL;
> 
> This is missing its TRACKING_CONTEXT line.
Oups...
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +13,5 @@
> >  const size_t kMAX_EXTENSION_GROUP_SIZE = 5;
> >  
> > +/**
> > + * ARB_ES2_compatibility is natively supported in OpenGL 4.1.
> > + */
> 
> Use single-line comments for single lines.
Fixed.
> 
> @@ +122,5 @@
> >      },
> >      {
> > +        "XXX_get_query_object_iv",
> > +        200, // OpenGL version
> > +        0,   // OpenGL ES version
> 
> We should probably use a sentinel for VERSION_NEVER, or similar. '0' is sort
> of a magic var that paradoxically means never, instead of always.
> 
> It would actually likely be cleaner to have VERSION_NEVER be 10000, so we
> can unconditionally check against the version number, instead of
> special-casing for zero. Also, zero can then mean 'always had this'.
This is not the purpose of this patch, but this is interesting.
> 
> @@ +129,5 @@
> > +        }
> > +        /*
> > +         * XXX_get_query_object_iv only provide GetQueryObjectiv provided by
> > +         * ARB_occlusion_query (added by OpenGL 2.0).
> > +         */
> 
> These doesn't need to be a block-format comment.
2 lines...
> 
> @@ +151,5 @@
> > +            GLContext::Extensions_End
> > +        }
> > +        /*
> > +         * XXX_occlusion_query depend on ARB_occlusion_query (added in OpenGL 2.0)
> > +         */
> 
> Don't use block comments for just one line.
> The rule of thumb is generally the third line of comments means to switch to
> a block comment.
Oups...
> 
> @@ +155,5 @@
> > +         */
> > +    },
> > +    {
> > +        "XXX_occlusion_query_boolean",
> > +        kGLCoreVersionForES3Compat,   // OpenGL version
> 
> This line doesn't really need this comment after the variable name change.
Fixed.
> 
> @@ +171,5 @@
> > +         */
> > +    },
> > +    {
> > +        "XXX_occlusion_query2",
> > +        330, // = min(330, kGLCoreVersionForES3Compat), // OpenGL version
> 
> Why not actually set it to std::min()?
Because it have to be a compile-time constant, where std::min is not.

Comment 16

[Guillaume Abadie]

The GL_VERSION parser now recursively depends on this bug.

Comment 17

[Guillaume Abadie]

Attachment: patch revision 4 (r+ by bjacob)

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 790525 [details] [diff] [review]
patch revision 4 (r+ by bjacob)

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

Good otherwise, but this last bit *needs* to be fixed.

::: gfx/gl/GLContext.cpp
@@ +666,5 @@
>  
>              if (!LoadSymbols(queryObjectsSymbols, trygl, prefix)) {
> +                NS_ERROR("GL supports query objects without supplying its functions.");
> +
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);

r- until this is fixed. Don't cause context init to abort for non-mandatory symbol issues.

Comment 19

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> Comment on attachment 790525 [details] [diff] [review]
> patch revision 4 (r+ by bjacob)
> 
> Review of attachment 790525 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good otherwise, but this last bit *needs* to be fixed.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +666,5 @@
> >  
> >              if (!LoadSymbols(queryObjectsSymbols, trygl, prefix)) {
> > +                NS_ERROR("GL supports query objects without supplying its functions.");
> > +
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);
> 
> r- until this is fixed. Don't cause context init to abort for non-mandatory
> symbol issues.
It was before, the behavior is exactly same. So you r- has no sense.

Comment 20

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #19)
> (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > Comment on attachment 790525 [details] [diff] [review]
> > patch revision 4 (r+ by bjacob)
> > 
> > Review of attachment 790525 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Good otherwise, but this last bit *needs* to be fixed.
> > 
> > ::: gfx/gl/GLContext.cpp
> > @@ +666,5 @@
> > >  
> > >              if (!LoadSymbols(queryObjectsSymbols, trygl, prefix)) {
> > > +                NS_ERROR("GL supports query objects without supplying its functions.");
> > > +
> > > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_query_objects);
> > 
> > r- until this is fixed. Don't cause context init to abort for non-mandatory
> > symbol issues.
> It was before, the behavior is exactly same. So you r- has no sense.

Ah, ok, I see it now. It's sometimes hard to keep track of things with moving+changing code.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 790525 [details] [diff] [review]
patch revision 4 (r+ by bjacob)

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

R+, now go fix that bug!

Comment 22

[Guillaume Abadie]

As green as an apple:
https://tbpl.mozilla.org/?tree=Try&rev=911d39864b37

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d52251e9123c

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d52251e9123c