Closed Bug 1311257 Opened 8 years ago Closed 7 years ago

stylo: support missing keyframe whose computed offset is 0 or 1 for servo backend

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

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

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 4 obsolete files)

Bug 1311257 - Add a function that returns a base computed values (i.e. computed values without any animations rules).
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1311257 - Add an FFI function that returns an AnimationValue for a given nsCSSPropertyID from computed values.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1311257 - Store base styles for stylo.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1311257 - Move AnimationPropertySegment in a separate header and expose it in FFI.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1311257 - Use underlying value for missing keyframes.
59 bytes, text/x-review-board-request
birtles
: review+
heycam
: review+
Details
Bug 1311257 - getKeyframes() returns base computed values in missing keyframes.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1311257 - Support missing keyframes handling for stylo.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1311257 - Update test expectations.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1311257 - Update comment refering to bug 1311257.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
We implement to support missing keyframes for Gecko in bug 1305325. 

To support missing keyframes, we need base values for each element.
I guess cascade_style in KeyframesAnimationState is the base value.
http://hg.mozilla.org/incubator/stylo/file/9f7b709e71b7/servo/components/style/animation.rs#l71
Moving this to block bug 1317211 instead since this is not required to bootstrap CSS animations/transitions.
Blocks: 1317211
No longer blocks: 1302945
Priority: -- → P3
Summary: stylo: suport missing keyframe whose computed offset is 0 or 1 for servo backend → stylo: support missing keyframe whose computed offset is 0 or 1 for servo backend
After applying patches of bug 1317209, we start to get this test failed:

Assertion failure: !aStyleContext->StyleSource().IsServoComputedValues() (Bug 1311257: Servo backend does not support the base value yet), at /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:955

TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1323114-2.html | application terminated with exit code 11
REFTEST PROCESS-CRASH | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1323114-2.html | application crashed [@ mozilla::EffectCompositor::GetBaseStyle]


And I think this bug can fix this assertion.
(In reply to Boris Chiou [:boris] (away 1/27 ~ 2/1) from comment #2)
> After applying patches of bug 1317209, we start to get this test failed:
> 
> Assertion failure: !aStyleContext->StyleSource().IsServoComputedValues()
> (Bug 1311257: Servo backend does not support the base value yet), at
> /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:955
> 
> TEST-UNEXPECTED-FAIL |
> file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/
> crashtests/1323114-2.html | application terminated with exit code 11
> REFTEST PROCESS-CRASH |
> file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/
> crashtests/1323114-2.html | application crashed [@
> mozilla::EffectCompositor::GetBaseStyle]
> 
> 
> And I think this bug can fix this assertion.

Bug 1329878 instead of this bug?
Depends on: 1340961
Blocks: stylo-css-animations
Depends on: 1346663
Blocks: 1329878
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
I am going to add an FFI which does a bunch of stuff what we currently do in KeyframeEffectReadOnly::ComposeStyleRule().  To do this, we need to expose AnimationPropertySegment and ComputedTiming, and another FFI to call ComputedTimingFunction::GetPortion().
I just realized that we need to expose the hash table which stores base styles for each properties.
Sorry, I'm not going to be able to get to these reviews before the weekend but I had a quick glance and I'm curious about the "Add FFI functions to get progress value and current position in a segment" part. Are they calculations we could just do on the Servo side? I guess we currently still do all timing calculations in Gecko so that wouldn't make sense, right?
(In reply to Brian Birtles (:birtles) from comment #21)
> Sorry, I'm not going to be able to get to these reviews before the weekend
> but I had a quick glance and I'm curious about the "Add FFI functions to get
> progress value and current position in a segment" part. Are they
> calculations we could just do on the Servo side? 

We could, but we need to expose Maybe<> and Nullable<>, and handle them in Rust.
Blocks: 1343710
Comment on attachment 8853171 [details]
Bug 1311257 - Add a function that returns a base computed values (i.e. computed values without any animations rules).

https://reviewboard.mozilla.org/r/125236/#review128370

::: layout/style/ServoBindingList.h:326
(Diff revision 1)
>                     mozilla::TraversalRootBehavior root_behavior)
>  
>  // Assert that the tree has no pending or unconsumed restyles.
>  SERVO_BINDING_FUNC(Servo_AssertTreeIsClean, void, RawGeckoElementBorrowed root)
>  
> +// Returns computed values for the given element withour any animations rules.

without
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review128366

::: dom/animation/KeyframeEffectReadOnly.cpp:455
(Diff revision 1)
>  void
>  KeyframeEffectReadOnly::EnsureBaseStyles(
> +  const ServoComputedValuesWithParent& aServoValues,
> +  const nsTArray<AnimationProperty>& aProperties)
> +{
> +  if (!mTarget) {
> +    return;
> +  }
> +
> +  RefPtr<ServoComputedValues> baseComputedValues;
> +
> +  nsPresContext* presContext =
> +    nsContentUtils::GetContextForContent(mTarget->mElement);
> +  MOZ_ASSERT(presContext);
> +
> +  for (const AnimationProperty& property : aProperties) {

Don't we need to clear mBaseStyleValuesForServo somewhere here? Otherwise will it just keep being added to?

::: dom/animation/KeyframeEffectReadOnly.cpp:479
(Diff revision 1)
> +      }
> +
> +      if (!baseComputedValues) {
> +        baseComputedValues =
> +          presContext->StyleSet()->AsServo()->
> +            GetBaseComputedValuesForElement(mTarget->mElement, nullptr);

Don't we want to pass the pseudo element type here instead of nullptr?
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.

https://reviewboard.mozilla.org/r/125246/#review128376

This looks fine but could we get a commit message explaining what this is for? It seems unfortunate to have to cross the FFI boundary for these trivial calculations but if they don't happen very frequently (e.g. only when creating a transitions, as opposed to testing if a transition is necessary etc.) then it's probably fine.

::: layout/style/ServoBindings.cpp:505
(Diff revision 1)
> +  double positionInSegment =
> +    (aComputedTiming->mProgress.Value() - aSegment->mFromKey) /
> +    (aSegment->mToKey - aSegment->mFromKey);

Is it ok to assume that aSegment->mToKey != aSegment->mFromKey ?

If so, we should assert that. If not, we should handle that case (presumably by returning 1.0?)
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.

https://reviewboard.mozilla.org/r/125250/#review128378

::: layout/style/ServoBindings.cpp:518
(Diff revision 1)
> +    reinterpret_cast<nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>*>
> +      (aBaseStyles);

I'm pretty sure that we should prefer static_cast here

::: servo/ports/geckolib/glue.rs:261
(Diff revision 1)
>      use style::properties::animated_properties::AnimationValueMap;
>  
> -    let property: TransitionProperty = property.into();
> +    let property: TransitionProperty = css_property.into();
>      let value_map = RwLock::<AnimationValueMap>::as_arc(&raw_value_map);
>  
> -    let from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
> +    // Get the underlying value from previous composed value or cached base values.

"If either of the segment endpoints are null, get the underlying value to use from the current value in the values map (set by a lower-priority effect), or, if there is no current value, look up the cached base value for this property."

?

::: servo/ports/geckolib/glue.rs:273
(Diff revision 1)
> +    if (segment.mFromValue.mServo.mRawPtr.is_null() ||
> +        segment.mToValue.mServo.mRawPtr.is_null()) &&
> +       underlying_value.is_none() {
> +        // This is a case that servo does not support a given property as animatable?
> +        return;
> +    }

What sort of cases cause this to happen?

::: servo/ports/geckolib/glue.rs:280
(Diff revision 1)
> +       underlying_value.is_none() {
> +        // This is a case that servo does not support a given property as animatable?
> +        return;
> +    }
> +
> +    // Declare for making derefenced raw pointer alive out side the if block.

Nit: s/out side/outside/

(Although I wonder if this comment is necessary, or if there is a neater way of doing this?)
Comment on attachment 8853179 [details]
Bug 1311257 - getKeyframes() returns base computed values in missing keyframes.

https://reviewboard.mozilla.org/r/125252/#review128386
Attachment #8853179 - Flags: review?(bbirtles) → review+
Comment on attachment 8853180 [details]
Bug 1311257 - Support missing keyframes handling for stylo.

https://reviewboard.mozilla.org/r/125254/#review128388

::: dom/animation/KeyframeUtils.cpp:472
(Diff revision 1)
> -  // FIXME: Bug 1311257: Support missing keyframes for Servo backend.
> +  if (!AnimationUtils::IsCoreAPIEnabled() &&
> -  if ((!AnimationUtils::IsCoreAPIEnabled() ||
> -       aDocument->IsStyledByServo()) &&

Is this right? We don't support additive animation yet do we?
There are a lot of patches so I haven't checked them all, but I notice there are references to this bug in:

  KeyframeUtils.cpp
  nsLayoutUtils.cpp
  nsDisplayList.cpp
  nsComputedDOMStyle.cpp

and I wonder if this bug drops / updates them all.
Comment on attachment 8853168 [details]
Bug 1311257 - Make TransitionProperty::from_declaration() convert PropertyDeclaration::{CSSWideKeyword,WithVariables} as well.

https://reviewboard.mozilla.org/r/125230/#review128418

::: commit-message-f9798:1
(Diff revision 1)
> +Bug 1311257 - TransitionProperty::from_declaration() converts PropertyDeclaration::{CSSWideKeyword,WithVariables} as well. r?heycam

Nit: Better to write the commit message in terms of what it's changing, rather than the state of the code after it's changed.  So:

Make TransitionProperty::from_declaration() convert PropertyDeclaration::{CSSWideKeyword,WithVariables} as well.
Attachment #8853168 - Flags: review?(cam) → review+
Comment on attachment 8853169 [details]
Bug 1311257 - Make cascade_with_rules take SharedStyleContext instead StyleContext.

https://reviewboard.mozilla.org/r/125232/#review128420

Is this just a cleanup, since cascade_with_rules doesn't need anything on the StyleContext itself?

::: commit-message-9726c:1
(Diff revision 1)
> +Bug 1311257 - cascade_with_rules takes SharedStyleContext instead StyleContext. r?heycam

Nit: similarly here: "Make cascade_with_rules take ...".
Attachment #8853169 - Flags: review?(cam) → review+
Comment on attachment 8853170 [details]
Bug 1311257 - Make cascade_with_rules() take a boolean representing whether the cascade is for pseudo element or not.

https://reviewboard.mozilla.org/r/125234/#review128422

::: commit-message-7091c:1
(Diff revision 1)
> +Bug 1311257 - cascade_with_rules() takes a boolean representing whether the cascade is for pseudo element or not. r?heycam

Nit: and here.
Attachment #8853170 - Flags: review?(cam) → review+
Comment on attachment 8853171 [details]
Bug 1311257 - Add a function that returns a base computed values (i.e. computed values without any animations rules).

https://reviewboard.mozilla.org/r/125236/#review128424

::: layout/style/ServoBindingList.h:331
(Diff revision 1)
> +// Returns computed values for the given element withour any animations rules.
> +SERVO_BINDING_FUNC(Servo_StyleSet_GetBaseComputedValuesForElement,
> +                   ServoComputedValuesStrong,
> +                   RawServoStyleSetBorrowed set,
> +                   RawGeckoElementBorrowed element,
> +                   nsIAtom* pseudoTag)

Nit: this should really be "pseudo_tag" to follow Rust identifier naming style.  Also, blank line after this.

::: servo/components/style/matching.rs:1225
(Diff revision 1)
>                                             });
>          }
>      }
> +
> +    /// Returns computed values without animation and transition rules.
> +    fn get_base_computed_values(&self,

Nit: Even though this function does something similar to get_after_change_style (i.e., computes styles with certain rule nodes removed), its naming doesn't match.  Should we call this one get_base_style?
Attachment #8853171 - Flags: review?(cam) → review+
Comment on attachment 8853172 [details]
Bug 1311257 - Add an FFI function that returns an AnimationValue for a given nsCSSPropertyID from computed values.

https://reviewboard.mozilla.org/r/125238/#review128426
Attachment #8853172 - Flags: review?(cam) → review+
Comment on attachment 8853169 [details]
Bug 1311257 - Make cascade_with_rules take SharedStyleContext instead StyleContext.

https://reviewboard.mozilla.org/r/125232/#review128420

Yes, that's right.
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review128366

> Don't we want to pass the pseudo element type here instead of nullptr?

I did totally foget this.  When I was writing this code, I couldn't remember the function name to get pseudo nsIAtom, so I thought, at that time, I write it later.  Thanks!
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.

https://reviewboard.mozilla.org/r/125246/#review128376

> Is it ok to assume that aSegment->mToKey != aSegment->mFromKey ?
> 
> If so, we should assert that. If not, we should handle that case (presumably by returning 1.0?)

Yeah, we should assert.
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.

https://reviewboard.mozilla.org/r/125250/#review128378

> What sort of cases cause this to happen?

This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
Comment on attachment 8853180 [details]
Bug 1311257 - Support missing keyframes handling for stylo.

https://reviewboard.mozilla.org/r/125254/#review128388

> Is this right? We don't support additive animation yet do we?

No, we don't support additive animation yet. But the function, RequiresAdditiveAnimation, does not check that composite is 'add', it just checks there is missing keyframe.
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.

https://reviewboard.mozilla.org/r/125250/#review128378

> This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.

I still don't understand. Does this ever happen? When? Why?
(In reply to Brian Birtles (:birtles) from comment #40)
> Comment on attachment 8853178 [details]
> Bug 1311257 - Use underlying value for missing keyframes.
> 
> https://reviewboard.mozilla.org/r/125250/#review128378
> 
> > This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
> 
> I still don't understand. Does this ever happen? When? Why?

This is a result of discussion with us in a security bug, I don't refer to the bug here because it's still not in public.
(In reply to Brian Birtles (:birtles) from comment #29)
> There are a lot of patches so I haven't checked them all, but I notice there
> are references to this bug in:
> 
>   KeyframeUtils.cpp
>   nsLayoutUtils.cpp
>   nsDisplayList.cpp
>   nsComputedDOMStyle.cpp
> 
> and I wonder if this bug drops / updates them all.

Thanks, I will update comments in an additional patch here.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> (In reply to Brian Birtles (:birtles) from comment #40)
> > Comment on attachment 8853178 [details]
> > Bug 1311257 - Use underlying value for missing keyframes.
> > 
> > https://reviewboard.mozilla.org/r/125250/#review128378
> > 
> > > This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
> > 
> > I still don't understand. Does this ever happen? When? Why?
> 
> This is a result of discussion with us in a security bug, I don't refer to
> the bug here because it's still not in public.

Then please CC me on the bug.
Blocks: 1353202
(In reply to Brian Birtles (:birtles) from comment #58)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > (In reply to Brian Birtles (:birtles) from comment #40)
> > > Comment on attachment 8853178 [details]
> > > Bug 1311257 - Use underlying value for missing keyframes.
> > > 
> > > https://reviewboard.mozilla.org/r/125250/#review128378
> > > 
> > > > This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
> > > 
> > > I still don't understand. Does this ever happen? When? Why?
> > 
> > This is a result of discussion with us in a security bug, I don't refer to
> > the bug here because it's still not in public.
> 
> Then please CC me on the bug.

After discussion with Brian, we decided to add a warning here.
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review128812

This looks fine but I should probably check once more if you decide to make any of the following changes.

::: dom/animation/KeyframeEffectReadOnly.cpp:464
(Diff revision 3)
> +  mBaseStyleValuesForServo.Clear();
> +
> +  RefPtr<ServoComputedValues> baseComputedValues;
> +
> +  nsPresContext* presContext =
> +    nsContentUtils::GetContextForContent(mTarget->mElement);
> +  MOZ_ASSERT(presContext);
> +
> +  nsIAtom* pseudoAtom = mTarget->mPseudoType < CSSPseudoElementType::Count
> +                      ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
> +                      : nullptr;
> +  for (const AnimationProperty& property : aProperties) {

It seems like baseComputedValues is a long way from where it is used? I wonder if this would be easier to read if we put the part where we fetch the pres context sooner?

e.g.

  mBaseStyleValuesForServo.Clear();

  nsPresContext* presContext =
    nsContentUtils::GetContextForContent(mTarget->mElement);
  MOZ_ASSERT(presContext);

  RefPtr<ServoComputedValues> baseComputedValues;
  nsIAtom* pseudoAtom = mTarget->mPseudoType < CSSPseudoElementType::Count
                      ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
                      : nullptr;
  for (const AnimationProperty& property : aProperties) {
    EnsureBaseStyle(property, presContext, baseComputedValues);
  }

::: dom/animation/KeyframeEffectReadOnly.cpp:468
(Diff revision 3)
> +  nsPresContext* presContext =
> +    nsContentUtils::GetContextForContent(mTarget->mElement);
> +  MOZ_ASSERT(presContext);

Why is this guaranteed to be non-null?

::: dom/animation/KeyframeEffectReadOnly.cpp:475
(Diff revision 3)
> +  for (const AnimationProperty& property : aProperties) {
> +    for (const AnimationPropertySegment& segment : property.mSegments) {
> +      if (segment.HasReplaceableValues()) {
> +        continue;
> +      }
> +
> +      if (!baseComputedValues) {
> +        baseComputedValues =
> +          presContext->StyleSet()->AsServo()->
> +            GetBaseComputedValuesForElement(mTarget->mElement, pseudoAtom);
> +      }
> +      RefPtr<RawServoAnimationValue> baseValue =
> +        Servo_ComputedValues_ExtractAnimationValue(
> +          baseComputedValues, property.mProperty).Consume();
> +      mBaseStyleValuesForServo.Put(property.mProperty, baseValue);
> +      break;
> +    }
> +  }

Nested for loops with breaks are a bit hard to read (although I see we do that in the Gecko-version of this method). Do you think it makes sense to factor out a method for the inner loop?
Attachment #8853173 - Flags: review?(bbirtles)
Comment on attachment 8853177 [details]
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.

https://reviewboard.mozilla.org/r/125248/#review128824

::: servo/ports/geckolib/glue.rs:264
(Diff revision 3)
> +    let from_value = AnimationValue::as_arc(&from_value).as_ref();
> +    let to_value = unsafe { &*segment.mToValue.mServo.mRawPtr };
> +    let to_value = AnimationValue::as_arc(&to_value).as_ref();
> +
> +    if segment.mToKey == segment.mFromKey {
> +        if unsafe { Gecko_ComputedTimingProgress(computed_timing) } < 0.5 {

This should be 0 not 0.5 right?
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.

https://reviewboard.mozilla.org/r/125246/#review128822

This seems fine but I wonder if it makes sense to just create a method that converts a Gecko ComputedTiming object into something that Servo can deal with? And then instead of having:

  double Gecko_ComputedTimingProgress(aComputedTiming)
  double Gecko_SegmentGetPosition(aSegment, aComputedTiming)

have:

  ServoComputedTiming Gecko_GetComputedTiming(aComputedTiming)
  double Gecko_GetPositionInSegment(aSegment, aProgress, aBeforeFlag)

?

::: layout/style/ServoBindings.h:190
(Diff revision 3)
> +double Gecko_ComputedTimingProgress(RawGeckoComputedTimingBorrowed aComputedTiming);
> +double Gecko_SegmentGetPosition(RawGeckoAnimationPropertySegmentBorrowed aSegment,

I wonder if the names could be more clear.

For the first one:

  Gecko_GetProgressFromComputedTiming?
  Gecko_GetProgress?

For the second one:

  Gecko_GetPositionInSegment?
  Gecko_GetSegmentPortion?

::: layout/style/ServoBindings.cpp:523
(Diff revision 3)
> +  MOZ_ASSERT(aSegment->mFromKey != aSegment->mToKey,
> +             "The segment from and to keys should be different");

Perhaps we should just assert that mFromKey < mToKey?
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.

https://reviewboard.mozilla.org/r/125250/#review128830

::: layout/style/ServoBindings.cpp:539
(Diff revision 3)
>  
> +RawServoAnimationValueBorrowedOrNull
> +Gecko_AnimationGetBaseStyle(void* aBaseStyles, nsCSSPropertyID aProperty)
> +{
> +  auto base =
> +    reinterpret_cast<nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>*>

I think you changed the wrong reinterpret_cast to a static_cast? (Although we probably should change the other one too unless I'm mistaken.)

::: servo/ports/geckolib/glue.rs:260
(Diff revision 3)
>      use style::properties::animated_properties::AnimationValueMap;
>  
> -    let property: TransitionProperty = property.into();
> +    let property: TransitionProperty = css_property.into();
>      let value_map = RwLock::<AnimationValueMap>::as_arc(&raw_value_map);
>  
> -    let from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
> +    // if either of segment endpoints are null, get the underlying value to use

s/if/If/
s/of segments/of the segments/

::: servo/ports/geckolib/glue.rs:278
(Diff revision 3)
> +        // This should not happen. But if it happened, we'd prefer *turbulent*
> +        // animation rather than crash.
> +        return;

As discussed, let's add a warning here and drop the comment.
Attachment #8853178 - Flags: review?(bbirtles) → review+
Comment on attachment 8853180 [details]
Bug 1311257 - Support missing keyframes handling for stylo.

https://reviewboard.mozilla.org/r/125254/#review128832
Attachment #8853180 - Flags: review?(bbirtles) → review+
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.

https://reviewboard.mozilla.org/r/125246/#review128822

We need to get |aComputedTiming|'s mProgress in Rust side, and mProgress is Nullable<double> in ComputedTiming.  Are you suggesting we pass mProgress as an argument of Servo_AnimationCompose()?
I was just suggesting it would be neater to be able to convert the whole struct at once. Then when we calculate the portion of a segment, just pass in the required info: progress and before flag.
Ah, OK. I think I understand it now. 

We can already get mBeforeFlag from ComputedTiming in Rust, so how about this:

let progress = unsafe { Gecko_ProgressFromComputedTimingProgress(computed_timing); };
...

let position = unsafe { Gecko_PositionInSegment(segment, progress, computed_timing.mBeforeFlag); }

?
Oh, I see. Yes, that's probably better (although I assume Gecko_ProgressFromComputedTimingProgress is a typo). If we support Nullable in the future then we can just drop the extra call.
(In reply to Brian Birtles (:birtles) from comment #79)
> Oh, I see. Yes, that's probably better (although I assume
> Gecko_ProgressFromComputedTimingProgress is a typo).

Yeah, it means we have been made *much* progress. ;-)
Comment on attachment 8853174 [details]
Bug 1311257 - Move AnimationPropertySegment in a separate header and expose it in FFI.

https://reviewboard.mozilla.org/r/125242/#review129154
Attachment #8853174 - Flags: review?(cam) → review+
Comment on attachment 8853175 [details]
Bug 1311257 - Expose ComputedTiming to FFI.

https://reviewboard.mozilla.org/r/125244/#review129156

::: commit-message-2b78a:1
(Diff revision 3)
> +Bug 1311257 - Expose ComptuedTiming to FFI. r?heycam

ComputedTiming
Attachment #8853175 - Flags: review?(cam) → review+
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review128812

> Why is this guaranteed to be non-null?

This EnsureBaseStyles() is called right after getting servo's computed values, we can't get the servo's computed values without nsPresContext.
Comment on attachment 8853177 [details]
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.

https://reviewboard.mozilla.org/r/125248/#review128824

> This should be 0 not 0.5 right?

Exactly. It's a terrible mistake. Thank you!
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review128812

> This EnsureBaseStyles() is called right after getting servo's computed values, we can't get the servo's computed values without nsPresContext.

We need either a comment or an assertion message to say that.
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.

https://reviewboard.mozilla.org/r/125246/#review129172

::: commit-message-b8e90:4
(Diff revision 4)
> +Bug 1311257 - Add FFI functions to get progress value and current position in a segment. r?birtles
> +
> +Two functions added in this patch get progress value from ComputedTiming
> +or get position in a given AnimationPropertySegment with ComputedTiming.

(This comment is no longer quite accurate. It should just say 'get the position in a given AnimationPropertySegment' or perhaps 'get the position in a given AnimationPropertySegment given the relevant timing properties'.)

::: layout/style/ServoBindings.cpp:524
(Diff revision 4)
> +  MOZ_ASSERT(aSegment->mFromKey < aSegment->mToKey,
> +             "The segment to key should be greater than from key");

(It's a bit odd that the code and message are opposites. Mind making them the same?)
Attachment #8853176 - Flags: review?(bbirtles) → review+
Comment on attachment 8853177 [details]
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.

https://reviewboard.mozilla.org/r/125248/#review129182

::: dom/animation/KeyframeEffectReadOnly.cpp
(Diff revision 4)
>    }
>  
> -  // Special handling for zero-length segments
> +  Servo_AnimationCompose(&aAnimationValues,
> -  if (aSegment.mToKey == aSegment.mFromKey) {
> -    if (aComputedTiming.mProgress.Value() < 0) {
> -      Servo_AnimationValueMap_Push(&aAnimationValues,

Are any of these Servo_Animation* functions now unused, and can be removed?
Attachment #8853177 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away Apr 1-4) from comment #102)
> Comment on attachment 8853177 [details]
> Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.
> 
> https://reviewboard.mozilla.org/r/125248/#review129182
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> (Diff revision 4)
> >    }
> >  
> > -  // Special handling for zero-length segments
> > +  Servo_AnimationCompose(&aAnimationValues,
> > -  if (aSegment.mToKey == aSegment.mFromKey) {
> > -    if (aComputedTiming.mProgress.Value() < 0) {
> > -      Servo_AnimationValueMap_Push(&aAnimationValues,
> 
> Are any of these Servo_Animation* functions now unused, and can be removed?

Probably. But I am not yet convinced which one will not be used for SMIL either. So I'd like to leave them for now.
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.

https://reviewboard.mozilla.org/r/125250/#review129190

::: layout/style/ServoBindingList.h:211
(Diff revision 4)
>  SERVO_BINDING_FUNC(Servo_AnimationCompose, void,
>                     RawServoAnimationValueMapBorrowed,
> +                   void*,
>                     nsCSSPropertyID property,
>                     RawGeckoAnimationPropertySegmentBorrowed,
>                     RawGeckoComputedTimingBorrowed)

Please document what the void* argument is for.  (It might be nice to give names to this and the other arguments, too.)

::: layout/style/ServoBindings.h:196
(Diff revision 4)
> +RawServoAnimationValueBorrowedOrNull Gecko_AnimationGetBaseStyle(
> +  void* aBaseStyles,
> +  nsCSSPropertyID aProperty);

And document it here too.
Attachment #8853178 - Flags: review?(cam) → review+
Comment on attachment 8853181 [details]
Bug 1311257 - Update test expectations.

https://reviewboard.mozilla.org/r/125256/#review129194
Attachment #8853181 - Flags: review?(cam) → review+
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review129232

::: dom/animation/KeyframeEffectReadOnly.cpp:474
(Diff revision 4)
> +  for (const AnimationProperty& property : aProperties) {
> +    for (const AnimationPropertySegment& segment : property.mSegments) {
> +      if (segment.HasReplaceableValues()) {
> +        continue;
> +      }
> +
> +      EnsureBaseStyle(property.mProperty,
> +                      pseudoAtom,
> +                      presContext,
> +                      baseComputedValues);
> +      break;
> +    }
> +  }
> +}
> +
> +void
> +KeyframeEffectReadOnly::EnsureBaseStyle(
> +  nsCSSPropertyID aProperty,
> +  nsIAtom* aPseudoAtom,
> +  nsPresContext* aPresContext,
> +  RefPtr<ServoComputedValues>& aBaseComputedValues)
> +{
> +  if (!aBaseComputedValues) {
> +    aBaseComputedValues =
> +      aPresContext->StyleSet()->AsServo()->
> +        GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> +  }
> +  RefPtr<RawServoAnimationValue> baseValue =
> +    Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> +                                               aProperty).Consume();
> +  mBaseStyleValuesForServo.Put(aProperty, baseValue);
> +}

Oh, I actually meant to factor out a method for the inner for-loop so we don't have a break within a nested for-loop.

Does that work?
Attachment #8853173 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #106)
> Comment on attachment 8853173 [details]
> Bug 1311257 - Store base styles for stylo.
> 
> https://reviewboard.mozilla.org/r/125240/#review129232
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp:474
> (Diff revision 4)
> > +  for (const AnimationProperty& property : aProperties) {
> > +    for (const AnimationPropertySegment& segment : property.mSegments) {
> > +      if (segment.HasReplaceableValues()) {
> > +        continue;
> > +      }
> > +
> > +      EnsureBaseStyle(property.mProperty,
> > +                      pseudoAtom,
> > +                      presContext,
> > +                      baseComputedValues);
> > +      break;
> > +    }
> > +  }
> > +}
> > +
> > +void
> > +KeyframeEffectReadOnly::EnsureBaseStyle(
> > +  nsCSSPropertyID aProperty,
> > +  nsIAtom* aPseudoAtom,
> > +  nsPresContext* aPresContext,
> > +  RefPtr<ServoComputedValues>& aBaseComputedValues)
> > +{
> > +  if (!aBaseComputedValues) {
> > +    aBaseComputedValues =
> > +      aPresContext->StyleSet()->AsServo()->
> > +        GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> > +  }
> > +  RefPtr<RawServoAnimationValue> baseValue =
> > +    Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> > +                                               aProperty).Consume();
> > +  mBaseStyleValuesForServo.Put(aProperty, baseValue);
> > +}
> 
> Oh, I actually meant to factor out a method for the inner for-loop so we
> don't have a break within a nested for-loop.
> 
> Does that work?

Oops. I did push revised patch coincidently.  Yeah, that should work. I will revise it.
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.

https://reviewboard.mozilla.org/r/125240/#review129732

Sorry for the delay. I didn't notice that this review had been updated.

::: dom/animation/KeyframeEffectReadOnly.h:426
(Diff revision 6)
>    // base style context will be resolved and stored in
>    // |aCachedBaseStyleContext|.
>    void EnsureBaseStyle(nsCSSPropertyID aProperty,
>                         nsStyleContext* aStyleContext,
>                         RefPtr<nsStyleContext>& aCachedBaseStyleContext);
> +  // Stylo version of the above function.

Stylo version of the above function that also first checks for an additive value in |aProperty|'s list of segments.

?

::: dom/animation/KeyframeEffectReadOnly.cpp:455
(Diff revision 6)
>  void
>  KeyframeEffectReadOnly::EnsureBaseStyles(
> +  const ServoComputedValuesWithParent& aServoValues,
> +  const nsTArray<AnimationProperty>& aProperties)
> +{

In the header file, the Gecko version comes first followed by the Servo version but in the cpp file its the reverse. Can we move these two methods down below the Gecko versions?

::: dom/animation/KeyframeEffectReadOnly.cpp:492
(Diff revision 6)
> +  for (const AnimationPropertySegment& segment : aProperty.mSegments) {
> +    if (segment.HasReplaceableValues()) {
> +      continue;
> +    }
> +
> +    if (!aBaseComputedValues) {
> +      aBaseComputedValues =
> +        aPresContext->StyleSet()->AsServo()->
> +          GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> +    }
> +    RefPtr<RawServoAnimationValue> baseValue =
> +      Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> +                                                 aProperty.mProperty).Consume();
> +    mBaseStyleValuesForServo.Put(aProperty.mProperty, baseValue);
> +    return;
> +  }

(Now that this is a separate method, I think it would be more clear to just write:

bool hasAdditiveValues = false;

for (const AnimationPropertySegment& segment : aProperty.mSegments) {
  if (!segment.HasReplaceableValues()) {
    hasAdditiveValues = true;
    break;
  }
}

if (!hasAdditiveValues) {
  return;
}

...

But it's up to you which you prefer.)
Attachment #8853173 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #128)

> ::: dom/animation/KeyframeEffectReadOnly.cpp:492
> (Diff revision 6)
> > +  for (const AnimationPropertySegment& segment : aProperty.mSegments) {
> > +    if (segment.HasReplaceableValues()) {
> > +      continue;
> > +    }
> > +
> > +    if (!aBaseComputedValues) {
> > +      aBaseComputedValues =
> > +        aPresContext->StyleSet()->AsServo()->
> > +          GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> > +    }
> > +    RefPtr<RawServoAnimationValue> baseValue =
> > +      Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> > +                                                 aProperty.mProperty).Consume();
> > +    mBaseStyleValuesForServo.Put(aProperty.mProperty, baseValue);
> > +    return;
> > +  }
> 
> (Now that this is a separate method, I think it would be more clear to just
> write:
> 
> bool hasAdditiveValues = false;
> 
> for (const AnimationPropertySegment& segment : aProperty.mSegments) {
>   if (!segment.HasReplaceableValues()) {
>     hasAdditiveValues = true;
>     break;
>   }
> }
> 
> if (!hasAdditiveValues) {
>   return;
> }

Oh, thanks. This looks more consistent with our coding rules.
A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e289630e5d738e2ddadee29e465e9943c3908ed4

I will pull binding changes from this try, and send a PR with the binding changes.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #130)
> A final try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e289630e5d738e2ddadee29e465e9943c3908ed4
> 
> I will pull binding changes from this try, and send a PR with the binding
> changes.

A commit for servo directory was missed in the try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43c5ad8e17e655785296f98b77350d026f0212c4
The try looks good so far.
https://github.com/servo/servo/pull/16280
Attachment #8853168 - Attachment is obsolete: true
Attachment #8853169 - Attachment is obsolete: true
Attachment #8853170 - Attachment is obsolete: true
Attachment #8853175 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f9acbd78af1
Add a function that returns a base computed values (i.e. computed values without any animations rules). r=heycam
https://hg.mozilla.org/integration/autoland/rev/b66975fb5bde
Add an FFI function that returns an AnimationValue for a given nsCSSPropertyID from computed values. r=heycam
https://hg.mozilla.org/integration/autoland/rev/96818915960d
Store base styles for stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ae42776a3d46
Move AnimationPropertySegment in a separate header and expose it in FFI. r=heycam
https://hg.mozilla.org/integration/autoland/rev/71b603b0f1cd
Add FFI functions to get progress value and current position in a segment. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f19d868126f3
Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust. r=heycam
https://hg.mozilla.org/integration/autoland/rev/17e382a02976
Use underlying value for missing keyframes. r=birtles,heycam
https://hg.mozilla.org/integration/autoland/rev/c1f723812558
getKeyframes() returns base computed values in missing keyframes. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d5b7c664155e
Support missing keyframes handling for stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7fc07c5261d7
Update test expectations. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bab909e1e4d3
Update comment refering to bug 1311257. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: