Use MOZ_BEGIN_ENUM_CLASS for the WebGLTexelFormat enum

RESOLVED FIXED in mozilla27

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 814191 [details] [diff] [review]
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.
Attachment #814191 - Flags: review?(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.
Attachment #814191 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 2

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

Comment 3

5 years ago
Filed bug 924382 about moving MOZ_ENUM_CLASS_INTEGER_TYPE to MFBT.
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/32cd027a960d
Assignee: nobody → bjacob
Target Milestone: --- → mozilla27
https://hg.mozilla.org/mozilla-central/rev/32cd027a960d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.