924191 - Use MOZ_BEGIN_ENUM_CLASS for the WebGLTexelFormat enum

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Attachment: patch

Note the b2g-specific hack, needed until we upgrade the compiler used there to something newer than GCC 4.4. Hope you agree it's still worth it at this point. Buys us useful type safety, and namespacing of these enum values with very conflict-prone names such as RGBA8.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 814191 [details] [diff] [review]
patch

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

::: content/canvas/src/WebGLTexelConversions.cpp
@@ +271,5 @@
>              case SrcFormat: \
>                  return run<SrcFormat>(dstFormat, premultiplicationOp);
>  
>          switch (srcFormat) {
> +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(WebGLTexelFormat::R8)

Why not change these macros to inject the WebGLTexelFormat:: prefix?

::: content/canvas/src/WebGLTexelConversions.h
@@ +53,5 @@
>      Premultiply,
>      Unpremultiply
>  };
>  
> +// remove this as soon as B2G uses a newer C++ compiler than the current GCC 4.4

Ew.

::: content/canvas/src/WebGLTypes.h
@@ -88,5 @@
>      UninitializedImageData,
>      InitializedImageData
>  MOZ_END_ENUM_CLASS(WebGLImageDataStatus)
>  
> -namespace WebGLTexelConversions {

Why remove this namespace? I think it's fine, but curious.

Comment 2

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

(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Comment on attachment 814191 [details] [diff] [review]
> patch
> 
> Review of attachment 814191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLTexelConversions.cpp
> @@ +271,5 @@
> >              case SrcFormat: \
> >                  return run<SrcFormat>(dstFormat, premultiplicationOp);
> >  
> >          switch (srcFormat) {
> > +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(WebGLTexelFormat::R8)
> 
> Why not change these macros to inject the WebGLTexelFormat:: prefix?

I hesitated.

In the end, the present approach has the merit of explicitness. RGBA8 is a very ambiguous identifier --- we have lots of pixel format enums flying around between OpenGL, WebGL, Thebes, Cairo, Moz2D, Skia, Android, DirectX... The present change is about making the present enums more explicit by making them mandatory-namespaced. Given that, I prefer slightly not to have this macro sidestep that.

> 
> ::: content/canvas/src/WebGLTexelConversions.h
> @@ +53,5 @@
> >      Premultiply,
> >      Unpremultiply
> >  };
> >  
> > +// remove this as soon as B2G uses a newer C++ compiler than the current GCC 4.4
> 
> Ew.

Yes. At least, that should be soon. Android has already moved on to newer GCC (and clang) so B2G should logically follow.

> 
> ::: content/canvas/src/WebGLTypes.h
> @@ -88,5 @@
> >      UninitializedImageData,
> >      InitializedImageData
> >  MOZ_END_ENUM_CLASS(WebGLImageDataStatus)
> >  
> > -namespace WebGLTexelConversions {
> 
> Why remove this namespace? I think it's fine, but curious.

Note that it's only moved a few lines down; the only thing that changes is that this enum is no longer in this namespace. The reason why it was in that namespace was that enum values like RGBA8 were very polluting to have directly in mozilla namespace, and plain C++98 enums don't namespace their values. Now that we've switched to enum classes that do namespace their values, that WebGLTexelConversions namespace is no longer needed there. It remains somewhat useful mostly because pack() and unpack() below would be somewhat polluting (though not really, because they take arguments whose types contain 'WebGL').

Comment 3

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

Filed bug 924382 about moving MOZ_ENUM_CLASS_INTEGER_TYPE to MFBT.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/32cd027a960d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32cd027a960d