1315157 - Stylo: Implement drop-shadow filter

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Shing Lyu [:shinglyu]]

drop-shadow filter was not supported in Servo, so I need to write the CSS parser first and then the glue.

Comment 1

[Shing Lyu [:shinglyu]]

Attachment: Bug 1315157 - Implemented drop-shadow parser in Servo

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

Comment 2

[Shing Lyu [:shinglyu]]

Attachment: Bug 1315157 - Stylo glue for drop-shadow

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

Comment 3

[Shing Lyu [:shinglyu]]

Attachment: Cat photo!

This is a cat photo (with transparent background) with two shadows.

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8807420 [details]
Bug 1315157 - Stylo glue for drop-shadow

https://reviewboard.mozilla.org/r/90560/#review90282

::: servo/components/style/properties/gecko.mako.rs:1666
(Diff revision 1)
>                                                    CoordDataValue::Factor(factor),
>                                                    gecko_filter),
> +                DropShadow(offset_x, offset_y, blur_radius, ref color) => {
> +                    gecko_filter.mType = NS_STYLE_FILTER_DROP_SHADOW;
> +
> +                    fn init_shadow(filter: &mut nsStyleFilter) -> &mut nsCSSShadowArray {

See what `text-shadow` does here, this leaks. Use the `replace_with_new()` function.

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8807419 [details]
Bug 1315157 - Implemented drop-shadow parser in Servo

https://reviewboard.mozilla.org/r/90558/#review90280

::: servo/components/style/properties/longhand/effects.mako.rs:676
(Diff revision 1)
>              _ => Err(())
>          }
>      }
>  
> +    % if product == "gecko":
> +    fn parse_drop_shadow(input: &mut Parser) -> Result<SpecifiedFilter, ()> {

This seems to be the exact same as text-shadow. Could you reuse the code for both, refactoring a `Shadow` type into `style::values::*`? Fine if done in followup, but would reduce the code in this PR.

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8807420 [details]
Bug 1315157 - Stylo glue for drop-shadow

https://reviewboard.mozilla.org/r/90560/#review90282

> See what `text-shadow` does here, this leaks. Use the `replace_with_new()` function.

Actually, no, it's not that simple. This doesn't leak. And `replace_with_new()` could cause issues here because it tries to interpret the raw ptr field, which can be anything here.

However, I'd prefer if `replace_with_new` was split into `replace_with_new_leaky()` (which doesn't have the raw pointer/release check), and `replace_with_new()` which has the raw pointer null check + release and then calls `replace_with_new_leaky`. Text shadow can use `replace_with_new()`, we can use `replace_with_new_leaky()`.

Or keep as-is I guess.

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8807419 [details]
Bug 1315157 - Implemented drop-shadow parser in Servo

https://reviewboard.mozilla.org/r/90558/#review90412

Yeah, everything else here looks good to me, just would like to share the parse/tocomputedvalue/serialization code with text-shadow. It's fine if done in a separate PR.

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8807420 [details]
Bug 1315157 - Stylo glue for drop-shadow

https://reviewboard.mozilla.org/r/90560/#review90414

::: servo/components/style/properties/gecko.mako.rs:1680
(Diff revision 1)
> +
> +                    let mut gecko_shadow = init_shadow(gecko_filter);
> +                    gecko_shadow.mArray[0].mXOffset = offset_x.0;
> +                    gecko_shadow.mArray[0].mYOffset = offset_y.0;
> +                    gecko_shadow.mArray[0].mRadius = blur_radius.0;
> +                    gecko_shadow.mArray[0].mSpread = 0;    // Not supported in spec level 1

I don't think we need to explicitly touch these? They get init'ed to 0 anyway.

Comment 9

[Shing Lyu [:shinglyu]]

Comment on attachment 8807419 [details]
Bug 1315157 - Implemented drop-shadow parser in Servo

https://reviewboard.mozilla.org/r/90558/#review90744

::: servo/components/style/properties/longhand/effects.mako.rs:676
(Diff revision 1)
>              _ => Err(())
>          }
>      }
>  
> +    % if product == "gecko":
> +    fn parse_drop_shadow(input: &mut Parser) -> Result<SpecifiedFilter, ()> {

This looks like a pure Servo-side task. I'd prefer doing that in a followup, because I'm more comfortable working with github PR than MozReview :p

Comment 10

[Shing Lyu [:shinglyu]]

Comment on attachment 8807420 [details]
Bug 1315157 - Stylo glue for drop-shadow

https://reviewboard.mozilla.org/r/90560/#review90742

::: servo/components/style/properties/gecko.mako.rs:1666
(Diff revision 1)
>                                                    CoordDataValue::Factor(factor),
>                                                    gecko_filter),
> +                DropShadow(offset_x, offset_y, blur_radius, ref color) => {
> +                    gecko_filter.mType = NS_STYLE_FILTER_DROP_SHADOW;
> +
> +                    fn init_shadow(filter: &mut nsStyleFilter) -> &mut nsCSSShadowArray {

One difference is that `mTextShadow` is a `RefPtr<nsCSSShadowArray>`, but the `mDropShadow` is a `__BindgenUnionField<*mut nsCSSShadowArray>`. The `replace_with_new()` function is implemented for `RefPtr<nsCSSShadowArray>`. Can I convert between `*mut` and `RefPtr`?

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8807420 [details]
Bug 1315157 - Stylo glue for drop-shadow

https://reviewboard.mozilla.org/r/90560/#review90742

> One difference is that `mTextShadow` is a `RefPtr<nsCSSShadowArray>`, but the `mDropShadow` is a `__BindgenUnionField<*mut nsCSSShadowArray>`. The `replace_with_new()` function is implemented for `RefPtr<nsCSSShadowArray>`. Can I convert between `*mut` and `RefPtr`?

In this case, yes. Rust's refptr has a method to construct from raw.

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#1124
https://github.com/Manishearth/servo/blob/bb736f41d32c1cb6117bbf327f2a6efa7deebd0c/components/style/gecko_bindings/sugar/refptr.rs#L45

Comment 12

[Shing Lyu [:shinglyu]]

Comment on attachment 8807420 [details]
Bug 1315157 - Stylo glue for drop-shadow

https://reviewboard.mozilla.org/r/90560/#review91478

::: servo/components/style/properties/gecko.mako.rs:1666
(Diff revision 1)
>                                                    CoordDataValue::Factor(factor),
>                                                    gecko_filter),
> +                DropShadow(offset_x, offset_y, blur_radius, ref color) => {
> +                    gecko_filter.mType = NS_STYLE_FILTER_DROP_SHADOW;
> +
> +                    fn init_shadow(filter: &mut nsStyleFilter) -> &mut nsCSSShadowArray {

I tried to use RefPtr, but the `gecko_bindings::structs::RefPtr<nsCSSShadowArray>`, which is used by the `replace_with_new()`, looks different from the `gecko_bindings::sugar::refptr::RefPtr<T>` you suggested. Are they interchangeable? I can't get it compiled, always getting a mismatched types error.

Comment 13

[Manish Goregaokar [:manishearth]]

They are interchangeable, see the methods on each.

However, I think using your old init_shadow is fine here given that this is a bit tricky. Ignore my comment about sharing init code.

Comment 14

[Manish Goregaokar [:manishearth]]

Fixed in https://github.com/servo/servo/pull/14359