This is the bug for stylo for what we did in bug 1339332.

Comment on attachment 8883745 [details] Bug 1360398 - Pass null servo's computed values into ComputedKeyframeValues. https://reviewboard.mozilla.org/r/154662/#review159744 ::: servo/ports/geckolib/glue.rs:2865 (Diff revision 1) > for (index, keyframe) in keyframes.iter().enumerate() { > let ref mut animation_values = computed_keyframes[index]; > > let mut seen = LonghandIdSet::new(); > > - // mServoDeclarationBlock is null in the case where we have an invalid css property. > + let iter = keyframe.mPropertyValues.iter(); Note about this comment. The only place we set null to mServoDeclarationBlock is Servo_StyleSet_GetKeyframesForName that was modified in the very previous patch. For CSS transitions, we don't create any keyframes with null value at all since we create keyframes only if both of start and end values are valid case [1]. [1] https://hg.mozilla.org/mozilla-central/file/ed4380532019/layout/style/nsTransitionManager.cpp#l885

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > Comment on attachment 8883745 [details] > Bug 1360398 - Pass null servo's computed values into ComputedKeyframeValues. > > https://reviewboard.mozilla.org/r/154662/#review159744 > > ::: servo/ports/geckolib/glue.rs:2865 > (Diff revision 1) > > for (index, keyframe) in keyframes.iter().enumerate() { > > let ref mut animation_values = computed_keyframes[index]; > > > > let mut seen = LonghandIdSet::new(); > > > > - // mServoDeclarationBlock is null in the case where we have an invalid css property. > > + let iter = keyframe.mPropertyValues.iter(); > > Note about this comment. > > The only place we set null to mServoDeclarationBlock is > Servo_StyleSet_GetKeyframesForName that was modified in the very previous > patch. > > For CSS transitions, we don't create any keyframes with null value at all > since we create keyframes only if both of start and end values are valid > case [1]. > > [1] > https://hg.mozilla.org/mozilla-central/file/ed4380532019/layout/style/ > nsTransitionManager.cpp#l885 As a side note, we also modify mServoDeclarationBlock in ElementPropertyTransition::UpdateStartValueFromReplacedTransition() but it should not be affected and, of course, set don't set null ptr there either.

Comment on attachment 8883745 [details] Bug 1360398 - Pass null servo's computed values into ComputedKeyframeValues. https://reviewboard.mozilla.org/r/154662/#review160612 ::: servo/ports/geckolib/glue.rs:2886 (Diff revision 4) > - // This is safe since we immediately write to the uninitialized values. > + // This is safe since we immediately write to the uninitialized values. > - unsafe { animation_values.set_len((property_index + 1) as u32) }; > + unsafe { animation_values.set_len((property_index + 1) as u32) }; > - seen.set_animatable_longhand_bit(&anim.0); > + animation_values[property_index].mProperty = (&property).into(); (This isn't related to this patch, but I'm curious about this part. Don't we already initialize the elements in ServoStyleSet::GetComputedKeyframeValuesFor. So is the comment and the following one accurate?) ::: servo/ports/geckolib/glue.rs:2907 (Diff revision 4) > + // In |keyframes| we have only animatable property there, so > + // unwrap() should be fine but just in case. "|keyframes.mPropertyValues| should only contain animatable properties, but we check the result from ns_nscsspropertyid just in case." (Should we add a debug_assert for this instead?) ::: servo/ports/geckolib/glue.rs:2909 (Diff revision 4) > + if animatable_longhand.is_some() { > + maybe_append_animation_value(animatable_longhand.unwrap(), None); > } Rather than doing is_some() followed by unwrap() can't we do something like: if let Some(property) = animatable_longhand { maybe_append_animation_value(property, None); }

Comment on attachment 8883744 [details] Bug 1360398 - Do not fill computed values in missing keyframes for CSS animations during generating Keyframes. https://reviewboard.mozilla.org/r/154660/#review160620

(In reply to Brian Birtles (:birtles) from comment #13) > Comment on attachment 8883745 [details] > Bug 1360398 - Pass null servo's computed values into ComputedKeyframeValues. > > https://reviewboard.mozilla.org/r/154662/#review160612 > > ::: servo/ports/geckolib/glue.rs:2886 > (Diff revision 4) > > - // This is safe since we immediately write to the uninitialized values. > > + // This is safe since we immediately write to the uninitialized values. > > - unsafe { animation_values.set_len((property_index + 1) as u32) }; > > + unsafe { animation_values.set_len((property_index + 1) as u32) }; > > - seen.set_animatable_longhand_bit(&anim.0); > > + animation_values[property_index].mProperty = (&property).into(); > > (This isn't related to this patch, but I'm curious about this part. Don't we > already initialize the elements in > ServoStyleSet::GetComputedKeyframeValuesFor. So is the comment and the > following one accurate?) This does not match to keyframe length. GetComputedKeyframeValuesFor allocates keyframe, whereas this function allocates PropertyStyleAnimationValuePair. > ::: servo/ports/geckolib/glue.rs:2909 > (Diff revision 4) > > + if animatable_longhand.is_some() { > > + maybe_append_animation_value(animatable_longhand.unwrap(), None); > > } > > Rather than doing is_some() followed by unwrap() can't we do something like: > > if let Some(property) = animatable_longhand { > maybe_append_animation_value(property, None); > } Oh Rust-ish! Thanks!

A final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=539e9f7e1ddd1f7244a02525d16d3ffeb0d383

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4f79e5c8fab Do not fill computed values in missing keyframes for CSS animations during generating Keyframes. r=birtles

Fixed duplicate bug 1376492. Nightly 56 x64 20170711100226 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480)