1328787 - Stylo: Convert Servo's animation keyframes and store them into Gecko's keyframes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

The basic idea here what I am doing to convert keyframes and store them in Gecko side is:

* Call a function named Servo_StyleSet_FillKeyframesForName in CSSAnimationBuilder::Build() to fill nsTArray<Keyframe> in for each animation's name in servo side.
* Call Gecko_AnimationAppendKeyframe from the Servo_StyleSet_FillKeyframesForName to append a new Gecko's Keyframe into the nsTArray. That's because the Keyframe struct has Maybe<ComputedTimingFunction> (ComputedTimingFunction ≠ nsTimingFunction), if we handle it in servo side, we need other FFI functions to initialize Maybe<> and ComputedTimingFunction.
* Call nsAnimationManager->UpdateAnimations() in ServoStyleSet::GetContext().  (I am not confident this place is the right place we should invoke CSS animations in stylo.)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h.

KeyframeEffectReadOnly.h has lots of unnecesarry stuff for stylo for now, so
is split stuff which needs for stylo into Keyframe.h.

Review commit: https://reviewboard.mozilla.org/r/105964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105964/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build.

Because, in case of stylo, we don't have nsCSSKeyframesRule.

Review commit: https://reviewboard.mozilla.org/r/105966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105966/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>.

The argument, ServoComputedValues*, will be used to get animation timing function
corresponding to animation's name in servo side.

Review commit: https://reviewboard.mozilla.org/r/105968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105968/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 4: Add Gecko_AnimationAppendKeyframe.

This is a function which is called in Servo_StyleSet_FillKeyframesForName to
append a Gecko's Keyframe into nsTArray<Keyframe>.

Without this function we need to end up exposing ComputedTimingFunction class
and Maybe<> as FFI.

Review commit: https://reviewboard.mozilla.org/r/105970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105970/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName.

PropertyValuePair will be set in subsequent patches.

Review commit: https://reviewboard.mozilla.org/r/105972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105972/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 6: Update build_gecko.rs and bindings.

'RawServoDeclarationBlock' in structs_types in build_gecko.rs and the change in
conversion.rs are needed for using set_arc_leaky() for
PropertyValuePair.mServoDeclarationBlock. Without this change,
mServoDeclarationBlock.set_arc_leaky() is called against RawServoDeclarationBlockVoid.

Review commit: https://reviewboard.mozilla.org/r/105974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105974/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 7: Implement From for specified TransitionTimingFunction.

Now animation-timing-function in keyframe is stored as specified
TransitionTimingFunction.  We need a way to convert it to nsTimingFunction too.

Review commit: https://reviewboard.mozilla.org/r/105976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105976/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 8: Animation timing function can be overridden by animation-timing-function specified in keyframe.

Review commit: https://reviewboard.mozilla.org/r/105978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105978/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 9: Set Keyframe.mPropertyValues for the case where keyframe is specified.

Review commit: https://reviewboard.mozilla.org/r/105980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105980/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 10: Set Keyframe.mPropertyValues for the case where keyframe is not specified.

Review commit: https://reviewboard.mozilla.org/r/105982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105982/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 11: The last keyframe's KeyframePercentage is 1.0.

Review commit: https://reviewboard.mozilla.org/r/105984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105984/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 12: Fill Gecko's Keyframe.

Review commit: https://reviewboard.mozilla.org/r/105986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105986/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

Review commit: https://reviewboard.mozilla.org/r/105988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105988/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I did not include generated binding stuff (structs_debug.rs and structs_release.rs) in this patch set, because, on my environment it results something weird (it causes an error without adding mozilla::binding_danger::AssertAndSuppressCleanupPolicy and mozilla::binding_danger::TErrorResult in whitelist.).

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

https://reviewboard.mozilla.org/r/105988/#review106954

::: layout/base/ServoRestyleManager.cpp:44
(Diff revision 1)
> -  // We allow posting change hints during restyling, but not restyle hints
> -  // themselves, since those would require us to re-traverse the tree.
> -  MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0);
> +  // We allow posting change hints without any restyles hints during restyling,
> +  // since those would require us to re-traverse the tree, and allow animation
> +  // only restyles without any change hints.
> +  MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0 ||
> +                                 (aRestyleHint == eRestyle_CSSAnimations &&
> +                                  !aMinChangeHint));

Hmm, so the problem here is that any restyle hints that are generated from UpdateAnimations() won't get handled, and actually we can't really handle them properly under ServoStyleSet::RecreateStyleContexts, since we have already restyled the entire tree, and we're just creating nsStyleContexts.  The assertion here was checking that we only generate change hints, since they don't require a re-traversal of the tree, which handling a restyle hint would.

I don't recall what the plan was to handle this.  The worst case would be to just do another whole tree traversal later to handle the eRestyle_CSSAnimations restyle hint, potentially wasting a lot of restyle work done on descendants of that element.  Did we have a plan we discussed in Hawaii, where we would do something like: stop the Servo restyle at an element, do the animation updating back in ServoStyleSet::RecreateStyleContexts, then return to Servo to restyle the element and its descendants?  Or am I misremembering a different conversation?

::: layout/style/ServoStyleSet.cpp:144
(Diff revision 1)
> +  // Update/build CSS animations in the case where animation properties are
> +  // changed.
> +  mPresContext->AnimationManager()->UpdateAnimations(result, element);

I think this should be in the other GetContext one, since that's the deepest GetContext call (and matches where we do it in nsStyleSet.)

Some questions about this:

1. nsStyleSet::GetContext checks |PresContext()->IsDynamic() && aElementForAnimation->IsInComposedDoc()| to see if it should skip animation rule updating.  Should we do that here too?

2. Can you add some comments in here saying that we need to return a new style context in case the UpdateAnimations call cause changes to animations?  I assume we'll do that work in a later bug.

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8828664 [details]
Bug 1328787 - Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h.

https://reviewboard.mozilla.org/r/105964/#review106956

::: dom/animation/Keyframe.h:1
(Diff revision 1)
> +#include "nsCSSPropertyID.h"

Please add the license / modeline boilerplate and #ifdef guards to this file.

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> I did not include generated binding stuff (structs_debug.rs and
> structs_release.rs) in this patch set, because, on my environment it results
> something weird (it causes an error without adding
> mozilla::binding_danger::AssertAndSuppressCleanupPolicy and
> mozilla::binding_danger::TErrorResult in whitelist.).

Hmm, I thought I fixed that.  I'll look at it in the next mozilla-central -> incubator/stylo merge.

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8828665 [details]
Bug 1328787 - Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build.

https://reviewboard.mozilla.org/r/105966/#review106960

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8828668 [details]
Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName.

https://reviewboard.mozilla.org/r/105972/#review106964

::: servo/ports/geckolib/glue.rs:1111
(Diff revision 1)
> +pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSetBorrowed,
> +                                                      name: *const nsACString,
> +                                                      style: ServoComputedValuesBorrowed,
> +                                                      keyframes: RawGeckoKeyframeListBorrowedMut) -> bool {
> +    let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
> +    let device = data.stylist.device.clone();

Do you need to clone() the Arc<Device> here?  Probably |let device = data.stylist.device.as_ref();| is sufficient.

::: servo/ports/geckolib/glue.rs:1115
(Diff revision 1)
> +    let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
> +    let device = data.stylist.device.clone();
> +    let name = unsafe { name.as_ref().unwrap().as_str_unchecked() };
> +    let style = ComputedValues::as_arc(&style);
> +
> +    let timing_function = style.get_box().animation_timing_function_mod(0);

I don't understand why we just look at the first animation-timing-function value.  Don't we want to look at the index the the |name| was found at in animation-name?

::: servo/ports/geckolib/glue.rs:1118
(Diff revision 1)
> +    for sheet in data.stylesheets.iter() {
> +        sheet.effective_keyframes_rules(&device, |ref keyframes_rule| {

data.stylesheets is stored in cascade order, with UA sheets coming first and document sheets coming last.  So, I think we should iterate over data.stylesheets in reverse order, and then quit as soon as we find a matching keyframes rule.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8828666 [details]
Bug 1328787 - Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>.

https://reviewboard.mozilla.org/r/105968/#review106962

r=me but this maybe needs to change if my comments about the animation-timing-function on the part 5 patch are right.

::: layout/style/ServoStyleSet.cpp:562
(Diff revision 1)
> +
> +

Nit: probably don't need two blank lines here.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8828667 [details]
Bug 1328787 - Part 4: Add Gecko_AnimationAppendKeyframe.

https://reviewboard.mozilla.org/r/105970/#review106970

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8828669 [details]
Bug 1328787 - Part 6: Update build_gecko.rs and bindings.

https://reviewboard.mozilla.org/r/105974/#review106972

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8828670 [details]
Bug 1328787 - Part 7: Implement From for specified TransitionTimingFunction.

https://reviewboard.mozilla.org/r/105976/#review106974

::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:15
(Diff revision 1)
> +use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
>  use std::mem;
>  
> -impl From<TransitionTimingFunction> for nsTimingFunction {
> -    fn from(function: TransitionTimingFunction) -> nsTimingFunction {
> -        let mut tf: nsTimingFunction = unsafe { mem::zeroed() };
> +impl nsTimingFunction {
> +    fn set_as_step(&mut self, function_type: nsTimingFunction_Type, steps: u32) {
> +        self.mType = function_type;

Please debug_assert!() that function_type is one of the step types.

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8828671 [details]
Bug 1328787 - Part 8: Animation timing function can be overridden by animation-timing-function specified in keyframe.

https://reviewboard.mozilla.org/r/105978/#review106976

::: servo/components/style/keyframes.rs:204
(Diff revision 1)
>              value: value,
>              declared_timing_function: declared_timing_function,
>          }
>      }
> +
> +    /// Return specified TransitionTimingFunction if this KeyframesSteps has 'animation-timing-funtion'.

s/funtion/function/

::: servo/components/style/keyframes.rs:218
(Diff revision 1)
> +                                // Use the first value.
> +                                Some(value.0[0])

Again, why the first value?

::: servo/components/style/keyframes.rs:221
(Diff revision 1)
> +                        match *value {
> +                            DeclaredValue::Value(ref value) => {
> +                                // Use the first value.
> +                                Some(value.0[0])
> +                            },
> +                            _ => None

Nit: rust style is to not leave off a comma from the last item.

::: servo/components/style/keyframes.rs:228
(Diff revision 1)
> +                    },
> +                    _ => panic!()
> +                }
> +            },
> +            KeyframesStepValue::ComputedValues => {
> +                panic!("Shouldn't happen to set aniamtion-timing-function in missing keyframes")

s/aniamtion/animation/

::: servo/ports/geckolib/glue.rs:1128
(Diff revision 1)
> +                        let mut timing_function = style_timing_function;
> +                        if let Some(val) = step.get_animation_timing_function() {
> +                            timing_function = val
> +                        }
> +
>                          let tf = timing_function.into();

This is fine, but I think it might look more natural as:

let timing_function = step.get_animation_timing_function().unwrap_or(style_timing_function);

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8828672 [details]
Bug 1328787 - Part 9: Set Keyframe.mPropertyValues for the case where keyframe is specified.

https://reviewboard.mozilla.org/r/105980/#review106980

::: servo/components/style/properties/helpers/animated_properties.mako.rs:110
(Diff revision 1)
> +        <%
> +            def to_nscsspropertyid(ident):
> +                if ident == "word_wrap":
> +                    return "nsCSSPropertyID_eCSSPropertyAlias_WordWrap"
> +
> +                if ident == "float":
> +                    ident = "float_"
> +                return "nsCSSPropertyID::eCSSProperty_" + ident
> +        %>

Hmm, can we just use the similar functions from properties.mako.rs, and put them somewhere we can call them from here?

::: servo/ports/geckolib/glue.rs:1156
(Diff revision 1)
> +                                         .filter(|&&(ref declaration, _)| {
> +                                             declaration.is_animatable()
> +                                         });
> +                                for (index, &(ref declaration, _)) in animatable.enumerate() {
> +                                    unsafe {
> +                                      (*keyframe).mPropertyValues.set_len((index+1) as u32);

Nit: spaces around "+".

Comment 27

[Cameron McCormack (:heycam)]

Comment on attachment 8828673 [details]
Bug 1328787 - Part 10: Set Keyframe.mPropertyValues for the case where keyframe is not specified.

https://reviewboard.mozilla.org/r/105982/#review106984

::: servo/ports/geckolib/glue.rs:1146
(Diff revision 1)
>                          match step.value {
>                              KeyframesStepValue::ComputedValues => {
> -                                unimplemented!();
> +                                for (index, property) in animation.properties_changed.iter().enumerate() {
> +                                   let block = style.to_declaration_block(property.clone().into());
> +                                   unsafe {
> +                                      (*keyframe).mPropertyValues.set_len((index+1) as u32);

Nit: spaces around +.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8828674 [details]
Bug 1328787 - Part 11: The last keyframe's KeyframePercentage is 1.0.

https://reviewboard.mozilla.org/r/105984/#review106986

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8828675 [details]
Bug 1328787 - Part 12: Fill Gecko's Keyframe.

https://reviewboard.mozilla.org/r/105986/#review106988

::: layout/style/nsAnimationManager.cpp:606
(Diff revision 1)
> +    ServoStyleSet* styleSet = aPresContext->StyleSet()->GetAsServo();
> +    MOZ_ASSERT(styleSet);

Just do aPresContext->StyleSet()->AsServo()->... below.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Thanks for the review!

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #16)
> ::: layout/base/ServoRestyleManager.cpp:44
> (Diff revision 1)
> > -  // We allow posting change hints during restyling, but not restyle hints
> > -  // themselves, since those would require us to re-traverse the tree.
> > -  MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0);
> > +  // We allow posting change hints without any restyles hints during restyling,
> > +  // since those would require us to re-traverse the tree, and allow animation
> > +  // only restyles without any change hints.
> > +  MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0 ||
> > +                                 (aRestyleHint == eRestyle_CSSAnimations &&
> > +                                  !aMinChangeHint));
> 
> Hmm, so the problem here is that any restyle hints that are generated from
> UpdateAnimations() won't get handled, and actually we can't really handle
> them properly under ServoStyleSet::RecreateStyleContexts, since we have
> already restyled the entire tree, and we're just creating nsStyleContexts. 
> The assertion here was checking that we only generate change hints, since
> they don't require a re-traversal of the tree, which handling a restyle hint
> would.

Oh, I was totally misunderstanding stylo's styling mechanism.

> I don't recall what the plan was to handle this.  The worst case would be to
> just do another whole tree traversal later to handle the
> eRestyle_CSSAnimations restyle hint, potentially wasting a lot of restyle
> work done on descendants of that element.  Did we have a plan we discussed
> in Hawaii, where we would do something like: stop the Servo restyle at an
> element, do the animation updating back in
> ServoStyleSet::RecreateStyleContexts, then return to Servo to restyle the
> element and its descendants?  Or am I misremembering a different
> conversation?

As far as I remember, there was nothing about CSS animations.

> ::: layout/style/ServoStyleSet.cpp:144
> (Diff revision 1)
> > +  // Update/build CSS animations in the case where animation properties are
> > +  // changed.
> > +  mPresContext->AnimationManager()->UpdateAnimations(result, element);
> 
> I think this should be in the other GetContext one, since that's the deepest
> GetContext call (and matches where we do it in nsStyleSet.)

Thanks. Will do.

> Some questions about this:
> 
> 1. nsStyleSet::GetContext checks |PresContext()->IsDynamic() &&
> aElementForAnimation->IsInComposedDoc()| to see if it should skip animation
> rule updating.  Should we do that here too?

Yes, I will do it.

> 2. Can you add some comments in here saying that we need to return a new
> style context in case the UpdateAnimations call cause changes to animations?
> I assume we'll do that work in a later bug.

Thanks, now I understood what we should do here.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

https://reviewboard.mozilla.org/r/105988/#review106954

> Hmm, so the problem here is that any restyle hints that are generated from UpdateAnimations() won't get handled, and actually we can't really handle them properly under ServoStyleSet::RecreateStyleContexts, since we have already restyled the entire tree, and we're just creating nsStyleContexts.  The assertion here was checking that we only generate change hints, since they don't require a re-traversal of the tree, which handling a restyle hint would.
> 
> I don't recall what the plan was to handle this.  The worst case would be to just do another whole tree traversal later to handle the eRestyle_CSSAnimations restyle hint, potentially wasting a lot of restyle work done on descendants of that element.  Did we have a plan we discussed in Hawaii, where we would do something like: stop the Servo restyle at an element, do the animation updating back in ServoStyleSet::RecreateStyleContexts, then return to Servo to restyle the element and its descendants?  Or am I misremembering a different conversation?

It turns out that the eRestyle_CSSAnimations is caused by a bug 1332958 that now I filed.  I will fix that bug first.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828664 [details]
Bug 1328787 - Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105964/diff/1-2/

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828665 [details]
Bug 1328787 - Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105966/diff/1-2/

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828666 [details]
Bug 1328787 - Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105968/diff/1-2/

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828667 [details]
Bug 1328787 - Part 4: Add Gecko_AnimationAppendKeyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105970/diff/1-2/

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828668 [details]
Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105972/diff/1-2/

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828669 [details]
Bug 1328787 - Part 6: Update build_gecko.rs and bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105974/diff/1-2/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828670 [details]
Bug 1328787 - Part 7: Implement From for specified TransitionTimingFunction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105976/diff/1-2/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828671 [details]
Bug 1328787 - Part 8: Animation timing function can be overridden by animation-timing-function specified in keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105978/diff/1-2/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828672 [details]
Bug 1328787 - Part 9: Set Keyframe.mPropertyValues for the case where keyframe is specified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105980/diff/1-2/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828673 [details]
Bug 1328787 - Part 10: Set Keyframe.mPropertyValues for the case where keyframe is not specified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105982/diff/1-2/

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828674 [details]
Bug 1328787 - Part 11: The last keyframe's KeyframePercentage is 1.0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105984/diff/1-2/

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828675 [details]
Bug 1328787 - Part 12: Fill Gecko's Keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105986/diff/1-2/

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105988/diff/1-2/

Comment 45

[Hiroyuki Ikezoe (:hiro)]

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #20)
> ::: servo/ports/geckolib/glue.rs:1115
> (Diff revision 1)
> > +    let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
> > +    let device = data.stylist.device.clone();
> > +    let name = unsafe { name.as_ref().unwrap().as_str_unchecked() };
> > +    let style = ComputedValues::as_arc(&style);
> > +
> > +    let timing_function = style.get_box().animation_timing_function_mod(0);
> 
> I don't understand why we just look at the first animation-timing-function
> value.  Don't we want to look at the index the the |name| was found at in
> animation-name?

Thanks, yes, right. Fixed.
To use the correct timing function (that matches to the index of the |name|), I did pass const* nsTimingFunction to Servo_StyleSet_FillKeyframesForName.  We can't get the correct timing function with the index in servo side, since there is a case that we have the same animation name but different timing functions.  E.g. style.animation = "anim1 ease; anim1 linear";

Whereas in case of animation-timing-function in @keyframes:

> ::: servo/components/style/keyframes.rs:218
> (Diff revision 1)
> > +                                // Use the first value.
> > +                                Some(value.0[0])
> 
> Again, why the first value?

This is intentional, because we use the first function in Gecko [1].
[1] https://hg.mozilla.org/mozilla-central/file/bd0cd9af94d9/layout/style/nsAnimationManager.cpp#l854

Comment 46

[Hiroyuki Ikezoe (:hiro)]

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #16)
> 2. Can you add some comments in here saying that we need to return a new
> style context in case the UpdateAnimations call cause changes to animations?
> I assume we'll do that work in a later bug.

If I understand correctly what you mean here, I think we don't need this comment if there is no extra eRestyle_CSSAnimations once bug 1332958 is fixed.  Right?  Let me know if I am still misunderstanding. Thanks!

Comment 47

[Cameron McCormack (:heycam)]

Comment on attachment 8828668 [details]
Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName.

https://reviewboard.mozilla.org/r/105972/#review107352

r=me with that fixed.

::: servo/ports/geckolib/glue.rs:1119
(Diff revision 2)
> +    let device = data.stylist.device.as_ref();
> +    let name = unsafe { name.as_ref().unwrap().as_str_unchecked() };
> +    let style_timing_function = unsafe { timing_function.as_ref().unwrap() };
> +
> +    fn find_animation(rules: &[CssRule], device: &Device, name: &str) -> Option<KeyframesAnimation> {
> +        for rule in rules {

Don't we need to go through |rules| in reverse too?

Comment 48

[Cameron McCormack (:heycam)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

https://reviewboard.mozilla.org/r/105988/#review107368

::: layout/style/ServoStyleSet.cpp:167
(Diff revision 2)
> +    mPresContext->AnimationManager()->UpdateAnimations(result,
> +                                                       aElementForAnimation);

Please add a comment in here saying that this probably isn't the right place to be calling UpdateAnimations, due to the issues I mentioned in the previous review (i.e., this can post restyle hints, which we can't handle during ServoRestyleManager::RecreateStyleContetxs).

Actually, why do we no longer hit the assertion about restyle hints being generated during RecreateStyleContexts?

::: layout/style/ServoStyleSet.cpp:246
(Diff revision 2)
> -  return GetContext(computedValues.forget(), aParentContext, pseudoTag, aType);
> +  bool isBeforeOrAfter = (aType == CSSPseudoElementType::before) ||
> +                         (aType == CSSPseudoElementType::after);

Nit: don't need the parens around these.

::: layout/style/ServoStyleSet.cpp:484
(Diff revision 2)
> -  if (computedValues &&
> -      (pseudoTag == nsCSSPseudoElements::before ||
> +  bool isBeforeOrAfter = (pseudoTag == nsCSSPseudoElements::before ||
> +                          pseudoTag == nsCSSPseudoElements::after);

Nit: here too.

Comment 49

[Cameron McCormack (:heycam)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

https://reviewboard.mozilla.org/r/105988/#review107368

> Please add a comment in here saying that this probably isn't the right place to be calling UpdateAnimations, due to the issues I mentioned in the previous review (i.e., this can post restyle hints, which we can't handle during ServoRestyleManager::RecreateStyleContetxs).
> 
> Actually, why do we no longer hit the assertion about restyle hints being generated during RecreateStyleContexts?

I see your comment about bug 1332958.  Does that mean we will never generate eRestyle_CSSAnimations during the RecreateStyleContexts call?

Comment 50

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #49)
> Comment on attachment 8828676 [details]
> Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().
> 
> https://reviewboard.mozilla.org/r/105988/#review107368
> 
> > Please add a comment in here saying that this probably isn't the right place to be calling UpdateAnimations, due to the issues I mentioned in the previous review (i.e., this can post restyle hints, which we can't handle during ServoRestyleManager::RecreateStyleContetxs).
> > 
> > Actually, why do we no longer hit the assertion about restyle hints being generated during RecreateStyleContexts?
> 
> I see your comment about bug 1332958.  Does that mean we will never generate
> eRestyle_CSSAnimations during the RecreateStyleContexts call?

I was wrong. We still see another eRestyle_CSSAnimations during the RecreateStyleContexts call.  I need to figure out a machinery to process the eRestyle_CSSAnimations.

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828664 [details]
Bug 1328787 - Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105964/diff/2-3/

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828665 [details]
Bug 1328787 - Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105966/diff/2-3/

Comment 53

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828666 [details]
Bug 1328787 - Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105968/diff/2-3/

Comment 54

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828667 [details]
Bug 1328787 - Part 4: Add Gecko_AnimationAppendKeyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105970/diff/2-3/

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828668 [details]
Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105972/diff/2-3/

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828669 [details]
Bug 1328787 - Part 6: Update build_gecko.rs and bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105974/diff/2-3/

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828670 [details]
Bug 1328787 - Part 7: Implement From for specified TransitionTimingFunction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105976/diff/2-3/

Comment 58

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828671 [details]
Bug 1328787 - Part 8: Animation timing function can be overridden by animation-timing-function specified in keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105978/diff/2-3/

Comment 59

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828672 [details]
Bug 1328787 - Part 9: Set Keyframe.mPropertyValues for the case where keyframe is specified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105980/diff/2-3/

Comment 60

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828673 [details]
Bug 1328787 - Part 10: Set Keyframe.mPropertyValues for the case where keyframe is not specified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105982/diff/2-3/

Comment 61

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828674 [details]
Bug 1328787 - Part 11: The last keyframe's KeyframePercentage is 1.0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105984/diff/2-3/

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828675 [details]
Bug 1328787 - Part 12: Fill Gecko's Keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105986/diff/2-3/

Comment 63

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105988/diff/2-3/

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8828668 [details]
Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName.

https://reviewboard.mozilla.org/r/105972/#review108808

::: servo/ports/geckolib/glue.rs:1155
(Diff revision 3)
> +    if let Some(ref animation) = data.stylist.animations().get(&name) {
> +       for step in &animation.steps {
> +          let timing_function = *style_timing_function;

As Cameron taught me, I did use stylist.animations() here instead of traversing stylesheets.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #48)
> ::: layout/style/ServoStyleSet.cpp:167
> (Diff revision 2)
> > +    mPresContext->AnimationManager()->UpdateAnimations(result,
> > +                                                       aElementForAnimation);
> 
> Please add a comment in here saying that this probably isn't the right place
> to be calling UpdateAnimations, due to the issues I mentioned in the
> previous review (i.e., this can post restyle hints, which we can't handle
> during ServoRestyleManager::RecreateStyleContetxs).

After some more thought, we can't avoid the recursive eRestyle_CSSAnimations for now. So, I just skipped it in ServoRestyleManager::PostRestyleEvent(). Because, I think cascade_node() in servo side (or other functions around this function) is a properer place to update css animations, but to do that we need to make UpdateAnimations (and relevant functions) independent from nsStyleContext and also make those processes thread safe.  I am guessing that we should write equivalent code in servo side instead of refactoring those processes.

Comment 66

[Cameron McCormack (:heycam)]

Comment on attachment 8828676 [details]
Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations().

https://reviewboard.mozilla.org/r/105988/#review108822

Comment 67

[Hiroyuki Ikezoe (:hiro)]

Thank you Cameron for reviewing!

Try on mozilla-central:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3944c15d48d67a0240789e24a6dc5071a447c2

Try on stylo:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b01e102ec6ecaa111588ec0f58f4e5ecd65d45f&exclusion_profile=false
Crashtest is orange but all of failures are not related to these changes.  For example there are two TEST-UNEXPECTED-PASSed tests in dom/animation but they have been already passed (See bug 1324691 comment 14).

For comparison here is another try on stylo without these patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a922db3616844c55569b08e60ce6a870dbf8b2e3

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a6ffc60945
Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6330abee716
Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/70432998700d
Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2162612b5857
Part 4: Add Gecko_AnimationAppendKeyframe. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/224a64d93324
Part 12: Fill Gecko's Keyframe. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe14ee4e5a8
Part 13: Call nsAnimationManager.UpdateAnimations(). r=heycam

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Servo PR:
https://github.com/servo/servo/pull/15287

Comment 70

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/31a6ffc60945
https://hg.mozilla.org/mozilla-central/rev/f6330abee716
https://hg.mozilla.org/mozilla-central/rev/70432998700d
https://hg.mozilla.org/mozilla-central/rev/2162612b5857
https://hg.mozilla.org/mozilla-central/rev/224a64d93324
https://hg.mozilla.org/mozilla-central/rev/2fe14ee4e5a8