Closed Bug 1354947 Opened 7 years ago Closed 7 years ago

stylo: merge keyframe values at the same offset

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 5 obsolete files)

Bug 1354947 - Rename FillKeyframesForName to GetKeyframesForName.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1354947 - Add equal operators for comparing ComputedTimingFunction with nsTimingFunction.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1354947 - Expose FindMatchingKeyframe and make it reusable for nsTimingFunction.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1354947 - Fill in missing keyframe values.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1354947 - Drop Gecko_AnimationAppendKeyframe.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1354947 - Update mochitest expectations for @keyframes merging.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
We should do the same thing what we do in Gecko [1] .

Some tests in test_animations.html fail because of this.

[1] https://hg.mozilla.org/mozilla-central/file/45692c884fdd/layout/style/nsAnimationManager.cpp#l640
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
With these patches, there are still 5 failures in css-animations/test_keyframeeffect-getkeyframes.html.

1. filter property test [1]; we don't implement filter animation yet.
2. text-shadow property [2]; the serialization of text-shadow on gecko is something broken? '1px 1px 2px 0px rgb(0, 0, 0)' on gecko, '1px 1px 2px rgb(0, 0, 0)' on stylo.
3. after updating current style expected [3];changing the base style does not reflect getKeyframes()?
4. css variables in keyframes [4]; translate(var(--var-100px), 0) on stylo, whereas translate(100px, 0px) on gecko
5. css variables in shorthand [5]; we don't support it yet?

[1] https://hg.mozilla.org/mozilla-central/file/5278e2a35fc8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#l545
[2] https://hg.mozilla.org/mozilla-central/file/5278e2a35fc8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#l570
[3] https://hg.mozilla.org/mozilla-central/file/5278e2a35fc8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#l620
[4] https://hg.mozilla.org/mozilla-central/file/5278e2a35fc8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#l631
[5] https://hg.mozilla.org/mozilla-central/file/5278e2a35fc8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#l649
Comment on attachment 8863269 [details]
Bug 1354947 - Rename FillKeyframesForName to GetKeyframesForName.

https://reviewboard.mozilla.org/r/135056/#review137964
Attachment #8863269 - Flags: review?(bbirtles) → review+
Comment on attachment 8863270 [details]
Bug 1354947 - Use match instead of 'if let'.

https://reviewboard.mozilla.org/r/135058/#review137966
Attachment #8863270 - Flags: review?(bbirtles) → review+
Comment on attachment 8863272 [details]
Bug 1354947 - Add equal operators for comparing ComputedTimingFunction with nsTimingFunction.

https://reviewboard.mozilla.org/r/135062/#review137968

::: dom/animation/ComputedTimingFunction.h:85
(Diff revision 1)
> +    return mType == aOther.mType &&
> +           (HasSpline() ?
> +            mTimingFunction.X1() == aOther.mFunc.mX1 &&
> +            mTimingFunction.Y1() == aOther.mFunc.mY1 &&
> +            mTimingFunction.X2() == aOther.mFunc.mX2 &&
> +            mTimingFunction.Y2() == aOther.mFunc.mY2 :
> +            mStepsOrFrames == aOther.mStepsOrFrames);

Nit: I think our coding style wants the '?' and ':' at the beginning of the line.[1]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

::: dom/animation/ComputedTimingFunction.h:127
(Diff revision 1)
> +inline bool
> +operator==(const Maybe<ComputedTimingFunction>& aLHS,
> +           const nsTimingFunction& aRHS)
> +{
> +  if (aLHS.isNothing()) {
> +    return (aRHS.mType == nsTimingFunction::Type::Linear);

Nit: I don't think the () are needed here?
Attachment #8863272 - Flags: review?(bbirtles) → review+
Comment on attachment 8863273 [details]
Bug 1354947 - Expose FindMatchingKeyframe and make it reusable for nsTimingFunction.

https://reviewboard.mozilla.org/r/135064/#review137986
Attachment #8863273 - Flags: review?(bbirtles) → review+
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review137988

I'm probably missing some context here but either this is not right, or it's right but needs some more explanation about why we take a sorted array of keyframes and then don't do a sorted insert.

::: layout/style/ServoBindings.h:364
(Diff revision 1)
>  mozilla::Keyframe* Gecko_AnimationAppendKeyframe(RawGeckoKeyframeListBorrowedMut keyframes,
>                                                   float offset,
>                                                   const nsTimingFunction* timingFunction);
> +// Returns existing Keyframe in |keyframes| if there is a Keyframe which has
> +// the same |offset| and the same |timingFunction|.
> +// Otherwise returns a new Keyframe inserted at the top of |keyframes|.

See comment below, but I don't understand why we want to put the new keyframe at the start of |keyframes|. That would make it impossible to call this method twice with the same |keyframes| argument.

::: layout/style/ServoBindings.cpp:1474
(Diff revision 1)
> +                                   const nsTimingFunction* aTimingFunction)
> +{
> +  MOZ_ASSERT(aKeyframes, "The keyframe array should be valid");
> +  MOZ_ASSERT(aTimingFunction, "The timing function should be valid");
> +
> +  nsTArray<Keyframe>& keyframes = static_cast<nsTArray<Keyframe>&>(*aKeyframes);

So as I understand it, RawGeckoKeyframeListBorrowedMut is just nsTArray<mozilla::Keyframe>* ?

But we are doing this cast to reference here so we have the right type to pass to FindMatchingKeyframe, right?

Does FindMatchingKeyframe(&*keyframes, ...) alone, not work?

This seems pretty confusing, but if there's no more elegant way of writing this, then we should at least add a comment saying why we're doing this.

::: layout/style/ServoBindings.cpp:1483
(Diff revision 1)
> +                                               *aTimingFunction,
> +                                               keyframeIndex)) {
> +    return &keyframes[keyframeIndex];
> +  }
> +
> +  Keyframe* keyframe = keyframes.InsertElementAt(0);

Shouldn't this be inserting the element at |keyframeIndex| ?

This method assumes |aKeyframes| is sorted but it returns a result where |aKeyframes| might not be sorted.
Attachment #8863274 - Flags: review?(bbirtles)
Comment on attachment 8863275 [details]
Bug 1354947 - Split off add_computed_property_value().

https://reviewboard.mozilla.org/r/135068/#review137990
Attachment #8863275 - Flags: review?(bbirtles) → review+
Comment on attachment 8863277 [details]
Bug 1354947 - Drop Gecko_AnimationAppendKeyframe.

https://reviewboard.mozilla.org/r/135072/#review137992
Attachment #8863277 - Flags: review?(bbirtles) → review+
Comment on attachment 8863278 [details]
Bug 1354947 - Update mochitest expectations for @keyframes merging.

https://reviewboard.mozilla.org/r/135074/#review137994
Attachment #8863278 - Flags: review?(bbirtles) → review+
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review137988

> So as I understand it, RawGeckoKeyframeListBorrowedMut is just nsTArray<mozilla::Keyframe>* ?
> 
> But we are doing this cast to reference here so we have the right type to pass to FindMatchingKeyframe, right?
> 
> Does FindMatchingKeyframe(&*keyframes, ...) alone, not work?
> 
> This seems pretty confusing, but if there's no more elegant way of writing this, then we should at least add a comment saying why we're doing this.

Yes, RawGeckoKeyframeListBorrowedMut is just nsTArray<Keyframe>*. 
Unfortunately FindMatchingKeyframe(&*keyframes, ...) does not work.
How about changing the argument type to nsTArray<Keyframe*> aKeyframes? Then we don't need the cast at all.

> Shouldn't this be inserting the element at |keyframeIndex| ?
> 
> This method assumes |aKeyframes| is sorted but it returns a result where |aKeyframes| might not be sorted.

Urgh, the case condition was the other way around. (I mistook while I was introducing KeyframeSearchDirection type) It should be

switch (aSearchDirection) {
  case Forward:
    if (FindMatchingKeyframe(keyframes, ..) {
      return keyframes[keyframeIndex];
    }
    break;
  case Backward:
    if (FindMatchingKeyframe(Reversed(keyframes), ..) {
      return ..
    }
    break;
}
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review138002

::: servo/ports/geckolib/glue.rs:2122
(Diff revision 1)
>          };
>  
> +        // Get the existing keyframe which is the same offset and of the same
> +        // timing function or new one inserted in keyframe array.
>          let keyframe = unsafe {
> -            Gecko_AnimationAppendKeyframe(keyframes,
> +            Gecko_AnimationGetOrInsertKeyframe(keyframes,

(In reply to Brian Birtles (:birtles) from comment #17)

> ::: layout/style/ServoBindings.cpp:1483
> (Diff revision 1)
> > +                                               *aTimingFunction,
> > +                                               keyframeIndex)) {
> > +    return &keyframes[keyframeIndex];
> > +  }
> > +
> > +  Keyframe* keyframe = keyframes.InsertElementAt(0);
> 
> Shouldn't this be inserting the element at |keyframeIndex| ?
> 
> This method assumes |aKeyframes| is sorted but it returns a result where
> |aKeyframes| might not be sorted.

In my understandings, even if we don't use keyframeIndex to insert an element, the keyframe array is still sorted by *offset*. If we need to sort the array by *offset* and *timing function*, we need to use keyframeIndex as well. Should we?

The reason why we insert the new element at the top of the keyframe array is that this function is also used for getting existing keyframe here while iterating over keyframes array.  Actually we can use keyframeIndex for this case as well, but if we do so, a test case [1] in test_keyframeeffect-getkeyframes.html will fail.  We will get padding-left  and margin-top frame first.  It seems to me that the order in the test case is not so important but I did adopt expected value as gecko one.

[1] https://hg.mozilla.org/mozilla-central/file/5278e2a35fc8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#l472
I did leave the index issue for now.
Comment on attachment 8863276 [details]
Bug 1354947 - Fill in missing keyframe values.

https://reviewboard.mozilla.org/r/135070/#review138000

I'd like to look at this after the changes to the FFI API.

::: commit-message-f9e64:3
(Diff revision 1)
> +Bug 1354947 - Fill in missing keyframe values. r?birtles
> +
> +This is mostly a mimic of what we do FillInMissingKeyframeValues().

We should probably mention that this is a method used in Gecko (and in which class).

::: commit-message-f9e64:4
(Diff revision 1)
> +A different point is;
> + We handle missing keyframes (i.e. no specified keyframe at offset 0 or 1)
> + separately since servo already adds those keyframes in
> + KeyframesAnimation.from_keyframes()

This comment isn't clear to me. If servo already adds the keyframes, why do we need to handle them at all?

::: layout/style/ServoBindings.h:362
(Diff revision 1)
>  // Returns existing Keyframe in |keyframes| if there is a Keyframe which has
>  // the same |offset| and the same |timingFunction|.
> -// Otherwise returns a new Keyframe inserted at the top of |keyframes|.
> +// If |aSearchDirection| is `Backward`, this function tries to find the existing
> +// keyframe from the last keyframe.
> +// Otherwise returns a new Keyframe. The new keyframe is inserted at the top of
> +// |keyframes| if |aSearchDirection| is `Forward`, is appended the the last of
> +// |keyframes| if |aSearchDirection| is `Backward`.
>  mozilla::Keyframe* Gecko_AnimationGetOrInsertKeyframe(
>    RawGeckoKeyframeListBorrowedMut keyframes,
>    float offset,
> -  const nsTimingFunction* timingFunction);
> +  const nsTimingFunction* timingFunction,
> +  mozilla::KeyframeSearchDirection aSearchDirection);

I think it's confusing to have a method that requires |keyframes| to be sorted, but doesn't ensure that the result is also sorted. Instead, I think we should split this into two methods:

Gecko_GetOrPrependInitialKeyframe()
 -- Searches from the start for a keyframe with the given offset and timing function. Asserts if it encounters an offset *less* than the specified offset. Prepends the keyframe if needed.

Gecko_GetOrAppendFinalKeyframe()
 -- Searches from the end for a keyframe with the given offset and timing function. Asserts if it encounters an offset *greater* than the specified offset. Appends the keyframe if needed.

Obviously most of the implementation of the two methods would be shared but I think that would be a lot more clear as an API as well as being more robust.

Alternatively, we could just do what GeckoCSSAnimationBuilder::FillInMissingKeyframeValues does and actually use the returned index and insert the new keyframe there. That would be more robust still and avoid relying on run-time assertions.

Furthermore, it will preserve the same ordering of these added keyframes. In Gecko, if we need to synthesize a 0% keyframe, it is the *last* of the 0% keyframes. If we synthesize a 100% keyframe it is the *last* of the 100% keyframes. With this code a synthesized 0% keyframe becomes the *first* 0% keyframe but a synthesized 100% keyframe becomes the *last* 100% keyframe. That seems a little inconsistent to me (and is observable through getKeyframes()).

::: layout/style/ServoBindings.cpp:1496
(Diff revision 1)
> -  Keyframe* keyframe = keyframes.InsertElementAt(0);
> +  Keyframe* keyframe = aSearchDirection == KeyframeSearchDirection::Forward
> +                     ? keyframes.InsertElementAt(0)
> +                     : keyframes.AppendElement();

Nit: The ? and : here should probably line up with the 'a' in 'aSearchDirection'

::: servo/ports/geckolib/glue.rs:2142
(Diff revision 1)
> +        }
> +    }
> +}
> +
>  #[no_mangle]
>  pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSetBorrowed,

(We should probably call this GetKeyframesForName.)

::: servo/ports/geckolib/glue.rs:2144
(Diff revision 1)
> +}
> +
>  #[no_mangle]
>  pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSetBorrowed,
>                                                        name: *const nsACString,
>                                                        timing_function: nsTimingFunctionBorrowed,

We should rename timing_function to inherited_timing_function.

::: servo/ports/geckolib/glue.rs:2146
(Diff revision 1)
>  #[no_mangle]
>  pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSetBorrowed,
>                                                        name: *const nsACString,
>                                                        timing_function: nsTimingFunctionBorrowed,
>                                                        style: ServoComputedValuesBorrowed,
>                                                        keyframes: RawGeckoKeyframeListBorrowedMut) -> bool {

Let's assert that |keyframes| is empty at the beginning of this function (since that's the only way our use of Gecko_AnimationGetOrInsertKeyframe makes any sense). Then again, if we make the changes I suggested above, then it probably won't matter if keyframes is non-empty provided it is sorted.

::: servo/ports/geckolib/glue.rs:2168
(Diff revision 1)
> +    let mut properties_set_at_end = LonghandIdSet::new();
> +    let mut no_specifed_keyframe_at_start = false;
> +    let mut no_specifed_keyframe_at_end = false;
>      let mut current_offset = -1.;
>  
>      // Walk backwards through the keyframes and drop overridden properties.

This comment doesn't seem right here.

Perhaps,

"Iterate over the keyframe rules backwards so we can drop overridden properties (since declarations in later rules override those in earlier ones)"

::: servo/ports/geckolib/glue.rs:2175
(Diff revision 1)
>          if step.start_percentage.0 != current_offset {
>              properties_set_at_current_offset.clear();
>              current_offset = step.start_percentage.0;
>          }
>  
>          // Override timing_function if the keyframe has animation-timing-function.

Nit: 'has an animation-timing-function'

::: servo/ports/geckolib/glue.rs:2241
(Diff revision 1)
> -                }
>              },
>          }
>      }
> +
> +    // Append missing property values in the initial or the finial keyframes.

final

::: servo/ports/geckolib/glue.rs:2242
(Diff revision 1)
> +    if !no_specifed_keyframe_at_start {
> +        fill_in_missing_keyframe_values(&animation.properties_changed,
> +                                        timing_function,
> +                                        style,
> +                                        &properties_set_at_start,
> +                                        0.,
> +                                        keyframes,
> +                                        &global_style_data.shared_lock);
> +    }
> +    if !no_specifed_keyframe_at_end {
> +        fill_in_missing_keyframe_values(&animation.properties_changed,
> +                                        timing_function,
> +                                        style,
> +                                        &properties_set_at_end,
> +                                        1.,
> +                                        keyframes,
> +                                        &global_style_data.shared_lock);
> +    }

I think I am probably missing some context from the first commit message, but I wonder why we don't just follow the pattern in GeckoCSSAnimationBuilder::FillInMissingKeyframeValues and handle both of these in the one loop to save looping through all the properties twice?
Attachment #8863276 - Flags: review?(bbirtles)
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review138004

Cancelling review since per comment 31 I think this FFI API needs work.
Attachment #8863274 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #31)

> I think I am probably missing some context from the first commit message,
> but I wonder why we don't just follow the pattern in
> GeckoCSSAnimationBuilder::FillInMissingKeyframeValues and handle both of
> these in the one loop to save looping through all the properties twice?

If we handle the two keyframes in the function,we need to call another FFI function to call add_computed_property_value () from C++ side.
Do you have any idea to avoid this another FFI? The FFI might need to convert LonghandIdSet to nsCSSPropertyIDSet or expose LonghandIdSet into FFI layer.

Or, should we add Gecko_AnimationGetOrInsertInitialAndFinalKeyframes which returns two keyframes pointer?
Comment on attachment 8863276 [details]
Bug 1354947 - Fill in missing keyframe values.

https://reviewboard.mozilla.org/r/135070/#review138000

> This comment isn't clear to me. If servo already adds the keyframes, why do we need to handle them at all?

for example,
@keyframes no-keyframe-at-offset-0 {
  to { color: 'red' }
}

For this keyframes servo KeyframesAnimation.from_keyframes() inserts at 0 offset keyframe, but

@keyframes missing-color-at-offset-0 {
  from { background-color: 'red' }
  to { color: 'red'; background-color: 'blue' }
}
The function does not insert 'color' at the offset 0.

> I think it's confusing to have a method that requires |keyframes| to be sorted, but doesn't ensure that the result is also sorted. Instead, I think we should split this into two methods:
> 
> Gecko_GetOrPrependInitialKeyframe()
>  -- Searches from the start for a keyframe with the given offset and timing function. Asserts if it encounters an offset *less* than the specified offset. Prepends the keyframe if needed.
> 
> Gecko_GetOrAppendFinalKeyframe()
>  -- Searches from the end for a keyframe with the given offset and timing function. Asserts if it encounters an offset *greater* than the specified offset. Appends the keyframe if needed.
> 
> Obviously most of the implementation of the two methods would be shared but I think that would be a lot more clear as an API as well as being more robust.
> 
> Alternatively, we could just do what GeckoCSSAnimationBuilder::FillInMissingKeyframeValues does and actually use the returned index and insert the new keyframe there. That would be more robust still and avoid relying on run-time assertions.
> 
> Furthermore, it will preserve the same ordering of these added keyframes. In Gecko, if we need to synthesize a 0% keyframe, it is the *last* of the 0% keyframes. If we synthesize a 100% keyframe it is the *last* of the 100% keyframes. With this code a synthesized 0% keyframe becomes the *first* 0% keyframe but a synthesized 100% keyframe becomes the *last* 100% keyframe. That seems a little inconsistent to me (and is observable through getKeyframes()).

OK, I will separate Gecko_GetOrPrependInitialKeyframe() and Gecko_GetOrPrependInitialKeyframe(), and try to match the position of the keyframes which has the inherited timing function.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> (In reply to Brian Birtles (:birtles) from comment #31)
> 
> > I think I am probably missing some context from the first commit message,
> > but I wonder why we don't just follow the pattern in
> > GeckoCSSAnimationBuilder::FillInMissingKeyframeValues and handle both of
> > these in the one loop to save looping through all the properties twice?
> 
> If we handle the two keyframes in the function,we need to call another FFI
> function to call add_computed_property_value () from C++ side.

Ok, so I think what this commit message wants to say is something like,

"In Gecko we iterate over the properties just once because we can take the index for both the synthesized start and end keyframe and easily look them up as needed. However, in this patch we synthesize the start and end keyframes separately and iterate over the properties twice because that's easier than getting two indices and then later calling another FFI to dereference each of them, and neater than getting back two pointers."

Does that sound right?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> Comment on attachment 8863276 [details]
> Bug 1354947 - Fill in missing keyframe values.
> 
> https://reviewboard.mozilla.org/r/135070/#review138000
> 
> > This comment isn't clear to me. If servo already adds the keyframes, why do we need to handle them at all?
> 
> for example,
> @keyframes no-keyframe-at-offset-0 {
>   to { color: 'red' }
> }
> 
> For this keyframes servo KeyframesAnimation.from_keyframes() inserts at 0
> offset keyframe, but
> 
> @keyframes missing-color-at-offset-0 {
>   from { background-color: 'red' }
>   to { color: 'red'; background-color: 'blue' }
> }
> The function does not insert 'color' at the offset 0.

Which function? How is it different to Gecko? Sorry, this commit message still isn't clear to me.
(In reply to Brian Birtles (:birtles) from comment #35)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> > (In reply to Brian Birtles (:birtles) from comment #31)
> > 
> > > I think I am probably missing some context from the first commit message,
> > > but I wonder why we don't just follow the pattern in
> > > GeckoCSSAnimationBuilder::FillInMissingKeyframeValues and handle both of
> > > these in the one loop to save looping through all the properties twice?
> > 
> > If we handle the two keyframes in the function,we need to call another FFI
> > function to call add_computed_property_value () from C++ side.
> 
> Ok, so I think what this commit message wants to say is something like,
> 
> "In Gecko we iterate over the properties just once because we can take the
> index for both the synthesized start and end keyframe and easily look them
> up as needed. However, in this patch we synthesize the start and end
> keyframes separately and iterate over the properties twice because that's
> easier than getting two indices and then later calling another FFI to
> dereference each of them, and neater than getting back two pointers."
> 
> Does that sound right?

Thanks! That's right.


> (In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> > Comment on attachment 8863276 [details]
> > Bug 1354947 - Fill in missing keyframe values.
> > 
> > https://reviewboard.mozilla.org/r/135070/#review138000
> > 
> > > This comment isn't clear to me. If servo already adds the keyframes, why do we need to handle them at all?
> > 
> > for example,
> > @keyframes no-keyframe-at-offset-0 {
> >   to { color: 'red' }
> > }
> > 
> > For this keyframes servo KeyframesAnimation.from_keyframes() inserts at 0
> > offset keyframe, but
> > 
> > @keyframes missing-color-at-offset-0 {
> >   from { background-color: 'red' }
> >   to { color: 'red'; background-color: 'blue' }
> > }
> > The function does not insert 'color' at the offset 0.
> 
> Which function? How is it different to Gecko? Sorry, this commit message
> still isn't clear to me.

I mean KeyframesAnimation.from_keyframes().  It would be better to fix from_keyframes() but unfortunately servo's PropertyDeclaration does not have an enum such as KeyframesStepValue::ComputedValue for now.
Comment on attachment 8863276 [details]
Bug 1354947 - Fill in missing keyframe values.

https://reviewboard.mozilla.org/r/135070/#review138000

> (We should probably call this GetKeyframesForName.)

I will rename this in a subsequent patch.
Thank for reviewing and useful comments. It looks much better now.
Note that I did name Gecko_AnimationGetOrInsertInitialKeyframe instead of Gecko_AnimationGetOrPrependInitialKeyframe because it may not prepend the new keyframe.
Comment on attachment 8863316 [details]
Bug 1354947 - Rename FillKeyframesForName to GetKeyframesForName.

https://reviewboard.mozilla.org/r/135108/#review138196
Attachment #8863316 - Flags: review?(bbirtles) → review+
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review138204

::: layout/style/ServoBindings.h:362
(Diff revision 3)
> +// Returns existing Keyframe in |keyframes| if there is a Keyframe which has
> +// the same |offset| and the same |timingFunction|.
> +// Otherwise returns a new Keyframe inserted at the top of |keyframes|.
> +mozilla::Keyframe* Gecko_AnimationGetOrPrependKeyframe(
> +  RawGeckoKeyframeListBorrowedMut keyframes,
> +  float offset,
> +  const nsTimingFunction* timingFunction);

This API still feels a little odd to me. Can we:

* Drop the "Animation" prefix
  (If we must use an Animation prefix, we should use "Animation_" or something
  that makes it obviously a prefix)
* Call it Gecko_GetOrCreateKeyframeAtStart
* Make the comment:

// Searches from the beginning of |keyframes| for a Keyframe object with the
// specified offset and timing function. If none, is found, a new Keyframe
// object with the specified |offset| and |timingFunction| will be appended to
// |keyframes|.
//
// @param keyframes  An array of Keyframe objects, sorted by offset.
//                   The first Keyframe in the array, if any, MUST have an
//                   offset greater than or equal to |offset|.
// @param offset  The offset to search for, or, if no suitable Keyframe is
//                found, the offset to use for the created Keyframe.
//                Must be a floating point number in the range [0.0, 1.0].
// @param timingFunction  The timing function to match, or, if no suitable
//                        Keyframe is found, to timing function to set on the
//                        created Keyframe.
//
// @returns  The matching or created Keyframe.

Then can we add the following checks to the implementation:

* |offset| is in the range [0.0, 1.0].
* The first offset in |keyframes| is >= |offset|.

For that last point, it's tempting to say we should return null in that case, but since we don't ever expect this to happen and dealing with a possible null pointer on the Servo wide might be awkward, perhaps we can just assert here.

::: servo/ports/geckolib/glue.rs:2119
(Diff revision 3)
> +        // Get the existing keyframe which is the same offset and of the same
> +        // timing function or new one prepended in keyframe array.

Look for an existing keyframe with the same offset and timing function or else add a new keyframe at the beginning of the keyframe array.

::: servo/ports/geckolib/glue.rs:2160
(Diff revision 3)
> -                let mut seen = LonghandIdSet::new();
> +                let len = unsafe { (*keyframe).mPropertyValues.len() };
> -
>                  for (index, &(ref declaration, _)) in animatable.enumerate() {
> +                    let property = TransitionProperty::from_declaration(declaration).unwrap();
> +                    if !properties_set_at_current_offset.has_transition_property_bit(&property) {
> +                        properties_set_at_current_offset.set_transition_property_bit(&property);
> +
> -                    unsafe {
> +                        unsafe {
> -                        let property = TransitionProperty::from_declaration(declaration).unwrap();
> +                            let property = TransitionProperty::from_declaration(declaration).unwrap();
> -                        (*keyframe).mPropertyValues.set_len((index + 1) as u32);
> -                        (*keyframe).mPropertyValues[index].mProperty = (&property).into();
> -                        (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
> +                            (*keyframe).mPropertyValues.set_len((index + len + 1) as u32);
> +                            (*keyframe).mPropertyValues[index + len].mProperty = (&property).into();
> +                            (*keyframe).mPropertyValues[index + len].mServoDeclarationBlock.set_arc_leaky(

I don't understand this part. If we skip some properties because they are already set, should we still be using the |index| as an offset into mPropertyValues?
Comment on attachment 8863276 [details]
Bug 1354947 - Fill in missing keyframe values.

https://reviewboard.mozilla.org/r/135070/#review138210

Sorry, I could use some help understanding this patch. Specifically,

* Why do we need a separate 'no_specifed_keyframe_at_end' as opposed to simply leaving 'properties_set_at_end' empty?
* If fill_in_missing_keyframe_values is only called when no_specifed_keyframe_at_{start,end} is true, why do we need Gecko_AnimationGetOrInsertInitialKeyframe? Should we just always insert a new keyframe and never return an existing one?
* My question below about how add_computed_property_value is supposed to work

Clearing review request for now since at very least this could use some more comments to explain how this works.

::: layout/style/ServoBindings.h:370
(Diff revision 3)
> +// Returns existing Keyframe in |keyframes| if there is a Keyframe at offset 0
> +// and having the same |timingFunction|.
> +// Otherwise returns a new Keyframe inserted at the last of existing keyframes
> +// at offset 0 (but having different timing-function).
> +mozilla::Keyframe* Gecko_AnimationGetOrInsertInitialKeyframe(
> +  RawGeckoKeyframeListBorrowedMut keyframes,
> +  const nsTimingFunction* timingFunction);
> +
> +// Returns existing Keyframe in |keyframes| if there is a Keyframe at offset 1
> +// and having the same |timingFunction|.
> +// Otherwise returns a new Keyframe appended at the last of |keyframes|.
> +mozilla::Keyframe* Gecko_AnimationGetOrAppendFinalKeyframe(
> +  RawGeckoKeyframeListBorrowedMut keyframes,
> +  const nsTimingFunction* timingFunction);

The comments for these two methods should probably be updated to include the same sort of information as I suggested for Gecko_GetOrCreateKeyframeAtStart (i.e. talk about the invariants surrounding keyframes, drop the 'Animation' part of the function name etc.)

::: layout/style/ServoBindings.cpp:1471
(Diff revision 3)
> +enum class KeyframeInsertPosition {
> +  ForceToPrepend,
> +  AtTheLastOfTheOffset,
> +};

How about just:

  Prepend
  LastForOffset

::: servo/ports/geckolib/glue.rs:2084
(Diff revision 3)
>  fn add_computed_property_value(keyframe: *mut structs::Keyframe,
>                                 index: usize,
>                                 style: &ComputedValues,
>                                 property: &TransitionProperty,
>                                 shared_lock: &SharedRwLock) {
>      let block = style.to_declaration_block(property.clone().into());
>      unsafe {
>          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
>          (*keyframe).mPropertyValues[index].mProperty = property.into();
>          // FIXME. Bug 1360398: Do not set computed values once we handles
>          // missing keyframes with additive composition.
>          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
>              Arc::new(shared_lock.wrap(block)));

This relates back to my question on the underlying patch -- why do we pass in an index and set mPropertyValues to be just big enough to accommodate that? Does this always append? (If so, it probably should be called append_* and just fetch the length itself? If not, it probably should be called set_*? And in that case is it ok if it truncates the array?)

I'm clearly missing something here.

::: servo/ports/geckolib/glue.rs:2128
(Diff revision 3)
> +    let mut index = unsafe { (*keyframe).mPropertyValues.len() };
> +    for ref property in all_properties.iter() {
> +        if !properties_set_at_offset.has_transition_property_bit(property) {
> +            add_computed_property_value(keyframe,
> +                                        index,
> +                                        style,
> +                                        property,
> +                                        shared_lock);
> +            index += 1;
> +        }
> +    }

This patch looks good but I'm trying to understand this part here, hence the question above.
Attachment #8863276 - Flags: review?(bbirtles)
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review138224

Clearing review request because I should probably at least double-check the changes to the FFI, even if the question about the |index| turns out to me be just being daft.
Attachment #8863274 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #52)
> Comment on attachment 8863276 [details]
> Bug 1354947 - Fill in missing keyframe values.
> 
> https://reviewboard.mozilla.org/r/135070/#review138210
> 
> Sorry, I could use some help understanding this patch. Specifically,
> 
> * Why do we need a separate 'no_specifed_keyframe_at_end' as opposed to
> simply leaving 'properties_set_at_end' empty?

Because I guessed Brian will ask about checking the properties_set_at_xx is empty there. Actually I used the set to trigger fill_in_missing_keyframe_values().  I will use the set there.

> * If fill_in_missing_keyframe_values is only called when
> no_specifed_keyframe_at_{start,end} is true, why do we need
> Gecko_AnimationGetOrInsertInitialKeyframe? Should we just always insert a
> new keyframe and never return an existing one?

We call fill_in_missing_keyframe_values() only if no_specifed_keyframe_at_XX is *false*.

> ::: layout/style/ServoBindings.cpp:1471
> (Diff revision 3)
> > +enum class KeyframeInsertPosition {
> > +  ForceToPrepend,
> > +  AtTheLastOfTheOffset,
> > +};
> 
> How about just:
> 
>   Prepend
>   LastForOffset

Nice shorter names! Thanks!

> ::: servo/ports/geckolib/glue.rs:2084
> (Diff revision 3)
> >  fn add_computed_property_value(keyframe: *mut structs::Keyframe,
> >                                 index: usize,
> >                                 style: &ComputedValues,
> >                                 property: &TransitionProperty,
> >                                 shared_lock: &SharedRwLock) {
> >      let block = style.to_declaration_block(property.clone().into());
> >      unsafe {
> >          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
> >          (*keyframe).mPropertyValues[index].mProperty = property.into();
> >          // FIXME. Bug 1360398: Do not set computed values once we handles
> >          // missing keyframes with additive composition.
> >          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
> >              Arc::new(shared_lock.wrap(block)));
> 
> This relates back to my question on the underlying patch -- why do we pass
> in an index and set mPropertyValues to be just big enough to accommodate
> that? Does this always append? (If so, it probably should be called append_*
> and just fetch the length itself? If not, it probably should be called
> set_*? And in that case is it ok if it truncates the array?)

This is apparently bug. I will fix it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> (In reply to Brian Birtles (:birtles) from comment #52)
> > Comment on attachment 8863276 [details]
> > Bug 1354947 - Fill in missing keyframe values.
> > 
> > https://reviewboard.mozilla.org/r/135070/#review138210
> > 
> > Sorry, I could use some help understanding this patch. Specifically,
> > 
> > * Why do we need a separate 'no_specifed_keyframe_at_end' as opposed to
> > simply leaving 'properties_set_at_end' empty?
> 
> Because I guessed Brian will ask about checking the properties_set_at_xx is
> empty there. Actually I used the set to trigger
> fill_in_missing_keyframe_values().  I will use the set there.

Ah, no. This is wrong.
If we use properties_set_at_end to check whether we call fill_in_missing_keyframe_values(), we need fill out all missing properties in properties_set_at_end().  And actually I did it before.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> > Because I guessed Brian will ask about checking the properties_set_at_xx is
> > empty there. Actually I used the set to trigger
> > fill_in_missing_keyframe_values().  I will use the set there.
> 
> Ah, no. This is wrong.
> If we use properties_set_at_end to check whether we call
> fill_in_missing_keyframe_values(), we need fill out all missing properties
> in properties_set_at_end().  And actually I did it before.

Oops. Needless parens.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> > * If fill_in_missing_keyframe_values is only called when
> > no_specifed_keyframe_at_{start,end} is true, why do we need
> > Gecko_AnimationGetOrInsertInitialKeyframe? Should we just always insert a
> > new keyframe and never return an existing one?
> 
> We call fill_in_missing_keyframe_values() only if no_specifed_keyframe_at_XX
> is *false*.

OK. Now I am thinking we should name no_specifed_keyframe_at_XX properly.  It should be no_need_to_fill_missing_properties_at_XXX?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> > (In reply to Brian Birtles (:birtles) from comment #52)
> > > Comment on attachment 8863276 [details]
> > > Bug 1354947 - Fill in missing keyframe values.
> > > 
> > > https://reviewboard.mozilla.org/r/135070/#review138210
> > > 
> > > Sorry, I could use some help understanding this patch. Specifically,
> > > 
> > > * Why do we need a separate 'no_specifed_keyframe_at_end' as opposed to
> > > simply leaving 'properties_set_at_end' empty?
> > 
> > Because I guessed Brian will ask about checking the properties_set_at_xx is
> > empty there. Actually I used the set to trigger
> > fill_in_missing_keyframe_values().  I will use the set there.
> 
> Ah, no. This is wrong.
> If we use properties_set_at_end to check whether we call
> fill_in_missing_keyframe_values(), we need fill out all missing properties
> in properties_set_at_end().  And actually I did it before.

To be clear, there are some cases that we can't check whether properties_set_at_end is empty or not.

@keyframes missing_0 {
  to { color: 'red'; }
}

@keyframes invalid_0 {
  from { animation-duration: 10s; }  // not animatable property
  to { color: 'red'; }
}

The former case is processed in KeyframesStepValue::ComputedValues block, but the latter is not, but both cases will have empty properties_set_at_start set.  We need to know the difference.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54)

> > ::: servo/ports/geckolib/glue.rs:2084
> > (Diff revision 3)
> > >  fn add_computed_property_value(keyframe: *mut structs::Keyframe,
> > >                                 index: usize,
> > >                                 style: &ComputedValues,
> > >                                 property: &TransitionProperty,
> > >                                 shared_lock: &SharedRwLock) {
> > >      let block = style.to_declaration_block(property.clone().into());
> > >      unsafe {
> > >          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
> > >          (*keyframe).mPropertyValues[index].mProperty = property.into();
> > >          // FIXME. Bug 1360398: Do not set computed values once we handles
> > >          // missing keyframes with additive composition.
> > >          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
> > >              Arc::new(shared_lock.wrap(block)));
> > 
> > This relates back to my question on the underlying patch -- why do we pass
> > in an index and set mPropertyValues to be just big enough to accommodate
> > that? Does this always append? (If so, it probably should be called append_*
> > and just fetch the length itself? If not, it probably should be called
> > set_*? And in that case is it ok if it truncates the array?)
> 
> This is apparently bug. I will fix it.

Because of this bug, we generated keyframes that make some test cases in test_animations.html accidentally pass!  Now the number of failure cases of test_animations.html is 17 in this bug. But with the patch for bug 1359669, it becomes 1!

The one will be fixed by bug 1345697.
Comment on attachment 8863274 [details]
Bug 1354947 - Merge keyframe values at the same offset and of the same timing function.

https://reviewboard.mozilla.org/r/135066/#review138308

Thanks for fixing this! r=me with the following comments addressed.

::: layout/style/ServoBindings.h:364
(Diff revision 4)
>  mozilla::Keyframe* Gecko_AnimationAppendKeyframe(RawGeckoKeyframeListBorrowedMut keyframes,
>                                                   float offset,
>                                                   const nsTimingFunction* timingFunction);
>  
> +// Searches from the beginning of |keyframes| for a Keyframe object with the
> +// specified offset and timing function. If none, is found, a new Keyframe

Sorry, this was probably my mistake:

s/If none, is found,/If none is found,/

::: layout/style/ServoBindings.h:374
(Diff revision 4)
> +// @param timingFunction  The timing function to match, or, if no suitable
> +//                        Keyframe is found, to set on the created Keyframe.

(See my comment later, but if we allow nullptr here should probably add a sentence saying that. e.g., "nullptr here indicates the linear timing function.")

::: layout/style/ServoBindings.cpp:1472
(Diff revision 4)
> +Gecko_GetOrCreateKeyframeAtStart(nsTArray<Keyframe>* aKeyframes,
> +                                 float aOffset,
> +                                 const nsTimingFunction* aTimingFunction)
> +{
> +  MOZ_ASSERT(aKeyframes, "The keyframe array should be valid");
> +  MOZ_ASSERT(aTimingFunction, "The timing function should be valid");

Later on we test if |aTimingFunction| is nullptr or not. I think it's ok to be nullptr right? Doesn't that mean "linear"? If it can't be nullptr, then I guess we shouldn't check for it below.

::: layout/style/ServoBindings.cpp:1475
(Diff revision 4)
> +{
> +  MOZ_ASSERT(aKeyframes, "The keyframe array should be valid");
> +  MOZ_ASSERT(aTimingFunction, "The timing function should be valid");
> +  MOZ_ASSERT(aOffset >= 0. && aOffset <= 1.,
> +             "The offset should be in the range of [0.0, 1.0]");
> +  MOZ_ASSERT((*aKeyframes).IsEmpty() ||

Doesn't aKeyframes->IsEmpty() work? Likewise for the line below.
Attachment #8863274 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #65)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> 
> > > ::: servo/ports/geckolib/glue.rs:2084
> > > (Diff revision 3)
> > > >  fn add_computed_property_value(keyframe: *mut structs::Keyframe,
> > > >                                 index: usize,
> > > >                                 style: &ComputedValues,
> > > >                                 property: &TransitionProperty,
> > > >                                 shared_lock: &SharedRwLock) {
> > > >      let block = style.to_declaration_block(property.clone().into());
> > > >      unsafe {
> > > >          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
> > > >          (*keyframe).mPropertyValues[index].mProperty = property.into();
> > > >          // FIXME. Bug 1360398: Do not set computed values once we handles
> > > >          // missing keyframes with additive composition.
> > > >          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
> > > >              Arc::new(shared_lock.wrap(block)));
> > > 
> > > This relates back to my question on the underlying patch -- why do we pass
> > > in an index and set mPropertyValues to be just big enough to accommodate
> > > that? Does this always append? (If so, it probably should be called append_*
> > > and just fetch the length itself? If not, it probably should be called
> > > set_*? And in that case is it ok if it truncates the array?)
> > 
> > This is apparently bug. I will fix it.
> 
> Because of this bug, we generated keyframes that make some test cases in
> test_animations.html accidentally pass!  Now the number of failure cases of
> test_animations.html is 17 in this bug. But with the patch for bug 1359669,
> it becomes 1!
> 
> The one will be fixed by bug 1345697.

In order to review this patch I still need to find out the answer to the questions above:

1) Why do we pass in an index to add_computed_property_value and then set mPropertyValues to be just big enough to accommodate that?

2) Does that function (add_computed_property_value) always append?
   If so, shouldn't we call it append_computed_property_values and make it fetch the length itself?
   If not, should it be called set_computed_property_values? And is it ok if it truncates the array?

Or am I completely misunderstanding this?
Flags: needinfo?(hikezoe)
(In reply to Brian Birtles (:birtles) from comment #67)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #65)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> > 
> > > > ::: servo/ports/geckolib/glue.rs:2084
> > > > (Diff revision 3)
> > > > >  fn add_computed_property_value(keyframe: *mut structs::Keyframe,
> > > > >                                 index: usize,
> > > > >                                 style: &ComputedValues,
> > > > >                                 property: &TransitionProperty,
> > > > >                                 shared_lock: &SharedRwLock) {
> > > > >      let block = style.to_declaration_block(property.clone().into());
> > > > >      unsafe {
> > > > >          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
> > > > >          (*keyframe).mPropertyValues[index].mProperty = property.into();
> > > > >          // FIXME. Bug 1360398: Do not set computed values once we handles
> > > > >          // missing keyframes with additive composition.
> > > > >          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
> > > > >              Arc::new(shared_lock.wrap(block)));
> > > > 
> > > > This relates back to my question on the underlying patch -- why do we pass
> > > > in an index and set mPropertyValues to be just big enough to accommodate
> > > > that? Does this always append? (If so, it probably should be called append_*
> > > > and just fetch the length itself? If not, it probably should be called
> > > > set_*? And in that case is it ok if it truncates the array?)
> > > 
> > > This is apparently bug. I will fix it.
> > 
> > Because of this bug, we generated keyframes that make some test cases in
> > test_animations.html accidentally pass!  Now the number of failure cases of
> > test_animations.html is 17 in this bug. But with the patch for bug 1359669,
> > it becomes 1!
> > 
> > The one will be fixed by bug 1345697.
> 
> In order to review this patch I still need to find out the answer to the
> questions above:
> 
> 1) Why do we pass in an index to add_computed_property_value and then set
> mPropertyValues to be just big enough to accommodate that?
> 
> 2) Does that function (add_computed_property_value) always append?
>    If so, shouldn't we call it append_computed_property_values and make it
> fetch the length itself?
>    If not, should it be called set_computed_property_values? And is it ok if
> it truncates the array?

Ah. The add_computed_porperty_value() should be renamed to append_computed_property_value() and drop the index argument.
It's actually called with a property that has not been added.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #68)
> Ah. The add_computed_porperty_value() should be renamed to
> append_computed_property_value() and drop the index argument.
> It's actually called with a property that has not been added.

Excellent, that's what I was trying to work out. Thank you!
Comment on attachment 8863276 [details]
Bug 1354947 - Fill in missing keyframe values.

https://reviewboard.mozilla.org/r/135070/#review138310

::: layout/style/ServoBindings.h:384
(Diff revision 4)
>    RawGeckoKeyframeListBorrowedMut keyframes,
>    float offset,
>    const nsTimingFunction* timingFunction);
>  
> +// Searches from the beginning of |keyframes| for a Keyframe object at offset 0
> +// with the specified timing function. If none, is found, a new Keyframe

Again, sorry, no ',' after 'none'.

We could probably abbreviate this comment to something like:

// As with Gecko_GetOrCreateKeyframeAtStart except that this method will search
// from the beginning of |keyframes| for a Keyframe with matching timing
// function and an offset of 0.0.
// Furthermore, if a matching Keyframe is not found, a new Keyframe will be
// inserted after the *last* Keyframe in |keyframes| with offset 0.0.

::: layout/style/ServoBindings.h:397
(Diff revision 4)
> +// Searches from the ending of |keyframes| for a Keyframe object at offset 1
> +// with the specified timing function. If none, is found, a new Keyframe
> +// object at offset 1 with the specified |timingFunction| will be appended to
> +// |keyframes|.
> +//
> +// @param keyframes  An array of Keyframe objects, sorted by offset.
> +// @param timingFunction  The timing function to match, or, if no suitable
> +//                        Keyframe is found, to set on the created Keyframe.
> +//
> +// @returns  The matching or created Keyframe.

Again, we could probably abbreviate this as:

// As with Gecko_GetOrCreateKeyframeAtStart except that this method will search
// from the *end* of |keyframes| for a Keyframe with matching timing function
// and an offset of 1.0. If a matching Keyframe is not found, a new Keyframe
// will be appended to the end of |keyframes|.

::: layout/style/ServoBindings.cpp:1467
(Diff revision 4)
> -Gecko_GetOrCreateKeyframeAtStart(nsTArray<Keyframe>* aKeyframes,
> +  Forward,
> +  Backward,

(Nit: I feel like 'Forwards' and 'Backwards' would be more natural but it probably doesn't matter.)

::: layout/style/ServoBindings.cpp:1529
(Diff revision 4)
> +  MOZ_ASSERT((*aKeyframes).IsEmpty() ||
> +             (*aKeyframes).ElementAt(0).mOffset.value() >= aOffset,

Again, doesn't aKeyframes->IsEmpty() work?

Likewise aKeyframes->ElementAt(0)....

::: servo/ports/geckolib/glue.rs:2110
(Diff revision 4)
> +    let needs_filling = all_properties.iter().any(|ref property| {
> +        !properties_set_at_offset.has_transition_property_bit(property)
> +    });
> +    if !needs_filling {
> +        return;
> +    }

This block could possibly do with a comment,

e.g. "Return early if all animated properties are already set."

::: servo/ports/geckolib/glue.rs:2127
(Diff revision 4)
> +            Gecko_GetOrCreateFinalKeyframe(keyframes, timing_function)
> +        },
> +        _ => unreachable!("offset should be 0. or 1."),
> +    };
> +
> +    // Append properties that have not been appeared at this offset.

Nit: s/been appeared/been set/

::: servo/ports/geckolib/glue.rs:2128
(Diff revision 4)
> +    let mut index = unsafe { (*keyframe).mPropertyValues.len() };
> +    for ref property in all_properties.iter() {
> +        if !properties_set_at_offset.has_transition_property_bit(property) {
> +            add_computed_property_value(keyframe,
> +                                        index,
> +                                        style,
> +                                        property,
> +                                        shared_lock);
> +            index += 1;
> +        }

As per comment 68, let's make a follow patch or follow up bug rename this method to append_computed_property_value and make it calculate the length itself instead of passing it in.

::: servo/ports/geckolib/glue.rs:2164
(Diff revision 4)
> +    let mut no_need_to_fill_missing_values_at_start = false;
> +    let mut no_need_to_fill_missing_values_at_end = false;

Thank you for changing this! However, I think using negative words (like no_need_to) can make it more confusing. Probably how you had it before was better. Or perhaps:

  let has_complete_initial_keyframe = false;
  let has_complete_final_keyframe = false;

::: servo/ports/geckolib/glue.rs:2193
(Diff revision 4)
>                                               step.start_percentage.0 as f32,
>                                               &timing_function)
>          };
>  
>          match step.value {
>              KeyframesStepValue::ComputedValues => {

Let's add a comment before this to explain what is happening, e.g.:

            // In KeyframesAnimation::from_keyframes if there is no 0% or 100% keyframe at all, we
            // will create a 'ComputedValues' step to represent that all properties animated by the 
            // keyframes animation should be set to the underlying computed value for that keyframe.

::: servo/ports/geckolib/glue.rs:2244
(Diff revision 4)
> -                }
>              },
>          }
>      }
> +
> +    // Append missing property values in the initial or the final keyframes.

Very very minor nit: I think this sentence would be little easier to read if we moved 'missing' later, e.g.

// Append property values that are missing in the initial or final keyframes.
Attachment #8863276 - Flags: review?(bbirtles) → review+
Comment on attachment 8863276 [details]
Bug 1354947 - Fill in missing keyframe values.

https://reviewboard.mozilla.org/r/135070/#review138310

> As per comment 68, let's make a follow patch or follow up bug rename this method to append_computed_property_value and make it calculate the length itself instead of passing it in.

I added the additional patch in this bug, but if I got r+ until attachment 8863271 [details] gets reviwed by Simon, I will drop the patch and open a new bug for it. Thanks!
Comment on attachment 8863668 [details]
Bug 1354947 - Rename add_computed_property_value to append_computed_property_value.

https://reviewboard.mozilla.org/r/135474/#review138450

Thank you!
Attachment #8863668 - Flags: review?(bbirtles) → review+
See Also: → 1361933
I got an assertion in dom/animation/test/crashtests/1323114-2.html [1] after enabling OMTA and applying these patches.

###!!! ASSERTION: uninitialized: 'mUnit != eUnit_Null', file /dist/include/mozilla/StyleAnimationValue.h, line 355
#01: mozilla::dom::KeyframeEffectReadOnly::CompositeValue()
#02: mozilla::layers::AnimationHelper::SampleAnimationForEachNode()
...

I checked SampleValue() in AnimationHelper.cpp, and it is possible that we have a _null_ |aUnderlyingValue| [2]. When calling StyleAnimationValue::Accmulate(), we try to get the unit of the underlying value [3], so this assertion happened!

[1] http://searchfox.org/mozilla-central/source/dom/animation/test/crashtests/1323114-2.html
[2] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/gfx/layers/AnimationHelper.cpp#100
[3] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/StyleAnimationValue.cpp#3317-3318
Boris, what I can tell is that this bug is @keyframes for CSS animations, whereas 1323114-2.html uses script animations.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #87)
> Boris, what I can tell is that this bug is @keyframes for CSS animations,
> whereas 1323114-2.html uses script animations.

OK. I will file another bug to trace this.
Also we don't support accumulate yet. bug 1353202.
Simon, comments you wrote has lost somewhere or not published yet?
Flags: needinfo?(simon.sapin)
Comment on attachment 8863271 [details]
Bug 1354947 - Add clear method for LonghandIdSet.

https://reviewboard.mozilla.org/r/135060/#review141496

::: servo/components/style/properties/properties.mako.rs:257
(Diff revision 3)
>      }
>  
> +    /// Clear all bits
> +    #[inline]
> +    pub fn clear(&mut self) {
> +        self.storage = [0; (${len(data.longhands)} - 1 + 32) / 32];

I don’t know if the compiler is smart enough to avoid allocating a large array on the stack and then copying it. Could this be a loop?

for cell in &mut self.storage {
    *cell = 0
}

r=me with that
Ugh sorry, I forgot to click "Publish"…
(In reply to Simon Sapin (:SimonSapin) from comment #91)
> Comment on attachment 8863271 [details]
> Bug 1354947 - Add clear method for LonghandIdSet.
> 
> https://reviewboard.mozilla.org/r/135060/#review141496
> 
> ::: servo/components/style/properties/properties.mako.rs:257
> (Diff revision 3)
> >      }
> >  
> > +    /// Clear all bits
> > +    #[inline]
> > +    pub fn clear(&mut self) {
> > +        self.storage = [0; (${len(data.longhands)} - 1 + 32) / 32];
> 
> I don’t know if the compiler is smart enough to avoid allocating a large
> array on the stack and then copying it. Could this be a loop?
> 
> for cell in &mut self.storage {
>     *cell = 0
> }

Thank you! Smart!
Flags: needinfo?(simon.sapin)
Attachment #8863270 - Attachment is obsolete: true
Attachment #8863271 - Attachment is obsolete: true
Attachment #8863271 - Flags: review?(simon.sapin)
Attachment #8863275 - Attachment is obsolete: true
Attachment #8863316 - Attachment is obsolete: true
Attachment #8863668 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec069eeba0af
Add equal operators for comparing ComputedTimingFunction with nsTimingFunction. r=birtles
https://hg.mozilla.org/integration/autoland/rev/bdc6e25aafac
Expose FindMatchingKeyframe and make it reusable for nsTimingFunction. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ff4aff7e1876
Merge keyframe values at the same offset and of the same timing function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/74d863bcc6dd
Fill in missing keyframe values. r=birtles
https://hg.mozilla.org/integration/autoland/rev/aba3d3439216
Rename FillKeyframesForName to GetKeyframesForName. r=birtles
https://hg.mozilla.org/integration/autoland/rev/675726c3ebca
Drop Gecko_AnimationAppendKeyframe. r=birtles
https://hg.mozilla.org/integration/autoland/rev/2c7facd1caa2
Update mochitest expectations for @keyframes merging. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: