Closed Bug 1328786 Opened 8 years ago Closed 8 years ago

Stylo: Store animation properties into nsStyleDisplay::mAnimations

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1328786 - Part 1: Add an FFI function for expanding nsStyleAutoArray.
59 bytes, text/x-review-board-request
heycam
: review+
Details
single_keywords supports custom_consts map for enum.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Add gecko_enum_prefix for animation-direction and animation-fill-mode.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Implement Index for nsStyleAutoArray.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Implement nsStyleAutoArray.ensure_len().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Regenerate bindings
59 bytes, text/x-review-board-request
Details
Store animation properties into nsDisplay.mAnimations.
59 bytes, text/x-review-board-request
heycam
: review+
Details
No description provided.
Blocks: stylo-css-animations
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review103168 ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:34 (Diff revision 1) > + TransitionTimingFunction::Steps(steps, StartEnd::Start) => { > + let gecko_step = GeckoStep { > + _type: nsTimingFunction_Type::StepStart, > + _steps: steps > + }; > + tf = unsafe { mem::transmute(&gecko_step) }; I did use transmute to set servo's value to gecko's struct which has union field, but I am not sure this is a preferable way in Stylo. I also wrote FFI functions[1] to set values to the struct as another solution, but I did throw them away because I think we might need such FFI functions more and more in future. [1] https://hg.mozilla.org/try/rev/bf32da4b9b6312040be99a5a9f0f7b410856becb
Comment on attachment 8824275 [details] Bug 1328786 - Part 1: Add an FFI function for expanding nsStyleAutoArray. https://reviewboard.mozilla.org/r/102446/#review103170
Attachment #8824275 - Flags: review?(cam) → review+
Comment on attachment 8824276 [details] single_keywords supports custom_consts map for enum. https://reviewboard.mozilla.org/r/102448/#review103172
Attachment #8824276 - Flags: review?(cam) → review+
Comment on attachment 8824277 [details] Add gecko_enum_prefix for animation-direction and animation-fill-mode. https://reviewboard.mozilla.org/r/102450/#review103174
Attachment #8824277 - Flags: review?(cam) → review+
Comment on attachment 8824278 [details] Implement Index for nsStyleAutoArray. https://reviewboard.mozilla.org/r/102452/#review103176 ::: servo/components/style/gecko_bindings/sugar/ns_style_auto_array.rs:7 (Diff revision 1) > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > use gecko_bindings::structs::nsStyleAutoArray; > use std::iter::{once, Chain, Once, IntoIterator}; > +use std::ops::{Index}; Nit: No braces around a single name here. ::: servo/components/style/gecko_bindings/sugar/ns_style_auto_array.rs:18 (Diff revision 1) > + if index > self.len() { > + panic!("out of range") > + } > + match index { > + 0 => &self.mFirstElement, > + _ => &self.mOtherElements[index - 1] Nit: Rust style is to have a trailing comma on the last item, too.
Attachment #8824278 - Flags: review?(cam) → review+
Comment on attachment 8824279 [details] Implement nsStyleAutoArray.ensure_len(). https://reviewboard.mozilla.org/r/102454/#review103182 ::: servo/components/style/gecko_bindings/sugar/ns_style_auto_array.rs:39 (Diff revision 1) > // in sync > pub fn len(&self) -> usize { > 1 + self.mOtherElements.len() > } > + > + pub fn set_len(&mut self, len: usize) { I think we should call this something else, since set_len sounds like the Vec function set_len, which will make the vector shorter if the current length is longer than the argument. How about ensure_len?
Attachment #8824279 - Flags: review?(cam) → review+
Comment on attachment 8824278 [details] Implement Index for nsStyleAutoArray. https://reviewboard.mozilla.org/r/102452/#review103176 > Nit: Rust style is to have a trailing comma on the last item, too. Oh! Thanks! I did drop it here and there..
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review103192 Instead of writing duplicate structs here for the cubic / steps union, I think we should just use the bindgen-generated types. Their names aren't so nice -- e.g. nsTimingFunction__bindgen_ty_1__bindgen_ty_1 -- but they are stable enough.
Attachment #8824280 - Flags: review?(cam) → review-
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review103196 ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:26 (Diff revision 1) > +} > + > +impl nsTimingFunction { > + // Create an nsTimingFunction from TransitionTimingFunction. > + #[inline] > + pub fn from_transition_timing_function(function: &TransitionTimingFunction) -> Self { Also, instead of having functions called from_transition_timing_function and to_transition_timing_function, it would be more idiomatic Rust to write From impls: impl From<TransitionTimingFunction> for nsTimingFunction impl From<nsTimingFunction> for TransitionTimingFunction
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review103196 > Also, instead of having functions called from_transition_timing_function and to_transition_timing_function, it would be more idiomatic Rust to write From impls: > > impl From<TransitionTimingFunction> for nsTimingFunction > > impl From<nsTimingFunction> for TransitionTimingFunction Thank you cameron! Actually I don't yet understand what the 'From' is, but I will use it.
Comment on attachment 8824282 [details] Store animation properties into nsDisplay.mAnimations. https://reviewboard.mozilla.org/r/102460/#review103212 r=me with these comments addressed. ::: servo/components/style/properties/gecko.mako.rs:1093 (Diff revision 1) > + unsafe { self.gecko.mAnimations.set_len(v.0.len()) }; > + self.gecko.mAnimation${gecko_ffi_name}Count = v.0.len() as u32; > + > + for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) { > + let result = match servo { > + % for value in keyword.values_for('gecko'): Nit: Can write |keyword.gecko_values()| instead of |keyword.values_for('gecko')|. ::: servo/components/style/properties/gecko.mako.rs:1109 (Diff revision 1) > + use properties::longhands::animation_${ident}::single_value::computed_value::T as Keyword; > + match self.gecko.mAnimations[index].m${gecko_ffi_name} ${keyword.maybe_cast("u32")} { > + % for value in keyword.values_for('gecko'): > + structs::${keyword.gecko_constant(value)} => Keyword::${to_rust_ident(value)}, > + % endfor > + _ => Keyword::${to_rust_ident(keyword.servo_values()[0])}, Why do we need this _ case? Are there some values we're not handling? ::: servo/components/style/properties/gecko.mako.rs:1402 (Diff revision 1) > + // XXX: Is there any effective ways? > + AnimationName(Atom::from(String::from_utf16_lossy(&self.gecko.mAnimations[index].mName[..]))) I don't know if there is a better way. Manish might know. ::: servo/components/style/properties/gecko.mako.rs:1433 (Diff revision 1) > + unsafe { self.gecko.mAnimations.set_len(v.0.len()) }; > + self.gecko.mAnimationIterationCountCount = v.0.len() as u32; > + for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) { > + match servo { > + AnimationIterationCount::Number(n) => gecko.mIterationCount = n as f32, > + AnimationIterationCount::Infinite => gecko.mIterationCount = f32::INFINITY Nit: trailing comma. ::: servo/components/style/properties/gecko.mako.rs:1439 (Diff revision 1) > + use properties::longhands::animation_iteration_count::computed_value::AnimationIterationCount; > + AnimationIterationCount::Number(self.gecko.mAnimations[index].mIterationCount as u32) I think we need to map f32::INFINITY to AnimationIterationCount::Infinite, here. ::: servo/components/style/properties/gecko.mako.rs:1464 (Diff revision 1) > + } > + pub fn copy_animation_timing_function_from(&mut self, other: &Self) { > + unsafe { self.gecko.mAnimations.set_len(other.gecko.mAnimations.len()) }; > + self.gecko.mAnimationTimingFunctionCount = other.gecko.mAnimationTimingFunctionCount; > + for (index, animation) in self.gecko.mAnimations.iter_mut().enumerate() { > + animation.mTimingFunction = other.gecko.mAnimations[index].mTimingFunction.clone(); Do we need clone() here? nsTimingFunction is Copy, so you can just assign it.
Attachment #8824282 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #17) > Comment on attachment 8824280 [details] > Add utility functions that convert nsTimingFunction to > TransitionTimingFunction and vice versa. > > https://reviewboard.mozilla.org/r/102456/#review103196 > > ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:26 > (Diff revision 1) > > +} > > + > > +impl nsTimingFunction { > > + // Create an nsTimingFunction from TransitionTimingFunction. > > + #[inline] > > + pub fn from_transition_timing_function(function: &TransitionTimingFunction) -> Self { > > Also, instead of having functions called from_transition_timing_function and > to_transition_timing_function, it would be more idiomatic Rust to write From > impls: > > impl From<TransitionTimingFunction> for nsTimingFunction As regard to setting StyleAnimation.mTimingFunction, I think we need: impl nsTimingFunction { pub fn assign_from(&mut self, function: &TransitionTimingFunction) } because we already have allocated StyleAnimtion.mTimingFunction by ensure_len() there. Right? I am thinking 'impl From<TransitionTimingFunction> for nsTimingFunction' will be necessary in bug 1328787 because we need to create a new nsTimingFunction from TransitionTimingFunction there to set the function in Keyframe object in Gecko. Anyway, thanks for the quick and informative review!
Comment on attachment 8824282 [details] Store animation properties into nsDisplay.mAnimations. https://reviewboard.mozilla.org/r/102460/#review103212 > Why do we need this _ case? Are there some values we're not handling? There is a 'EndGuard_' entry. And, I did not notice though, FillMode in Gecko has 'Auto' entry (for web animation API?, I thought CSS Animation has it too). I can solve 'EndGuard_' case somehow, but I have no idea about 'Auto'.
Comment on attachment 8824282 [details] Store animation properties into nsDisplay.mAnimations. https://reviewboard.mozilla.org/r/102460/#review103212 > There is a 'EndGuard_' entry. And, I did not notice though, FillMode in Gecko has 'Auto' entry (for web animation API?, I thought CSS Animation has it too). I can solve 'EndGuard_' case somehow, but I have no idea about 'Auto'. Oops! I did misunderstand the unhandle case. We can just use panic!() for both of EndGuard_ and Auto.
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review103302 r=me with these changes. ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:9 (Diff revision 2) > +impl nsTimingFunction { > + // Assign nsTimingFunction member variables from TransitionTimingFunction. > + pub fn assign_from(&mut self, function: &TransitionTimingFunction) { I think we can make this one a From impl, too. Since nsTimingFunction is Copy, we don't have to worry about the From impl consuming the value it is converting from. So we should just make this: impl From<TransitionTimingFunction> for nsTimingFunction { fn from(function: TransitionTimingFunction) -> nsTimingFunction { ... } } ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:40 (Diff revision 2) > +impl <'a> From<&'a nsTimingFunction> for TransitionTimingFunction { > + fn from(function: &'a nsTimingFunction) -> TransitionTimingFunction { And here we can just make this From<nsTimingFunction>, rather than using the reference type. In theory using the reference is cheaper, but for a small-ish object like nsTimingFunction, Manish says that it's not a problem that we might memcpy its contents when doing the conversion. (And that copy might get optimized out, anyway.) I'll add some more comments on the following patch.
Attachment #8824280 - Flags: review?(cam) → review+
Comment on attachment 8824282 [details] Store animation properties into nsDisplay.mAnimations. https://reviewboard.mozilla.org/r/102460/#review103304 ::: servo/components/style/properties/gecko.mako.rs:1455 (Diff revision 2) > + pub fn set_animation_timing_function(&mut self, v: longhands::animation_timing_function::computed_value::T) { > + unsafe { self.gecko.mAnimations.ensure_len(v.0.len()) }; > + > + self.gecko.mAnimationTimingFunctionCount = v.0.len() as u32; > + for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) { > + gecko.mTimingFunction.assign_from(&servo); This can be: gecko.mTimingFunction = servo.into(); ::: servo/components/style/properties/gecko.mako.rs:1461 (Diff revision 2) > + use properties::longhands::animation_timing_function::computed_value::TransitionTimingFunction; > + TransitionTimingFunction::from(&self.gecko.mAnimations[index].mTimingFunction) This can be: self.gecko.mAnimations[index].mTimingFunction.into()
Comment on attachment 8824282 [details] Store animation properties into nsDisplay.mAnimations. https://reviewboard.mozilla.org/r/102460/#review103306 ::: servo/components/style/properties/gecko.mako.rs:1109 (Diff revision 2) > + use properties::longhands::animation_${ident}::single_value::computed_value::T as Keyword; > + match self.gecko.mAnimations[index].m${gecko_ffi_name} ${keyword.maybe_cast("u32")} { > + % for value in keyword.gecko_values(): > + structs::${keyword.gecko_constant(value)} => Keyword::${to_rust_ident(value)}, > + % endfor > + _ => panic!(), Let's add a message in here, e.g. x => panic!("Found unexpected value for animation-${ident}: {:?}", x),
Comment on attachment 8824282 [details] Store animation properties into nsDisplay.mAnimations. https://reviewboard.mozilla.org/r/102460/#review103304 > This can be: gecko.mTimingFunction = servo.into(); Wohoo! What a magical Rust world!
Cameron, would you mind reviewing 'impl From<TransitionTimingFunction> for nsTimingFunction' part again? Especially the part of initializing nsTimingFunction. Thanks!
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review103338 ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:10 (Diff revision 3) > +use euclid::point::TypedPoint2D; > +use gecko_bindings::structs::{nsTimingFunction, nsTimingFunction_Type}; > +use properties::longhands::animation_timing_function::computed_value::{StartEnd, TransitionTimingFunction}; > +use std::mem; > + > +impl From<TransitionTimingFunction > for nsTimingFunction { Nit: no space before ">".
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41) > Cameron, would you mind reviewing 'impl From<TransitionTimingFunction> for > nsTimingFunction' part again? Especially the part of initializing > nsTimingFunction. Looks good, thanks! Not sure if there is a better way to initialize the unions, so I think using zeroed() is OK.
Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf8fd47ac64 Part 1: Add an FFI function for expanding nsStyleAutoArray. r=heycam
https://hg.mozilla.org/mozilla-central/rev/ccf8fd47ac64
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
See Also: → 1343751
Comment on attachment 8824280 [details] Add impl From<nsTimingFunction> for TransitionTimingFunction and impl From<TransitionTimingFunction> for nsTimingFunction. https://reviewboard.mozilla.org/r/102456/#review118558 ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:36 (Diff revision 3) > + // FIXME: we need to add more types once TransitionTimingFunction > + // has more types. Thanks for this comment. I will add frames timing function here. :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: