1353966 - stylo: implement discrete type animation for all properties

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

+++ This bug was initially created as a clone of Bug #1336668 +++

In bug 1336668 I am going to just introduce a mechanism for discrete type animation and implement a property as discrete type animation.
In this bug we will implement rest of properties of discrete animations.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1353966 - Part 1: Implement discrete type animation for position related properties.

In this patch, implement following position related properties.

* align-content
* align-self
* box-sizing
* flex-direction
* flex-wrap
* grid-auto-columns
* grid-auto-flow
* grid-auto-rows
* grid-column-end
* grid-column-startgrid-row-end
* grid-row-start,
* justify-content
* justify-items
* justify-self
* object-fit

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Hi Hiro,

I implemented discrete animation for only position related properties.
I'd like to get your review at first since I'd like to know whether the basic thinking is not wrong.

Thanks!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8865273 [details]
Bug 1353966 - Part 1: Implement discrete type animation for position related properties.

https://reviewboard.mozilla.org/r/136906/#review139872

Thought I haven't look into clone_grid_auto_xx() closely, this looks basically good to me.

::: servo/components/style/values/specified/align.rs:118
(Diff revision 1)
>  ///
>  /// The 16-bit field stores the primary value in its lower 8 bits, and the optional fallback value
>  /// in its upper 8 bits.  This matches the representation of these properties in Gecko.
>  #[derive(Copy, Clone, Debug, Eq, PartialEq)]
>  #[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))]
> -pub struct AlignJustifyContent(u16);
> +pub struct AlignJustifyContent(pub u16);

Do we really need to make this pub?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8865273 [details]
Bug 1353966 - Part 1: Implement discrete type animation for position related properties.

https://reviewboard.mozilla.org/r/136906/#review139886

::: servo/components/style/gecko_bindings/sugar/ns_style_coord.rs:35
(Diff revision 1)
> +        unsafe {
> +            match self.unit() {
> +                eStyleUnit_Null |
> +                eStyleUnit_Normal |
> +                eStyleUnit_Auto |
> +                eStyleUnit_None => { true }

It seems to me that this PartialEq is not correct, especially here.

Instead using PartialEq, we should use from_gecko_style_coord() and then use match. Given that grid-auto-{columns,rows} is not popular for animations, I think it's OK with performance loss.

Comment 6

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8865273 [details]
Bug 1353966 - Part 1: Implement discrete type animation for position related properties.

https://reviewboard.mozilla.org/r/136906/#review139872

Thank you very much!

> Do we really need to make this pub?

I got "cannot construct with a private field" error if there is no "pub".
But, I found "new" fn, I'll use this instead.
Thanks!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8865273 [details]
Bug 1353966 - Part 1: Implement discrete type animation for position related properties.

https://reviewboard.mozilla.org/r/136906/#review145898

Comment 8

[Daisuke Akatsuka (:daisuke)]

Thank you for your review, Hiro!
I'll check and fix your points.

Also, I'm so sorry, please let me append two more patches before test patch.
* box related properties as patch 7
* position related properties as patch 8
* test patch as patch 9

Thanks.

Comment 9

[Daisuke Akatsuka (:daisuke)]

Oh, sorry, not this bug..

(In reply to Daisuke Akatsuka (:daisuke) from comment #8)
> Thank you for your review, Hiro!
> I'll check and fix your points.
> 
> Also, I'm so sorry, please let me append two more patches before test patch.
> * box related properties as patch 7
> * position related properties as patch 8
> * test patch as patch 9
> 
> Thanks.

Comment 10

[Brian Birtles (:birtles)]

Significant behavioral change from Gecko, blocks turning on important regression test(s), P1.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Finally!  Great works daisuke!

Comment 12

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.

Comment 13

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.