Convert NS_STYLE_BORDER_IMAGE_REPEAT_* to an enum class.

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: grhegde09, Assigned: grhegde09)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments, 6 obsolete attachments)

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
Blocks: 1277133
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)
Posted patch Incorporated review comments (obsolete) — Splinter Review
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)
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.
Attachment #8822182 - Attachment is obsolete: true
Attachment #8822322 - Flags: review?(xidorn+moz)
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.
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)
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)
Attachment #8822325 - Attachment is obsolete: true
Attachment #8822352 - Flags: review?(xidorn+moz)
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
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.
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 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 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 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...
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
https://hg.mozilla.org/mozilla-central/rev/06ac73f3f76f
https://hg.mozilla.org/mozilla-central/rev/c9fceb1a76ec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.