1465066 - Cleanup transform animation.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

The recent changes we made to it allows for it to be cleaner.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1465066: Cleanup transform animation.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8981415 [details]
Bug 1465066: Cleanup transform animation.

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8981415 [details]
Bug 1465066: Cleanup transform animation.

https://reviewboard.mozilla.org/r/247540/#review253816

Thanks for doing this!  I did forget to merge the empty list cases into the function in bug 1464647.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2542
(Diff revision 2)
> +            let already_mismatched = matches!(
> +                longer[0],
> +                TransformOperation::InterpolateMatrix { .. } |
> +                TransformOperation::AccumulateMatrix { .. }
> +            );

At first glance I didn't understand why we need the InterpolationMatrix|AccumulateMatrix check in the function, but yeah it's the case that the length mismatches but the first nth operations matches (nth == the shorter list length).  In such cases, we should bail out before extending the shorter list, I guess?  Extending identity matrices shouldn't cause any issues but..

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8981415 [details]
> Bug 1465066: Cleanup transform animation.
> 
> https://reviewboard.mozilla.org/r/247540/#review253816
> 
> Thanks for doing this!  I did forget to merge the empty list cases into the
> function in bug 1464647.
> 
> :::
> servo/components/style/properties/helpers/animated_properties.mako.rs:2542
> (Diff revision 2)
> > +            let already_mismatched = matches!(
> > +                longer[0],
> > +                TransformOperation::InterpolateMatrix { .. } |
> > +                TransformOperation::AccumulateMatrix { .. }
> > +            );
> 
> At first glance I didn't understand why we need the
> InterpolationMatrix|AccumulateMatrix check in the function, but yeah it's
> the case that the length mismatches but the first nth operations matches
> (nth == the shorter list length).  In such cases, we should bail out before
> extending the shorter list, I guess?  Extending identity matrices shouldn't
> cause any issues but..

I thought it wouldn't be needed either but it is. We cannot really treat InterpolateMatrix { .. } as matched, because then we fail to interpolate it and return an error instead when we interpolate it with, let's say, `none`.

We do need to extend it though, or at least we were doing that already. This was caught by: dom/animation/test/style/test_interpolation-from-interpolatematrix-to-none.html

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > Comment on attachment 8981415 [details]
> > Bug 1465066: Cleanup transform animation.
> > 
> > https://reviewboard.mozilla.org/r/247540/#review253816
> > 
> > Thanks for doing this!  I did forget to merge the empty list cases into the
> > function in bug 1464647.
> > 
> > :::
> > servo/components/style/properties/helpers/animated_properties.mako.rs:2542
> > (Diff revision 2)
> > > +            let already_mismatched = matches!(
> > > +                longer[0],
> > > +                TransformOperation::InterpolateMatrix { .. } |
> > > +                TransformOperation::AccumulateMatrix { .. }
> > > +            );
> > 
> > At first glance I didn't understand why we need the
> > InterpolationMatrix|AccumulateMatrix check in the function, but yeah it's
> > the case that the length mismatches but the first nth operations matches
> > (nth == the shorter list length).  In such cases, we should bail out before
> > extending the shorter list, I guess?  Extending identity matrices shouldn't
> > cause any issues but..
> 
> I thought it wouldn't be needed either but it is. We cannot really treat
> InterpolateMatrix { .. } as matched, because then we fail to interpolate it
> and return an error instead when we interpolate it with, let's say, `none`.
> 
> We do need to extend it though, or at least we were doing that already. This
> was caught by:
> dom/animation/test/style/test_interpolation-from-interpolatematrix-to-none.
> html

Oh I see. emtpry list slips through iter().zip() check, right?  Ok, that's fair.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2331837a98
Cleanup transform animation. r=hiro

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0b2331837a98