1371354 - Change macro definition NS_STYLE_IMAGELAYER_REPEAT to enum class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jeff Hajewski]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170601133324

Steps to reproduce:

Changes the macro definitions for NS_STYLE_IMAGELAYER_REPEAT_* to enum class StyleImagelayerRepeat. This is a fix that is part of the tracking bug 1277133


Actual results:

Pre-fix code used macro definitions for these values


Expected results:

Post-fix code now uses the enum class StyleImagelayerRepeat : uint8_t

Comment 1

[Jeff Hajewski]

Attachment: Changes NS_STYLE_IMAGELAYER_REPEAT macro defined constants to enum class StyleImagelayerRepeat

Comment 2

[Jeff Hajewski]

I just noticed I forgot to delete the old macro defs. I will attach an updated patch shortly. I'd also be interested in taking a stab at the stylo changes if I can get a bit of direction. I'm still learning Rust, so by no means an expert.

Comment 3

[Jeff Hajewski]

Attachment: Changes macro defined constants for NS_STYLE_IMAGELAYER_REPEAT to enum class StyleImagelayerRepeat

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875839 [details] [diff] [review]
Changes NS_STYLE_IMAGELAYER_REPEAT macro defined constants to enum class StyleImagelayerRepeat

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

::: layout/painting/nsCSSRendering.cpp
@@ +3178,4 @@
>      bgClipRect = positionArea + aBorderArea.TopLeft();
>    }
>  
> +  StyleImagelayerRepeat repeatX = static_cast<StyleImagelayerRepeat>(aLayer.mRepeat.mXRepeat);

why is this cast necessary?

::: layout/style/nsCSSParser.cpp
@@ +12568,4 @@
>    if (ParseEnum(xValue, nsCSSProps::kImageLayerRepeatKTable)) {
>      int32_t value = xValue.GetIntValue();
>      // For single values set yValue as eCSSUnit_Null.
> +    if (value == uint8_t(StyleImagelayerRepeat::RepeatX) ||

nit: use static_cast to convert enum to an int

::: layout/style/nsStyleConsts.h
@@ +349,4 @@
>  #define NS_STYLE_IMAGELAYER_POSITION_RIGHT           (1<<4)
>  
>  // See nsStyleImageLayers
> +// #define NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT         0x00

nit: remove these comments

@@ +354,5 @@
> +// #define NS_STYLE_IMAGELAYER_REPEAT_REPEAT_Y          0x02
> +// #define NS_STYLE_IMAGELAYER_REPEAT_REPEAT            0x03
> +// #define NS_STYLE_IMAGELAYER_REPEAT_SPACE             0x04
> +// #define NS_STYLE_IMAGELAYER_REPEAT_ROUND             0x05
> +enum class StyleImagelayerRepeat : uint8_t {

nit: StyleImageLayerRepeat

Comment 5

[Jeff Hajewski]

(In reply to Manish Goregaokar [:manishearth] from comment #4)
> Comment on attachment 8875839 [details] [diff] [review]
> Changes NS_STYLE_IMAGELAYER_REPEAT macro defined constants to enum class
> StyleImagelayerRepeat
> 
> Review of attachment 8875839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/nsCSSRendering.cpp
> @@ +3178,4 @@
> >      bgClipRect = positionArea + aBorderArea.TopLeft();
> >    }
> >  
> > +  StyleImagelayerRepeat repeatX = static_cast<StyleImagelayerRepeat>(aLayer.mRepeat.mXRepeat);
> 
> why is this cast necessary?

It's not -- it was initially required because mXRepeat was an int, but I missed this change after updating mXRepeat to StyleImagelayerRepeat

> ::: layout/style/nsCSSParser.cpp
> @@ +12568,4 @@
> >    if (ParseEnum(xValue, nsCSSProps::kImageLayerRepeatKTable)) {
> >      int32_t value = xValue.GetIntValue();
> >      // For single values set yValue as eCSSUnit_Null.
> > +    if (value == uint8_t(StyleImagelayerRepeat::RepeatX) ||
> 
> nit: use static_cast to convert enum to an int

I posed this question on #developers IRC and was told constructor style casts for casting enum classes are generally preferred. I am happy to make this change to static_cast, but wanted to mention this first. 


> ::: layout/style/nsStyleConsts.h
> @@ +349,4 @@
> >  #define NS_STYLE_IMAGELAYER_POSITION_RIGHT           (1<<4)
> >  
> >  // See nsStyleImageLayers
> > +// #define NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT         0x00
> 
> nit: remove these comments
> 
> @@ +354,5 @@
> > +// #define NS_STYLE_IMAGELAYER_REPEAT_REPEAT_Y          0x02
> > +// #define NS_STYLE_IMAGELAYER_REPEAT_REPEAT            0x03
> > +// #define NS_STYLE_IMAGELAYER_REPEAT_SPACE             0x04
> > +// #define NS_STYLE_IMAGELAYER_REPEAT_ROUND             0x05
> > +enum class StyleImagelayerRepeat : uint8_t {

These are removed in the most recent attachement.

> nit: StyleImageLayerRepeat

I initially used this as the enum class name, however the macro does not have an underscore between IMAGE and LAYER so for consistency I kept the L lowercase. I personally think ImageLayer is better, but stuck with prior convention. Do you want me to update this as well?

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875839 [details] [diff] [review]
Changes NS_STYLE_IMAGELAYER_REPEAT macro defined constants to enum class StyleImagelayerRepeat

(please mark older attachments as obsolete)

Comment 7

[Jeff Hajewski]

(In reply to Manish Goregaokar [:manishearth] from comment #6)
> Comment on attachment 8875839 [details] [diff] [review]
> Changes NS_STYLE_IMAGELAYER_REPEAT macro defined constants to enum class
> StyleImagelayerRepeat
> 
> (please mark older attachments as obsolete)

Sorry about that - I thought the update under comment 3 marked the attachment as obsolete automatically

Comment 8

[Manish Goregaokar [:manishearth]]

> I posed this question on #developers IRC and was told constructor style casts for casting enum classes are generally preferred. I am happy to make this change to static_cast, but wanted to mention this first. 

Huh, I've been told the opposite, but I'm fine with that.

> I initially used this as the enum class name, however the macro does not have an underscore between IMAGE and LAYER so for consistency I kept the L lowercase.

Yeah, but for the enum we won't need this. The capitalization of the prefix can be any way, it's the keyword value part of the macro that needs to be capitalized the same.

It was IMAGELAYER in the macro because "ImageLayer" is one unit, but here we can just have ImageLayer capitalized that way instead of Imagelayer.

Comment 9

[Jeff Hajewski]

Attachment: Changes macro defined NS_SYTLE_IMAGELAYER_ to enum class StyleImageLayer

Makes changes per discussion

Comment 10

[Manish Goregaokar [:manishearth]]

(once again, I'll have to do the servo bindings thing for this to work, so you may have to wait a bit for this to land)

Comment 11

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/17262

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d949a67f1fc
Changes macro defined NS_SYTLE_IMAGELAYER_ to enum class StyleImageLayer; r=manishearth

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2d949a67f1fc