1300731 - stylo: Implement mask longhands

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manish Goregaokar [:manishearth]]

Implement all of the longhands for the `mask` shorthand.

(shorthand will be implemented later on the servo side)

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1300731 - stylo: Implement mask longhands (except mask-image), write glue for most;

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

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1300731 - stylo: Glue for mask-position, reuse code between background and mask;

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

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1300731 - stylo: Implement mask-image;

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

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8788403 [details]
Bug 1300731 - stylo: Implement mask longhands (except mask-image), write glue for most;

https://reviewboard.mozilla.org/r/76904/#review76148

::: servo/components/style/properties/gecko.mako.rs:504
(Diff revision 1)
>  
>                     "background-size", # background
>                     "column-count", # column
>                     ]
>  
> +    force_stub += ["mask-position", "mask-size"]

Please add a comment above this stating why we need to force_stub them.

::: servo/components/style/properties/gecko.mako.rs:972
(Diff revision 1)
>  
>      ${impl_simple_copy('page_break_after', 'mBreakAfter')}
>  
>  </%self:impl_trait>
>  
> -<%def name="simple_background_array_property(name, field_name)">
> +<%def name="simple_array_property(name, shorthand, field_name)">

Can you name this "simple_image_array_property" instead?  In Gecko, we renamed the "background layer" concept to "image layer" when we made masks use them too.

::: servo/components/style/properties/gecko.mako.rs:974
(Diff revision 1)
>  
>  </%self:impl_trait>
>  
> -<%def name="simple_background_array_property(name, field_name)">
> -    pub fn copy_background_${name}_from(&mut self, other: &Self) {
> +<%def name="simple_array_property(name, shorthand, field_name)">
> +    <%
> +        gecko_shorthand = "mImage" if shorthand == "background" else "mMask"

I think "gecko_shorthand" isn't exactly the right name, since "mImage" isn't really the name of a shorthand.  How about "image_layers_field"?

::: servo/components/style/properties/gecko.mako.rs:1561
(Diff revision 1)
> +        use properties::longhands::mask_repeat::single_value::computed_value::T;
> +        use gecko_bindings::structs::nsStyleImageLayers_Repeat;
> +        use gecko_bindings::structs::NS_STYLE_IMAGELAYER_REPEAT_REPEAT;
> +        use gecko_bindings::structs::NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT;

Nit: Keep this sorted?

::: servo/components/style/properties/gecko.mako.rs:1593
(Diff revision 1)
> +        match servo {
> +            T::border_box => structs::NS_STYLE_IMAGELAYER_CLIP_BORDER as u8,
> +            T::padding_box => structs::NS_STYLE_IMAGELAYER_CLIP_PADDING as u8,
> +            T::content_box => structs::NS_STYLE_IMAGELAYER_CLIP_CONTENT as u8,
> +        }

I guess it's not possible to re-use the mapping we do in 'clip' above?

::: servo/components/style/properties/longhand/svg.mako.rs:99
(Diff revision 1)
> +
> +<%helpers:longhand name="mask-position" products="gecko" animatable="True">
> +    use properties::longhands::background_position;
> +    pub mod computed_value {
> +        pub type T = ::properties::longhands::background_position::computed_value::T;
> +

Nit: remove blank line.

::: servo/components/style/properties/longhand/svg.mako.rs:108
(Diff revision 1)
> +    #[inline]
> +    pub fn get_initial_value() -> computed_value::T {
> +        background_position::get_initial_value()
> +    }
> +
> +    pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue,()> {

Nit: space before "()".

::: servo/components/style/properties/longhand/svg.mako.rs:111
(Diff revision 1)
> +    }
> +
> +    pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue,()> {
> +        background_position::parse(context, input)
> +    }
> +

Nit: remove blank line.  (Or add a blank line at the start of the <%helpers:longhand>.)

::: servo/components/style/properties/longhand/svg.mako.rs:114
(Diff revision 1)
> +        background_position::parse(context, input)
> +    }
> +
> +</%helpers:longhand>
> +
> +// missing:  margin-box fill-box stroke-box view-box no-clip

Nit: only one space after ":".

::: servo/components/style/properties/longhand/svg.mako.rs:134
(Diff revision 1)
> +
> +<%helpers:longhand name="mask-size" products="gecko" animatable="True">
> +    use properties::longhands::background_size;
> +    pub mod computed_value {
> +        pub type T = ::properties::longhands::background_size::computed_value::T;
> +

Nit: remove blank line.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8788404 [details]
Bug 1300731 - stylo: Glue for mask-position, reuse code between background and mask;

https://reviewboard.mozilla.org/r/76906/#review76150

::: servo/components/style/properties/gecko.mako.rs:1003
(Diff revision 1)
>                  ${caller.body()}
>              };
>          }
>      }
>  </%def>
> -// TODO: Gecko accepts lists in most background-related properties. We just use
> +<%def name="impl_common_background_mask_properties(shorthand)">

In line with the comment on the previous patch, maybe call this "impl_common_image_layer_properties"?

::: servo/components/style/properties/gecko.mako.rs:1055
(Diff revision 1)
> +        self.gecko.${gecko_shorthand}.mLayers.mFirstElement.mPosition =
> +            other.gecko.${gecko_shorthand}.mLayers.mFirstElement.mPosition;

Nit: Should you put the "=" on the next line, and indent one more step, to make the formatting consistent with the previous two statements (and the ones at the end of the function)?

::: servo/components/style/properties/gecko.mako.rs:1179
(Diff revision 1)
> +// TODO: Gecko accepts lists in most background-related properties. We just use
> +// the first element (which is the common case), but at some point we want to
> +// add support for parsing these lists in servo and pushing to nsTArray's.

Is this comment out of date now?  If so, remove it.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8788405 [details]
Bug 1300731 - stylo: Implement mask-image;

https://reviewboard.mozilla.org/r/76908/#review76154

::: layout/style/nsRuleNode.cpp:9410
(Diff revision 1)
> +  // property was specified.
> +  aMask.mLayers.TruncateLengthNonZero(aMaxItemCount);
> +
> +  uint32_t fillCount = aMask.mImageCount;
> +
> +  FillBackgroundList(aMask.mLayers,

Would you mind renaming FillBackground{,PositionCoord}List and FillImageLayer{,PositionCoord}List?

::: servo/components/style/properties/gecko.mako.rs:1275
(Diff revision 1)
> -                            Gecko_SetGradientImageValue(&mut geckoimage.mImage, gecko_gradient);
> +                Gecko_SetGradientImageValue(geckoimage, gecko_gradient);
> +            }
> +        }
> +
> +        unsafe {
> +            // Prevent leaking of the last element we did set

Should this be "last elements"?

::: servo/components/style/properties/gecko.mako.rs:1279
(Diff revision 1)
> +        unsafe {
> +            // Prevent leaking of the last element we did set
> +            for image in &mut self.gecko.${gecko_shorthand}.mLayers {
> +                Gecko_SetNullImageValue(&mut image.mImage)
> +            }
> +            // XXXManishearth clear mFragment for masks

Do you mean mSourceURI?

::: servo/components/style/properties/gecko.mako.rs:1286
(Diff revision 1)
> +        self.gecko.${gecko_shorthand}.mImageCount
> +            = cmp::max(self.gecko.${gecko_shorthand}.mLayers.len() as u32,
> +                       self.gecko.${gecko_shorthand}.mImageCount);

I don't understand why we take the existing mImageCount value into account here.  Shouldn't we just be overwriting it with mLayers.len()?

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8788403 [details]
Bug 1300731 - stylo: Implement mask longhands (except mask-image), write glue for most;

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

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8788404 [details]
Bug 1300731 - stylo: Glue for mask-position, reuse code between background and mask;

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

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8788405 [details]
Bug 1300731 - stylo: Implement mask-image;

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

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8788405 [details]
Bug 1300731 - stylo: Implement mask-image;

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

Comment 11

[Manish Goregaokar [:manishearth]]

Servo PR: https://github.com/servo/servo/pull/13213

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8788405 [details]
Bug 1300731 - stylo: Implement mask-image;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76908/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cd8e43dd653
stylo: Implement mask-image; r=heycam

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9cd8e43dd653