Closed Bug 1328786 Opened 3 years ago Closed 3 years ago

Stylo: Store animation properties into nsStyleDisplay::mAnimations

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

No description provided.
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: 3 years ago
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.