900729 - Make TextureFlags constant names all caps and prepend TEXTURE_ in CompositorTypes.h

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

All of our constants and enums are all caps in gfx/layers/CompositorTypes.h, except some of the TextureFlags constants. let's make them all caps too.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Were these left different so that they match the TextureImage::Flags enums?

Comment 2

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Were these left different so that they match the TextureImage::Flags enums?

Every time I needed a constant for the new textures I would convert it to all caps but I didn't do it for all of them to not make the patch even bigger (a big part of the texture refactoring patches are search'n replace stuff already).

It doesn't really matter that they match the TextureImage flags I think (worse at first I thought they were using the same constants, which is not the case so it may leed to confusion and bug if we start using layers TextureFlags in gl::TextureImage).

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Makes sense.  Just wanted to clarify for posterity.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make TextureFlags constant names all caps in CompositorTypes.h

Make TextureFlags constant names (UseNearestFilter, NeedsYFlip, AllowRepeat, NewTile, ComponentAlpha) all caps in CompositorTypes.h. ThebesLayerBuffer was already using ALLOW_REPEAT, so to avoid confusion, that one got renamed to ALLOW_REPEAT_MODE and AllowRepeat from ComponentTypes became ALLOWING_REPEAT.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 785399 [details] [diff] [review]
Make TextureFlags constant names all caps in CompositorTypes.h

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

::: gfx/gl/GLContext.h
@@ +2654,5 @@
>       * |aContentType|.  The TextureImage's texture is configured to
>       * use |aWrapMode| (usually GL_CLAMP_TO_EDGE or GL_REPEAT) and by
>       * default, GL_LINEAR filtering.  Specify
> +     * |aFlags=USE_NEAREST_FILTER| for GL_NEAREST filtering. Specify
> +     * |aFlags=NEEDS_Y_FLIP| if the image is flipped. Return

These two refer to TextureImage flags which are not all caps.

::: gfx/layers/CompositorTypes.h
@@ +37,2 @@
>  // The texture is part of a component-alpha pair
> +const TextureFlags COMPONENT_ALPHA            = 1 << 5;

Please prefix the constant names with TEXTURE_ to follow the convention of the other texture flags.

::: gfx/layers/ThebesLayerBuffer.h
@@ +220,5 @@
>    PaintState BeginPaint(ThebesLayer* aLayer, ContentType aContentType,
>                          uint32_t aFlags);
>  
>    enum {
> +    ALLOW_REPEAT_MODE = 0x01,

nit: is this a better name or just to avoid collision with the TextureFlag name?
If it is the latter, prefixing texture flags with TEXTURE_ will make it so that you don't have to change this one.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Nicolas Silva [:nical] from comment #5)
> Comment on attachment 785399 [details] [diff] [review]
> Make TextureFlags constant names all caps in CompositorTypes.h
> 
> 
> ::: gfx/layers/CompositorTypes.h
> @@ +37,2 @@
> >  // The texture is part of a component-alpha pair
> > +const TextureFlags COMPONENT_ALPHA            = 1 << 5;
>
> 
> Please prefix the constant names with TEXTURE_ to follow the convention of
> the other texture flags.

I'll change the summary to reflect this requirement, it wasn't obvious from the original description.

> 
> ::: gfx/layers/ThebesLayerBuffer.h
> @@ +220,5 @@
> >    PaintState BeginPaint(ThebesLayer* aLayer, ContentType aContentType,
> >                          uint32_t aFlags);
> >  
> >    enum {
> > +    ALLOW_REPEAT_MODE = 0x01,
> 
> nit: is this a better name or just to avoid collision with the TextureFlag
> name?
> If it is the latter, prefixing texture flags with TEXTURE_ will make it so
> that you don't have to change this one.

Yes, that is the reason (see comment 4), and you're right if TEXTURE_ is in the CompositorTypes, this is not an issue.

Thanks!

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make TextureFlags constant names all caps, starting with TEXTURE_ in CompositorTypes.h

Make TextureFlags constant names (UseNearestFilter, NeedsYFlip, AllowRepeat, NewTile, ComponentAlpha) all caps in CompositorTypes.h and prefix them with TEXTURE_ for consistency and uniqueness.  With TEXTURE_ at the start, there is no clash with the existing ALLOW_REPEAT in ThebesLayerBuffer anymore.

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 786374 [details] [diff] [review]
Make TextureFlags constant names all caps, starting with TEXTURE_ in CompositorTypes.h

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

Sweet :)

Comment 9

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37855f545804

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37855f545804