1325940 - Convert NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[grhegde09]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Comment 1

[grhegde09]

Attachment: Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822139 [details] [diff] [review]
Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class

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

::: layout/painting/nsCSSRendering.cpp
@@ +5818,5 @@
>   * aUnitSize The size of the source rect in dest coords.
>   */
>  static nsRect
> +ComputeTile(nsRect&              	aFill,
> +			StyleBorderImageRepeat  aHFill,

Use spaces instead of tabs for indenting these.

@@ +5896,5 @@
>   * for argument descriptions.
>   */
>  static bool
> +RequiresScaling(const nsRect&        	aFill,
> +				StyleBorderImageRepeat  aHFill,

And these

@@ +5914,5 @@
> +                                          nsRenderingContext&  		aRenderingContext,
> +                                          const nsRect&        		aDirtyRect,
> +                                          const nsRect&        		aFill,
> +                                          const CSSIntRect&    		aSrc,
> +										  StyleBorderImageRepeat    aHFill,

And these.

::: layout/painting/nsCSSRendering.h
@@ +245,5 @@
> +                           nsRenderingContext&  			aRenderingContext,
> +                           const nsRect&        			aDirtyRect,
> +                           const nsRect&        			aFill,
> +                           const mozilla::CSSIntRect& 		aSrc,
> +						   mozilla::StyleBorderImageRepeat	aHFill,

And these.

Comment 3

[Ravi Shankar [:waffles]]

Comment on attachment 8822139 [details] [diff] [review]
Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class

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

I'll pass this to emilio :)

Comment 4

[grhegde09]

Attachment: Incorporated review comments

Incorporated review comments in this patch.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822150 [details] [diff] [review]
Incorporated review comments

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

Looks great! One last thing:

Please include your author information in the patch. You can do this using |git format-patch| or |hg export|, depending on whether you use git or mercurial.

Meanwhile I'll push this patch to the try server, to ensure it passes all the tests.

Thanks for the patch!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=048202cfd414b0e21318640c7517816f6b26b778

Comment 7

[grhegde09]

Attachment: Added author details to the patch.

Updated the patch with author details.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822182 [details] [diff] [review]
Added author details to the patch.

This looks good to me, though I think my review is not enough to land this.

Xidorn, could you sign-off and land this patch? Try run is in a comment above.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822182 [details] [diff] [review]
Added author details to the patch.

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

Some of the lines are over 80 characters, please fix that. It otherwise looks fine to me.

::: layout/painting/nsCSSRendering.cpp
@@ +5919,5 @@
> +                                          StyleBorderImageRepeat  aVFill,
> +                                          const nsSize&           aUnitSize,
> +                                          uint8_t                 aIndex,
> +                                          const Maybe<nsSize>&    aSVGViewportSize,
> +                                          const bool              aHasIntrinsicRatio)

There are several lines in this list is over 80 characters. Please remove some of the whitespaces between type and name to keep them under 80 characters. It is not strictly necessary to keep alignment here.

::: layout/style/nsCSSDataBlock.cpp
@@ +456,4 @@
>    const nsCSSValuePair &repeat =
>      ValueFor(eCSSProperty_border_image_repeat)->GetPairValue();
>    return repeat.BothValuesEqualTo(
> +    nsCSSValue(static_cast<int32_t>(StyleBorderImageRepeat::Stretch), eCSSUnit_Enumerated));

This line is too long, please wrap eCSSUnit_Enumerated to the second line.

::: layout/style/nsCSSParser.cpp
@@ +13333,4 @@
>    // border-image-repeat: repeat
>    nsCSSValue repeat;
>    nsCSSValuePair repeatPair;
> +  repeatPair.SetBothValuesTo(nsCSSValue(static_cast<int32_t>(StyleBorderImageRepeat::Stretch),

This line is too long, please wrap in an earlier different position, probably before nsCSSValue.

::: layout/style/nsStyleStruct.h
@@ +1385,4 @@
>    nsStyleSides   mBorderImageOutset;  // [reset] length, factor
>  
>    uint8_t        mBorderImageFill;    // [reset]
> +  mozilla::StyleBorderImageRepeat mBorderImageRepeatH; // [reset] see nsStyleConsts.h

"see nsStyleConsts.h" is not needed, please remove it to make this line have less than 80 characters.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822182 [details] [diff] [review]
Added author details to the patch.

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

::: layout/style/nsCSSDataBlock.cpp
@@ +456,4 @@
>    const nsCSSValuePair &repeat =
>      ValueFor(eCSSProperty_border_image_repeat)->GetPairValue();
>    return repeat.BothValuesEqualTo(
> +    nsCSSValue(static_cast<int32_t>(StyleBorderImageRepeat::Stretch), eCSSUnit_Enumerated));

Hmmm, actually, rather than doing static_cast here, could you add a constructor to nsCSSValue to take enum values? You can see the template version of nsCSSValue::SetIntValue as an example of how to write that.

And adding the constructor should be put in a separate patch.

Comment 11

[grhegde09]

Attachment: Made whitespace changes to be adhere to formatting rules.

Comment 12

[grhegde09]

Xidorn: I will create another patch with the constructor changes for nsCSSValue in layout/style/nsCSSDataBlock.cpp
Should that be another bug?
Also is there a formatting tool like clang-format that is used by all Mozilla developers? If not how to maintain consistency? (Pardon me if this is too stupid a question. I am new here!)

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(In reply to grhegde09 from comment #12)
> Xidorn: I will create another patch with the constructor changes for
> nsCSSValue in layout/style/nsCSSDataBlock.cpp
> Should that be another bug?

No need in another bug. Just an additional patch for this bug is enough.

(Actually I would prefer have that patch be a preceding patch, so that you can use it directly in the patch where the conversion happens. But either way is fine, and I'm also fine with your current code as-is, since it seems to be the only place which needs it.)

> Also is there a formatting tool like clang-format that is used by all
> Mozilla developers? If not how to maintain consistency? (Pardon me if this
> is too stupid a question. I am new here!)

I don't think we officially use any formatting tool, and I'm not aware of anything in our CI which checks format for C++ code. (There seems to be a linter for JS code, though). So I don't think there is any way to maintain consistency other than sense of reviewers at the moment. It's sad, but... :(

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

(In reply to grhegde09 from comment #11)
> Created attachment 8822322 [details] [diff] [review]
> Made whitespace changes to be adhere to formatting rules.

This file includes two patches? Please squash them together. You can use "hg histedit" to do that.

Comment 15

[grhegde09]

Attachment: Added a constructor for nsCSSValue to accept enumerations.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822325 [details] [diff] [review]
Added a constructor for nsCSSValue to accept enumerations.

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

::: layout/style/nsCSSValue.h
@@ +607,5 @@
>    }
> +  template<typename T,
> +           typename = typename std::enable_if<std::is_enum<T>::value>::type>
> +  nsCSSValue(T aValue, nsCSSUnit aUnit)
> +    : mUnit(aUnit)

I think you can omit aUnit, and just initialize mUnit with eCSSUnit_Enumerated, as what I did to SetIntValue in bug 1326125.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822322 [details] [diff] [review]
Made whitespace changes to be adhere to formatting rules.

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

Please squash the two patches in this file into one commit.

::: layout/style/nsCSSDataBlock.cpp
@@ +460,1 @@
>  }

This should be aligned with the first argument of the same function. So it becomes
> nsCSSValue(static_cast<int32_t>(StyleBorderImageRepeat::Stretch),
>            eCSSUnit_Enumerated)

If you are adding the template constructor, you would be able to just do
> nsCSSValue(StyleBorderImageRepeat::Stretch)
which looks much nicer.

::: layout/style/nsCSSParser.cpp
@@ +13337,2 @@
>                                          eCSSUnit_Enumerated));
>    repeat.SetPairValue(&repeatPair);

Ditto.

Comment 18

[grhegde09]

Attachment: Squashed the commits.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822333 [details] [diff] [review]
Squashed the commits.

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

Please address issues mentioned in comment 17.

Comment 20

[grhegde09]

Attachment: Added constructor for nsCSSValue. (Review comments incorporated)

Comment 21

[grhegde09]

Attachment: Added constructor for nsCSSValue. (Review comments incorporated)

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822352 [details] [diff] [review]
Added constructor for nsCSSValue. (Review comments incorporated)

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

r=me with the following issue addressed.

::: layout/style/nsCSSValue.h
@@ +606,5 @@
>      aOther.mUnit = eCSSUnit_Null;
>    }
> +  template<typename T,
> +           typename = typename std::enable_if<std::is_enum<T>::value>::type>
> +  nsCSSValue(T aValue)

You need to prepend "explicit" for constructor which has only one parameter.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822333 [details] [diff] [review]
Squashed the commits.

With the other patch addressing the alignment issue, r+.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be602e750e39
part 1 - Added a constructor for nsCSSValue to accept enumerations. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/a035c2b6ae72
part 2 - Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class. r=xidorn

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

It seems grhegde09 didn't manage to update the patch. Since the patches are almost done, I decided to just address the issues and land it.

Comment 26

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=83349106&repo=mozilla-inbound

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a26629278490
Backed out changeset a035c2b6ae72 
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7d9aa4c2f3
Backed out changeset be602e750e39 for stylo build bustage

Comment 28

[David Teller [:Yoric] - still alive but not very active]

This didn't land :/

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1325940 part 1 - Added a constructor for nsCSSValue to accept enumerations.

Review commit: https://reviewboard.mozilla.org/r/209034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209034/

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1325940 part 2 - Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class.

Review commit: https://reviewboard.mozilla.org/r/209036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209036/

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8938262 [details]
Bug 1325940 part 1 - Added a constructor for nsCSSValue to accept enumerations.

https://reviewboard.mozilla.org/r/209034/#review214702

Comment 32

[Xidorn Quan [:xidorn] UTC+11]

The code has changed a lot since last time, so I reworked the second patch. It now requires Servo side change as well, which was the reason I didn't want to work on landing it...

I cannot see the bustage log from comment 26 anymore, but I suppose that may be related to stylo as well.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938263 [details]
Bug 1325940 part 2 - Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class.

https://reviewboard.mozilla.org/r/209036/#review214706

::: layout/painting/nsImageRenderer.cpp:835
(Diff revision 1)
>   * the dest rect to be scaled from the source tile. See comment on ComputeTile
>   * for argument descriptions.
>   */
>  static bool
>  RequiresScaling(const nsRect&        aFill,
> -                uint8_t              aHFill,
> +                StyleBorderImageRepeat aHFill,

nit: Maybe fixup the argument alignment and such in the functions touched by this.

```
git diff HEAD~ -U0 | clang-format-diff -p1 -i
```

Should take care of it automatically.

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8938263 [details]
Bug 1325940 part 2 - Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class.

https://reviewboard.mozilla.org/r/209036/#review214706

> nit: Maybe fixup the argument alignment and such in the functions touched by this.
> 
> ```
> git diff HEAD~ -U0 | clang-format-diff -p1 -i
> ```
> 
> Should take care of it automatically.

That also does lots of other unrelated formatting... which I'm not too happy with.

I'd just leave the patch as-is. I don't really like those alignments, and would rather see them go away completely at some point...

Comment 35

[Xidorn Quan [:xidorn] UTC+11]

Landing a patch with binding change is still hard... or even harder nowadays...

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06ac73f3f76f
part 1 - Added a constructor for nsCSSValue to accept enumerations. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/c9fceb1a76ec
part 2 - Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class. r=emilio

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06ac73f3f76f
https://hg.mozilla.org/mozilla-central/rev/c9fceb1a76ec