Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Blocks: 1277133
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Attachment #8772384 - Flags: review?(cam)
Comment on attachment 8772384 [details] [diff] [review]
Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class. v1

Trying to use reviewboard this time
Attachment #8772384 - Attachment is obsolete: true
Attachment #8772384 - Flags: review?(cam)
Interestingly, Stroke and Fill don't seem to be used at all. Any idea why?
(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?
Flags: needinfo?(cku)
Attachment #8772387 - Flags: review?(cam) → review+
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 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/
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=185ce868e862db8b01ad5d48243710f376f96ef0

(accidentally triggered two try builds, cancelled the second one)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc98fa975862
Replace NS_STYLE_CLIP_SHAPE_SIZING_* with an enum class; r=heycam
https://hg.mozilla.org/mozilla-central/rev/cc98fa975862
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(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.
Flags: needinfo?(cku)
You need to log in before you can comment on or make changes to this bug.