Implement serialization of transform functions

[upsuper]

Currently, only serialization of translate() function is implmeneted. We should have the rest implemented as well.

Spec: https://drafts.csswg.org/css-transforms/#serialization-of-transform-functions

The relevant code is https://github.com/servo/servo/blob/master/components/style/properties/longhand/box.mako.rs#L1183-L1237

(I think this is an easy one, but not sure. cc @Manishearth)

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[dpyro]

Am working on now!

[upsuper]

Great! Please ask questions here if there is anything unclear.

[dpyro]

It looks like we are implementing serialization of the specified transforms, not the computed ones. If that is correct, it looks like we are discarding needed information to regenerate the original specified transforms in parse. Should I then modify the specified data structures to retain the necessary information and drop it into the compute section?

[Manishearth]

Nah. It is okay to drop info during computing. Should parse+compute back to the same computed value.

[dpyro]

Advice requested: when should I use ref while matching?

enum SpecifiedOperation {
    Matrix(SpecifiedMatrix),
    Skew(specified::Angle, specified::Angle),
    Translate(TranslateKind,
              specified::LengthOrPercentage,
              specified::LengthOrPercentage,
              specified::Length),
    Scale(CSSFloat, CSSFloat, CSSFloat),
    Rotate(CSSFloat, CSSFloat, CSSFloat, specified::Angle),
    Perspective(specified::Length),
}

For example, currently there is: SpecifiedOperation::Translate(kind, ref tx, ref ty, ref tz)

[Manishearth]

Whenever the type is not copyable. The length ones and perhaps the angle ones aren't. It's okay to use an unnecessary ref.

[dpyro]

How would I properly test this using the css-tests? Currently I am trying $ ./mach test-css css-transforms-1_dev, but this takes a long time to run (I may try running it on my desktop instead) and outputs verbose info instead of a summary of expectations vs. pass/fails.

[Manishearth]

I don't think there's a way to step around the verbose summary -- it only outputs a list of things which have changed. You can get a shorter (but still verbose) summary via --log-errorsummary foo.log. You can also make a pull request containing your changes and ask one of us to trigger a run on the try server which will run all the tests.

I'm unsure if those tests actually contain any serialization tests. Create a new one in wpt if you can't find one (via ./mach create-wpt). Or just add to the existing roundtrip parse/serialization tests in tests/unit/style.

[jdm]

The verbose summary is caused by #15193.

[dpyro]

I’m starting to get stuff to work! There is an issue: the current implementation expresses Angle in terms of radians, but the canonical and default unit is degrees. Do we also need to keep track of the specified unit for serializing Angles back out?

[upsuper]

Per spec, and other major browsers' behavior, I think for specified value, the original unit should be preserved. (That said, 10rad is serialized as 10rad, and 10deg is serialized as 10deg.)

This means you'll probably also need to change some of the parsing code to preserve the units. In addition to that, it seems that Servo is currently normalizing rotate{X,Y,Z} to rotate3d, skew{X,Y} to skew etc., which also loses information necessary for serialization.

For computed value... I guess it doesn't really matter, because we don't need to serialize transform functions in their original form for value returned by getComputedStyle. They are always resolved to matrix.

[wafflespeanut]

Previous work and reviews are in #15508

[highfive]

cc @emilio

[pyfisch]

@upsuper I would like to work on this. What is the best way to start?

[upsuper]

@pyfisch Probably you can have a look at #15508 to see where the code should go. Actually you can probably just try simplifying the code in that PR.

[pyfisch]

I can't figure out how the impl would look like. :( Can you please write a complete impl block?

[Manishearth]

impl<T: AsRef<U>, U: ToCss> Display for Css<U>?

Though, that may fall afoul of the covered rule, hm.

[emilio]

Why not just struct Css<'a, T>(&'a T)? Seems like the most straight-forward way to implement this.

[Manishearth]

They want it to be usable in other cases too, without forcing a borrow.

But yes, that's a decent solution.

[emilio]

Who's "They", if I can ask? And what's the use case?

[wafflespeanut]

I think having Css<'a, T> should just be enough. It's gonna be created/used only in ToCss impls, right?

[Manishearth]

"they" is @pyfisch

Usecase was a bad way of framing it, I meant that @pyfisch wanted it to be usable for both Css(T) and Css(&T)

[emilio]

Right, but what's the use case for Css taking ownership of anything ever?

[Manishearth]

Just ergonomics. But yeah, not much of a use case, Css(&T) should be fine

[emilio]

And apologies, didn't know that usage of "they" as a gender-neutral, singular pronoun (I'd have expected a mention, "the assignee" or something like that). Didn't want to be mean :)

[pyfisch]

Thanks for your help. I looked up how Display works and found that it is implemented for borrowed values. I added a similar impl for ToCss. If this causes any problems I can also insert '&' chars everywhere. The PR is also uploaded.