Emilio Cobos Álvarez (:emilio)
	Comment 2 • 5 years ago

Comment on attachment 9036320 [details] [diff] [review]
[css-logical] Implement the padding-block/inline shorthands

Review of attachment 9036320 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, please do send an intent to implement / ship to dev-platform.

Thanks! (And sorry for the lag with the other reviews, I need to find some time to go through all of them :/)

::: servo/components/style/properties/shorthands/padding.mako.rs
@@ +27,5 @@
> +            input: &mut Parser<'i, 't>,
> +        ) -> Result<Longhands, ParseError<'i>> {
> +            let start_value = NonNegativeLengthPercentage::parse(context, input)?;
> +            let end_value =
> +                input.try(|input| NonNegativeLengthPercentage::parse(context, input)).unwrap_or(start_value.clone());

nit: unwrap_or_else(|| start_value.clone()) avoids the copy if you specify both values.

@@ +38,5 @@
> +
> +        impl<'a> ToCss for LonghandsToSerialize<'a>  {
> +            fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result where W: fmt::Write {
> +                self.padding_${axis}_start.to_css(dest)?;
> +        

nit: Please remove the trailing whitespace.

@@ +43,5 @@
> +                if self.padding_${axis}_end != self.padding_${axis}_start {
> +                    dest.write_str(" ")?;
> +                    self.padding_${axis}_end.to_css(dest)?;
> +                }
> +        

nit: And here.

::: testing/web-platform/meta/css/css-logical/logical-box-padding.html.ini
@@ +2,1 @@
>    [Test that padding shorthand sets longhands and serializes correctly.]

Why are these still failing? Do they test the computed style serialization of the shorthands or something?

If so, it'd be good to add `bug: https://bugzilla.mozilla.org/show_bug.cgi?id=137688` to these lines.

	Pulsebot
	Comment 6 • 5 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15d04c80f5b
[css-logical] Implement the padding-block/inline shorthands.  r=emilio