1207734 - Implement individual transform properties: the 'translate', 'scale', and 'rotate' properties

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should implement the separate translate, rotate, and scale properties described in https://drafts.csswg.org/css-transforms-2/#individual-transforms .  I think the proposal is pretty non-controversial, and it's frequently requested by developers.

Comment 1

[Jeremy Chen ([e-mail])]

Here is the plan:

Phase 1: CSSValue parsing, storing, and computing
=> Parsing: Parse each individual transform property as a new CSS attribute
=> Storing: Store each parsed value into a rule node (maybe more customized data structure if needed)
=> Computing: Compute and transform each individual property to a geometric transform matrix

Phase 2: Combine the computed matrix with original css-transform matrix, and then do the rendering
=> Either do the combination first, then take a single combined transform matrix to the renderer; or take separated matrices to the renderer and let the renderer to the compositing. Haven't study yet, just few thoughts.

Phase 3: Test case
=> Refer to the original css-transform and make similar basic test cases

I'm about in 50% of Phase 1 before interrupted by another bug. I should be back soon. Once I back to this bug, I'll upload a working patch and keep update it until I resolve this.

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: wip

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: (wip) Implement individual translate property

With this wip, I can get individual translate work w/ following css style:

#div-transform {
  translate: 50px 50px;
}

However, when setting individual translate together w/ transform, none of both would work. For example, if I set a div with following css style:

#div-transform {
  translate: 50px 50px;
  transform: translateX(30px);
}

Neither two translate properties will work. Maybe there's some other code paths I should've taken into consideration.


Hi David, could you take a look at this? I'm wondering if this is the right place to combine parsed individual transforms into the existing transform? If yes, what else I may miss to get the property work? If not, could you guide me to the right direction? Any suggestion would be very appreciated.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8727704 [details] [diff] [review]
(wip) Implement individual translate property

This is not the right place to implement the combining.

The 'translate' property should not change the computed value of 'transform'.  But presumably it should change nearly everything that operates on nsStyleDisplay::mSpecifiedTransform.

Also, animations of 'transform' and 'translate' should essentially work independently -- which actually makes implementing this a bit more complicated than I was thinking.


I'm actually having second thoughts about whether this is worth implementing; I think it's a little more complicated than I previously thought.  Do we know if any other browser vendors are doing it?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #4)
> Also, animations of 'transform' and 'translate' should essentially work
> independently -- which actually makes implementing this a bit more
> complicated than I was thinking.

In particular, I believe this means we'd need to send them separately to the compositor, add add significantly more animation capability to the compositor, which is a good bit of work.

Comment 6

[Jeremy Chen ([e-mail])]

(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #4)
> Comment on attachment 8727704 [details] [diff] [review]
> (wip) Implement individual translate property
> 
> This is not the right place to implement the combining.
> 
> The 'translate' property should not change the computed value of
> 'transform'.  But presumably it should change nearly everything that
> operates on nsStyleDisplay::mSpecifiedTransform.
> 
> Also, animations of 'transform' and 'translate' should essentially work
> independently -- which actually makes implementing this a bit more
> complicated than I was thinking.
> 
> 
> I'm actually having second thoughts about whether this is worth
> implementing; I think it's a little more complicated than I previously
> thought.  Do we know if any other browser vendors are doing it?

Thank you for the feedback. Actually, there's a bug filed in chromium :https://bugs.chromium.org/p/chromium/issues/detail?id=492455 But there are no further actions since last May. AFAIK, it seems that none of other vendors have plans about implementing this.

After discussing with David offline, I'm going to un-take this bug and work on other topics.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #4)
> I'm actually having second thoughts about whether this is worth
> implementing; I think it's a little more complicated than I previously
> thought.  Do we know if any other browser vendors are doing it?

Blink has shipped this, and according to the issue Edge reported to CSSWG [1], they are going to ship (or also have shipped) this feature.

[1] https://github.com/w3c/csswg-drafts/issues/856

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #5)
> (In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #4)
> > Also, animations of 'transform' and 'translate' should essentially work
> > independently -- which actually makes implementing this a bit more
> > complicated than I was thinking.
> 
> In particular, I believe this means we'd need to send them separately to the
> compositor, add add significantly more animation capability to the
> compositor, which is a good bit of work.

It seems to me that there aren't many places accessing nsStyleDisplay::mSpecifiedTransform (probably three outside the style system), but there are certain amount of specific code for eCSSProperty_transform which may need investigation. That is ~35 places which are mainly in dom/animation, gfx/layers, and layout code (excluding style system).

I'm not completely understand the difficulty here. I guess it would be a good bit of work, but shouldn't be too bad. Given that two other engines is shipping this, we may want to do it as well.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I agree we want to do this. It makes animation a lot easier, at least until we ship additive animation (and even then, it provides a fixed order of operation which is useful).

The dom/animation side shouldn't be too hard--we need to add this to the list of compositor-animatable properties and make sure all the optimizations that, for example, disable async transform animations in certain situations, also apply to these properties. The hardest part of the dom/animation work is probably refactoring the tests accordingly but perhaps we can just extend the existing tests to cover some of these extra combinations.

Comment 10

[Eric Willigers]

> Blink has shipped this

The properties are behind a flag in Blink (enable experimental web platform features).

Web platform tests are in
https://github.com/w3c/web-platform-tests/tree/master/css/css-transforms-2


For WebKit I've created bug
https://bugs.webkit.org/show_bug.cgi?id=178117

Comment 11

[u459114]

https://bugs.chromium.org/p/chromium/issues/detail?id=496550

Comment 12

[u459114]

Attachment: WIP-without-animation-support

Comment 13

[u459114]

Attachment: WIP

Comment 14

[u459114]

Attachment: Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 15

[u459114]

Attachment: Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

nsDisplayList::mSpecifiedTransform is a RefPtr, there is no need put it into
intialize list.

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

Comment 16

[u459114]

Attachment: Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

We store computed values of individual transform properties in separate list,
instead of putting them all in mSpecifiedTransform, so that we can animate them
independently.

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

Comment 17

[u459114]

Attachment: Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 18

[u459114]

Attachment: Bug 1207734 - Part 3. Update alias names for rotate/translate/scale.

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

Comment 19

[u459114]

Attachment: Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 20

[u459114]

Attachment: Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 21

[u459114]

Attachment: Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 22

[u459114]

Attachment: Bug 1207734 - Part 7. Pass a combined transform list to the nsDisplayTransform.

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

Comment 23

[u459114]

Attachment: Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 24

[u459114]

Attachment: Bug 1207734 - Part 6.a. Update LayerAnimationInfo list according to the new added transform properties.

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

Comment 25

[u459114]

Attachment: Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

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

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review211738

OK, the original transform list handle `None` by a different way which based on the whole transform list, instead of the individual transform operation. For the individual transform properties, we have to implement `Animate` by ourself. This makes sense.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2252
(Diff revision 1)
>      }
>  }
>  
>  /// <https://drafts.csswg.org/css-transforms/#interpolation-of-transforms>
> +impl ComputedRotate {
> +    fn fill_unspecified(rotate_: &ComputedRotate)

I think you don't need to add `_` after the variable name.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2257
(Diff revision 1)
> +    fn fill_unspecified(rotate_: &ComputedRotate)
> +        -> Result<(Number, Number, Number, Angle), ()>{
> +        // If the axis is unspecified, it defaults to "0 0 1"
> +        match rotate_ {
> +            &Rotate::None =>
> +                Ok((0., 0., 1., Angle::Deg(0.))),

`Angle::zero()`?
nit: And maybe put this in the same line with `&Rotate::None` might be better.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2262
(Diff revision 1)
> +                Ok((0., 0., 1., Angle::Deg(0.))),
> +            &Rotate::Specified(None, None, None, angle) =>
> +                Ok((0., 0., 1., angle)),
> +            &Rotate::Specified(Some(rx), Some(ry), Some(rz), angle) =>
> +                Ok((rx, ry, rz, angle)),
> +            &_ =>

I guess using `_` is enough?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2272
(Diff revision 1)
> +
> +impl Animate for ComputedRotate {
> +    #[inline]
> +    fn animate(
> +        &self,
> +        other_: &Self,

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2288
(Diff revision 1)
> +        Ok(Rotate::Specified(Some(rx), Some(ry), Some(rz), angle))
> +    }
> +}
> +
> +impl ComputedTranslate {
> +    fn fill_unspecified(translate_: &ComputedTranslate)

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2292
(Diff revision 1)
> +            &Translate::None =>
> +                Ok((LengthOrPercentage::Length(Length::new(0.)),
> +                    LengthOrPercentage::Length(Length::new(0.)),
> +                    Length::new(0.))),

I prefer use `{}` for multiple lines.
i.e.
```
&Translate:None => {
  Ok((
      LengthOrPercentage::Length(Length::new(0.)),
      LengthOrPercentage::Length(Length::new(0.)),
      Length::new(0.),
  ))
},
```
Besides, maybe you can use `Length::zero()`, I guess.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2300
(Diff revision 1)
> +                    Length::new(0.))),
> +            &Translate::Specified(tx, Some(ty), None) =>
> +                Ok((tx, ty, Length::new(0.))),
> +            &Translate::Specified(tx, Some(ty), Some(tz)) =>
> +                Ok((tx, ty, tz)),
> +            &_ =>

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2310
(Diff revision 1)
> +
> +impl Animate for ComputedTranslate {
> +    #[inline]
> +    fn animate(
> +        &self,
> +        other_: &Self,

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2325
(Diff revision 1)
> +        Ok(Translate::Specified(sx, Some(sy), Some(sz)))
> +    }
> +}
> +
> +impl ComputedScale {
> +    fn fill_unspecified(scale_: &ComputedScale)

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2335
(Diff revision 1)
> +                Ok((1.0, 1.0, 1.0)),
> +            &Scale::Specified(sx, Some(sy), None) =>
> +                Ok((sx, sy, 1.0)),
> +            &Scale::Specified(sx, Some(sy), Some(sz)) =>
> +                Ok((sx, sy, sz)),
> +            &_ =>

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2345
(Diff revision 1)
> +
> +impl Animate for ComputedScale {
> +    #[inline]
> +    fn animate(
> +        &self,
> +        other_: &Self,

same here

Comment 27

[u459114]

boris said: add test cases in test_distance_of_transform.html for ComputedSquareDistance

Comment 28

[Boris Chiou [:boris]]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review211744

BTW, we still need to check ComputeSqauredDistance and add some tests in [1].
[1] https://searchfox.org/mozilla-central/source/dom/animation/test/mozilla/test_distance_of_transform.html

Comment 29

[Boris Chiou [:boris]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #27)
> boris said: add test cases in test_distance_of_transform.html for
> ComputedSquareDistance

Yaya. Or file a new bug for this because there are too many patches in this bug, I think.

Comment 30

[u459114]

test cases in chromium for individual transform:
https://src.chromium.org/viewvc/blink?view=revision&revision=197547

Comment 31

[u459114]

Attachment: Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 32

[u459114]

Attachment: Bug 1207734 - Part 4.b. (stylo) Update nsStyleDisplay::mCombinedTransform once transform value changed.

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

Comment 33

[u459114]

Attachment: Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 34

[u459114]

Attachment: Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 35

[u459114]

Attachment: Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 36

[u459114]

Attachment: Bug 1207734 - Part 8.a. (gecko) Update alias names for rotate/translate/scale.

Add webkit alias for individual transform since we already have webkit alias for
'transform' property.

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

Comment 37

[u459114]

Attachment: Bug 1207734 - Part 8.b. (stylo) Update alias names for rotate/translate/scale.

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

Comment 38

[u459114]

Attachment: Bug 1207734 - Part 9. Make Inspector happy.

Inspector still uses nsCSSParser for some reasons.

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

Comment 39

[u459114]

Comment on attachment 8935258 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 40

[u459114]

Comment on attachment 8935259 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 41

[u459114]

Comment on attachment 8935260 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 42

[u459114]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 43

[u459114]

Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 44

[u459114]

Comment on attachment 8935264 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 45

[u459114]

Comment on attachment 8935265 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 46

[u459114]

Comment on attachment 8935267 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 47

[u459114]

Comment on attachment 8935268 [details]
Bug 1207734 - Part 6.a. Update LayerAnimationInfo list according to the new added transform properties.

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

Comment 48

[u459114]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

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

Comment 49

[Brian Birtles (:birtles, Feb 26~ parental leave)]

CJ, I haven't gone through all these patches, but from what I skimmed, I didn't see any changes to the data we send to the compositor. I would have thought we'd need to let the compositor know which property we're animating so we make sure to compose the transform in the correct order on the compositor. Did I miss it? Or does it just work somehow?

Comment 50

[u459114]

(In reply to Brian Birtles (:birtles) from comment #49)
> CJ, I haven't gone through all these patches, but from what I skimmed, I
> didn't see any changes to the data we send to the compositor. I would have
> thought we'd need to let the compositor know which property we're animating
> so we make sure to compose the transform in the correct order on the
> compositor. Did I miss it? Or does it just work somehow?

In Part 4.c., nsDisplayTransform read the final transform from nsStyleDisplay, and nsDisplayTransform will take care of other thing(I guess)

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

From what I understand, I don't think that works. I think that means we will end up sending the combined transform to the compositor but I think we actually need to be able to animate 'scale' etc. independently, run those animations on the compositor, and then compose them in the correct order on the compositor.

Comment 52

[u459114]

(In reply to Brian Birtles (:birtles) from comment #51)
> From what I understand, I don't think that works. I think that means we will
> end up sending the combined transform to the compositor but I think we
> actually need to be able to animate 'scale' etc. independently, run those
> animations on the compositor, and then compose them in the correct order on
> the compositor.
div {
  scale: 1 2;
  transform: scale(2, 3);
}

The data that we store in the combined transform is
["scale3d", "1", "2" "2"], ["scale2d", "2" , "3"]

'scale' property will be converted into a scale3d function and be put into mCombinedTransform. ('translate' property will be converted into a translate3d function, and 'rotate' will be converted into a rotate3d one).


Although I named it as mCombinedTransform, it actually stores transform-functions separately. 'combined' here means combine individual transform functions with transform functions in 'transform' property. The logic of how to combine all these function is in 4.a(
)

Comment 53

[u459114]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #52)
> (In reply to Brian Birtles (:birtles) from comment #51)
> > From what I understand, I don't think that works. I think that means we will
> > end up sending the combined transform to the compositor but I think we
> > actually need to be able to animate 'scale' etc. independently, run those
> > animations on the compositor, and then compose them in the correct order on
> > the compositor.
> div {
>   scale: 1 2;
>   transform: scale(2, 3);
> }
> 
> The data that we store in the combined transform is
> ["scale3d", "1", "2" "2"], ["scale2d", "2" , "3"]
> 
> 'scale' property will be converted into a scale3d function and be put into
> mCombinedTransform. ('translate' property will be converted into a
> translate3d function, and 'rotate' will be converted into a rotate3d one).
> 
> 
> Although I named it as mCombinedTransform, it actually stores
> transform-functions separately. 'combined' here means combine individual
> transform functions with transform functions in 'transform' property. The
> logic of how to combine all these function is in 4.a(
> )

in 4.a(nsStyleDisplay::UpdateCombinedTransform())

Comment 54

[u459114]

(In reply to Brian Birtles (:birtles) from comment #51)
> From what I understand, I don't think that works. I think that means we will
> end up sending the combined transform to the compositor but I think we
> actually need to be able to animate 'scale' etc. independently, run those
> animations on the compositor, and then compose them in the correct order on
> the compositor.

div {
  scale: 1 2;
  transform: scale(2, 3);
}

The data that we store in the combined transform is
["scale3d", "1", "2" "1"], ["scale2d", "2" , "3"]

'scale' property will be converted into a scale3d function and be put into mCombinedTransform. ('translate' property will be converted into a translate3d function, and 'rotate' will be converted into a rotate3d one).


Although I named it as mCombinedTransform, it actually stores transform-functions separately. 'combined' here means combine individual transform functions with transform functions in 'transform' property. The logic of how to combine all these function is in 4.a(nsStyleDisplay::UpdateCombinedTransform())

Comment 55

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935258 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

https://reviewboard.mozilla.org/r/206148/#review213114

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935259 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

https://reviewboard.mozilla.org/r/206150/#review213116

Comment 57

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935260 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

https://reviewboard.mozilla.org/r/206152/#review213118

::: layout/style/nsStyleStruct.cpp:3751
(Diff revision 2)
>        shapeImage->ResolveImage(aPresContext);
>      }
>    }
>  }
>  
> +#define COMPARE_TRANSFORM_VALUES(list_) \

Can this be a static function instead?

```
static inline nsChangeHint
CompareTransformValues(..)
{
}
```

Where the "they're equal" path returns `nsChangeHint(0)`?

Then you can use:

```
transformHint |= CompareTransformValues(mSpecifiedTransform, other.mSpecifiedTransform);
// ...
```

Comment 58

[u459114]

I probably should change these two place to use mCombinedTransform?
https://searchfox.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#4639
https://searchfox.org/mozilla-central/source/layout/painting/ActiveLayerTracker.cpp#251

hi Brian,
And I guess I don't need to take care of StyleAnimationValue?
https://searchfox.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#4639

Comment 59

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213120

::: layout/style/nsCSSPropList.h:3631
(Diff revision 2)
> +    CSS_PROPERTY_PARSE_FUNCTION |
> +        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH |
> +        CSS_PROPERTY_CREATES_STACKING_CONTEXT |
> +        CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR |
> +        CSS_PROPERTY_FIXPOS_CB,
> +    "",

Should we pref them off by default?

::: layout/style/nsCSSPropList.h:3635
(Diff revision 2)
> +        CSS_PROPERTY_FIXPOS_CB,
> +    "",
> +    0,
> +    nullptr,
> +    offsetof(nsStyleDisplay, mSpecifiedRotate),
> +    eStyleAnimType_Custom)

I'm assuming you're checking with Birtles all the animation bits and such.

::: layout/style/nsCSSPropList.h:4278
(Diff revision 2)
> +        CSS_PROPERTY_FIXPOS_CB,
> +    "",
> +    0,
> +    nullptr,
> +    offsetof(nsStyleDisplay, mSpecifiedScale),
> +eStyleAnimType_Custom)

nit: indentation.

Comment 60

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/206158/#review213126

::: servo/components/style/values/generics/transform.rs:761
(Diff revision 2)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Rotate` property

Please link to the spec.

::: servo/components/style/values/generics/transform.rs:766
(Diff revision 2)
> +/// A value of the `Rotate` property
> +pub enum Rotate<Number, Angle> {
> +    /// 'none'
> +    None,
> +    /// '<number>{3}? <angle>'
> +    Specified(Option<Number>, Option<Number>, Option<Number>, Angle)

This should be `Option<(Number, Number, Number)>, Angle`, and that way you can remove the `panic!`s.

::: servo/components/style/values/generics/transform.rs:769
(Diff revision 2)
> +    None,
> +    /// '<number>{3}? <angle>'
> +    Specified(Option<Number>, Option<Number>, Option<Number>, Angle)
> +}
> +
> +impl<Number: ToCss + Copy + fmt::Debug, Angle: ToCss + Copy + fmt::Debug> ToCss for Rotate<Number, Angle> {

I think this can just be `derive`d, adding `ToCss` to the `derive()` list, any reason it cannot be done?

Comment 61

[u459114]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213120

> I'm assuming you're checking with Birtles all the animation bits and such.

I think this value is for old system

Comment 62

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #54)
> div {
>   scale: 1 2;
>   transform: scale(2, 3);
> }
> 
> The data that we store in the combined transform is
> ["scale3d", "1", "2" "1"], ["scale2d", "2" , "3"]
> 
> 'scale' property will be converted into a scale3d function and be put into
> mCombinedTransform. ('translate' property will be converted into a
> translate3d function, and 'rotate' will be converted into a rotate3d one).
> 
> 
> Although I named it as mCombinedTransform, it actually stores
> transform-functions separately. 'combined' here means combine individual
> transform functions with transform functions in 'transform' property. The
> logic of how to combine all these function is in
> 4.a(nsStyleDisplay::UpdateCombinedTransform())

Right, but I'm still not sure that if, for example, we start a scale animation, then later start a translate animation, that when we animate those two on the compositor, we'll know that we need to combine the scale after the translate (since normally we'll send those independent animations in composite order, which in this case will be based on when they are created).

Comment 63

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935264 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/206160/#review213130

::: servo/components/style/values/computed/transform.rs:15
(Diff revision 2)
>  use values::animated::ToAnimatedZero;
>  use values::computed::{Angle, Integer, Length, LengthOrPercentage, Number, Percentage};
>  use values::computed::{LengthOrNumber, LengthOrPercentageOrNumber};
>  use values::generics::transform::{self, Matrix as GenericMatrix, Matrix3D as GenericMatrix3D};
>  use values::generics::transform::{Transform as GenericTransform, TransformOperation as GenericTransformOperation,
> -    Rotate as GenericRotate};
> +    Rotate as GenericRotate, Translate as GenericTranslate};

use multiple `use` lines instead of multi-line imports.

::: servo/components/style/values/generics/transform.rs:798
(Diff revision 2)
>          }
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]

I think you could `derive(ToCss)` here too.

::: servo/components/style/values/generics/transform.rs:799
(Diff revision 2)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Translate` property

Link to spec.

::: servo/components/style/values/generics/transform.rs:804
(Diff revision 2)
> +/// A value of the `Translate` property
> +pub enum Translate<LengthOrPercentage, Length> {
> +    /// 'none'
> +    None,
> +    /// '<number>{1,3}'
> +    Specified(LengthOrPercentage, Option<LengthOrPercentage>, Option<Length>)

These are definitely not `<number>`s, so either the comment needs updating, or the types here should change.

Comment 64

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935265 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/206162/#review213136

::: servo/components/style/properties/gecko.mako.rs:683
(Diff revision 2)
> +        use values::generics::transform::TransformOperation;
> +
> +        unsafe { self.gecko.${gecko_ffi_name}.clear() };
> +
> +        match other {
> +            Scale::None => (),

This needs to reset the property, not do nothing.

::: servo/components/style/properties/gecko.mako.rs:685
(Diff revision 2)
> +        unsafe { self.gecko.${gecko_ffi_name}.clear() };
> +
> +        match other {
> +            Scale::None => (),
> +            Scale::Specified(sx, None, None) => {
> +                let operation = vec![TransformOperation::ScaleX(sx)];

Please (this also applies to the previous patches) don't use `vec![]` here, that implies a memory allocation that you can trivially avoid everywhere.

::: servo/components/style/properties/longhand/box.mako.rs:626
(Diff revision 2)
>                            spec="https://drafts.csswg.org/css-transforms-2/#individual-transforms")}
>  
> +${helpers.predefined_type("scale", "Scale",
> +                          "generics::transform::Scale::None",
> +                          animation_value_type="ComputedValue",
> +                          gecko_ffi_name="mSpecifiedScale",

I think gecko_ffi_name (in the previous patches too) is useless here.

::: servo/components/style/values/generics/transform.rs:841
(Diff revision 2)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Scale` property

As before, I think you should add a comment to the spec and derive `ToCss`.

::: servo/components/style/values/generics/transform.rs:845
(Diff revision 2)
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Scale` property
> +pub enum Scale<Number> {
> +    /// 'none'
> +    None,
> +    /// '<length-percentage> [ <length-percentage> <length>? ]?'

These are definitely not lengths, so just as before, either this comment should be updated, or the types should change.

Comment 65

[u459114]

(In reply to Brian Birtles (:birtles) from comment #62)
> Right, but I'm still not sure that if, for example, we start a scale
> animation, then later start a translate animation, that when we animate
> those two on the compositor, we'll know that we need to combine the scale
> after the translate (since normally we'll send those independent animations
> in composite order, which in this case will be based on when they are
> created).
According to the spec:
https://drafts.csswg.org/css-transforms-2/#ctm
'translate' is always before 'scale'. If you want to combine 'translate' after 'scale', you can only use 'transform' property.

Comment 66

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> (In reply to Brian Birtles (:birtles) from comment #62)
> > Right, but I'm still not sure that if, for example, we start a scale
> > animation, then later start a translate animation, that when we animate
> > those two on the compositor, we'll know that we need to combine the scale
> > after the translate (since normally we'll send those independent animations
> > in composite order, which in this case will be based on when they are
> > created).
> According to the spec:
> https://drafts.csswg.org/css-transforms-2/#ctm
> 'translate' is always before 'scale'. If you want to combine 'translate'
> after 'scale', you can only use 'transform' property.

Right, but what makes us preserve that order when animating on the compositor? We don't tell the compositor if we are animating 'translate' or 'transform' right?

Comment 67

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Once I have a build of this patch series going, I'll make up a test case to demonstrate what I mean.

Comment 68

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213142

I'm not sure how this plays with all the animation stuff... Probably birtles is better reviewer for this.

::: layout/style/nsStyleStruct.h:2601
(Diff revision 1)
>    StyleGeometryBox mTransformBox; // [reset] see nsStyleConsts.h
>    RefPtr<nsCSSValueSharedList> mSpecifiedTransform; // [reset]
>    RefPtr<nsCSSValueSharedList> mSpecifiedRotate; // [reset]
>    RefPtr<nsCSSValueSharedList> mSpecifiedTranslate; // [reset]
>    RefPtr<nsCSSValueSharedList> mSpecifiedScale; // [reset]
> +  RefPtr<nsCSSValueSharedList> mCombinedTransform;

Please document.

::: layout/style/nsStyleStruct.cpp:4022
(Diff revision 1)
> +  // According to the spec:
> +  // https://drafts.csswg.org/css-transforms-2/#ctm
> +  //
> +  // Combine transform properties by the following sequence:
> +  //   translate > rotate > scale > transform
> +  const RefPtr<nsCSSValueSharedList> shareLists[] = { mSpecifiedTranslate,

There's no need for this to addref everything, `const RefPtr<nsCSSValueSharedList>&`?

::: layout/style/nsStyleStruct.cpp:4027
(Diff revision 1)
> +  const RefPtr<nsCSSValueSharedList> shareLists[] = { mSpecifiedTranslate,
> +                                                      mSpecifiedRotate,
> +                                                      mSpecifiedScale,
> +                                                      mSpecifiedTransform};
> +
> +  nsTArray<nsCSSValueList*> valueLists;

Let's reserve space beforehand to avoid reallocating.

Comment 69

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936572 [details]
Bug 1207734 - Part 4.b. (stylo) Update nsStyleDisplay::mCombinedTransform once transform value changed.

https://reviewboard.mozilla.org/r/207328/#review213148

You should set this after cascading all the properties, look at:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/servo/components/style/properties/properties.mako.rs#3508

::: servo/components/style/properties/gecko.mako.rs:569
(Diff revision 1)
>              x => {
>                  panic!("Found unexpected value in style struct for rotate property: {:?}", x)
>              }
>          }
> +        unsafe {
> +            Gecko_UpdateTransform(&mut self.gecko);

This is not the right place to do this, this doesn't handle `inherit` or `reset` keywords properly.

Comment 70

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

https://reviewboard.mozilla.org/r/207330/#review213154

I don't really know about `FrameTransformProperties` :)

Comment 71

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935267 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

https://reviewboard.mozilla.org/r/206166/#review213156

Comment 72

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935268 [details]
Bug 1207734 - Part 6.a. Update LayerAnimationInfo list according to the new added transform properties.

https://reviewboard.mozilla.org/r/206168/#review213158

If birtles can review this, that'd be great. I can take a look at what LayerAnimationInfo does, but I'm not terribly familiar with this code.

Comment 73

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936577 [details]
Bug 1207734 - Part 8.b. (stylo) Update alias names for rotate/translate/scale.

https://reviewboard.mozilla.org/r/207338/#review213164

::: servo/components/style/properties/longhand/box.mako.rs:611
(Diff revision 1)
>                            flags="CREATES_STACKING_CONTEXT FIXPOS_CB",
>                            spec="https://drafts.csswg.org/css-transforms/#propdef-transform")}
>  
>  ${helpers.predefined_type("rotate", "Rotate",
>                            "generics::transform::Rotate::None",
> +                          extra_prefixes="webkit",

Can you elaborate on why the alias? Are they in any compat spec?

Does this have any webcompat implications?

I'd prefer not supporting the webkit prefix if we don't need to.

Comment 74

[u459114]

(In reply to Brian Birtles (:birtles) from comment #66)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> > (In reply to Brian Birtles (:birtles) from comment #62)
> > > Right, but I'm still not sure that if, for example, we start a scale
> > > animation, then later start a translate animation, that when we animate
> > > those two on the compositor, we'll know that we need to combine the scale
> > > after the translate (since normally we'll send those independent animations
> > > in composite order, which in this case will be based on when they are
> > > created).
> > According to the spec:
> > https://drafts.csswg.org/css-transforms-2/#ctm
> > 'translate' is always before 'scale'. If you want to combine 'translate'
> > after 'scale', you can only use 'transform' property.
> 
> Right, but what makes us preserve that order when animating on the
> compositor? We don't tell the compositor if we are animating 'translate' or
> 'transform' right?

(To be honest, I am not that familiar of how animation coworks with compositor. So, I try to explain how I think it works in my mind)

Let said we have a div matches these two rules
div {
  scale: 1 1;
  translate: 50px 50px;
  transition: scale 2s, translate 2s;
}
div:hover {
  scale: 2 2;
  translate: 100px 100px;
}
<div></div>


When we hover on that div, transition being trigger, the transform-functions that we give to nsDisplayTransform looks like
1. At the beginning, the content in mCombinedTransform is
   [['translate3d', '50px', '50px', '0px'], ['scale3d', '1', '1', '1']]
2. 1 second pass, by the function that I added in part 6.b, the content of mCombinedTransform becomes:
   [['translate3d', '75px', '75px', '0px'], ['scale3d', '1.5', '1.5', '1.5']]
3. In the end(2 seconds pass), the content of mCombinedTransform becomes:
   [['translate3d', '100px', '100px', '0px'], ['scale3d', '2', '2', '2']]
The animation trait of individual transform(in part 6.b) keep update interpolated prop value for scale/translate and rotate separatly.

Comment 75

[u459114]

(In reply to Brian Birtles (:birtles) from comment #67)
> Once I have a build of this patch series going, I'll make up a test case to
> demonstrate what I mean.

That will be really helpful. Thanks

Comment 76

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

::: layout/style/nsCSSPropList.h:3626
(Diff revision 2)
>      eStyleAnimType_Sides_Right)
> +CSS_PROP_DISPLAY(
> +    rotate,
> +    rotate,
> +    Rotate,
> +    CSS_PROPERTY_PARSE_FUNCTION |

Hmm, I'm not sure this should use `CSS_PROPERTY_PARSE_FUNCTION`, should it?

Let's cam have a look, since he's more familiar with Gecko's CSS parser.

Comment 77

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936576 [details]
Bug 1207734 - Part 8.a. (gecko) Update alias names for rotate/translate/scale.

https://reviewboard.mozilla.org/r/207336/#review213172

I don't really think we should add a webkit alias for these properties just because transform happens to have it too. What's the reasoning for this change?

Comment 78

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936578 [details]
Bug 1207734 - Part 9. Make Inspector happy.

https://reviewboard.mozilla.org/r/207340/#review213174

I'm not sure this is right, this doesn't parse the properties at all, so inspector is going to do the wrong thing, right?

Comment 79

[u459114]

Comment on attachment 8936578 [details]
Bug 1207734 - Part 9. Make Inspector happy.

https://reviewboard.mozilla.org/r/207340/#review213174

Without this change, we will hit the assertion at [1](for sure)

And, no, it acutally works correctly. I can set and get the prop value of individual transform correctly, all I need to do is to bypass that assertion. 

[1] https://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#11835

Comment 80

[u459114]

Comment on attachment 8936577 [details]
Bug 1207734 - Part 8.b. (stylo) Update alias names for rotate/translate/scale.

https://reviewboard.mozilla.org/r/207338/#review213164

> Can you elaborate on why the alias? Are they in any compat spec?
> 
> Does this have any webcompat implications?
> 
> I'd prefer not supporting the webkit prefix if we don't need to.

Will remove these two patches

Comment 81

[Emilio Cobos Álvarez (:emilio)]

Do we also know why Blink implemented this but never shipped this?

Comment 82

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case for compositor animations

It seems like the animations in this test case are not running on the compositor. Let me try to work out a list of the places that need to be fixed.

Comment 83

[Hiroyuki Ikezoe (:hiro)]

I am curious how the spec defines animations for these properties, especially transitions.   For example;

* changes scale value to the same value of transform

  element.style = 'transition: scale 1s; transform: scale(2);';

  element.style.scale = '2';

This case triggers a transition for scale, I think, and users want to see a transition from scale(2) to scale(4)?.  I guess this case does not work with the current patches.

Another test case that does not work I can think of

* changes scale value with static translate value

  element.style = 'transition: scale 1s; translate: 100px';

  element.style.scale = '2';

Anyway, I'd highly recommend to write various animation test cases first.

Comment 84

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I had a look at what we need to support compositor animations and I think the main part we need to change is the interaction between EffectCompositor and nsDisplayList.

The simplest thing is probably to not change the compositor but to just send a suitably sorted list of transform animations to the compositor.
  
So we could either change the set of animations returned by EffectCompositor / Animation or we could handle it all in nsDisplayList.cpp
  
I think the simplest thing would be to do the grouping and sorting in nsDisplayList although that will probably be less efficient since we'll do the loop in KeyframeEffectReadOnly::GetEffectiveAnimationOfProperty more times than are strictly needed. That's probably ok though, and we can optimize that later if needed.
  
We need to bear in mind that we can have, for example, two animations that animation *both* translate and transform, for example. In that case, we'll want to sort the translate components of both animations before the transform components.

I think one way to do this is to handle it all in AddAnimationsForProperty (in nsDiplayList.cpp).

Where we call EffectCompositor::GetAnimationsForCompositor, we'll want to call it, then iterate over the result, four times for translate, rotate, scale, and transform, and in that order.

Apart from that we'll also need to change at least these places:

dom/animation/EffectCompositor.cpp
  FindAnimationsForCompositor
   - Needs to check for the new properties when synchronizing with geometric properties
     (Needs tests for this too)
   
dom/animation/KeyframeEffectReadOnly.cpp
  CanThrottle
   - Needs to check for the new properties
  ShouldBlockAsyncTransformAnimations
   - Likewise
  MaybeUpdateFrameForCompositor()
   - Likewise
  ContainsAnimatedScale
   - Likewise
  UpdateEffectSet
   - Likewise
   
dom/animation/Animation.cpp
  ShouldBeSynchronizedWithMainThread
   - Needs to check for the new properties

layout/painting/nsDisplayList.cpp
  SetAnimatable
   - Needs to check for the new properties
  nsDisplayTransform::MayBeAnimated
   - Likewise
   
layout/base/nsLayoutUtils.cpp
  ComputeSuitableScaleForAnimation
   - Needs to look for both scale and transform animations

Comment 85

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #83)
> I am curious how the spec defines animations for these properties,
> especially transitions.   For example;
> 
> * changes scale value to the same value of transform
> 
>   element.style = 'transition: scale 1s; transform: scale(2);';
> 
>   element.style.scale = '2';
> 
> This case triggers a transition for scale, I think, and users want to see a
> transition from scale(2) to scale(4)?.

Looking into this case I notice that the spec says:

> The scale property accepts 1-3 values, each specifying a scale along one axis, in order X, Y, then Z. Unspecified scales default to 1.

As in, 'scale: 2' actually is the same as 'scale: 2 1' NOT 'scale: 2 2'. That seems inconsistent with 'scale(2)' so we should clarify if that is what the spec intends.

Also, as hiro points out, transitions don't appear to work. (I haven't checked but I guess we're failing to get the correct before-change and after-change scale values.)

Once they are implemented we'll need to make sure that 'none' -> 2 interpolates as '1 1' to '2 2'

Comment 86

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213318

::: layout/style/nsStyleStruct.cpp:4025
(Diff revision 1)
> +  // Combine transform properties by the following sequence:
> +  //   translate > rotate > scale > transform
> +  const RefPtr<nsCSSValueSharedList> shareLists[] = { mSpecifiedTranslate,
> +                                                      mSpecifiedRotate,
> +                                                      mSpecifiedScale,
> +                                                      mSpecifiedTransform};

Nit: Space before }

::: layout/style/nsStyleStruct.cpp:4028
(Diff revision 1)
> +                                                      mSpecifiedRotate,
> +                                                      mSpecifiedScale,
> +                                                      mSpecifiedTransform};
> +
> +  nsTArray<nsCSSValueList*> valueLists;
> +  for (uint8_t i = 0; i < sizeof(shareLists) / sizeof(shareLists[0]); i++) {

Does ArrayLength not work here? Or, better still, a range-based for loop?

Comment 87

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #85)
> As in, 'scale: 2' actually is the same as 'scale: 2 1' NOT 'scale: 2 2'.
> That seems inconsistent with 'scale(2)' so we should clarify if that is what
> the spec intends.

Started following up this issue: https://github.com/w3c/csswg-drafts/issues/856#issuecomment-351547064

I also noticed there's some discussion about the rotate property syntax:

  https://github.com/w3c/csswg-drafts/issues/1269

Comment 88

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'm not sure if I should go ahead and review these patches any further. It seems like it might be better to wait until these patches support transitions first since that might require revisions to some of them.

Comment 89

[Cameron McCormack (:heycam)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213120

> Should we pref them off by default?

I agree, we should.

> I think this value is for old system

You may need to add some case somewhere in StyleAnimationValue to avoid an assertion about not handling this property's animation in the old style system.  Can you make it eStyleAnimType_None, or would that prevent animation in Servo too?

Comment 90

[Cameron McCormack (:heycam)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

> Hmm, I'm not sure this should use `CSS_PROPERTY_PARSE_FUNCTION`, should it?
> 
> Let's cam have a look, since he's more familiar with Gecko's CSS parser.

As you mention in comment 79, this means that when Stylo is disabled, we'll get into nsCSSParser::ParsePropertyByFunction and assert that we got an unexpected property.  If we don't want to implement support for these properties in the old style system, we don't have a great way to make the property completely invisible (including e.g. on getComputedStyle objects).

I guess it's probably fine to just make ParsePropertyByFunction not assert and just return false, and live with the fact that we'd expose the property on getComputedStyle objects.  As long as we put these properties behind a pref and don't enable the pref by default before we remove the old style system.

Comment 91

[Cameron McCormack (:heycam)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213370

Comment 92

[Eric Willigers]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #81)
> Do we also know why Blink implemented this but never shipped this?

There are no implementation issues blocking Blink shipping.

I didn't send a Blink Intent to Ship as
- The properties are defined in Transforms Level 2, and Transforms Level 1 is still a Working Draft.
- There wasn't a second implementation.

Transforms Level 1 is soon moving to CR.

We started a polyfill for web animation of individual transform properties and motion path: https://github.com/Motion-Path-Polyfill/motion-path-js with examples tested in Firefox https://ewilligers.github.io/motion-path-polyfill-examples/  This was an intern project, we don't have engineering resources assigned to polish this. The second implementation being a native UA implementation is much preferred.

Comment 93

[Emilio Cobos Álvarez (:emilio)]

There's something that bothers me to some extent, which is that these properties introduce some really weird behavior.

In particular, you can no longer set transform: none and expect the element to not be transformed... I wonder if `transform` should become some sort of shorthand property for these three...

Comment 94

[u459114]

(In reply to Brian Birtles (:birtles) from comment #87)
> (In reply to Brian Birtles (:birtles) from comment #85)
> > As in, 'scale: 2' actually is the same as 'scale: 2 1' NOT 'scale: 2 2'.
> > That seems inconsistent with 'scale(2)' so we should clarify if that is what
> > the spec intends.
> 
> Started following up this issue:
> https://github.com/w3c/csswg-drafts/issues/856#issuecomment-351547064
> 
> I also noticed there's some discussion about the rotate property syntax:
> 
>   https://github.com/w3c/csswg-drafts/issues/1269
thThat's why I rewite Animation trait for individual transform rather than reuse the code of TransformFunction for interop in Part 6.b. The behavior of unspecified value is totally different between individual transform and transform function in 'transform' property.

Comment 95

[u459114]

(In reply to Brian Birtles (:birtles) from comment #82)
> Created attachment 8936659 [details]
> Test case for compositor animations
> 
> It seems like the animations in this test case are not running on the
> compositor. Let me try to work out a list of the places that need to be
> fixed.
Hi Brian,
May I know how to make sure whether an animation is running on the compositor?

Comment 96

[Hiroyuki Ikezoe (:hiro)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #95)
> (In reply to Brian Birtles (:birtles) from comment #82)
> > Created attachment 8936659 [details]
> > Test case for compositor animations
> > 
> > It seems like the animations in this test case are not running on the
> > compositor. Let me try to work out a list of the places that need to be
> > fixed.
> Hi Brian,
> May I know how to make sure whether an animation is running on the
> compositor?

You can easily check it on animation inspector in devtools panel.  If the animation is running on the compositor, you will see a lightning bolt icon.

Comment 97

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks Eric. I've filed [1] for making the values for `scale` consistent. Any comments appreciated!

[1] https://github.com/w3c/csswg-drafts/issues/2109

Comment 98

[u459114]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #96)
> > Hi Brian,
> > May I know how to make sure whether an animation is running on the
> > compositor?
> 
> You can easily check it on animation inspector in devtools panel.  If the
> animation is running on the compositor, you will see a lightning bolt icon.

Got it, thanks, hiro

Comment 99

[u459114]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

> As you mention in comment 79, this means that when Stylo is disabled, we'll get into nsCSSParser::ParsePropertyByFunction and assert that we got an unexpected property.  If we don't want to implement support for these properties in the old style system, we don't have a great way to make the property completely invisible (including e.g. on getComputedStyle objects).
> 
> I guess it's probably fine to just make ParsePropertyByFunction not assert and just return false, and live with the fact that we'd expose the property on getComputedStyle objects.  As long as we put these properties behind a pref and don't enable the pref by default before we remove the old style system.

Instead of remove that assertion, we can modify it from
  MOZ_ASSERT(false, "should not be called");
to
  MOZ_ASSERT(nsLayoutUtils::StyloEnabled(), "should not be called in the old"
                                            "style system.");

Comment 100

[Cameron McCormack (:heycam)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

> Instead of remove that assertion, we can modify it from
>   MOZ_ASSERT(false, "should not be called");
> to
>   MOZ_ASSERT(nsLayoutUtils::StyloEnabled(), "should not be called in the old"
>                                             "style system.");

I'd prefer to list the properties that we are deciding not to implement in the old style system explicitly, so like:

  case eCSSPropertyID_translate:
  case eCSSPropertyID_rotate:
  case eCSSPropertyID_scale:
    // These properties aren't implemented in the old style system.
    return false;
  default:
    MOZ_ASSERT(false, ...);

just to avoid weakening the existing assertion for current cases where we're still using the old style system (in chrome windows, and any sites in the stylo blocklist).

Comment 101

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213452

I don't think you need my review on this -- emilio and heycam's review seems sufficient.

The only feedback I have is that if we decide to split supporting animation on the compositor to a separate bug then we should probably drop CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR from this patch.

Comment 102

[u459114]

Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/206158/#review213126

> This should be `Option<(Number, Number, Number)>, Angle`, and that way you can remove the `panic!`s.

If I do this change, I need to impl the following trait:
  ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero

Option<()> is not handled in those marcos(I guess)

Comment 103

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Transition test case

This test case animates for me on Chrome Canary (I guess I have enabled experimental web platform features) but with these patches:

* It animated for me once
* Every time since it doesn't animate
* Once the content process crashed

Also, getComputedStyle(elem).scale doesn't seem to produce any output. I guess we need to modify nsComputedDOMStyle.

See: https://developer.mozilla.org/en-US/docs/Mozilla/Adding_a_new_style_property#Computed_Style

Comment 104

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8935268 [details]
Bug 1207734 - Part 6.a. Update LayerAnimationInfo list according to the new added transform properties.

https://reviewboard.mozilla.org/r/206168/#review213466

Comment 105

[Emilio Cobos Álvarez (:emilio)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #102)
> Comment on attachment 8935263 [details]
> Bug 1207734 - Part 3.a. (stylo) Implement rotate property styling.
> 
> https://reviewboard.mozilla.org/r/206158/#review213126
> 
> > This should be `Option<(Number, Number, Number)>, Angle`, and that way you can remove the `panic!`s.
> 
> If I do this change, I need to impl the following trait:
>   ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero
> 
> Option<()> is not handled in those marcos(I guess)

You should implement those like:

impl<T, U, D> ToComputedValue for (T, U, D)
where
    T: ToComputedValue,
    U: ToComputedValue,
    D: ToComputedValue,
{
    type ComputedValue = (T::ComputedValue, U::ComputedValue, D::ComputedValue);

    fn to_computed_value(&self) -> Self::ComputedValue {
        (self.0.to_computed_value(), self.1.to_computed_value(), self.2.to_computed_value())
    }
}

Comment 106

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review213480

Clearing review request for now, since we at least need to use animate_multiplicative_factor. It would be helpful to write tests for this too.

If you find that supporting animation is too much work, feel free to split it off into a separate bug.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2364
(Diff revision 2)
> +        Ok(Scale::Specified(from.0.animate(&to.0, procedure)?,
> +                            Some(from.1.animate(&to.1, procedure)?),
> +                            Some(from.2.animate(&to.2, procedure)?)))

I think this needs to use animate_multiplicative_factor.

But we should really add tests for this to confirm.

A good place to start for that would be:

  testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js

Comment 107

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213488

Clearing review request for now since it seems like there is a bit of review feedback that needs to be addressed here.

(I also wonder if it makes sense to just invalidate the combined transform -- e.g. make it null -- and then lazily generate it when needed.)

Comment 108

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8936572 [details]
Bug 1207734 - Part 4.b. (stylo) Update nsStyleDisplay::mCombinedTransform once transform value changed.

https://reviewboard.mozilla.org/r/207328/#review213490

Clearing review request now since it seems at least Emilio's comment needs to be addressed.

Comment 109

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

https://reviewboard.mozilla.org/r/207330/#review213492

This is probably fine (but in reviewing this I start to wonder if we'll need to keep these separate further down the pipeline so we can send independent base values to the compositor for additive animation).

Comment 110

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213142

> There's no need for this to addref everything, `const RefPtr<nsCSSValueSharedList>&`?

arrays of references is not legal in c++ standard, so, we will declare it as const RefPtr<nsCSSValueSharedList>* instead.

Comment 111

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936575 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

https://reviewboard.mozilla.org/r/207334/#review213846

This probably needs a reftest too for `reset` values, and ideally also the other wide keywords, since reset was broken with the other patches I believe.

r=me with one, or with another patch that adds one.

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-1-ref.html:42
(Diff revision 1)
> +      .row_2 {
> +        top: 250px;
> +      }
> +      .scale_2{
> +        left:100px;
> +        width:50px;

nit: Super minor, but let's use whitespace consistently? That is, always a space after the colon, or never (though always is the most common style I guess?)

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-2d.html:7
(Diff revision 1)
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>Individual transform: combine individual transform properties</title>
> +    <link rel="author" title="CJ Ku" href="mailto:[e-mail]">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">

I think these need `<link rel="match">` tags?

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-2e.html:3
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +  <head>

nit: I personally see better what's going on if you remove superfluous markup, like `<html>` / `<head>` / `<body>`, though if you disagree it's not a big deal.

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-2e.html:8
(Diff revision 1)
> +  <head>
> +    <meta charset="utf-8">
> +    <title>Individual transform: combine individual transform properties</title>
> +    <link rel="author" title="CJ Ku" href="mailto:[e-mail]">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <style type="text/css">

nit: not need for `type="text/css"`, here nor anywhere else.

Comment 112

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/206158/#review213850

::: servo/components/style/values/specified/transform.rs:529
(Diff revision 2)
> +
> +        if let Some(rx) = input.try(|i| Number::parse(context, i)).ok() {
> +            // <number>{3} <angle>
> +            let ry = Number::parse(context, input)?;
> +            let rz = Number::parse(context, input)?;
> +            let angle = specified::Angle::parse_with_unitless(context, input)?;

This needs a link to the spec, why do we allow unitless angles here? I don't see anything like that on the spec.

Comment 113

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936574 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

https://reviewboard.mozilla.org/r/207332/#review213848

Clearing because I'm not convinced it's correct.

::: layout/style/test/property_database.js:6300
(Diff revision 1)
> +  gCSSProperties["rotate"] = {
> +    domProp: "rotate",
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "45deg", "45grad", "0",  "72rad", "0.25turn", ".57rad",

Looking at the spec, it looks to me like `0` shouldn't be a valid value? I left a comment on another commit regarding this. If we allow unitless angle here, needs a comment on the code.

::: layout/style/test/property_database.js:6310
(Diff revision 1)
> +  gCSSProperties["translate"] = {
> +    domProp: "translate",
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "-4px", "3px", "4em", "50%", "4px 5px 6px",

Add tests for `calc`? Here and the others.

Comment 114

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review213852

Clearing because there seems to be a large amount of animation issues, and the patch may need to change significantly. Birtles is probably a better reviewer than me on this, but happy to take a look too.

Comment 115

[Hiroyuki Ikezoe (:hiro)]

Yeah, we might want to split making these properties animatable in a later bug.  Brian suggested to make these property non-compositor runnable properties in comment 101, but even so I suspect there are bunch of annoying test cases that we need to care about, e.g. transform and scale animations on an element. 

So, I'd suggest that we make these properties non-animatable in this bug and make sure transform animations run on the compositor with various *static* values for these properties properly, then make them animatable in a later bug.

Comment 116

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #113)
> Looking at the spec, it looks to me like `0` shouldn't be a valid value? I
> left a comment on another commit regarding this. If we allow unitless angle
> here, needs a comment on the code.

I suspect the spec is wrong here since I believe the rotate() function etc. explicitly allow this and we should be consistent with that. So, I think we should file a spec issue to check/clarify this.

Comment 117

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #115)
> So, I'd suggest that we make these properties non-animatable in this bug and
> make sure transform animations run on the compositor with various *static*
> values for these properties properly, then make them animatable in a later
> bug.

Yes, I agree.

Comment 118

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(I am a bit concerned that the approach we have taken here might now allow animating correctly--i.e. we might be merging the transforms too soon--but I guess we can fix that in the animation bug.)

Comment 119

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #116)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #113)
> > Looking at the spec, it looks to me like `0` shouldn't be a valid value? I
> > left a comment on another commit regarding this. If we allow unitless angle
> > here, needs a comment on the code.
> 
> I suspect the spec is wrong here since I believe the rotate() function etc.
> explicitly allow this and we should be consistent with that. So, I think we
> should file a spec issue to check/clarify this.

Yeah, those are supposed to be exceptions rather than the rule, to avoid web compat issues, but I guess it makes sense to be consistent with transform.

(In reply to Brian Birtles (:birtles) from comment #118)
> (I am a bit concerned that the approach we have taken here might now allow
> animating correctly--i.e. we might be merging the transforms too soon--but I
> guess we can fix that in the animation bug.)

If we can remove that merging code from style I'd feel much more comfortable with the patches.

Comment 120

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #119)
> Yeah, those are supposed to be exceptions rather than the rule, to avoid web
> compat issues, but I guess it makes sense to be consistent with transform.

I filed https://github.com/w3c/csswg-drafts/issues/2116... I have mixed feelings about it, so whatever the WG decides would be fine with me.

Comment 121

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #120)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #119)
> > Yeah, those are supposed to be exceptions rather than the rule, to avoid web
> > compat issues, but I guess it makes sense to be consistent with transform.
> 
> I filed https://github.com/w3c/csswg-drafts/issues/2116... I have mixed
> feelings about it, so whatever the WG decides would be fine with me.

Not that my personal feeling matters much anyway, though :P

Testing whatever the resolution for that is on WPT would be nice.

Comment 122

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #119)
> (In reply to Brian Birtles (:birtles) from comment #118)
> > (I am a bit concerned that the approach we have taken here might now allow
> > animating correctly--i.e. we might be merging the transforms too soon--but I
> > guess we can fix that in the animation bug.)
> 
> If we can remove that merging code from style I'd feel much more comfortable
> with the patches.

In light of that, it might make sense to include animation in this patch.

For example, if we discover that we can't support animation properly so long as we're merging transforms in style, then maybe much of the work in this patch would need to be redone?

For what it's worth, my particular concern is how additive animations of 'scale', 'translate' etc. will work, particularly when we send them to the compositor. I fear they need to be treated as separate channels there (i.e. we can't just send them as a bunch of otherwise transform animations).

It also feels a little unfortunate to have to remember to call "UpdateTransform". If we could generate the combined transform on demand somewhere later in the pipeline that would seem to be simpler, but then I guess that runs the risk of one of the many users of transforms forgetting to include the individual properties.

Comment 123

[Eric Willigers]

(In reply to Brian Birtles (:birtles) from comment #122)
> For what it's worth, my particular concern is how additive animations of
> 'scale', 'translate' etc. will work, particularly when we send them to the
> compositor. I fear they need to be treated as separate channels there (i.e.
> we can't just send them as a bunch of otherwise transform animations).

If translate, scale, rotate, transform are animated with different timing functions, then they would need to be separate channels (or not animated on the compositor). (In Chromium, we currently animate on the main thread if two or more of the properties are in effect for an element.)

Comment 124

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yeah, the approach I suggested in comment 84 still sends them to the compositor as independent transform animations which will be interpolated separately and then combined. However, once you have additive scale and additive translate and additive rotate animations (or perhaps even just those without missing to/from keyframes), I think you need to get separate base values to send to the compositor, then interpolate them independently, add their stacks independently, and then combine them, which I think means you need to differentiate between the sources of the different transform animations on the compositor too.

Comment 125

[u459114]

Comment on attachment 8935265 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/206162/#review213136

> This needs to reset the property, not do nothing.

This property is reset by #680(3 lines above) alreay
  unsafe { self.gecko.${gecko_ffi_name}.clear() };

Comment 126

[u459114]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review213480

> I think this needs to use animate_multiplicative_factor.
> 
> But we should really add tests for this to confirm.
> 
> A good place to start for that would be:
> 
>   testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js

Filed bug 1425837 as a follow-up.

Comment 127

[u459114]

Attachment: Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 128

[u459114]

Attachment: Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 129

[u459114]

Attachment: bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

We still need manually do it before bug 1423971 been fixed.

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

Comment 130

[u459114]

Attachment: Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

Update manifest by the following command:
  ./mach wpt-manifest-update

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

Comment 131

[u459114]

Comment on attachment 8935258 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 132

[u459114]

Comment on attachment 8935259 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 133

[u459114]

Comment on attachment 8935260 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 134

[u459114]

Comment on attachment 8935267 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 135

[u459114]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 136

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 137

[u459114]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 138

[u459114]

Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 139

[u459114]

Comment on attachment 8935264 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 140

[u459114]

Comment on attachment 8935265 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 141

[u459114]

Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

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

Comment 142

[u459114]

Comment on attachment 8936574 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 143

[u459114]

Comment on attachment 8936575 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 144

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review214454

::: layout/style/nsStyleStruct.cpp:4027
(Diff revision 2)
> +nsStyleDisplay::GetCombinedTransform() const
> +{
> +  // Return mSpecifiedTransform directly if there is no individual transform
> +  // at all.
> +  if (!mSpecifiedTranslate && !mSpecifiedRotate && !mSpecifiedScale) {
> +    return mSpecifiedTransform;

return do_AddRef(mSpecifiedTransform);

Comment 145

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 146

[u459114]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 147

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 148

[u459114]

Comment on attachment 8937971 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 149

[u459114]

Attachment: Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 150

[u459114]

Attachment: Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 151

[u459114]

Attachment: Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 152

[u459114]

Attachment: Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 153

[u459114]

Attachment: Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 154

[u459114]

Attachment: Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 155

[u459114]

Attachment: bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

We still need manually do it before bug 1423971 been fixed.

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

Comment 156

[u459114]

Attachment: Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

Update manifest by the following command:
  ./mach wpt-manifest-update

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

Comment 157

[u459114]

Hi Emilio,
Because of an incorrect push, some review history disappear. Sorry for that. But I did fixed issues that you posted, please have a look again.

Comment 158

[u459114]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #157)
> Hi Emilio,
> Because of an incorrect push, some review history disappear. Sorry for that.
> But I did fixed issues that you posted, please have a look again.
Update the status before those patched been removed
 Bug 1207734 - Part 7.a. - (r? cleared) The main concern is to replace Specified(Option<Number>, Option<Number>, Option<Number>, Angle) by Specified(Option<(Number, Number, Number)>, Angle) in comment 60
 Bug 1207734 - Part 7.b/c. - (r? cleared)
 Bug 1207734 - Part 9.a. - (r? cleared) You have some concern at  Comment 113
 Bug 1207734 - Part 9.b. - r+ already
 bug 1207734 - Part 9.c./d. - new added

Comment 159

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review214530

Looks good, but could do another look once the comments are addressed.

::: servo/components/style/properties/gecko.mako.rs:546
(Diff revision 1)
>      pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
>          convert_nscolor_to_rgba(${get_gecko_property(gecko_ffi_name)})
>      }
>  </%def>
>  
> +<%def name="impl_rotate(ident, gecko_ffi_name)">

Given this is only used in one place, I'd avoid the mako function, and I'd just implement the functions without all the mako stuff.

You can do that adding `rotate`, etc to `skip_box_longhands` (terrible name, I know), and just defining `set_rotate`, `copy_rotate_from`, etc.

Also, there's no need for the `allow()`s, nor anything like that, that's needed for transform since we share that code for `-moz-window-transform`.

::: servo/components/style/values/animated/mod.rs:455
(Diff revision 1)
> +    U: ToAnimatedZero,
> +    V: ToAnimatedZero,
> +{
> +    #[inline]
> +    fn to_animated_zero(&self) -> Result<Self, ()> {
> +        Ok((self.0.to_animated_zero()?, self.1.to_animated_zero()?,

nit: Use block indentation:

```
Ok((
    self.0.to_animated_zero()?,
    self.1.to_animated_zero()?,
    self.2.to_animated_zero()?,
))
```

Same below.

::: servo/components/style/values/computed/mod.rs:310
(Diff revision 1)
>  
> +impl<A, B, C> ToComputedValue for (A, B, C)
> +    where A: ToComputedValue, B: ToComputedValue, C: ToComputedValue,
> +{
> +    type ComputedValue = (
> +        <A as ToComputedValue>::ComputedValue,

I think you don't need the `<A as ToComputedValue>::ComputedValue`, and only `A::ComputedValue` should be enough.

::: servo/components/style/values/computed/mod.rs:317
(Diff revision 1)
> +        <C as ToComputedValue>::ComputedValue,
> +    );
> +
> +    #[inline]
> +    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
> +        (self.0.to_computed_value(context), self.1.to_computed_value(context),

nit: indentation.

::: servo/components/style/values/generics/transform.rs:764
(Diff revision 1)
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> +/// A value of the `Rotate` property
> +///
> +/// <https://drafts.csswg.org/css-transforms-2/#individual-transforms>
> +pub enum Rotate<Number, Angle> {

nit: Maybe `Rotation` is a more descriptive type name?

::: servo/components/style/values/specified/transform.rs:531
(Diff revision 1)
> +            // <number>{3} <angle>
> +            let ry = Number::parse(context, input)?;
> +            let rz = Number::parse(context, input)?;
> +            let angle = specified::Angle::parse(context, input)?;
> +            return Ok(GenericRotate::Specified(Some((rx, ry, rz)), angle));
> +        } else {

nit: No `else` after return.

::: servo/components/style/values/specified/transform.rs:533
(Diff revision 1)
> +            let rz = Number::parse(context, input)?;
> +            let angle = specified::Angle::parse(context, input)?;
> +            return Ok(GenericRotate::Specified(Some((rx, ry, rz)), angle));
> +        } else {
> +            // <angle>
> +            let angle = specified::Angle::parse_with_unitless(context, input)?;

No unitless angle, per https://github.com/w3c/csswg-drafts/issues/2116.

Comment 160

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review214532

::: servo/components/style/properties/gecko.mako.rs:604
(Diff revision 1)
>              }
>          }
>      }
>  </%def>
>  
> +<%def name="impl_translate(ident, gecko_ffi_name)">

Same re. the mako stuff than for `rotate`.

::: servo/components/style/values/generics/transform.rs:776
(Diff revision 1)
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> +/// A value of the `Translate` property
> +///
> +/// <https://drafts.csswg.org/css-transforms-2/#individual-transforms>
> +pub enum Translate<LengthOrPercentage, Length> {

nit: Maybe `Translation`?

::: servo/components/style/values/specified/transform.rs:15
(Diff revision 1)
>  use style_traits::{ParseError, StyleParseErrorKind};
>  use values::computed::{Context, LengthOrPercentage as ComputedLengthOrPercentage};
>  use values::computed::{Percentage as ComputedPercentage, ToComputedValue};
>  use values::computed::transform::TimingFunction as ComputedTimingFunction;
>  use values::generics::transform::{Matrix3D, Transform as GenericTransform,
> -    Rotate as GenericRotate};
> +    Rotate as GenericRotate, Translate as GenericTranslate};

nit: Use multiple `use` lines instead of multiline imports.

::: servo/components/style/values/specified/transform.rs:551
(Diff revision 1)
> +    ) -> Result<Self, ParseError<'i>> {
> +        if input.try(|i| i.expect_ident_matching("none")).is_ok() {
> +            return Ok(GenericTranslate::None);
> +        }
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {

Maybe just `if let Ok(..) = input.try(..) {` instead of `if let Some(..) = input.try(..).ok() {`?

::: servo/components/style/values/specified/transform.rs:555
(Diff revision 1)
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {
> +            if let Some(tz) = input.try(|i| specified::Length::parse(context, i)).ok() {
> +                // <number>{3}
> +                return Ok(GenericTranslate::Specified(tx, Some(ty), Some(tz)));
> +            } else {

nit: no else after `return`.

Comment 161

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/208766/#review214536

Same as before, I'll take a look at it once the mako stuff is gone, but looks pretty good!

Comment 162

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938039 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

https://reviewboard.mozilla.org/r/208770/#review214538

r=me

Comment 163

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938040 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

https://reviewboard.mozilla.org/r/208772/#review214542

::: layout/reftests/w3c-css/submitted/transforms/reftest.list:11
(Diff revision 1)
>  == perspective-zero-2.html perspective-zero-2-ref.html
> +
> +default-preferences pref(layout.css.individual-transform.enabled,true)
> +# skip stylo-vs-gecko comparison since we support individual transform on new
> +# style system only.
> +skip-if(!stylo||styloVsGecko) == individual-transform-1.html individual-transform-1-ref.html

Maybe just `fails-if` instead of `skip-if`? That way we can notice if something goes pretty wrong :)

Comment 164

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938041 [details]
bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

https://reviewboard.mozilla.org/r/208774/#review214544

Looks fine, but seems unfortunate to have duplicated tests. Why not just landing the WPT ones? There are some known issues about our WPT stuff, like non-fatal assertions and such, but I think it's not a big deal in this case, since the new style system doesn't use those anyway.

Comment 165

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938042 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

https://reviewboard.mozilla.org/r/208776/#review214546

Shouldn't need to ask for review for this IMO :)

Comment 166

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Scale tests

These three animations should look the same

Comment 167

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Base value test

This animation should not jump

Comment 168

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review214672

Clearing review for now because we should get attachment 8938223 [details] and attachment 8938224 [details] to pass first.

Can you also please add wpt tests for these properties to:

  testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js
  testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js

that cover these cases?

(If you make the changes to a fork of web-platform-tests repository then you can use ./wpt serve to run them as a local web server where you can also run them against Chrome. Unfortunately Chrome doesn't appear to support `composite: 'accumulate'` yet so you won't be able to test that there.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2398
(Diff revision 1)
> +        Ok(Scale::Specified(animate_multiplicative_factor(from.0, to.0, procedure)?,
> +                            Some(animate_multiplicative_factor(from.1, to.1, procedure)?),
> +                            Some(animate_multiplicative_factor(from.2, to.2, procedure)?)))

I don't think this is right. I think this might be the right behavior when |procedure| is Procedure::Interpolate or Procedure::Accumulate, but not Procedure::Add.

See attachment 8938223 [details].

If you _add_ `scale: 2 2` with `scale: 2 2` the visual result, for consistency with 'transform', should be the same as `scale(2) scale(2)`. It's only when you _accumulate_ that it should be `scale: 3 3`. (And we should add a test for that too.)

e.g. the following should not jump

anim.animate({ scale: [ '1 1', '2 2' ] }, { duration: 1000, iterations: 2, iterationComposite: 'accumulate' })

Comment 169

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

This looks mostly fine, but clearing review now because if we are *not* supposed to resolve percentages then I guess this will change a bit.

::: layout/style/nsComputedDOMStyle.cpp:1746
(Diff revision 2)
> +static already_AddRefed<CSSValue>
> +ReadIndividualTransformValue(nsCSSValueSharedList* aList,
> +                             const std::function<void(const nsCSSValue::Array*,
> +                                                      nsString&)>& aCallback)

(This indentation seems a little bit odd, but if that's what ./mach clang-format does it must be ok.)

::: layout/style/nsComputedDOMStyle.cpp:1773
(Diff revision 2)
> +already_AddRefed<CSSValue>
> +nsComputedDOMStyle::DoGetTranslate()
> +{
> +  typedef nsStyleTransformMatrix::TransformReferenceBox TransformReferenceBox;
> +
> +  RefPtr<nsComputedDOMStyle> self(this);

(It's a pity we need this, but our static analysis insists on it.)

::: layout/style/nsComputedDOMStyle.cpp:1785
(Diff revision 2)
> +
> +      switch (nsStyleTransformMatrix::TransformFunctionOf(aData)) {
> +        /* translate : <length-percentage> */
> +        case eCSSKeyword_translatex: {
> +          NS_PRECONDITION(aData->Count() == 2, "Invalid array!");
> +          float tx = ProcessTranslatePart(aData->Item(1),

Out of curiouslty, how does this work? This function is defined in the nsStyleTransformMatrix namespace. How come we don't need to add the namespace prefix here?

This is mostly just my curiosity but I'm also wondering if this only works because of a stray use/typedef in the unified build, although I can't find one.

::: layout/style/nsComputedDOMStyle.cpp:1792
(Diff revision 2)
> +          // According to the spec, unspecified translations default to 0px.
> +          aResult.AppendLiteral("px 0px 0px");

I think the "shortest form"[1] means we should return just the one value here.

Also, are we supposed to resolve percentages to pixels here? I thought normally we didn't? (Except for the exceptional values like width, height etc. whose used values we return.) e.g. see the transform-origin property[2]

But, then again, we are proposing to make the serialization of 'transform' resolve to a single matrix[3] so it sounds like we resolve percentages there.

Do you happen to know if this is defined/discussed anywhere? If not then as the spec stands, I think we are supposed to preserve percentages--but we should clarify that.

(For what it's worth, if we *don't* need to resolve percentages, then I suspect we can drop the 'self' capture from the lambda and no longer need to use std::function either since a lambda with no captures has a conversion operator to a concrete type.)

[1] https://github.com/w3c/csswg-drafts/issues/1564
[2] https://drafts.csswg.org/css-transforms/#transform-origin-property
[3] https://drafts.csswg.org/css-transforms-2/#serialization-of-the-computed-value

::: layout/style/nsComputedDOMStyle.cpp:1806
(Diff revision 2)
> +          aResult.AppendLiteral("px ");
> +          float ty = ProcessTranslatePart(aData->Item(2),
> +                                          contextIfGecko,
> +                                          self->mStyleContext->PresContext(),
> +                                          dummy,
> +                                          &refBox,
> +                                          &TransformReferenceBox::Height);
> +          aResult.AppendFloat(ty);
> +          // According to the spec, unspecified translations default to 0px.
> +          aResult.AppendLiteral("px 0px");
> +          break;

The "shortest form" here would also mean that if ty == 0, we don't serialize it. And we also don't add the final "0px".

::: layout/style/nsComputedDOMStyle.cpp:1829
(Diff revision 2)
> +          float ty = ProcessTranslatePart(aData->Item(2),
> +                                          contextIfGecko,
> +                                          self->mStyleContext->PresContext(),
> +                                          dummy,
> +                                          &refBox,
> +                                          &TransformReferenceBox::Height);
> +          aResult.AppendFloat(ty);
> +          aResult.AppendLiteral("px ");
> +          float tz = ProcessTranslatePart(aData->Item(3),
> +                                          contextIfGecko,
> +                                          self->mStyleContext->PresContext(),
> +                                          dummy,
> +                                          &refBox,
> +                                          nullptr);
> +          aResult.AppendFloat(tz);
> +          aResult.AppendLiteral("px");

Likewise here, we need to omit from the end any arguments that are 0.

::: layout/style/nsComputedDOMStyle.cpp:1864
(Diff revision 2)
> +        /* scale : <number> */
> +        case eCSSKeyword_scalex:
> +        NS_PRECONDITION(aData->Count() == 2, "Invalid array!");
> +          aResult.AppendFloat(aData->Item(1).GetFloatValue());
> +          // According to the spec, unspecified scales default to 1.
> +          aResult.AppendLiteral(" 1.0 1.0");

Once again, short form here means we should not append the 1.0 1.0.

(And if other browsers are appending that, we should make sure there are web-platform-tests covering it so they have a chance to update their implementations.)

::: layout/style/nsComputedDOMStyle.cpp:1873
(Diff revision 2)
> +        NS_PRECONDITION(aData->Count() == 3, "Invalid array!");
> +          aResult.AppendFloat(aData->Item(1).GetFloatValue());
> +          aResult.AppendLiteral(" ");
> +          aResult.AppendFloat(aData->Item(2).GetFloatValue());
> +          // According to the spec, unspecified scales default to 1.
> +          aResult.AppendLiteral(" 1.0");

Here too, plus we should omit the second argument if it is 1.0.

::: layout/style/nsComputedDOMStyle.cpp:1879
(Diff revision 2)
> +          aResult.AppendLiteral(" ");
> +          aResult.AppendFloat(aData->Item(2).GetFloatValue());
> +          aResult.AppendLiteral(" ");
> +          aResult.AppendFloat(aData->Item(3).GetFloatValue());

Likewise, we should omit the 3rd argument if it is 1.0, and omit both 2nd and 3rd arguments if they are 1.0.

::: layout/style/nsComputedDOMStyle.cpp:1900
(Diff revision 2)
> +          // According to the spec, if the axis is unspecified, it defaults to
> +          // '0 0 1'
> +          aResult.AppendLiteral("0 0 1 ");

Again, shortest form says we don't need this.

Comment 170

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8937971 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

https://reviewboard.mozilla.org/r/208698/#review214678

Comment 171

[u459114]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review214530

> nit: Maybe `Rotation` is a more descriptive type name?

Although it more descriptive, I think it's clearer to just match the keyword in spec.

Comment 172

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> Out of curiouslty, how does this work? This function is defined in the nsStyleTransformMatrix namespace. How come we don't need to add the namespace prefix here?
> 
> This is mostly just my curiosity but I'm also wondering if this only works because of a stray use/typedef in the unified build, although I can't find one.

that's because there is a typedef in the beginning of this function:
  typedef nsStyleTransformMatrix::TransformReferenceBox TransformReferenceBox;

Comment 173

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> I think the "shortest form"[1] means we should return just the one value here.
> 
> Also, are we supposed to resolve percentages to pixels here? I thought normally we didn't? (Except for the exceptional values like width, height etc. whose used values we return.) e.g. see the transform-origin property[2]
> 
> But, then again, we are proposing to make the serialization of 'transform' resolve to a single matrix[3] so it sounds like we resolve percentages there.
> 
> Do you happen to know if this is defined/discussed anywhere? If not then as the spec stands, I think we are supposed to preserve percentages--but we should clarify that.
> 
> (For what it's worth, if we *don't* need to resolve percentages, then I suspect we can drop the 'self' capture from the lambda and no longer need to use std::function either since a lambda with no captures has a conversion operator to a concrete type.)
> 
> [1] https://github.com/w3c/csswg-drafts/issues/1564
> [2] https://drafts.csswg.org/css-transforms/#transform-origin-property
> [3] https://drafts.csswg.org/css-transforms-2/#serialization-of-the-computed-value

No idea, issue filed:
https://github.com/w3c/csswg-drafts/issues/2124

Since both chromium and edge resolve percentages to pxs, I think we may keep current behavior unless we get a conclusion from w3c that we shouldn't. how do you think?

Comment 174

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> Once again, short form here means we should not append the 1.0 1.0.
> 
> (And if other browsers are appending that, we should make sure there are web-platform-tests covering it so they have a chance to update their implementations.)

chromiun returns shortest form whereas edge returns longest form. will create a test case for this

Comment 175

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> chromiun returns shortest form whereas edge returns longest form. will create a test case for this

Create test cases in Part 9.f

Comment 176

[u459114]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review214672

Done in Part 9.f

Comment 177

[u459114]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review214672

attachment 8938223 [details] was fixed in this patch. attachment 8938224 [details] was fixed(temp workaround) in Part 4.c

Comment 178

[u459114]

Attachment: Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

Since we do not support async-transform for individual-transform yet.

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

Comment 179

[u459114]

Attachment: Bug 1207734 - Part 9.g. (testing) Update animation wpt for individual transform.

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

Comment 180

[u459114]

Comment on attachment 8935258 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 181

[u459114]

Comment on attachment 8935259 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 182

[u459114]

Comment on attachment 8935260 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 183

[u459114]

Comment on attachment 8935267 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 184

[u459114]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 185

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 186

[u459114]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 187

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 188

[u459114]

Comment on attachment 8937971 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 189

[u459114]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 190

[u459114]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 191

[u459114]

Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 192

[u459114]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 193

[u459114]

Comment on attachment 8938039 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 194

[u459114]

Comment on attachment 8938040 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 195

[u459114]

Comment on attachment 8938041 [details]
bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

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

Comment 196

[u459114]

Comment on attachment 8938042 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

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

Comment 197

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217118

This looks _way_ more consistent :).

r=me

::: servo/components/style/properties/gecko.mako.rs:3122
(Diff revision 2)
> +        }
> +
> +        let list = unsafe { (*self.gecko.${gecko_ffi_name}.to_safe().get()).mHead.as_ref() };
> +
> +        let mut transform = clone_transform_from_list(list);
> +        debug_assert!(transform.0.len() == 1);

nit: `debug_assert_eq!`.

::: servo/components/style/properties/longhand/box.mako.rs:417
(Diff revision 2)
>                            gecko_ffi_name="mSpecifiedTransform",
>                            flags="CREATES_STACKING_CONTEXT FIXPOS_CB",
>                            spec="https://drafts.csswg.org/css-transforms/#propdef-transform")}
>  
> +${helpers.predefined_type("rotate", "Rotate",
> +                          "generics::transform::Rotate(generics::transform::IndividualTransform::None)",

Maybe create a `fn none() -> Self` on rotate and call it, much like `Transform`? I think it reads a bit nicer.

::: servo/components/style/values/generics/transform.rs:762
(Diff revision 2)
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> +/// A base component for each individual transform.
> +pub enum IndividualTransform<T> {

s/T/TransformOperation/?

Also maybe a comment that this is needed to get serialization right for the `None` variant instead of just using `Option` would be nice.

::: servo/components/style/values/specified/transform.rs:537
(Diff revision 2)
> +            return Ok(GenericRotate(IndividualTransform::Specified(GenericTransformOperation::Rotate3D(rx, ry, rz, angle))));
> +        }
> +
> +        // 'rotate: <angle>'
> +        let angle = specified::Angle::parse(context, input)?;
> +        Ok(GenericRotate(IndividualTransform::Specified(GenericTransformOperation::Rotate(angle))))

I think you can avoid using `GenericRotate` and just use `Rotate`? `GenericTransformOperation` is needed due to a long-standing rust issue with enums and associated types.

Comment 198

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review217126

r=me, with the same comments as the previous patch, and this extra comment addressed in both.

::: servo/components/style/values/specified/transform.rs:555
(Diff revision 2)
> +        if input.try(|i| i.expect_ident_matching("none")).is_ok() {
> +            return Ok(GenericTranslate(IndividualTransform::None));
> +        }
> +
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {

Why not `if let Ok(..)`, and remove the `.ok()`? Here and below, and also on the previous patch.

Comment 199

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review217132

Sorry, one last comment I overlooked.

::: servo/components/style/values/specified/transform.rs:557
(Diff revision 2)
> +        }
> +
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {
> +            if let Some(tz) = input.try(|i| specified::Length::parse(context, i)).ok() {
> +                // 'translate : <number>{3}'

Hmm, this comments need fixing, this is `<length-percentage> <length-percentage> <length>`, no `<number> {..}`, ditto below.

Comment 200

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217136

::: servo/components/style/values/computed/transform.rs:299
(Diff revision 2)
>              .collect::<Result<Vec<_>, _>>()?))
>      }
>  }
> +
> +/// A computed CSS `rotate`
> +pub type Rotate = GenericRotate<TransformOperation>;

Hmm, so another comment I initially overlooked and realised when looking at the third patch (whoops).

TransformOperation::to_css serializes the functions, so how is this right?

Wouldn't this patch cause this behavior?

```
element.style.rotate = "10deg";
element.style.rotate; // rotate(10deg)
```

I think this deserves an explanation before I can grant r+ on this patch.

The rest of the patches are equally affected, but maybe I'm misunderstanding something, this should be tested.

Whatever solution we have for rotate we need to adopt for the rest.

Comment 201

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/208766/#review217138

Same comments as the rest, and need to clarify that serialization issue.

::: servo/components/style/values/specified/transform.rs:586
(Diff revision 2)
> +        }
> +
> +        let sx = Number::parse(context, input)?;
> +        if let Some(sy) = input.try(|i| Number::parse(context, i)).ok() {
> +            if let Some(sz) = input.try(|i| Number::parse(context, i)).ok() {
> +                // 'scale : <length-percentage><length-percentage><length>'

Comment needs fixing, these are numbers.

Comment 202

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review217140

The serialization issue needs to be addressed, so clearing review until that is clarified, because I'm either misunderstanding something, or the type representation may need to change to be more specific.

Other than that it looks fine to me.

Comment 203

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review217368

::: layout/style/nsStyleStruct.h:2600
(Diff revision 4)
>    RefPtr<nsCSSValueSharedList> mSpecifiedScale; // [reset]
> +
> +  // Used to store the final combination of mSpecifiedTranslate,
> +  // mSpecifiedRotate, mSpecifiedScale and mSpecifiedTransform.
> +  // Use GetCombinedTransform() to get the final transform, instead of
> +  // accessing mCombinedTransform directly, since this value is initiated

s/initiated/initialized/

or simply "calculated"

::: layout/style/nsStyleStruct.cpp:4034
(Diff revision 4)
> +  const RefPtr<nsCSSValueSharedList>* shareLists[] = { &mSpecifiedTranslate,
> +                                                       &mSpecifiedRotate,
> +                                                       &mSpecifiedScale,
> +                                                       &mSpecifiedTransform };
> +
> +  // Generally, we will have 3 transform functions(one rotate, one trasnlate
> +  // and one scale) plus one rotate, once translate and one scale property
> +  // at most. That's where 6 comes from.
> +  AutoTArray<nsCSSValueList*, 6> valueLists;
> +  for (auto list: shareLists) {
> +    if (*list) {
> +      valueLists.AppendElement((*list)->mHead->Clone());

I don't quite follow the comment. shareLists only has 4 elements? We don't seem to traverse mSpecifiedTransform, so why are we allocating 6 slots of inline storage? Is it because of how mCombinedTransform will be used elsewhere?

If this is correct, I think this could be more clear.

Also, any reason to prefer pointers to RefPtrs? Wouldn't

  const nsCSSValueSharedList* sharedLists[] = { mSpecifiedTransform.get(), ... };

be simpler?

::: layout/style/nsStyleStruct.cpp:4039
(Diff revision 4)
> +  const RefPtr<nsCSSValueSharedList>* shareLists[] = { &mSpecifiedTranslate,
> +                                                       &mSpecifiedRotate,
> +                                                       &mSpecifiedScale,
> +                                                       &mSpecifiedTransform };
> +
> +  // Generally, we will have 3 transform functions(one rotate, one trasnlate

s/trasnlate/translate/

::: layout/style/nsStyleStruct.cpp:4040
(Diff revision 4)
> +                                                       &mSpecifiedRotate,
> +                                                       &mSpecifiedScale,
> +                                                       &mSpecifiedTransform };
> +
> +  // Generally, we will have 3 transform functions(one rotate, one trasnlate
> +  // and one scale) plus one rotate, once translate and one scale property

s/once/one/

::: layout/style/nsStyleStruct.cpp:4048
(Diff revision 4)
> +  MOZ_ASSERT(valueLists.Length());
> +
> +  for (uint32_t i = 0; i < valueLists.Length() - 1; i++) {

It might be worth adding a comment here:

  // Check we have at least one list or else valueLists.Length() - 1 below will
  // underflow.

Comment 204

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review217380

I'm not sure if it's related to this patch, but if I open up attachment 8938224 [details] in DevTools and inspect the box I see:

  translate: translateX(100px);

which seems wrong.

::: layout/style/nsComputedDOMStyle.h:519
(Diff revision 3)
> +  already_AddRefed<CSSValue> DoGetTranslate();
> +  already_AddRefed<CSSValue> DoGetRotate();
> +  already_AddRefed<CSSValue> DoGetScale();

(Should these go above TransformBox and after Transform? It seems odd to put them between TransformBox and TransformOrigin.)

::: layout/style/nsComputedDOMStyle.cpp:1753
(Diff revision 3)
> +
> +    /* Set it to "none." */

(Not sure the comment and blank line are necessary.)

::: layout/style/nsComputedDOMStyle.cpp:1781
(Diff revision 3)
> +      GeckoStyleContext* contextIfGecko =
> +        self->mStyleContext ? self->mStyleContext->GetAsGecko() : nullptr;
> +      TransformReferenceBox refBox(self->mInnerFrame, nsSize(0, 0));
> +      RuleNodeCacheConditions dummy;
> +
> +      switch (nsStyleTransformMatrix::TransformFunctionOf(aData)) {

Can you add a comment here saying that even though the spec doesn't say to resolve percentage values, Blink and Edge do and so until that is clarified we do as well:

  https://github.com/w3c/csswg-drafts/issues/2124

(Personally I think we should *not* resolve percentages here. It's simpler not to and more common and by resolving percentages here we make it harder to spec editors to argue for preserving percentages since, "all three implementations resolve them". But I'll leave it up to you.)

Comment 205

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review217390

::: layout/style/nsStyleStruct.h:2602
(Diff revision 4)
> +  // Used to store the final combination of mSpecifiedTranslate,
> +  // mSpecifiedRotate, mSpecifiedScale and mSpecifiedTransform.
> +  // Use GetCombinedTransform() to get the final transform, instead of
> +  // accessing mCombinedTransform directly, since this value is initiated
> +  // lazily in GetCombinedTransform().
> +  mutable RefPtr<nsCSSValueSharedList> mCombinedTransform;

I'm somewhat concerned about this `mutable` member, since we do poke at style structs off-main-thread.

At the very least, this needs a `MOZ_ASSERT(NS_IsMainThread())` in everything that reads or writes from it.

Even with that, I'd probably try to avoid it.

Comment 206

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

https://reviewboard.mozilla.org/r/211238/#review217386

::: commit-message-d0eb5:1
(Diff revision 1)
> +Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.
> +
> +Since we do not support async-transform for individual-transform yet.

Is this patch supposed to be blocking transform animations whenever we have an individual transform property specified, or when we are trying to animate one of them, or both?

It would be nice to have a few tests to check this, if possible.

If we only really care about individual transforms being animated then we should probably put the check in KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations instead.

::: layout/generic/nsFrame.cpp:2827
(Diff revision 1)
>        }
>      }
>      inTransform = true;
>    }
>  
> +  // XXX cku temporary disable async-animation when this frame has any

(nit: s/temporary/temporarily/)

::: layout/style/nsStyleStruct.h:2810
(Diff revision 1)
> +    return mSpecifiedRotate != nullptr ||
> +           mSpecifiedTranslate != nullptr ||
> +           mSpecifiedScale != nullptr;

I believe this should be:

  return !mSpecifiedRotate || !mSpecifiedTranslate || !mSpecifiedScale;

According to our coding style:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

I know HasTransformStyle() does this but I think it's wrong too.

Comment 207

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

https://reviewboard.mozilla.org/r/211238/#review217396

::: layout/style/nsStyleStruct.h:2810
(Diff revision 1)
> +    return mSpecifiedRotate != nullptr ||
> +           mSpecifiedTranslate != nullptr ||
> +           mSpecifiedScale != nullptr;

Err, obviously that should be:

  return mSpecifiedRotate || mSpecifiedTranslate || mSpecifiedScale;

Comment 208

[u459114]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217136

> Hmm, so another comment I initially overlooked and realised when looking at the third patch (whoops).
> 
> TransformOperation::to_css serializes the functions, so how is this right?
> 
> Wouldn't this patch cause this behavior?
> 
> ```
> element.style.rotate = "10deg";
> element.style.rotate; // rotate(10deg)
> ```
> 
> I think this deserves an explanation before I can grant r+ on this patch.
> 
> The rest of the patches are equally affected, but maybe I'm misunderstanding something, this should be tested.
> 
> Whatever solution we have for rotate we need to adopt for the rest.

hmm... property serialization is not done, will do it. And will add test cases in test_sepcified_value_serialization.html

Comment 209

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8940970 [details]
Bug 1207734 - Part 9.g. (testing) Update animation wpt for individual transform.

https://reviewboard.mozilla.org/r/211240/#review217402

This is pretty good but needs a few additional tests as indicated below.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1588
(Diff revision 1)
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      const animation = target.animate(
> +        {
> +          [idlName]: ['45deg', '135deg'],
> +        },
> +        1000
> +      );
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '90deg' }]);
> +    }, `${property} without rotation axes`);
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      const animation =
> +        target.animate({ [idlName]: [ '0 1 0 0deg',
> +                                      '0 1 0 90deg'] },
> +                       1000);
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500, expected: '0 1 0 45deg' }]);
> +    }, `${property}  with rotation axes`);

This looks good but could we add one more test for, say 0deg -> 720deg, and then check that at 250 we are at 180deg?

(The reason is just to catch implementations that internally represent rotations as matrices.)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1606
(Diff revision 1)
> +        target.animate({ [idlName]: [ '0 1 0 0deg',
> +                                      '0 1 0 90deg'] },

Don't we need to test the interpolation/addition/accumulation of the axis too?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1618
(Diff revision 1)
> +  },
> +  testAddition: function(property, setup) {
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style[idlName] = '45deg';

Would it be worth setting target.style.transform = 'rotate(23deg)' just to check that the underlying transform is not reflected? (assuming that property !== 'transform').

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1631
(Diff revision 1)
> +    }, `${property} without rotation axes`);
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style.transform = 'translateX(100px)';

Oh wait, I see you've done that!

Would it be more interesting to add a rotate() value, however?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1676
(Diff revision 1)
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      const animation = target.animate(
> +        {
> +          [idlName]: ['200px', '400px'],
> +        },
> +        1000
> +      );
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '300px' }]);
> +    }, `${property} with two unspecified values`);

I think we need a test for interpolating between lengths and percentages.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1834
(Diff revision 1)
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style[idlName] = '2 2 2';
> +      const animation = target.animate({ [idlName]: ['-3 -3 -3', '5 5 5'] },

We should test with different values (e.g. '1 2 3' -> '4 5 6') to make sure we are interpolating the correct channels.

Comment 210

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review217394

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2323
(Diff revision 2)
> +        let (fx, fy, fz, fa) =
> +            transform::get_normalized_vector_and_angle(from.0, from.1, from.2,
> +                                                       from.3);
> +        let (tx, ty, tz, ta) =
> +            transform::get_normalized_vector_and_angle(to.0, to.1, to.2, to.3);
> +        if (fx, fy, fz) == (tx, ty, tz) {
> +            return Ok(Rotate(IndividualTransform::Specified(TransformOperation::Rotate3D(fx, fy, fz, fa.animate(&ta, procedure)?))));
> +        }

Is this the formatting rustfmt gives you?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2339
(Diff revision 2)
> +        
> +        /*let (rx, ry, rz, angle) =
> +            ComputedRotate::matrix3d_to_rotation(&matrix_f.animate(&matrix_t, procedure)?);
> +        Ok(Rotate(IndividualTransform::Specified(TransformOperation::Rotate3D(rx, ry, rz, angle))))*/

Pretty sure this is not supposed to be here?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2432
(Diff revision 2)
> +            // scale(x1,y1,z1)*scale(x2,y2,z2) = scale(x1*x2, y1*y2, z1*z2)
> +            return Ok(Scale(IndividualTransform::Specified(TransformOperation::Scale3D(from.0 * to.0, from.1 * to.1, from.2 * to.2))));

I'm curious where this comes from?

I mean, it makes sense: presumably if you have an additive animation with a value of 'scale: 2' and the underlying value is 'scale: 10', you expect the result to be 'scale: 20' (while accumulate should give you 'scale: 12').

However, is this specified anywhere? Does blink do this? (I guess it's not possible to test Edge since it doesn't have Web Animations yet.)

I think this is ok but we should specify it somewhere and make sure that there is a test that fails without this behavior.

Comment 211

[Hiroyuki Ikezoe (:hiro)]

I believe animations for these individual transform properties also create a stacking context, so I think we should modify nsIFrame::HasAnimationOfTransform() as well.

Comment 212

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review217380

Emilio also found this issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1207734#c200

It will be fixed in Part 7.a/b/c

Comment 213

[u459114]

Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

https://reviewboard.mozilla.org/r/211238/#review217570

::: layout/generic/nsFrame.cpp:2831
(Diff revision 1)
>  
> +  // XXX cku temporary disable async-animation when this frame has any
> +  // individual transforms before bug 1425837 been fixed.
> +  if (allowAsyncAnimation && disp->HasIndividualTransform()) {
> +    allowAsyncAnimation = false;
> +  }

Fix birtles's comment:
If we only really care about individual transforms being animated then we should probably put the check in KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations instead.

Comment 214

[u459114]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217118

> I think you can avoid using `GenericRotate` and just use `Rotate`? `GenericTransformOperation` is needed due to a long-standing rust issue with enums and associated types.

Will get a compile error by doing this change:
error[E0423]: expected function, found type alias `Rotate`

Comment 215

[u459114]

Attachment: Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

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

Comment 216

[u459114]

Attachment: Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

So that we can reuse the code in ComputedRotate.

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

Comment 217

[u459114]

Attachment: Bug 1207734 - Part 9.e. (testing) Add specified value serialization test cases for individual transform.

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

Comment 218

[u459114]

Attachment: Bug 1207734 - Part 9.f. (testing) Implement testAnimationSampleRotate3d.

testAnimationSamples is not enough for comparing the computed value of rotate
property so we need this new function.

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

Comment 219

[u459114]

Comment on attachment 8935258 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 220

[u459114]

Comment on attachment 8935259 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 221

[u459114]

Comment on attachment 8935260 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 222

[u459114]

Comment on attachment 8935267 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 223

[u459114]

Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 224

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 225

[u459114]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 226

[u459114]

Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

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

Comment 227

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 228

[u459114]

Comment on attachment 8937971 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 229

[u459114]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 230

[u459114]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 231

[u459114]

Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 232

[u459114]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 233

[u459114]

Comment on attachment 8938039 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 234

[u459114]

Comment on attachment 8938040 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 235

[u459114]

Comment on attachment 8938041 [details]
bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

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

Comment 236

[u459114]

Comment on attachment 8938042 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

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

Comment 237

[u459114]

Comment on attachment 8940970 [details]
Bug 1207734 - Part 9.g. (testing) Update animation wpt for individual transform.

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

Comment 238

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8942583 [details]
Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

https://reviewboard.mozilla.org/r/212840/#review218462

Comment 239

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8942586 [details]
Bug 1207734 - Part 9.f. (testing) Implement testAnimationSampleRotate3d.

https://reviewboard.mozilla.org/r/212846/#review218466

Comment 240

[Boris Chiou [:boris]]

Comment on attachment 8942584 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

https://reviewboard.mozilla.org/r/212842/#review218464

r=me with the following comment addressed.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2072
(Diff revision 1)
> +impl Animate for MatrixDecomposed3D {
> +    /// <https://drafts.csswg.org/css-transforms/#interpolation-of-decomposed-3d-matrix-values>
> +    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
> +        use std::f64;
> +
> +        let (this_weight, other_weight) = procedure.weights();

`this_weight` and `other_weight` are only used in `debug_assert!(...)`, so let's move this into it, too.

e.g.
```
debug_assert!(
    (procedure.weights().this_weight +
        procedure.weights().other_weight - 1.0f64).abs() <= f64::EPSILON ||
    procedure.weights().other_weight == 1.0f64 ||
    procedure.weights().other_weight == 0.0f64,
    "..."
);
```

or you could add some inline functions into `Precedure` [1], so we don't need to call `prodecure.weigths()` so many times.

or you might just add a function: `is_valid_for_animation()` in `Procedure`.

[1] https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/servo/components/style/values/animated/mod.rs#101

Comment 241

[Boris Chiou [:boris]]

Comment on attachment 8942584 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

https://reviewboard.mozilla.org/r/212842/#review218468

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2072
(Diff revision 1)
> +impl Animate for MatrixDecomposed3D {
> +    /// <https://drafts.csswg.org/css-transforms/#interpolation-of-decomposed-3d-matrix-values>
> +    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
> +        use std::f64;
> +
> +        let (this_weight, other_weight) = procedure.weights();

Or probably, move the debug assert into `Slerp()` and change the description for Slerp. Either way is ok to me.

Comment 242

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review218502

::: servo/components/style/values/generics/transform.rs:760
(Diff revision 3)
>          (vector.x, vector.y, vector.z, angle)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]

It's confusing that this `derive`s ToCss. It should not be needed, right? (It'd be the bogus serialization)

::: servo/components/style/values/specified/transform.rs:516
(Diff revision 3)
>          }
>      }
>  }
> +
> +/// A specified CSS `rotate`
> +pub type Rotate = GenericRotate<TransformOperation>;

I still think it would be cleaner to just have something like:

```rust
#[derive(ToCss)]
pub enum GenericRotate<Angle, Number> {
    None,
    Rotate(Angle),
    Rotate3D(Number, Number, Number, Angle),
}

// in computed/
impl Rotate {
    pub fn to_transform_operation(&self) -> Option<TransformOperation> {
        Some(match *self {
            Rotate::None => return None,
            Rotate::Rotate(angle) => TransformOperation::Rotate(angle),
            Rotate::Rotate3d(rx, ry, rz, angle) => TransformOperation::Roate3d(rx, ry, rz, angle),
        })
    }
}
```

That way you don't need to manually write the serialization code, and remove the fishyness of deriving `ToCss` above. You should be able to still write `impl_transform_operation` without duplicating code using `to_transform_operation`.

WDYT? Has that been considered? I'm ok r+'ing the patch in its current state, I just want to know why not doing it that way, since I think it's a bit nicer.

::: servo/components/style/values/specified/transform.rs:544
(Diff revision 3)
> +    }
> +}
> +
> +impl ToCss for Rotate {
> +    fn to_css<W>(&self, dest: &mut W)  -> fmt::Result where W: fmt::Write {
> +        match &self.0 {

nit: Just `match self.0`, then remove the extra ampersands.

::: servo/components/style/values/specified/transform.rs:561
(Diff revision 3)
> +                    },
> +                    &GenericTransformOperation::Rotate(angle) => {
> +                        angle.to_css(dest)
> +                    },
> +                    ref x => {
> +                        panic!("Found unexpected value for rotate property: {:?}", x)

nit: just `unreachable!()`.

::: servo/components/style/values/specified/transform.rs:561
(Diff revision 3)
> +                    },
> +                    &GenericTransformOperation::Rotate(angle) => {
> +                        angle.to_css(dest)
> +                    },
> +                    ref x => {
> +                        panic!("Found unexpected value for rotate property: {:?}", x)

nit: just unreachable!(), that way you don't pull the code to debug-formatting `TransformOperation` in release builds.

Comment 243

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942585 [details]
Bug 1207734 - Part 9.e. (testing) Add specified value serialization test cases for individual transform.

https://reviewboard.mozilla.org/r/212844/#review218508

It'd be extra-nice to have these in WPT so other vendors don't need to rewrite them. But this looks good, thanks for adding them! :)

Comment 244

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review218510

Comment 245

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review218520

r=me with the issue filed on the followup (adding `from_transform_operation` / `to_transform_operation`, and deriving ToCss), and the nits fixed.

::: servo/components/style/values/specified/transform.rs:516
(Diff revision 3)
>          }
>      }
>  }
> +
> +/// A specified CSS `rotate`
> +pub type Rotate = GenericRotate<TransformOperation>;

I guess you'd also need a `from_transform_operation(op: &TransformOperation) -> Self`.

::: servo/components/style/values/specified/transform.rs:542
(Diff revision 3)
> +        let angle = specified::Angle::parse(context, input)?;
> +        Ok(GenericRotate(IndividualTransform::Specified(GenericTransformOperation::Rotate(angle))))
> +    }
> +}
> +
> +impl ToCss for Rotate {

Could you file a Servo issue or a bug on doing the changes outlined above, and reference it from here?

Something like:

```rust
/// TODO(#issue-number): This can be derived if we move some stuff around.
```

Comment 246

[Emilio Cobos Álvarez (:emilio)]

Welp, sorry for reviewing ^, I thought the review request flag was set for all the patches in the bug. Haven't reviewed the rest yet, just feel free to send them my way when you think they're ready.

Comment 247

[u459114]

Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207326/diff/5-6/

Comment 248

[u459114]

Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207330/diff/5-6/

Comment 249

[u459114]

Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

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

Comment 250

[u459114]

Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 251

[u459114]

Comment on attachment 8937971 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 252

[u459114]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 253

[u459114]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 254

[u459114]

Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 255

[u459114]

Comment on attachment 8942584 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

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

Comment 256

[u459114]

Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 257

[u459114]

Comment on attachment 8938039 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 258

[u459114]

Comment on attachment 8938040 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 259

[u459114]

Comment on attachment 8938041 [details]
bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

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

Comment 260

[u459114]

Comment on attachment 8938042 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

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

Comment 261

[u459114]

Comment on attachment 8942585 [details]
Bug 1207734 - Part 9.e. (testing) Add specified value serialization test cases for individual transform.

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

Comment 262

[u459114]

Comment on attachment 8942586 [details]
Bug 1207734 - Part 9.f. (testing) Implement testAnimationSampleRotate3d.

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

Comment 263

[u459114]

Comment on attachment 8940970 [details]
Bug 1207734 - Part 9.g. (testing) Update animation wpt for individual transform.

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

Comment 264

[u459114]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #246)
> Welp, sorry for reviewing ^, I thought the review request flag was set for
> all the patches in the bug. Haven't reviewed the rest yet, just feel free to
> send them my way when you think they're ready.

Had implemented both to_transform_operation & from_transform_operation, please have a check when you have time.

Comment 265

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review218636

Looks great, thank you!

::: servo/components/style/values/computed/transform.rs:317
(Diff revision 4)
> +    pub fn from_transform_operation(operation: &TransformOperation) -> Rotate {
> +        match *operation {
> +            GenericTransformOperation::Rotate(angle) => GenericRotate::Rotate(angle),
> +            GenericTransformOperation::Rotate3D(rx, ry, rz, angle) =>
> +                GenericRotate::Rotate3D(rx, ry, rz, angle),
> +            ref x => unreachable!("Found unexpected value for rotate property: {:?}", x),

nit: Please don't include the debug code in release builds since it bloats the binary, see https://github.com/servo/servo/pull/19756.

This applies to the rest of the patches.

Perhaps we should make some macro to print arguments in debug mode but not on release...

Comment 266

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review218638

r=me, with the same gotcha as the previous code.

Again, looks great, thanks for fixing up the nits instead of filing followups! :)

Comment 267

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/208766/#review218640

r=me, with the `{:?}` removed as the previous patches, and the comment fixed. Thanks!

::: servo/components/style/values/specified/transform.rs:584
(Diff revision 4)
> +        }
> +
> +        let sx = Number::parse(context, input)?;
> +        if let Some(sy) = input.try(|i| Number::parse(context, i)).ok() {
> +            if let Some(sz) = input.try(|i| Number::parse(context, i)).ok() {
> +                // 'scale: <length-percentage> <length-percentage> <length>'

Fix the comments, `s/<length-percentage>/<number>`, and ditto for `<length>`.

Comment 268

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8940970 [details]
Bug 1207734 - Part 9.g. (testing) Update animation wpt for individual transform.

https://reviewboard.mozilla.org/r/211240/#review218686

r=me with the extra length/percentage tests added and test names fixed.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1415
(Diff revision 3)
> +    setup: t => {
> +      const element = createElement(t);
> +      element.style.width = '100px';
> +      element.style.height = '100px';
> +      return element;
> +    }

This probably deserves a comment, e.g. "We need to set the width/height for resolving percentages against."

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1612
(Diff revision 3)
> +                                      '0 1 0 90deg'] },
> +                       1000);
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500, expected: '0 1 0 45deg' }]);
> +    }, `${property}  with rotation axes`);

(Super minor nit: double space here between ${property} and with)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1625
(Diff revision 3)
> +                                      '0 1 0 720deg'] },
> +                       1000);
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 250, expected: '0 1 0 180deg' }]);
> +    }, `${property}  with rotation axes`);

This needs a unique test name here--we've used `${property}  with rotation axes` already.

Having a unique test name allows annotating individual failing tests, makes debugging failures on CI easier, and is a requirement for wpt tests. I'm pretty sure './wpt lint' will complain about this.

I guess in this case it should be `${property} with rotation axes and range over 360 degrees` or something.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1637
(Diff revision 3)
> +                                      '0 1 1 90deg'] },
> +                       1000);
> +
> +      testAnimationSampleRotate3d(animation, idlName,
> +        [{ time: 500, expected: '0 0.707107 0.707107 45deg' }]);
> +    }, `${property}  with rotation axes`);

This too needs a unique test name.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1651
(Diff revision 3)
> +                                         composite: 'add' });
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 0,    expected: '-45deg' },
> +         { time: 1000, expected: '135deg' }]);
> +    }, `${property} without rotation axes`);

Again, unique test name.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1656
(Diff revision 3)
> +    }, `${property} without rotation axes`);
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style.transform = 'rotate(20deg)';

(Perhaps add a comment:
"// Rotation specified in transform property should not affect the computed value of |property|." ?)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1678
(Diff revision 3)
> +                                      '1 1 1 270deg'] },
> +                       { duration: 1000, fill: 'both', composite: 'add' });
> +
> +      testAnimationSampleRotate3d(animation, idlName,
> +        [{ time: 500, expected: '0.57735 0.57735 0.57735 135deg' }]);
> +    }, `${property}  with rotation axes`);

Test name.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1762
(Diff revision 3)
> +        },
> +        1000
> +      );
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '300px 100px 200px' }]);
> +    }, `${property}`);

Could we write a better test name for this?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1775
(Diff revision 3)
> +        },
> +        1000
> +      );
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '200px -25.5px 200px' }]);
> +    }, `${property}`);

Test name.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1777
(Diff revision 3)
> +      );
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '200px -25.5px 200px' }]);
> +    }, `${property}`);
> +  },
> +  testAddition: function(property, setup) {

I think we need at least one test that does addition between percentages, and one that does addition between a percentage and a length.

Comment 269

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f27af783ce1
Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/71f635d9a86f
Part 1.b. Do not initialize mSpecifiedTransform in ctor. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa892d6aa84
Part 1.c. Fix coding style of HasTransformStyle. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6be6571ad12
Part 1.d. Carry the computed value of individual transform in nsStyleDisplay. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5913531948
Part 2. Add a preference to enable/disable individual transform. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc1fdf79e03
Part 3. Add rotate/translate/scale properties into nsCSSPropList. r=emilio,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/8991d0468642
Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4626910ce09
Part 4.b. Use the final combined transform in the nsDisplayTransform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3dfc8f3031
Part 4.c. Temporary disable async-transform for individual-transform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42b83c0d1b4
Part 5. Implement the getComputedStyle API for individual transform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/4efc37f978d2
Part 6. Fix an assertion from the early pref access checking. r=billm

Comment 270

[Cosmin Sabou [:CosminS]]

Backed out for asserting at layout/painting/nsDisplayList.h:2835 while running mda's dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=156871782&repo=mozilla-inbound&lineNumber=35976

Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4efc37f978d2817177068b0df224fcdc3abebeee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-searchStr=mda&selectedJob=156871782

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/6002f08576ab18bc099e7fcbdefbec04da23e3f7

Comment 271

[Matt Woodrow (:mattwoodrow)]

(In reply to Cosmin Sabou [:cosmin_sabou] from comment #270)
> Backed out for asserting at layout/painting/nsDisplayList.h:2835 while
> running mda's dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html 
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=156871782&repo=mozilla-
> inbound&lineNumber=35976
> 
> Treeherder link:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=4efc37f978d2817177068b0df224fcdc3abebeee&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=success&filter-
> searchStr=mda&selectedJob=156871782
> 
> Backout link:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 6002f08576ab18bc099e7fcbdefbec04da23e3f7

This was because of bug 1420737, not this bug. We can reland the patches from here now.

Comment 272

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 273

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

nsDisplayList::mSpecifiedTransform is a RefPtr, no need put it into intialize
list.

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

Comment 274

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

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

Comment 275

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

Add three more attributes in nsStyleDisplay
1. mSpecifiedRotate to carry computed value of 'rotate' property.
2. mSpecifiedTranslate to carry computed value of 'translate' property.
3. mSpecifiedScale to carry computed value of 'scale' property.

Since each of these tree new property can be an animation target, we have to store
them in speparate attributes, instead of putting them all in mSpecifiedTransform.

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

Comment 276

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 277

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 3.a. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 278

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 279

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 280

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 4.c. Temporarily disable async-transform for individual-transform.

Since we do not support async-transform for individual-transform yet.

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

Comment 281

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 282

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 283

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 284

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 285

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 286

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

So that we can reuse the code in ComputedRotate.

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

Comment 287

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 288

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 289

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 290

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

We still need manually do it before bug 1423971 been fixed.

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

Comment 291

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

Update manifest by the following command:
  ./mach wpt-manifest-update

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

Comment 292

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.f. (testing) Add specified value serialization test cases for individual transform.

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

Comment 293

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.g. (testing) Implement testAnimationSampleRotate3d.

testAnimationSamples is not enough for comparing the computed value of rotate
property so we need this new function.

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

Comment 294

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.h. (testing) Update animation wpt for individual transform.

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

Comment 295

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I've updated the patches to include the last round of review feedback since I guess CJ made those changes locally but never updated them in MozReview.

But I guess I was too optimistic about MozReview being able to match up the rebased review requests when pushed by a different author. :(

Comment 296

[Brian Birtles (:birtles, Feb 26~ parental leave)]

What's more, some of the people who previous reviewed these patches are now no longer with us.

I guess this is going to be a mozilla-inbound job.

Comment 297

[Code Review Bot [:reviewbot]]

Comment on attachment 8944998 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

https://reviewboard.mozilla.org/r/215172/#review220754


Static analysis found 5 defects in this patch.
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/style/test/property_database.js:6295
(Diff revision 1)
>      return info.get_computed(cs, property);
>    return cs.getPropertyValue(property);
>  }
>  
> +if (IsCSSPropertyPrefEnabled("layout.css.individual-transform.enabled")) {
> +  gCSSProperties["rotate"] = {

Error: ["rotate"] is better written in dot notation. [eslint: dot-notation]

::: layout/style/test/property_database.js:6312
(Diff revision 1)
> +                      "0 0 0 0",
> +                      /* invalid calc() values */
> +                      "0.5 1 0 calc(45deg + 10)", "calc(0.5turn + 10%)", ],
> +  };
> +
> +  gCSSProperties["translate"] = {

Error: ["translate"] is better written in dot notation. [eslint: dot-notation]

::: layout/style/test/property_database.js:6324
(Diff revision 1)
> +                    /* valid calc() values */
> +                    "calc(5px + 10%)",
> +                    "calc(0.25 * 5px + 10% / 3)",
> +                    "calc(5px - 10% * 3)",
> +                    "calc(5px - 3 * 10%) 50px",
> +                    "-50px calc(5px - 10% * 3)",],

Error: A space is required after ','. [eslint: comma-spacing]

::: layout/style/test/property_database.js:6329
(Diff revision 1)
> +                    "-50px calc(5px - 10% * 3)",],
> +    invalid_values: [ "1", "-moz-min(5px,10%)", "4px, 5px, 6px",
> +                      "3px 4px 1px 7px", "4px 5px 10%",
> +                      /* invalid calc() values */
> +                      "10px calc(min(5px,10%))",
> +                      "calc(max(5px,10%) 10%", "calc(nonsense)",],

Error: A space is required after ','. [eslint: comma-spacing]

::: layout/style/test/property_database.js:6331
(Diff revision 1)
> +                      "3px 4px 1px 7px", "4px 5px 10%",
> +                      /* invalid calc() values */
> +                      "10px calc(min(5px,10%))",
> +                      "calc(max(5px,10%) 10%", "calc(nonsense)",],
> +  };
> +  gCSSProperties["scale"] = {

Error: ["scale"] is better written in dot notation. [eslint: dot-notation]

Comment 298

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I believe CJ's landing strategy was:

1. Land the first set of patches on m-i
   Updated try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=085e9ea63e17569655b77b3522b4bee9d94bbb3a
2. Land the servo PR and wait for it to merge to m-i
   Updated try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a3dfb3eaa8e41aeabc8676a04988c9e2c69b605
3. Land the last set of patches on m-i
   Updated try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbdd47b05a8dc407d213abd71d9160959936a093&selectedJob=158130264

So far it looks like we have failures in:

* layout/style/test/test_bug1112014.html
  (Test was updated on 11 Jan so this might be a new failure)
* layout/style/test/test_bug657143.html | CSS property list should be alphabetical
  (Not sure why this wasn't noticed before. Maybe some auto merge broke this?)
* layout/style/test/test_property_syntax_errors.html | invalid value 'translate: calc(max(5px,10%) 10%' is balanced and does not lead to parsing errors afterwards - got "red", expected "green"
  (Not sure about this)
* layout/style/test/test_transitions_per_property.html | transitions not supported for property rotate value none - got "190.000003010502deg", expected "none"
  (Not sure about this either)

So we'll need to fix those first, as well as the eslint errors reported in comment 297.

Comment 299

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #298)
> * layout/style/test/test_bug1112014.html
>   (Test was updated on 11 Jan so this might be a new failure)

This seems to be because:

  InspectorUtils.cssPropertySupportsType('rotate', TYPE_ANGLE)

is returning false even when the pref is on (which mochitest.ini) ensures.

Looks like we need to update PropertySupportsVariant in layout/inspector/InspectorUtils.cpp.

(There's a comment there saying, "Properties that are parsed by functions must have their attributes hand-maintained here." and looking at part 3, nsCSSProps::PropertyParseType(aPropertyID) == CSS_PROPERTY_PARSE_FUNCTION will be true for these functions.)

Comment 300

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Looks like the servo patches too don't build standalone. I get errors like:

> error[E0407]: method `Rotate` is not a member of trait `CSSStyleDeclarationMethods`
>   --> components/script/dom/cssstyledeclaration.rs:166:13

So there will need to be a little work there too.

Comment 301

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #298)
> * layout/style/test/test_bug657143.html | CSS property list should be
> alphabetical
>   (Not sure why this wasn't noticed before. Maybe some auto merge broke
> this?)

This is due to the ordering in layout/style/nsComputedDOMStylePropertyList.h. It looks like it was fixed in CJ's push to inbound so I guess he noticed it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a42b83c0d1b4

I wonder how many of the other test failures are fixed in that partial push.

Comment 302

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Looks like there were also some changes in part 4a too.

Here is a try run with the updated parts 4a and part 5 for just the mochitests:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b9302e613863c55bdb800160e0718f5282366c

This should fix at least the second test failure from comment 298.

I've had a look through CJ's recent try runs too to see if he made changes to layout/inspector/InspectorUtils.cpp but I haven't found anything yet.

Comment 303

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #300)
> Looks like the servo patches too don't build standalone. I get errors like:
> 
> > error[E0407]: method `Rotate` is not a member of trait `CSSStyleDeclarationMethods`
> >   --> components/script/dom/cssstyledeclaration.rs:166:13
> 
> So there will need to be a little work there too.

You just need to add it to servo/components/script/dom/webidls/CSSStyleDeclaration.webidl I think.

Comment 304

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I've fixed the eslint errors, fixed all the test errors except for layout/style/test/test_transitions_per_property.html (still not sure what is going on there yet), fixed the servo build errors, but now running `./mach test-unit -p style` I get:

> Your changes have increased the size of rotate SpecifiedValue to 40. The threshold is currently
> 24. SpecifiedValues affect size of PropertyDeclaration enum and increasing the size may negative
> affect style system performance. Please consider using `boxed="True"` in this longhand.
> 
> Your changes have increased the size of translate SpecifiedValue to 56. The threshold is currently
> 24. SpecifiedValues affect size of PropertyDeclaration enum and increasing the size may negative
> affect style system performance. Please consider using `boxed="True"` in this longhand.
> 
> Your changes have increased the size of scale SpecifiedValue to 28. The threshold is currently 24.
> SpecifiedValues affect size of PropertyDeclaration enum and increasing the size may negative
> affect style system performance. Please consider using `boxed="True"` in this longhand.
> 
> Your changes have increased the stack size of properties::SourcePropertyDeclaration from 568 to
> 1112. Please consider choosing a design which avoids this increase. If you feel that the increase
> is necessary, update the size in tests/unit/style/size_of.rs.
> 
> Your changes have increased the stack size of properties::PropertyDeclaration from 32 to 64.
> Please consider choosing a design which avoids this increase. If you feel that the increase is
> necessary, update the size in tests/unit/style/size_of.rs.

Still need to fix a few `test-tidy` issues too.

Comment 305

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #304)
> I've fixed the eslint errors, fixed all the test errors except for
> layout/style/test/test_transitions_per_property.html (still not sure what is
> going on there yet), fixed the servo build errors, but now running `./mach
> test-unit -p style` I get:
> 
> > Your changes have increased the size of rotate SpecifiedValue to 40. The threshold is currently
> > 24. SpecifiedValues affect size of PropertyDeclaration enum and increasing the size may negative
> > affect style system performance. Please consider using `boxed="True"` in this longhand.

Just ad boxed="true" in the predefined_type(..) calls.

> > Your changes have increased the stack size of properties::SourcePropertyDeclaration from 568 to
> > 1112. Please consider choosing a design which avoids this increase. If you feel that the increase
> > is necessary, update the size in tests/unit/style/size_of.rs.

This should go away after that I think.

Comment 306

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 3.b. Add rotate/translate/scale to InspectorUtils::CssPropertySupportsType;

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

Comment 307

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.e. (testing) Add transition test cases.

Going forward we should add the more detailed tests to web-platform-tests but
for now if we don't add tests here test_transitions_for_property.html will
complain that a property that we think is not transitionable, does in fact
transition.

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

Comment 308

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944982 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 309

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944983 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 310

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944984 [details]
Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

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

Comment 311

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944985 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 312

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944986 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 313

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944987 [details]
Bug 1207734 - Part 3.a. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 314

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944988 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 315

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944989 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 316

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944990 [details]
Bug 1207734 - Part 4.c. Temporarily disable async-transform for individual-transform.

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

Comment 317

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944991 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 318

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944992 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 319

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944993 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 320

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944994 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 321

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944995 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 322

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944996 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

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

Comment 323

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944997 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 324

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944998 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 325

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944999 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 326

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945000 [details]
Bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

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

Comment 327

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945001 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

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

Comment 328

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945002 [details]
Bug 1207734 - Part 9.f. (testing) Add specified value serialization test cases for individual transform.

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

Comment 329

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945003 [details]
Bug 1207734 - Part 9.g. (testing) Implement testAnimationSampleRotate3d.

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

Comment 330

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945004 [details]
Bug 1207734 - Part 9.h. (testing) Update animation wpt for individual transform.

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

Comment 331

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I've fixed all build issues and tidy issues on Servo now and hopefully fixed all test failures. Or at least this try run should tell us:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=76d3bf9bbfe6fc68e87b3db0d7e2fae11b275de9

I've added two patches that still need review, however.

I haven't included the servo changes in this updated MozReview review request since I'm maintaining them on my servo branch here:

  https://github.com/birtles/servo/commits/individual-transforms

Comment 332

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Servo PR #19859

Comment 333

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Looks like there are some test failures in layout/style/test/test_value_storage.html too:

> parse+compute+serialize should be idempotent for 'rotate: 72rad'
> got "4125.2958984375deg", expected "4125.29612494193deg"

Comment 334

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945326 [details]
Bug 1207734 - Part 3.b. Add rotate/translate/scale to InspectorUtils::CssPropertySupportsType;

https://reviewboard.mozilla.org/r/215534/#review221244

Comment 335

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945327 [details]
Bug 1207734 - Part 9.e. (testing) Add transition test cases.

https://reviewboard.mozilla.org/r/215536/#review221246

Looks good, I haven't done the math for the interpolation but looks reasonable. Let me know if you want me to do that :)

::: layout/style/test/test_transitions_per_property.html:1337
(Diff revision 1)
> +  div.style[prop] = '50';
> +  is(cs[prop], '20', `number property ${prop}: interpolation of numbers`);
> +  check_distance(prop, '10', '20', '50');
> +}
> +
> +function test_angle_transition(prop) {

Huh, weird that these weren't already there.

Comment 336

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #335)
> Comment on attachment 8945327 [details]
> Bug 1207734 - Part 9.e. (testing) Add transition test cases.
> 
> https://reviewboard.mozilla.org/r/215536/#review221246
> 
> Looks good, I haven't done the math for the interpolation but looks
> reasonable. Let me know if you want me to do that :)

No, that's fine. Like I said in the commit message, going forward we should really be doing all the complicated interpolation tests in web-platform-tests. test_transitions_per_property.html should really just serve as a check that we are transitioning the properties we think we are (i.e. don't accidentally make them transitionable or vice versa). And ideally even that check should eventually end up in web-platform-tests. This patch series has web-platform-tests for interpolation so these additional tests are not really important.

Comment 337

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Unfortunately I didn't get any time to work on this today. I just need to work out what the precision issue is with test_value_storage.html then I can land the first chunk.

Comment 338

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20087a84b5b6c49c4406286d36b96e0ef565a908

Comment 339

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hmm, so if we add these properties to property_database.js then when stylo is disabled test_bug1112014.html will fail because we end up parsing properties we say we don't support. But if we don't add it to property_database.js when stylo is disabled then test_property_database.html will fail because it compares the properties in our C++ prop list with gCSSProperties. We have facilities to handling properties disabled by a pref but not for stylo vs non-stylo. I'm just going to add code to InspectorUtils.cpp to return false from CssPropertySupportsType when stylo is disabled for these properties.

Comment 340

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles, busy 1/29~2/5) from comment #339)
> Hmm, so if we add these properties to property_database.js then when stylo
> is disabled test_bug1112014.html will fail because we end up parsing
> properties we say we don't support. But if we don't add it to
> property_database.js when stylo is disabled then test_property_database.html
> will fail because it compares the properties in our C++ prop list with
> gCSSProperties. We have facilities to handling properties disabled by a pref
> but not for stylo vs non-stylo. I'm just going to add code to
> InspectorUtils.cpp to return false from CssPropertySupportsType when stylo
> is disabled for these properties.

I tried doing this but the global I was getting back from InspectorUtils.cssPropertySupportsType happens to differ from the one in CSS.supports and I was having trouble detecting Servo support there. I'm sure it's possible somehow but after discussing with dholbert it's probably easier just to modify test_bug1112014.html to skip these properties when stylo is turned on.

Comment 341

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.a. (testing) Replaces tabs with spaces in bug_1112014.html.

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

Comment 342

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.b. (testing) Skip the individual transform functions in when stylo is disabled in test_bug1112014.html.

The individual transform functions are not implemented in the Gecko style
system. Making InspectorUtils.cssPropertySupportsType return false when using
the Gecko style system proved difficult and since stylo disabled builds are no
longer expected to be shipped, this patch simply skips testing these properties
when using the Gecko style system.

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

Comment 343

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944982 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 344

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944983 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 345

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944984 [details]
Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

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

Comment 346

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944985 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 347

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944986 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 348

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944987 [details]
Bug 1207734 - Part 3.a. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 349

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945326 [details]
Bug 1207734 - Part 3.b. Add rotate/translate/scale to InspectorUtils::CssPropertySupportsType;

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

Comment 350

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944988 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 351

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944989 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 352

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944990 [details]
Bug 1207734 - Part 4.c. Temporarily disable async-transform for individual-transform.

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

Comment 353

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944991 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 354

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944992 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 355

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944993 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 356

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944994 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 357

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944995 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 358

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944996 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

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

Comment 359

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944997 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 360

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944998 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 361

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944999 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 362

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945000 [details]
Bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

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

Comment 363

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945001 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

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

Comment 364

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945327 [details]
Bug 1207734 - Part 9.e. (testing) Add transition test cases.

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

Comment 365

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945002 [details]
Bug 1207734 - Part 9.f. (testing) Add specified value serialization test cases for individual transform.

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

Comment 366

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945003 [details]
Bug 1207734 - Part 9.g. (testing) Implement testAnimationSampleRotate3d.

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

Comment 367

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945004 [details]
Bug 1207734 - Part 9.h. (testing) Update animation wpt for individual transform.

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

Comment 368

[Daniel Holbert [:dholbert]]

Comment on attachment 8946760 [details]
Bug 1207734 - Part 9.a. (testing) Replaces tabs with spaces in bug_1112014.html.

https://reviewboard.mozilla.org/r/216704/#review222500

r=me; tabs begone

Comment 369

[Daniel Holbert [:dholbert]]

Comment on attachment 8946761 [details]
Bug 1207734 - Part 9.b. (testing) Skip the individual transform functions in when stylo is disabled in test_bug1112014.html.

https://reviewboard.mozilla.org/r/216706/#review222502

::: commit-message-d9883:1
(Diff revision 1)
> +Bug 1207734 - Part 9.b. (testing) Skip the individual transform functions in when stylo is disabled in test_bug1112014.html. r=dholbert

I think you're missing a word here ("in when")?

Comment 370

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Oh, I overlooked that there were various other test failures on non-Stylo. Looks like there are fewer failures when I make gCSSProperties conditional on stylo so I'm going to drop those previous patches and patch the other test failures.

Comment 371

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60c1b2561888
Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1ccb13ad477
Part 1.b. Do not initialize mSpecifiedTransform in ctor. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/51838d42b78c
Part 1.c. Fix coding style of HasTransformStyle. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fe554d8edf
Part 1.d. Carry the computed value of individual transform in nsStyleDisplay. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee95a3caf73
Part 2. Add a preference to enable/disable individual transform.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d4390b773f
Part 3.a. Add rotate/translate/scale properties into nsCSSPropList.  r=emilio,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae68e6125b9d
Part 3.b. Add rotate/translate/scale to InspectorUtils::CssPropertySupportsType; r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a33079d7cba
Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8377fde8990
Part 4.b. Use the final combined transform in the nsDisplayTransform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/46ce0b7c23be
Part 4.c. Temporarily disable async-transform for individual-transform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/e01109731276
Part 5. Implement the getComputedStyle API for individual transform. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c14ea47865
Part 6. Fix an assertion from the early pref access checking.  r=billm

Comment 372

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944982 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

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

Comment 373

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944983 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

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

Comment 374

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944984 [details]
Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

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

Comment 375

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944985 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

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

Comment 376

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944986 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

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

Comment 377

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944987 [details]
Bug 1207734 - Part 3.a. Add rotate/translate/scale properties into nsCSSPropList.

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

Comment 378

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945326 [details]
Bug 1207734 - Part 3.b. Add rotate/translate/scale to InspectorUtils::CssPropertySupportsType;

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

Comment 379

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944988 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

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

Comment 380

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944989 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

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

Comment 381

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944990 [details]
Bug 1207734 - Part 4.c. Temporarily disable async-transform for individual-transform.

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

Comment 382

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944991 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

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

Comment 383

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944992 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

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

Comment 384

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944993 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

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

Comment 385

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944994 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

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

Comment 386

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944995 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

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

Comment 387

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944996 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

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

Comment 388

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944997 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

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

Comment 389

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944998 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

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

Comment 390

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8944999 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

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

Comment 391

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945000 [details]
Bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

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

Comment 392

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945001 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

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

Comment 393

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945327 [details]
Bug 1207734 - Part 9.e. (testing) Add transition test cases.

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

Comment 394

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945002 [details]
Bug 1207734 - Part 9.f. (testing) Add specified value serialization test cases for individual transform.

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

Comment 395

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945003 [details]
Bug 1207734 - Part 9.g. (testing) Implement testAnimationSampleRotate3d.

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

Comment 396

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8945004 [details]
Bug 1207734 - Part 9.h. (testing) Update animation wpt for individual transform.

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

Comment 397

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1207734 - Part 9.i. (testing) Replaces tabs with spaces in bug_1112014.html.

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

Comment 398

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/60c1b2561888
https://hg.mozilla.org/mozilla-central/rev/d1ccb13ad477
https://hg.mozilla.org/mozilla-central/rev/51838d42b78c
https://hg.mozilla.org/mozilla-central/rev/d1fe554d8edf
https://hg.mozilla.org/mozilla-central/rev/dee95a3caf73
https://hg.mozilla.org/mozilla-central/rev/c1d4390b773f
https://hg.mozilla.org/mozilla-central/rev/ae68e6125b9d
https://hg.mozilla.org/mozilla-central/rev/2a33079d7cba
https://hg.mozilla.org/mozilla-central/rev/a8377fde8990
https://hg.mozilla.org/mozilla-central/rev/46ce0b7c23be
https://hg.mozilla.org/mozilla-central/rev/e01109731276
https://hg.mozilla.org/mozilla-central/rev/03c14ea47865

Comment 399

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d24438fb6d92
Part 9.a. Update gCSSProperties for the properties of individual transforms. r=emilio
https://hg.mozilla.org/integration/autoland/rev/52ace9f5ace2
Part 9.b. Add basic reftests for individual transform.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/1038fa25f424
Part 9.c. Clone reftests from w3c-css to web-platform-test r=emilio
https://hg.mozilla.org/integration/autoland/rev/3b9bf89b96c4
Part 9.d. Update manifest.json for new added wp-tests.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/55c6ccbdbbaa
Part 9.e. Add transition test cases. r=emilio
https://hg.mozilla.org/integration/autoland/rev/eea1c6de121f
Part 9.f. Add specified value serialization test cases for individual transform. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c32dea89a3d8
Part 9.g. Implement testAnimationSampleRotate3d.  r=birtles
https://hg.mozilla.org/integration/autoland/rev/de618ff28559
Part 9.h. Update animation wpt for individual transform.  r=birtles
https://hg.mozilla.org/integration/autoland/rev/d727648d21a1
Part 9.i. Replace tabs with spaces in bug_1112014.html. r=dholbert

Comment 400

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6d0c164e1b2
followup: Temporarily avoid some tests for this bug. r=me

Comment 401

[Emilio Cobos Álvarez (:emilio)]

Brian, had to avoid some of the layout/style/tests because android was complaining about the `prefs =` line added to the mochitest.ini:

  runByManifest mode must be enabled to set the `prefs` key

Mind looking into it?

Comment 402

[Hiroyuki Ikezoe (:hiro)]

Yeah, we need bug 1393326.

Comment 403

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'm so sorry about the failing tests. The patches in this bug just keep surprising me!

I tried to stay awake to see the landing through but jet lag and intense nerve pain got the better of me. Thanks for taking care of this.

I'll look into re-enabling the tests later today.

Comment 404

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d24438fb6d92
https://hg.mozilla.org/mozilla-central/rev/52ace9f5ace2
https://hg.mozilla.org/mozilla-central/rev/1038fa25f424
https://hg.mozilla.org/mozilla-central/rev/3b9bf89b96c4
https://hg.mozilla.org/mozilla-central/rev/55c6ccbdbbaa
https://hg.mozilla.org/mozilla-central/rev/eea1c6de121f
https://hg.mozilla.org/mozilla-central/rev/c32dea89a3d8
https://hg.mozilla.org/mozilla-central/rev/de618ff28559
https://hg.mozilla.org/mozilla-central/rev/d727648d21a1
https://hg.mozilla.org/mozilla-central/rev/d6d0c164e1b2

Comment 405

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Re-enable disabled tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad49d9bb80504c78374309160410976c8aff8a38&selectedJob=159958161

Comment 406

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947678 [details] [diff] [review]
Re-enable disabled tests

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

r=me, thank you for dealing with this :)

::: layout/style/test/file_specified_value_serialization_individual_transforms.html
@@ +1,2 @@
> +<!doctype html>
> +<html>

nit: I'd usually not bother including <html> and <head> tags that are superfluous.

@@ +1,4 @@
> +<!doctype html>
> +<html>
> +<head>
> +<meta charset=utf-8>

Maybe add a NOTE / FIXME that this can be moved back to test_specified_value_serialization when we enable the pref by default, or fix bug 1393326?

@@ +7,5 @@
> +const is = opener.is.bind(opener);
> +function finish() {
> +  const o = opener;
> +  self.close();
> +  o.SimpleTest.finish();

So nasty :)

::: layout/style/test/test_property_database.html
@@ +41,5 @@
>    var prop = gLonghandProperties[idx];
>    if (prop.pref && !IsCSSPropertyPrefEnabled(prop.pref)) {
>      continue;
>    }
> +  if (!SpecialPowers.DOMWindowUtils.isStyledByServo &&

I guess this needs no extra handling because they'll never be in gLonghandProperties if the pref is disabled, right?

It's unfortunate to lose test-coverage just because a feature is pref'd-off by default, do you know if there's any bug around to fix that? Otherwise, mind filing?

::: layout/style/test/test_specified_value_serialization.html
@@ +276,5 @@
>  
>    p.remove();
>  })();
>  
> +SimpleTest.waitForExplicitFinish();

It's unfortunate that we don't have a better setup for this, given the next property we want to test with a pref won't be able to use the opener.SimpleTest.finish() hack... I guess this is fine for now, but I really want to look into being able to not need to do this. Would fixing bug 1393326 suffice here?

@@ +282,5 @@
> +  {
> +    set: [['layout.css.individual-transform.enabled', true]],
> +  },
> +  () =>
> +    window.open('file_specified_value_serialization_individual_transforms.html')

/me cries a bit each time we use this pattern to work around the prefs = <..> in the manifest stuff... Not your fault of course :)

Comment 407

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947678 [details] [diff] [review]
Re-enable disabled tests

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

Just leaving the comment for reference since I wrote it.

::: layout/style/test/test_specified_value_serialization.html
@@ +282,5 @@
> +  {
> +    set: [['layout.css.individual-transform.enabled', true]],
> +  },
> +  () =>
> +    window.open('file_specified_value_serialization_individual_transforms.html')

Hmm, now I think of this, isn't this a bit racy? The window load is async, right?

--

Oh, as I read pushPrefEnv, I realize that there's a popPrefEnv function too, that we barely call on our tests... Really fascinating, I was assuming pushPrefEnv would pop the pref after the callback completed.

So the test is fine, but it's non-trivial why.

Comment 408

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #406)
> ::: layout/style/test/test_property_database.html
> @@ +41,5 @@
> >    var prop = gLonghandProperties[idx];
> >    if (prop.pref && !IsCSSPropertyPrefEnabled(prop.pref)) {
> >      continue;
> >    }
> > +  if (!SpecialPowers.DOMWindowUtils.isStyledByServo &&
> 
> I guess this needs no extra handling because they'll never be in
> gLonghandProperties if the pref is disabled, right?

Yes, they'll be in gLonghandProperties even if disabled. You'll get entries like:

  { name: "scale", prop: "scale", pref: "layout.css.individual-transform.enabled" },

Maybe I misunderstand your question.

> It's unfortunate to lose test-coverage just because a feature is pref'd-off
> by default, do you know if there's any bug around to fix that? Otherwise,
> mind filing?

I think the intention is that we pref-on the properties we want to test. That will be a lot easier once we can use the pref= annotation.

But given that the purpose of this test is simply to check that we don't forget to add properties to gCSSProperties that we add to nsCSSPropList it seems ok?

> It's unfortunate that we don't have a better setup for this, given the next
> property we want to test with a pref won't be able to use the
> opener.SimpleTest.finish() hack... I guess this is fine for now, but I
> really want to look into being able to not need to do this. Would fixing bug
> 1393326 suffice here?

Yes.

Comment 409

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2fdd805667d
Re-enable disabled transform property tests; r=emilio

Comment 410

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c2fdd805667d

Comment 413

[Chris Mills [:cmills]]

I have documented this:

https://developer.mozilla.org/en-US/docs/Web/CSS/translate
https://developer.mozilla.org/en-US/docs/Web/CSS/rotate
https://developer.mozilla.org/en-US/docs/Web/CSS/scale
https://developer.mozilla.org/en-US/Firefox/Experimental_features#CSS

And added property and compat data to the relevant repos:
https://github.com/mdn/data/pull/183
https://github.com/mdn/browser-compat-data/pull/1199

Let me know if that looks OK. Cheers!

Comment 414

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks for doing this.

Unfortunately this spec is still in flux. I believe the rotate syntax was just changed in the CSS WG telcon a few weeks' back but given that your scale doc refers to the latest resolution surrounding that, I suspect you know that!

What does the x_y_z part of the translate BCD pull request refer to?

Also, I believe this is only enabled in Chrome when experimental web features are enabled.

Edge have implemented this too but not released it yet, as far as I can tell.

From our end we still need to do quite a bit of work before this is ready to ship and there's no-one currently assigned to doing it (the engineer working on this was from the Taipei office).

Comment 415

[Eric Willigers]

> I believe the rotate syntax was just changed in the CSS WG telcon a few weeks' back

I updated Blink and the web platform tests for the rotate syntax change, and the change in scale's meaning when only one number is supplied.

The one change I haven't yet addressed is the distinction between 2D and 3D transforms. If translate/scale/rotate are specified without a z value or an axis, then the transform is 2D, and should serialize as 2D. Same for animation involving no 3D values.

Comment 416

[Chris Mills [:cmills]]

(In reply to Brian Birtles (:birtles) from comment #414)
> Thanks for doing this.
> 
> Unfortunately this spec is still in flux. I believe the rotate syntax was
> just changed in the CSS WG telcon a few weeks' back but given that your
> scale doc refers to the latest resolution surrounding that, I suspect you
> know that!

I didn't. Happy accident ;-)


> 
> What does the x_y_z part of the translate BCD pull request refer to?

Sorry, that was on rotate, but I incorrectly wrote translate in to it. It is supposed to refer to the "axis plus angle" syntax that rotate can accept. In my testing, it sdidn't seem to work in the Firefox implementation.

> 
> Also, I believe this is only enabled in Chrome when experimental web
> features are enabled.

Ah, looks like you are correct. I must've tested it in Canary the first time round - that looks to have it on by default. Do you know which pref in Chrome controls it?

> 
> Edge have implemented this too but not released it yet, as far as I can tell.

OK.

> 
> From our end we still need to do quite a bit of work before this is ready to
> ship and there's no-one currently assigned to doing it (the engineer working
> on this was from the Taipei office).

Well, I am on hand to do updates as needed.

Comment 417

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #416)
> (In reply to Brian Birtles (:birtles) from comment #414)
> > Also, I believe this is only enabled in Chrome when experimental web
> > features are enabled.
> 
> Ah, looks like you are correct. I must've tested it in Canary the first time
> round - that looks to have it on by default. Do you know which pref in
> Chrome controls it?

I believe it's just "Experimental Web Platform features" but Eric can correct me if I'm wrong.