1335998 - stylo: Handle interpolation (interpolatematrix) and accumulation (accumulatematrix) of mismatched transform lists

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

This is the counter part of servo's transform interpolation problem that needs reference box [1].
Once servo has ComputedOperation::InterpolateMatrix type (or something), I guess we need to delegate its interpolation to servo when we build transform display list.  (Or we need to convert it to gecko's interpolatematrix.)

[1] https://github.com/servo/servo/issues/13267

Comment 1

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> This is the counter part of servo's transform interpolation problem that
> needs reference box [1].
> Once servo has ComputedOperation::InterpolateMatrix type (or something), I
> guess we need to delegate its interpolation to servo when we build transform
> display list.  (Or we need to convert it to gecko's interpolatematrix.)
> 
> [1] https://github.com/servo/servo/issues/13267

I think this makes sense, so let's try this way on stylo for now.

Comment 2

[Boris Chiou [:boris]]

The interesting thing is: in order to test this, I have to disable OMTA first because compositor makes it work.

Comment 3

[Boris Chiou [:boris]]

And it seems we really need to convert it into gecko interpolatematrix (in convert_transform), so we could store it in nsFrame->StyleDisplay() or nsDisplayList, as |nsCSSValueShardList| type. Therefore, we can get the result matrix in many places, e.g. for computing the bound or others.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you Boris for taking care of this!
I will mark several bugs as duplicate of this.

Comment 8

[Boris Chiou [:boris]]

Attachment: Test case for mismatched transform lists

Borrowed from Bug 1365851

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

We use this type to store the interpolated result of two mismatched
transform lists, and we resolve it until we know the reference box size
(on Gecko side). The conversion from ComputedOperation::InterpolateMatrix
to eCSSKeyword_interpolatematrix will be implemented in the next patch.

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

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

We convert ComputedOperation::InterpolateMatrix into gecko type not on
the main thread, so we cannot use nsCSSValueList_heap (which is not
thread safe so we cannot create it and destroy it on different threads).
Therefore, we use nsCSSValueSharedList to represent the cloned
from_list/to_list.

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

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 6: Add some unit tests for basic transform operations for Servo.

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

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 8: Enable table-overflowed-by-animation.html on stylo.

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

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review147936

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review147938

::: layout/style/nsStyleTransformMatrix.cpp:443
(Diff revision 1)
>                        TransformReferenceBox& aRefBox,
>                        bool* aContains3dTransform)
>  {
>    NS_PRECONDITION(aData->Count() == 4, "Invalid array!");
>  
> -  Matrix4x4 matrix1, matrix2;
> +  auto readTransform = [&](const nsCSSValue& aValue) -> Matrix4x4 {

Since this has no captures, can it just be a function? AIUI lambdas are less common in C++.

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:276
(Diff revision 1)
> +
> +        let mut item_ptr = unsafe {
> +            self.mValue.mSharedList.as_ref()
> +                .as_mut().expect("List pointer should be non-null")
> +        }.mHead;
> +        while let Some(item) = unsafe { item_ptr.as_mut() } {

generally it would be preferable to implement this as an iterator, but it seems like a one-off thing so probably not worth it

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:278
(Diff revision 1)
> +            self.mValue.mSharedList.as_ref()
> +                .as_mut().expect("List pointer should be non-null")
> +        }.mHead;
> +        while let Some(item) = unsafe { item_ptr.as_mut() } {
> +            item.mValue = values.next().expect("Values shouldn't have been exhausted");
> +            item_ptr = item.mNext;

debug assert that it isn't null

::: servo/components/style/properties/gecko.mako.rs:2285
(Diff revision 1)
> -                    pattern = "ComputedMatrix { %s }" % ", ".join(single_patterns)
> +                    pattern = "(ComputedMatrix { %s })" % ", ".join(single_patterns)
>                  else:
> -                    pattern = "ComputedMatrixWithPercents { %s }" % ", ".join(single_patterns)
> +                    pattern = "(ComputedMatrixWithPercents { %s })" % ", ".join(single_patterns)
> +            elif keyword == "interpolatematrix":
> +                fields = zip(["from_list: ref ", "to_list: ref ", "progress: "], items)
> +                pattern = " { %s }" % ", ".join([b[0] + b[1] + str(a+1) for (a,b) in enumerate(fields)])

kinda feel like directly typing out `{ from_list: ref list1, to_list: ref list2, progress: percentage3 }` would be better here. As it stands it's hard to figure out what the code does.

If not, leave a comment with the expanded form or at least part of it, like we've done in the matrix code

::: servo/components/style/properties/gecko.mako.rs:2312
(Diff revision 1)
>                  bindings::Gecko_CSSValue_GetArrayItem(gecko_value, 0),
>                  eCSSKeyword_${keyword}
>              );
>              % for index, item in enumerate(items):
> +                % if item == "list":
> +                debug_assert!(${item}${index + 1}.0.is_some());

nit: indent

::: servo/components/style/properties/longhand/box.mako.rs:1220
(Diff revision 1)
> +        impl ComputedOperation {
> +            #[inline]
> +            pub fn is_interpolate_matrix(&self) -> bool {
> +                use self::ComputedOperation::InterpolateMatrix;
> +                match self {
> +                    &InterpolateMatrix { from_list: _, to_list: _, progress: _ } => true,

`match *self { InterpolateMatrix {...} => true, }

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8872309 [details]
Bug 1335998 - Part 8: Enable table-overflowed-by-animation.html on stylo.

https://reviewboard.mozilla.org/r/143810/#review147960

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8872308 [details]
Bug 1335998 - Part 6: Add some unit tests for basic transform operations for Servo.

https://reviewboard.mozilla.org/r/143808/#review147962

Comment 17

[Manish Goregaokar [:manishearth]]

I assume this is still WIP with the actual interpolation code missing.

Comment 18

[Boris Chiou [:boris]]

(In reply to Manish Goregaokar [:manishearth] from comment #17)
> I assume this is still WIP with the actual interpolation code missing.

Actually, these patches use the interpolation code on Gecko now, and I am still trying to make them use the interpolation code of two matrices on Servo. Thanks for reviewing them.

Comment 19

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review147938

> Since this has no captures, can it just be a function? AIUI lambdas are less common in C++.

Oh sorry for my indention is not clear enough. Actually there are some captures, and that's why I use lambda here.

> generally it would be preferable to implement this as an iterator, but it seems like a one-off thing so probably not worth it

Yes. I agree. actually we also iterate nsCSSValueList in gecko.mako.rs, so it might be worth to do it.

> debug assert that it isn't null

I just noticed that the mNext of the last item is null. Looks like using iterator could be better. :)

> kinda feel like directly typing out `{ from_list: ref list1, to_list: ref list2, progress: percentage3 }` would be better here. As it stands it's hard to figure out what the code does.
> 
> If not, leave a comment with the expanded form or at least part of it, like we've done in the matrix code

OK, I will just type it out. Thanks for the suggestion.

Comment 20

[Boris Chiou [:boris]]

try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b9364b15a1f346a2bf75ba631146336d0f6d69

Comment 21

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #18)
> (In reply to Manish Goregaokar [:manishearth] from comment #17)
> > I assume this is still WIP with the actual interpolation code missing.
> 
> Actually, these patches use the interpolation code on Gecko now, and I am
> still trying to make them use the interpolation code of two matrices on
> Servo. Thanks for reviewing them.

Just finish the AccumulateMatrix part. The current implementation still decompose/interpolate/recompose the matrix in Gecko (i.e. in nsStyleTransformMatrix.cpp). I will make it use Servo code for this part tomorrow. (i.e. Pass the two matrices to Servo, and decompose/interpolate/recompose them to a new interpolated matrix, and then return it back to Gecko side.)

Comment 22

[Manish Goregaokar [:manishearth]]

> The current implementation still decompose/interpolate/recompose the matrix in Gecko (i.e. in nsStyleTransformMatrix.cpp).

Ah, nice. I didn't realize that we still used the gecko backend for transform animations. For OMT animations I suppose.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #21)
> Just finish the AccumulateMatrix part. The current implementation still
> decompose/interpolate/recompose the matrix in Gecko (i.e. in
> nsStyleTransformMatrix.cpp). I will make it use Servo code for this part
> tomorrow. (i.e. Pass the two matrices to Servo, and
> decompose/interpolate/recompose them to a new interpolated matrix, and then
> return it back to Gecko side.)

AccumulateMatrices normally are nested in an InterpolateMatrix, you will need to send whole set of matrices, I guess.

Comment 24

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to Boris Chiou [:boris] from comment #21)
> > Just finish the AccumulateMatrix part. The current implementation still
> > decompose/interpolate/recompose the matrix in Gecko (i.e. in
> > nsStyleTransformMatrix.cpp). I will make it use Servo code for this part
> > tomorrow. (i.e. Pass the two matrices to Servo, and
> > decompose/interpolate/recompose them to a new interpolated matrix, and then
> > return it back to Gecko side.)
> 
> AccumulateMatrices normally are nested in an InterpolateMatrix, you will
> need to send whole set of matrices, I guess.

Yes, we got something like this on Servo:
e.g.
Interpolate { from_list: AccmulateMatrix { from_list: rotate(45deg), to_list: translateX(100px), count: 2 },
              to_list: rotate(90deg),
              progress: 0.5 }

And convert it into a nsCSSValueSharedList. Gecko will read this as matrices (by applying layout info), and we use Servo code to decompose/interpolate/recompose these matrices to get the final result.

Comment 25

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

If we set the composite operation to accumulate, we may need to accumulate two
mismatched transform lists, and then to interpolate them. In order to
accumulate two mismatched transform lists, we also need the reference
box information, so use the same idea as that for interpolation. i.e.
use AccmulateMatrix to store it temporarily, and convert it into matrix
later.

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

Comment 26

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 3: Use IntoIterator for nsCSSValueList.

Use rust iterator to iterate nsCSSValueList.

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

Comment 27

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

Use Servo backend to decompose/interpolate/recompose matrices on both
main thread and compositor thread.

Note: There are some tiny differences between interpolation results of 2d/3d
      matrices on Gecko and Servo, especially if there is skew() or any 3d
      transform function.

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

Comment 28

[Boris Chiou [:boris]]

Attachment: Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

Add this crashtest for interpolation/accumulation on mismatched transform lists
to make sure it doesn't crash and there is no leak because we use
nsCSSValueSharedList, instead of nsCSSValueList_heap, for Servo backend.

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

Comment 29

[Boris Chiou [:boris]]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

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

Comment 30

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

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

Comment 31

[Boris Chiou [:boris]]

Comment on attachment 8872308 [details]
Bug 1335998 - Part 6: Add some unit tests for basic transform operations for Servo.

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

Comment 32

[Boris Chiou [:boris]]

Comment on attachment 8872309 [details]
Bug 1335998 - Part 8: Enable table-overflowed-by-animation.html on stylo.

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

Comment 33

[Manish Goregaokar [:manishearth]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149560

::: layout/style/ServoBindings.h:503
(Diff revision 2)
>  void Gecko_CSSValue_SetInt(nsCSSValueBorrowedMut css_value, int32_t integer, nsCSSUnit unit);
>  void Gecko_CSSValue_SetPair(nsCSSValueBorrowedMut css_value,
>                              nsCSSValueBorrowed xvalue, nsCSSValueBorrowed yvalue);
>  void Gecko_CSSValue_SetList(nsCSSValueBorrowedMut css_value, uint32_t len);
>  void Gecko_CSSValue_SetPairList(nsCSSValueBorrowedMut css_value, uint32_t len);
> +void Gecko_CSSValue_SetSharedList(nsCSSValueBorrowedMut css_value, uint32_t len);

Change name to InitSharedList?

Comment 34

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review149562

don't completely understand Accumulate, but it seems to be ok.

Comment 35

[Boris Chiou [:boris]]

For Servo issue, https://github.com/servo/servo/issues/13267, I think it will be easier to fix in layout/fragment.rs after introducing InterpolateMatrix. (We can send an extra PR for it.)

Comment 36

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149560

> Change name to InitSharedList?

OK, I will update its name. Thanks for suggestion.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

https://reviewboard.mozilla.org/r/145566/#review149568

2000ms is too long for crashtest.
What is the exact purpose of this test? Do we really need to wait for the end of the animation?
If we just want to check that InterpolateMatrix and AccumulateMatrix work fine on gecko side, we should wait for the animation being sent to compositor?

Comment 38

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874293 [details]
Bug 1335998 - Part 3: Use IntoIterator for nsCSSValueList.

https://reviewboard.mozilla.org/r/145562/#review149564

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:233
(Diff revision 1)
>      /// This is only supported on the main thread.
> -    pub fn set_list<I>(&mut self, mut values: I) where I: ExactSizeIterator<Item=nsCSSValue> {
> +    pub fn set_list<I>(&mut self, values: I) where I: ExactSizeIterator<Item=nsCSSValue> {
>          debug_assert!(values.len() > 0, "Empty list is not supported");
>          unsafe { bindings::Gecko_CSSValue_SetList(self, values.len() as u32); }
>          debug_assert_eq!(self.mUnit, nsCSSUnit::eCSSUnit_List);
> -        let mut item_ptr = &mut unsafe {
> +        let list = &mut unsafe {

I would use an explicit type annotation here to be sure

Comment 39

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874293 [details]
Bug 1335998 - Part 3: Use IntoIterator for nsCSSValueList.

https://reviewboard.mozilla.org/r/145562/#review149572

Comment 40

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2050
(Diff revision 1)
>  
> +#[cfg(feature = "gecko")]
> +impl From<Matrix4x4> for ComputedMatrix {
> +    fn from(matrix: Matrix4x4) -> ComputedMatrix {
> +        // Matrix4x4 is [u32; 16usize]. However, it should be f32 on Gecko, so we
> +        // have to transmute it first.

Wait, why is this?

I'm not very comfortable with this transmute -- in Gecko are we storing the bit representations of floats in u32 fields? That doesn't seem right.

Comment 41

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

> Wait, why is this?
> 
> I'm not very comfortable with this transmute -- in Gecko are we storing the bit representations of floats in u32 fields? That doesn't seem right.

Yes. it's weird. In structs_debug.rs, we set ```pub type Matrix4x4 = [u32; 16usize];```. However, in Gecko, we use ```gfx::Float``` (i.e. float) [1] in Matrix4x4. Do you have any idea to let bindgen generate f32 for it?

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko/generated/structs_debug.rs#2214
[2] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Matrix.h#483

Comment 42

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

> Yes. it's weird. In structs_debug.rs, we set ```pub type Matrix4x4 = [u32; 16usize];```. However, in Gecko, we use ```gfx::Float``` (i.e. float) [1] in Matrix4x4. Do you have any idea to let bindgen generate f32 for it?
> 
> [1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko/generated/structs_debug.rs#2214
> [2] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Matrix.h#483

OK. I will use a workaround. Just pass Matrix4x4::components[16], which is a native array type. This way may avoid this problem.

Comment 43

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

> OK. I will use a workaround. Just pass Matrix4x4::components[16], which is a native array type. This way may avoid this problem.

Ohh, the root cause is, gfx::Float is defined as ```u32``` in structs_debug.rs. I think this is a bug...

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review149570

::: commit-message-45411:6
(Diff revision 2)
> +Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.
> +
> +We use this arm to store the interpolated result of two mismatched
> +transform lists, and we resolve it until we know the reference box size
> +(on Gecko side). The conversion from ComputedOperation::InterpolateMatrix
> +to eCSSKeyword_interpolatematrix will be implemented in this patch series.

(Nit: later in this patch series)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1635
(Diff revision 2)
> +            TransformOperation::InterpolateMatrix { .. } => {
> +                // We delay the matrix interpolation after getting frame info, so use an identity
> +                // matrix.
> +                let identity = ComputedMatrix::identity();
> +                result.push(TransformOperation::Matrix(identity));
> +            }

I don't really understand this comment, or maybe it's the code I don't follow.

This is in build_identity_transform_list. We use that when we try to do add_weighted on two transform lists and one of them is None. This relates to my comment a little later, but it's not clear why identity is the right value here.

Do we ever expect to pass the result of this to add_weighted? If so, then I guess we need to modify can_interpolate_list. If not, then do we need to put anything in the list at all?

I'm not saying it's wrong, it's just not clear why this is the correct thing to do -- perhaps simply because we haven't documented how interpolatematrix is expected to be used.

::: servo/components/style/properties/longhand/box.mako.rs:1184
(Diff revision 2)
>                        computed::LengthOrPercentage,
>                        computed::Length),
>              Scale(CSSFloat, CSSFloat, CSSFloat),
>              Rotate(CSSFloat, CSSFloat, CSSFloat, computed::Angle),
>              Perspective(computed::Length),
> +            // For interpolated mismatched transform lists.

Nit: For mismatched transform lists.

::: servo/components/style/properties/longhand/box.mako.rs:1267
(Diff revision 2)
>          /// Part of CSS Transform Module Level 2 and defined at
>          /// [§ 13.1. 3D Transform Function](https://drafts.csswg.org/css-transforms-2/#funcdef-perspective).
>          ///
>          /// The value must be greater than or equal to zero.
>          Perspective(specified::Length),
> +        /// A intermediate type for interpolation on mismatched transform lists.

Nit: interpolation of

::: servo/components/style/properties/longhand/box.mako.rs:1878
(Diff revision 2)
> +                        debug_assert!(result.is_empty(),
> +                                      "transform list can only contain one InterpolateMatrix");

Why is that?

It's not really clear to me what the invariants are around using interpolatematrix. They probably need to be documented somewhere in this patch.

e.g.

* If we use interpolatematrix it should be the only function in the list
* If we have a transform list with interpolatematrix in it, we should never call add_weighted on it (or pass it to add_weighted)
* etc.

I'm not sure if any of those are true, they're just examples of the sorts of invariants we need to document if they exist.

Comment 45

[Boris Chiou [:boris]]

Hi, Manish, Emilio,

I got a bug in bindgen. When I add "mozilla::gfx::Float" into ServoBindings.toml, its definition is |u32| [1]. However, in gecko |mozilla::gfx::Float| is defined as |float| [2] in gfx/2d/Types.h

[1] in structs_debug.rs (my local build)
    pub mod gfx {
        #[allow(unused_imports)]
        use self::super::super::super::root;
        pub type IntRegion = [u64; 3usize];
        pub type Float = u32;
        ...
    }

[2] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Types.h#18

Do you know what is the potential problem in rust-bindgen to generate this type? This result seems incorrect.

Comment 46

[Brian Birtles (:birtles)]

Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review149588

::: servo/components/style/properties/helpers/animated_properties.mako.rs:654
(Diff revision 1)
> +                        }
> +                    % endif
> +                % endif
> +            % endfor
> +            _ => {
> +                panic!("Expected weighted addition of computed values of the same \

This comment should be "Expected accumulation of ..."

(I notice the comment in the previous definition of add() is also wrong. That's probably my fault.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2638
(Diff revision 1)
> +                let to_list = build_identity_transform_list(from_list);
> +                Ok(add_weighted_transform_lists(from_list, &to_list, count as f64, 1.0))

Do we need to build an identity transform list here?

The corresponding code on the Gecko side is just:

  resultList = AddTransformLists(0.0, listA, aCount, listA,
                                 eCSSKeyword_accumulatematrix);

Could we do the same here:

  Ok(add_weighted_transform_lists(from_list, from_list, count as f64, 0.0))

Does that work?

(And is this the only place we need build_identity_transform_list to work with interpolatematrix/accumulatematrix?)

::: servo/components/style/properties/longhand/box.mako.rs:1188
(Diff revision 1)
>              Perspective(computed::Length),
>              // For interpolated mismatched transform lists.
>              InterpolateMatrix { from_list: T,
>                                  to_list: T,
>                                  progress: Percentage },
> +            // For accumulate operation on mismatched transform lists.

Nit: of mismatched transform lists.

::: servo/components/style/properties/longhand/box.mako.rs:2000
(Diff revision 1)
> +                            debug_assert!(result.is_empty(),
> +                                          "transform list can only contain one AccumulateMatrix");

(As with the previous patch, somewhere we need to explain why this assertion makes sense.)

Comment 47

[Boris Chiou [:boris]]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review149570

> I don't really understand this comment, or maybe it's the code I don't follow.
> 
> This is in build_identity_transform_list. We use that when we try to do add_weighted on two transform lists and one of them is None. This relates to my comment a little later, but it's not clear why identity is the right value here.
> 
> Do we ever expect to pass the result of this to add_weighted? If so, then I guess we need to modify can_interpolate_list. If not, then do we need to put anything in the list at all?
> 
> I'm not saying it's wrong, it's just not clear why this is the correct thing to do -- perhaps simply because we haven't documented how interpolatematrix is expected to be used.

I think it is possible to pass an InterpolateMatirx into add_weighted(). gfx/tests/crashtests/815489.html has this situation:
e.g.
```
[InterpolateMatrix { from_list: T(Some([Rotate(1, 0, 0, Degree(67166.57))])),
                     to_list: T(Some([InterpolateMatrix { from_list: T(Some([Rotate(1, 0, 0, Degree(67166.57))])),
                                                          to_list: T(Some([Skew(Radian(0), Degree(86))])),
                                                          progress: Percentage(0.0145912245) }])),
                     progress: Percentage(0.018843267) }]
```
For this case, can_interpolate_list() returns false, so we may have a nested InterpolateMatrix.

However, if one of the transform list is none, it is impossible to create an InterpolateMatrix (and pass it into build_identity_transform_list() later). Hence, we can just assert this case. (i.e. build_identity_transform_list() should assert if we pass InterpolateMatrix in.)

> Why is that?
> 
> It's not really clear to me what the invariants are around using interpolatematrix. They probably need to be documented somewhere in this patch.
> 
> e.g.
> 
> * If we use interpolatematrix it should be the only function in the list
> * If we have a transform list with interpolatematrix in it, we should never call add_weighted on it (or pass it to add_weighted)
> * etc.
> 
> I'm not sure if any of those are true, they're just examples of the sorts of invariants we need to document if they exist.

> * If we use interpolatematrix it should be the only function in the list

Yes, that is what I want to assert. TransfromList can only contain one InterpolateMatrix:
e.g.
```
[ InterpolateMatrix { from_list: ..., to_list: InterpolateMatrix {...} } ]
[ InterpolateMatrix { from_list: InterpolateMatrix {...}, to_list: ... } ]
```
This is valid, because this transform list has only one InterpolateMatrix. (Therefore, we may pass an InterpolateMatirx into add_weighed()). A test case has this situation: http://searchfox.org/mozilla-central/source/gfx/tests/crashtests/815489.html. And this may happen if we trigger a transition from a middle point of the interpolation to its original value. (i.e. mouse hover)

e.g.
```
[ InterpolateMatrix { from_list: ..., to_list: ... },
  Interpolatematrix { from_list: ..., to_list: ... } ]
```
This is not correct. We only creat one InterpolateMatirx for a list.

I will document this into box.mako.rs, before the arm of ```InterpolateMatrix```.

Comment 48

[Brian Birtles (:birtles)]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149596

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:273
(Diff revision 2)
> +        for (item, new_value) in list.unwrap().into_iter().zip(values) {
> +            *item = new_value;
> +        }

Does this work? |list|, at this point, points to mHead which is a nsCSSValueList* pointer which we should navigate through by looking up its mNext members. Something like [1]

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko_bindings/sugar/ns_css_value.rs#236

Unless IntoIterator is defined for this somewhere else?

::: servo/components/style/properties/gecko.mako.rs:2393
(Diff revision 2)
> +                "list" : "TransformList(Some(convert_shared_list_to_operations(%s)))",
> +            }
> +            pre_symbols = "("
> +            post_symbols = ")"
> +            if keyword == "interpolatematrix" or keyword == "accumulatematrix":
> +                pre_symbols = " {"

Is the space before the { here intentional?

Comment 49

[Brian Birtles (:birtles)]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review149602

Clearing review on this for now since it sounds like there are a few tweaks to be made.

Comment 50

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #47)
> e.g.
> ```
> [ InterpolateMatrix { from_list: ..., to_list: ... },
>   Interpolatematrix { from_list: ..., to_list: ... } ]
> ```
> This is not correct. We only creat one InterpolateMatirx for a list.

When an underlying value is InterpolateMatrix and a subsequent transform is additive, what happens? Also more additive or accumulative transform on the underlying InterpolateMatrix?

Comment 51

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149596

> Does this work? |list|, at this point, points to mHead which is a nsCSSValueList* pointer which we should navigate through by looking up its mNext members. Something like [1]
> 
> [1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko_bindings/sugar/ns_css_value.rs#236
> 
> Unless IntoIterator is defined for this somewhere else?

Yes. In Part 3:
```
/// Mutable Iterator of nsCSSValueList.
#[allow(non_camel_case_types)]
pub struct nsCSSValueListMutIterator<'a> {
    current: *mut nsCSSValueList,
    phantom: PhantomData<&'a mut nsCSSValue>,
}

impl<'a> Iterator for nsCSSValueListMutIterator<'a> {
    type Item = &'a mut nsCSSValue;
    fn next(&mut self) -> Option<Self::Item> {
        match unsafe { self.current.as_mut() } {
            Some(item) => {
                self.current = item.mNext;
                Some(&mut item.mValue)
            },
            None => None
        }
    }
}

impl<'a> IntoIterator for &'a mut nsCSSValueList {
    type Item = &'a mut nsCSSValue;
    type IntoIter = nsCSSValueListMutIterator<'a>;

    fn into_iter(self) -> Self::IntoIter {
        nsCSSValueListMutIterator { current: self as *mut nsCSSValueList,
                                    phantom: PhantomData }
    }

```
When calling next() at the first time, we check the first nsCSSValueList and assign the mNext to current. This should be the same as that in [1].

> Is the space before the { here intentional?

Yes.

Comment 52

[Brian Birtles (:birtles)]

Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

https://reviewboard.mozilla.org/r/145566/#review149576

::: commit-message-e9aaf:1
(Diff revision 1)
> +Bug 1335998 - Part 7: Add a crashtest for mismatched transfrom lists.

Nit: transform lists

Comment 53

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> (In reply to Boris Chiou [:boris] from comment #47)
> > e.g.
> > ```
> > [ InterpolateMatrix { from_list: ..., to_list: ... },
> >   Interpolatematrix { from_list: ..., to_list: ... } ]
> > ```
> > This is not correct. We only creat one InterpolateMatirx for a list.
> 
> When an underlying value is InterpolateMatrix and a subsequent transform is
> additive, what happens? Also more additive or accumulative transform on the
> underlying InterpolateMatrix?

I cannot run wpt now, and just pull new code to test it (i.e. accumulation-per-property.html/addition-per-property.html) again to make sure the case you mentioned. Thanks for the reminder.

Comment 54

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Chiou [:boris] from comment #45)
> Hi, Manish, Emilio,
> 
> I got a bug in bindgen. When I add "mozilla::gfx::Float" into
> ServoBindings.toml, its definition is |u32| [1]. However, in gecko
> |mozilla::gfx::Float| is defined as |float| [2] in gfx/2d/Types.h
> 
> [1] in structs_debug.rs (my local build)
>     pub mod gfx {
>         #[allow(unused_imports)]
>         use self::super::super::super::root;
>         pub type IntRegion = [u64; 3usize];
>         pub type Float = u32;
>         ...
>     }
> 
> [2]
> http://searchfox.org/mozilla-central/rev/
> 507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Types.h#18
> 
> Do you know what is the potential problem in rust-bindgen to generate this
> type? This result seems incorrect.

So this is because we mark as opaque all types in mozilla::gfx::*:

http://searchfox.org/mozilla-central/source/layout/style/ServoBindings.toml#264

So bindgen only tries to make it a type with the appropriate layout.

If we need more types from mozilla::gfx we need to make that condition more precise.

Comment 55

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #54)
> So this is because we mark as opaque all types in mozilla::gfx::*:
> 
> http://searchfox.org/mozilla-central/source/layout/style/ServoBindings.
> toml#264
> 
> So bindgen only tries to make it a type with the appropriate layout.
> 
> If we need more types from mozilla::gfx we need to make that condition more
> precise.

OK. I see. Thanks. So I have to let gfx::Float non-opaque at least.

Comment 56

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

Still need some changes for RawGeckoGfxMatrix4x4, so clear r?.

Comment 57

[Brian Birtles (:birtles)]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149938

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:273
(Diff revision 2)
> +        for (item, new_value) in list.unwrap().into_iter().zip(values) {
> +            *item = new_value;
> +        }

Ok thanks. Sorry, I didn't see part 3.

::: servo/components/style/properties/gecko.mako.rs:2393
(Diff revision 2)
> +                "list" : "TransformList(Some(convert_shared_list_to_operations(%s)))",
> +            }
> +            pre_symbols = "("
> +            post_symbols = ")"
> +            if keyword == "interpolatematrix" or keyword == "accumulatematrix":
> +                pre_symbols = " {"

Ok, it feels like there should be some other way of factoring this that would avoid this oddity, but for now a short comment describing the expected output might make this code less confusing for the next person who looks at it.

Comment 58

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> (In reply to Boris Chiou [:boris] from comment #47)
> > e.g.
> > ```
> > [ InterpolateMatrix { from_list: ..., to_list: ... },
> >   Interpolatematrix { from_list: ..., to_list: ... } ]
> > ```
> > This is not correct. We only creat one InterpolateMatirx for a list.
> 
> When an underlying value is InterpolateMatrix and a subsequent transform is
> additive, what happens? Also more additive or accumulative transform on the
> underlying InterpolateMatrix?

I haven't seen a case which concatenates two or more InterpolateMatrices until now, actually. In other words, I have no idea to make a underlying value be InterpolateMatrix by contaminating the iterationComposite: 'accumulate' and composite: 'add'. Besides, all transform related tests in {additive, accumulate}-per-property.html are passed. If you find one, please let me know to debug it. I believe the current implementation could support it, and only some assertions to make sure this case doesn't happen. Therefore, if it is possible, we can just remove the assertions. Thanks for the reminder.

Comment 59

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #58)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> > (In reply to Boris Chiou [:boris] from comment #47)
> > > e.g.
> > > ```
> > > [ InterpolateMatrix { from_list: ..., to_list: ... },
> > >   Interpolatematrix { from_list: ..., to_list: ... } ]
> > > ```
> > > This is not correct. We only creat one InterpolateMatirx for a list.
> > 
> > When an underlying value is InterpolateMatrix and a subsequent transform is
> > additive, what happens? Also more additive or accumulative transform on the
> > underlying InterpolateMatrix?
> 
> I haven't seen a case which concatenates two or more InterpolateMatrices
> until now, actually. In other words, I have no idea to make a underlying
> value be InterpolateMatrix by contaminating the iterationComposite:
> 'accumulate' and composite: 'add'. Besides, all transform related tests in
> {additive, accumulate}-per-property.html are passed. If you find one, please
> let me know to debug it. I believe the current implementation could support
> it, and only some assertions to make sure this case doesn't happen.
> Therefore, if it is possible, we can just remove the assertions. Thanks for
> the reminder.

I am sorry for the confusion. I meant we have multiple *nested* InterpolateMatrix into a list.
say;
　div.animate({ transform: [ 'rotate(100deg)', 'translateX(100px)' ] }, { duration: 1000 });
  div.animate({ transform: [ 'scale(2)', 'translateY(200px)' ] }, { duration: 10, composite: 'add', iterationComposite: 'accumulate' });

Comment 60

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> I am sorry for the confusion. I meant we have multiple *nested*
> InterpolateMatrix into a list.
> say;
> 　div.animate({ transform: [ 'rotate(100deg)', 'translateX(100px)' ] }, {
> duration: 1000 });
>   div.animate({ transform: [ 'scale(2)', 'translateY(200px)' ] }, {
> duration: 10, composite: 'add', iterationComposite: 'accumulate' });

Thanks for this case! Let me double-check it.

Comment 61

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> I am sorry for the confusion. I meant we have multiple *nested*
> InterpolateMatrix into a list.
> say;
> 　div.animate({ transform: [ 'rotate(100deg)', 'translateX(100px)' ] }, {
> duration: 1000 });
>   div.animate({ transform: [ 'scale(2)', 'translateY(200px)' ] }, {
> duration: 10, composite: 'add', iterationComposite: 'accumulate' });

I see. So we may have a nested InterpolateMatrix like this (so complicated):

[
  InterpolateMatrix { from_list: [ AccumulateMatrix { from_list: ...,
                                                      to_list: [ InterpolateMatrix { from_list: ...,
                                                                                     to_list: ...,
                                                                                     progress: ... },
                                                                 Scale(2, 2, 1) ],
                                                      count: 2 } ])),
                      to_list: [ AccumulateMatrix { from_list: ...,
                                                    to_list: [ InterpolateMatrix { from_list: ...,
                                                                                   to_list: ...,
                                                                                   progress: ... },
                                                               Translate(0px, 20px, 0px) ],
                                                    count: 2 } ],
                      progress: Percentage(0.7808252) }
]

In other words, an InterpolateMatrix could be concatenated with other ComputedOperations. I will remove that assertion. Thanks for this example.

Comment 62

[Boris Chiou [:boris]]

Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review149588

> Do we need to build an identity transform list here?
> 
> The corresponding code on the Gecko side is just:
> 
>   resultList = AddTransformLists(0.0, listA, aCount, listA,
>                                  eCSSKeyword_accumulatematrix);
> 
> Could we do the same here:
> 
>   Ok(add_weighted_transform_lists(from_list, from_list, count as f64, 0.0))
> 
> Does that work?
> 
> (And is this the only place we need build_identity_transform_list to work with interpolatematrix/accumulatematrix?)

This simplication works. Thanks!

And this is not the only place we need build_identity_transform_list for accumulatematrix:
e.g.
```
div {
  transfrom: rotate(45deg)
}

div.animate( [ { transform: 'translateX(100px)', composite: 'accumulate' },
               { transform: 'none' } ],
             1000);
```

We create an AccumualteMatrix { from_list: Rotate(45deg), to_list: translateX(100px), count: ... }, and do the interpolation of this AccumulateMatrix and none, which calls add_weighted(). So we may build an identity transform from add_weighted(). Therefore, I think we still need to handle AccumulateMatrix. (i.e. return an identity matrix for it)

Comment 63

[Boris Chiou [:boris]]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

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

Comment 64

[Boris Chiou [:boris]]

Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

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

Comment 65

[Boris Chiou [:boris]]

Comment on attachment 8874293 [details]
Bug 1335998 - Part 3: Use IntoIterator for nsCSSValueList.

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

Comment 66

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

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

Comment 67

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

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

Comment 68

[Boris Chiou [:boris]]

Comment on attachment 8872308 [details]
Bug 1335998 - Part 6: Add some unit tests for basic transform operations for Servo.

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

Comment 69

[Boris Chiou [:boris]]

Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

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

Comment 70

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review150258

Comment 71

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] (away 6/9~6/12) from comment #62)
> Comment on attachment 8874292 [details]
> Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.
> 
> https://reviewboard.mozilla.org/r/145560/#review149588
> 
> > Do we need to build an identity transform list here?
> > 
> > The corresponding code on the Gecko side is just:
> > 
> >   resultList = AddTransformLists(0.0, listA, aCount, listA,
> >                                  eCSSKeyword_accumulatematrix);
> > 
> > Could we do the same here:
> > 
> >   Ok(add_weighted_transform_lists(from_list, from_list, count as f64, 0.0))
> > 
> > Does that work?
> > 
> > (And is this the only place we need build_identity_transform_list to work with interpolatematrix/accumulatematrix?)
> 
> This simplication works. Thanks!
> 
> And this is not the only place we need build_identity_transform_list for
> accumulatematrix:
> e.g.
> ```
> div {
>   transfrom: rotate(45deg)
> }
> 
> div.animate( [ { transform: 'translateX(100px)', composite: 'accumulate' },
>                { transform: 'none' } ],
>              1000);
> ```
> 
> We create an AccumualteMatrix { from_list: Rotate(45deg), to_list:
> translateX(100px), count: ... }, and do the interpolation of this
> AccumulateMatrix and none, which calls add_weighted(). So we may build an
> identity transform from add_weighted(). Therefore, I think we still need to
> handle AccumulateMatrix. (i.e. return an identity matrix for it)

Ok, so as I understand it, in this case we will do:

  (&Some(ref from_list), &None) => {
      // http://dev.w3.org/csswg/css-transforms/#none-transform-animation
      let to_list = build_identity_transform_list(from_list);
      add_weighted_transform_lists(from_list, &to_list, self_portion, other_portion)
  }

build_identity_transform_list will do:

  TransformOperation::AccumulateMatrix { .. } => {
      // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
      let identity = ComputedMatrix::identity();
      result.push(TransformOperation::Matrix(identity));
  }

then we will pass that to add_weighted_transform_lists which does:

  if can_interpolate_list(from_list, to_list) {

But can_interpolate_list returns false so we just do:

  } else {
      use values::specified::Percentage;
      let from_transform_list = TransformList(Some(from_list.to_vec()));
      let to_transform_list = TransformList(Some(to_list.to_vec()));
      result.push(
          TransformOperation::InterpolateMatrix { from_list: from_transform_list,
                                                  to_list: to_transform_list,
                                                  progress: Percentage(other_portion as f32) });
  }

Strictly speaking we don't need to generate an identity matrix in this case. We could just pass in the from_transform twice. That is, after all, what we do in Gecko:

  if (HasAccumulateMatrix(list1)) {
    result = AddDifferentTransformLists(0, list1,
                                        aCoeff1, list1,
                                        eCSSKeyword_interpolatematrix);
  } else {
    result = AddTransformLists(0, list1, aCoeff1, list1);
  }

But this seems reasonable too.

I still think we should explain why identity is a suitable value to return in this case.

Comment 72

[Brian Birtles (:birtles)]

Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review150418

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1642
(Diff revision 3)
>              TransformOperation::Perspective(..) => {
>                  // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
>                  let identity = ComputedMatrix::identity();
>                  result.push(TransformOperation::Matrix(identity));
>              }
> +            TransformOperation::InterpolateMatrix { .. } => panic!("Not supported"),

s/Not supported/Interpolation of InterpolateMatrix is not supported/ ?

::: servo/components/style/properties/longhand/box.mako.rs:1185
(Diff revision 3)
>                        computed::Length),
>              Scale(CSSFloat, CSSFloat, CSSFloat),
>              Rotate(CSSFloat, CSSFloat, CSSFloat, computed::Angle),
>              Perspective(computed::Length),
> +            // For mismatched transform lists.
> +            // A vector of |ComptuedOperation| could contain an |InterpolateMatrix| and other

ComputedOperation

Comment 73

[Brian Birtles (:birtles)]

Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review150434

::: servo/components/style/properties/helpers/animated_properties.mako.rs:662
(Diff revision 2)
> +                        }
> +                    % endif
> +                % endif
> +            % endfor
> +            _ => {
> +                panic!("Expected Expected accumulation of computed values of the same \

Expected

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1663
(Diff revision 2)
>                  // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
>                  let identity = ComputedMatrix::identity();
>                  result.push(TransformOperation::Matrix(identity));

This comment doesn't make sense. We should explain why identity is a suitable value to use here.

Comment 74

[Brian Birtles (:birtles)]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149604

::: commit-message-1a600:6
(Diff revision 1)
> +Note: There are some tiny differences between interpolation results of 2d/3d
> +      matrices on Gecko and Servo, especially if there is skew() or any 3d
> +      transform function.

What are the differences / why?

(We don't necessarily need a full explanation, just something that makes this less mysterious. e.g. "due differences in precision used to represent the components" etc.)

Comment 75

[Boris Chiou [:boris]]

Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review150434

> This comment doesn't make sense. We should explain why identity is a suitable value to use here.

Understood. I will update this.

Comment 76

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

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

Comment 77

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

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

Comment 78

[Boris Chiou [:boris]]

Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

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

Comment 79

[Boris Chiou [:boris]]

Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

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

Comment 80

[Boris Chiou [:boris]]

Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

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

Comment 81

[Boris Chiou [:boris]]

Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

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

Comment 82

[Boris Chiou [:boris]]

Attachment: Servo PR, #17202

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86c0fdac3bc2
Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/ab49fc64a4bf
Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/1df01865d100
Part 3: Add a crashtest for mismatched transform lists. r=hiro

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86c0fdac3bc2
https://hg.mozilla.org/mozilla-central/rev/ab49fc64a4bf
https://hg.mozilla.org/mozilla-central/rev/1df01865d100