Convert NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class.

RESOLVED FIXED in Firefox 59

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: grhegde09, Assigned: grhegde09)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1277133
(Assignee)

Comment 1

2 years ago
Created attachment 8822139 [details] [diff] [review]
Converted NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class
Attachment #8822139 - Flags: review?(wafflespeanut)
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.
Assignee: nobody → grhegde09
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 :)
Attachment #8822139 - Flags: review?(wafflespeanut) → review?(emilio+bugs)
(Assignee)

Comment 4

2 years ago
Created attachment 8822150 [details] [diff] [review]
Incorporated review comments

Incorporated review comments in this patch.
Attachment #8822139 - Attachment is obsolete: true
Attachment #8822139 - Flags: review?(emilio+bugs)
Attachment #8822150 - Flags: review?(emilio+bugs)
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!
Attachment #8822150 - Flags: review?(emilio+bugs)
(Assignee)

Comment 7

2 years ago
Created attachment 8822182 [details] [diff] [review]
Added author details to the patch.

Updated the patch with author details.
Attachment #8822150 - Attachment is obsolete: true
Attachment #8822182 - Flags: review?(emilio+bugs)
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.
Attachment #8822182 - Flags: review?(xidorn+moz)
Attachment #8822182 - Flags: review?(emilio+bugs)
Attachment #8822182 - Flags: feedback+
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.
Attachment #8822182 - Flags: review?(xidorn+moz)
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.
(Assignee)

Comment 11

2 years ago
Created attachment 8822322 [details] [diff] [review]
Made whitespace changes to be adhere to formatting rules.
Attachment #8822182 - Attachment is obsolete: true
Attachment #8822322 - Flags: review?(xidorn+moz)
(Assignee)

Comment 12

2 years ago
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!)
(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... :(
(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.
(Assignee)

Comment 15

2 years ago
Created attachment 8822325 [details] [diff] [review]
Added a constructor for nsCSSValue to accept enumerations.
Attachment #8822325 - Flags: review?(xidorn+moz)
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.
Attachment #8822325 - Flags: review?(xidorn+moz)
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.
Attachment #8822322 - Flags: review?(xidorn+moz)
(Assignee)

Comment 18

2 years ago
Created attachment 8822333 [details] [diff] [review]
Squashed the commits.
Attachment #8822322 - Attachment is obsolete: true
Attachment #8822333 - Flags: review?(xidorn+moz)
Comment on attachment 8822333 [details] [diff] [review]
Squashed the commits.

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

Please address issues mentioned in comment 17.
Attachment #8822333 - Flags: review?(xidorn+moz)
(Assignee)

Comment 20

2 years ago
Created attachment 8822352 [details] [diff] [review]
Added constructor for nsCSSValue. (Review comments incorporated)
Attachment #8822325 - Attachment is obsolete: true
Attachment #8822352 - Flags: review?(xidorn+moz)
(Assignee)

Comment 21

2 years ago
Created attachment 8822353 [details] [diff] [review]
Added constructor for nsCSSValue. (Review comments incorporated)
Attachment #8822325 - Attachment is obsolete: true
Attachment #8822353 - Flags: review?(xidorn+moz)
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.
Attachment #8822352 - Flags: review?(xidorn+moz) → review+
Attachment #8822353 - Attachment is obsolete: true
Attachment #8822353 - Flags: review?(xidorn+moz)
Comment on attachment 8822333 [details] [diff] [review]
Squashed the commits.

With the other patch addressing the alignment issue, r+.
Attachment #8822333 - Flags: review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 24

2 years ago
Pushed by xquan@mozilla.com:
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
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.
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=83349106&repo=mozilla-inbound
Flags: needinfo?(grhegde09)

Comment 27

2 years ago
Backout by cbook@mozilla.com:
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
This didn't land :/
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

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

https://reviewboard.mozilla.org/r/209034/#review214702
Attachment #8938262 - Flags: review?(xidorn+moz) → review+
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.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(grhegde09)

Comment 33

a year ago
mozreview-review
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.
Attachment #8938263 - Flags: review?(emilio) → review+

Comment 34

a year ago
mozreview-review-reply
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...
Landing a patch with binding change is still hard... or even harder nowadays...

Comment 36

a year ago
Pushed by xquan@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06ac73f3f76f
https://hg.mozilla.org/mozilla-central/rev/c9fceb1a76ec
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.