Closed Bug 1353966 Opened 3 years ago Closed 2 years ago

stylo: implement discrete type animation for all properties

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: hiro, Assigned: daisuke)

References

Details

Attachments

(1 file)

+++ 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.
Depends on: 1356071
No longer depends on: 1356071
Duplicate of this bug: 1356071
Assignee: nobody → dakatsuka
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 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 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 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!
Depends on: 1367283
Comment on attachment 8865273 [details]
Bug 1353966 - Part 1: Implement discrete type animation for position related properties.

https://reviewboard.mozilla.org/r/136906/#review145898
Attachment #8865273 - Flags: review?(hikezoe)
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.
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.
Depends on: 1368610
Depends on: 1371115
Blocks: stylo-wpt
Depends on: 1378076
Depends on: 1379921
Depends on: 1379922
Depends on: 1382136
Depends on: 1382137
Depends on: 1382138
Priority: P2 → --
Significant behavioral change from Gecko, blocks turning on important regression test(s), P1.
Priority: -- → P1
Depends on: 1386963
Depends on: 1387941
No longer depends on: 1387941
Depends on: 1389452
Finally!  Great works daisuke!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Too late for 56. Mass won't fix for 56.
Too late for 56. Mass won't fix for 56.
You need to log in before you can comment on or make changes to this bug.