Closed
Bug 1353966
Opened 7 years ago
Closed 7 years ago
stylo: implement discrete type animation for all properties
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: hiro, Assigned: daisuke)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
+++ 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.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → dakatsuka
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years 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•7 years 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•7 years 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•7 years 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!
Reporter | ||
Comment 7•7 years 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•7 years 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•7 years 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.
Blocks: 1371478
Updated•7 years ago
|
Priority: P2 → --
Comment 10•7 years ago
|
||
Significant behavioral change from Gecko, blocks turning on important regression test(s), P1.
Priority: -- → P1
Reporter | ||
Comment 11•7 years ago
|
||
Finally! Great works daisuke!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
status-firefox57:
--- → fixed
Comment 13•7 years ago
|
||
Too late for 56. Mass won't fix for 56.
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•