stylo: implement discrete type animation for all properties

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
4 months ago
8 days ago

People

(Reporter: hiro, Assigned: daisuke)

Tracking

(Depends on: 4 bugs, Blocks: 5 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
+++ 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.
Blocks: 1243581
Depends on: 1356071
No longer depends on: 1356071
Duplicate of this bug: 1356071
(Reporter)

Updated

3 months ago
Assignee: nobody → dakatsuka
Comment hidden (mozreview-request)
(Assignee)

Comment 3

3 months ago
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!
(Reporter)

Comment 4

3 months ago
mozreview-review
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?
(Reporter)

Comment 5

3 months ago
mozreview-review
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.
(Assignee)

Comment 6

3 months ago
mozreview-review-reply
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!
(Assignee)

Updated

2 months ago
Depends on: 1367283
(Reporter)

Comment 7

2 months ago
mozreview-review
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)
(Assignee)

Comment 8

2 months ago
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.
(Assignee)

Comment 9

2 months ago
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.
(Assignee)

Updated

2 months ago
Depends on: 1368610
(Assignee)

Updated

2 months ago
Depends on: 1371115
Blocks: 1371478
(Reporter)

Updated

2 months ago
Blocks: 1370466
(Assignee)

Updated

23 days ago
Depends on: 1378076
(Assignee)

Updated

16 days ago
Depends on: 1379921
(Assignee)

Updated

9 days ago
Depends on: 1379922
(Assignee)

Updated

8 days ago
Depends on: 1382136
(Assignee)

Updated

8 days ago
Depends on: 1382137
(Assignee)

Updated

8 days ago
Depends on: 1382138
You need to log in before you can comment on or make changes to this bug.