Closed Bug 1360398 Opened 5 years ago Closed 5 years ago

stylo: Do not fill computed values in missing 0%/100% keyframes in CSS Animation

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

This is the bug for stylo for what we did in bug 1339332.
Blocks: 1376492
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
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.
No longer blocks: 1376492
Duplicate of this bug: 1376492
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);
  }
Attachment #8883745 - Flags: review?(bbirtles) → review+
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
Attachment #8883744 - Flags: review?(bbirtles) → review+
(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!
Attachment #8883745 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/a4f79e5c8fab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Fixed duplicate bug 1376492.
Nightly 56 x64 20170711100226 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.