1287755 - Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class. v1

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1287755 - Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class;

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

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8772384 [details] [diff] [review]
Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class. v1

Trying to use reviewboard this time

Comment 4

[Manish Goregaokar [:manishearth]]

Interestingly, Stroke and Fill don't seem to be used at all. Any idea why?

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Manish Goregaokar [:manishearth] from comment #4)
> Interestingly, Stroke and Fill don't seem to be used at all. Any idea why?

I don't think we implement them.  (At least, we don't implement CSS Shapes-based clip-path on SVG elements, where the fill-box and stroke-box values are useful.)  Although, I think there should be some sort of fallback from fill-box and stroke-box to the other values (padding-box and border-box maybe?) although from a quick glance through css-masking-3 I couldn't find where that's defined.

CJ, do you know if we should be doing something with these values?

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8772387 [details]
Bug 1287755 - Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class;

https://reviewboard.mozilla.org/r/65212/#review62426

::: layout/style/StyleAnimationValue.cpp:3576
(Diff revision 1)
>      }
>      default:
>        MOZ_ASSERT_UNREACHABLE("Unknown shape type");
>        return false;
>    }
> -  aResult->Item(1).SetIntValue(aClipPath.GetSizingBox(), eCSSUnit_Enumerated);
> +  aResult->Item(1).SetIntValue(uint8_t(aClipPath.GetSizingBox()), eCSSUnit_Enumerated);

Nit: please wrap this line to keep to 80 columns.

::: layout/style/StyleAnimationValue.cpp:3966
(Diff revision 1)
>                                           doc->NodePrincipal());
>              auto result = MakeUnique<nsCSSValue>();
>              result->SetURLValue(url);
>              aComputedValue.SetAndAdoptCSSValueValue(result.release(), eUnit_URL);
>            } else if (type == NS_STYLE_CLIP_PATH_BOX) {
> -            aComputedValue.SetIntValue(clipPath.GetSizingBox(), eUnit_Enumerated);
> +            aComputedValue.SetIntValue(uint8_t(clipPath.GetSizingBox()), eUnit_Enumerated);

Nit: and here.

::: layout/style/nsComputedDOMStyle.h:646
(Diff revision 1)
>    void SetCssTextToCoord(nsAString& aCssText, const nsStyleCoord& aCoord);
>    already_AddRefed<CSSValue> CreatePrimitiveValueForStyleFilter(
>      const nsStyleFilter& aStyleFilter);
>  
>    already_AddRefed<CSSValue> CreatePrimitiveValueForClipPath(
> -    const nsStyleBasicShape* aStyleBasicShape, uint8_t aSizingBox);
> +    const nsStyleBasicShape* aStyleBasicShape, mozilla::StyleClipShapeSizing aSizingBox);

Nit: and here.

::: layout/style/nsComputedDOMStyle.cpp:5977
(Diff revision 1)
>  nsComputedDOMStyle::CreatePrimitiveValueForClipPath(
> -  const nsStyleBasicShape* aStyleBasicShape, uint8_t aSizingBox)
> +  const nsStyleBasicShape* aStyleBasicShape, mozilla::StyleClipShapeSizing aSizingBox)

Nit: and here.

::: layout/style/nsStyleStruct.h:3525
(Diff revision 1)
>      NS_ASSERTION(mType == NS_STYLE_CLIP_PATH_SHAPE, "wrong clip-path type");
>      return mBasicShape;
>    }
>  
>    void SetBasicShape(nsStyleBasicShape* mBasicShape,
> -                     uint8_t aSizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX);
> +                     mozilla::StyleClipShapeSizing aSizingBox = mozilla::StyleClipShapeSizing::NoBox);

Nit: and here (after the "=").

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8772387 [details]
Bug 1287755 - Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65212/diff/1-2/

Comment 8

[Manish Goregaokar [:manishearth]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=185ce868e862db8b01ad5d48243710f376f96ef0

(accidentally triggered two try builds, cancelled the second one)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc98fa975862
Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class; r=heycam

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc98fa975862

Comment 11

[u459114]

(In reply to Cameron McCormack (:heycam) from comment #5)
> (In reply to Manish Goregaokar [:manishearth] from comment #4)
> > Interestingly, Stroke and Fill don't seem to be used at all. Any idea why?
> 
> I don't think we implement them.  (At least, we don't implement CSS
> Shapes-based clip-path on SVG elements, where the fill-box and stroke-box
> values are useful.)  Although, I think there should be some sort of fallback
> from fill-box and stroke-box to the other values (padding-box and border-box
> maybe?) although from a quick glance through css-masking-3 I couldn't find
> where that's defined.
> 
> CJ, do you know if we should be doing something with these values?
We fallback to border box
https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsCSSClipPathInstance.cpp#81

And yes, I think we should do something to go a step further. Bug 1289011 filed.