Closed Bug 1317209 Opened 8 years ago Closed 7 years ago

Stylo: Interpolate servo animation values using and add to the cascade

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

Details

Attachments

(10 files, 1 obsolete file)

736 bytes, text/html
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
41 bytes, text/x-github-pull-request
Details | Review
41 bytes, text/x-github-pull-request
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details
This bug covers a few different pieces that are probably easiest done together so they can be tested. Specifically.

1. Take the AnimationValue objects stored in bug 1317208.
2. Call into Servo code to interpolate them to create new values.
   (Ignore compositing values for now--we'll deal with that in a separate bug.)
3. Uncompute Servo computed values to ServoDeclarationBlock (or something
   similar) for re-adding to the cascade. Apparently we have a function for this
   (I think either Manish or Emilio wrote this?) but we've yet to use it.
4. Put the values back into the cascade. I think Servo/Stylo's handling of the
   style attribute is mostly similar and we can use a very similar approach.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Hi Brian, I think I may need your advises to make Animation::ComposeStyle() and KeyframeEffectReadOnly::ComposeStyle() work in stylo. (or don't need to do this?) I would like to test the interpolation between two RawServoAnimationValues, and we do interpolation in KeyframeEffectReadOnly::ComposeStyle(). However, it looks like EffectCompositor doesn't work now, and I cannot get a valid RawServoAnimationValue pair in KeyframeEffectReadOnly::ComposeStyle(). Both AnimationPropertySegment::mServoFromValue/mServoToValue are nullptr in ComposeStyle() by the simple test case: div.animate() API. 

Another problem is: In KeyframeEffectReadOnly::ComposeStyle(), GetPresContext()->StyleSet()->IsServo() returns false, but in other places, e.g. GetComputedKeyframeValues, the servo backend flag is true. :(
Flags: needinfo?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #1)
> Hi Brian, I think I may need your advises to make Animation::ComposeStyle()
> and KeyframeEffectReadOnly::ComposeStyle() work in stylo. (or don't need to
> do this?) I would like to test the interpolation between two
> RawServoAnimationValues, and we do interpolation in
> KeyframeEffectReadOnly::ComposeStyle(). However, it looks like
> EffectCompositor doesn't work now, and I cannot get a valid
> RawServoAnimationValue pair in KeyframeEffectReadOnly::ComposeStyle(). Both
> AnimationPropertySegment::mServoFromValue/mServoToValue are nullptr in
> ComposeStyle() by the simple test case: div.animate() API. 
> 
> Another problem is: In KeyframeEffectReadOnly::ComposeStyle(),
> GetPresContext()->StyleSet()->IsServo() returns false, but in other places,
> e.g. GetComputedKeyframeValues, the servo backend flag is true. :(

Looks like we cannot restyle for animation now. I tried heycam's idea to hack the nsRestyleHint by Self|Subtree in EffectCompositor::RestyleForAnimation(), and the error message [1] was disappeared, but ComposeStyle still didn't be called.

[1] https://dxr.mozilla.org/servo/rev/94d62a2afbbbb08d0225dd45fd8a8480d4bdf98d/components/style/restyle_hints.rs#52
Boris, I just realized that ServoRestyleManager does not call EffectCompositor::AddStyleUpdatesTo() at all.  We need to invoke it in ProcessPendingRestyle(I am not sure this is a right place though).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Boris, I just realized that ServoRestyleManager does not call
> EffectCompositor::AddStyleUpdatesTo() at all.  We need to invoke it in
> ProcessPendingRestyle(I am not sure this is a right place though).

It is possible because ServoRestyleManager doesn't handle animations. :(
Blocks: 1329878
I thought EffectCompositor::AddStyleUpdatesTo() was only used when we need to bring animations up-to-date? I thought for regular animation samples the flow was that we post a restyle to the RestyleManager (with the eRestyle_CSSAnimations hint); then when we gather rule processors in nsStyleSet::GatherRuleProcessors we pick up the AnimationStyleRuleProcessor that calls EffectCompositor::GetAnimationRule that ends up calling MaybeUpdateAnimationRule -> ComposeAnimationRule? That is we don't actually compose style until we go to get the animation rule.

When I had a brief look, I thought the handling of the style attribute in Servo/Stylo was similar and we could probably try following that pattern for adding animation rules to the cascade.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #5)
> I thought EffectCompositor::AddStyleUpdatesTo() was only used when we need
> to bring animations up-to-date? I thought for regular animation samples the
> flow was that we post a restyle to the RestyleManager (with the
> eRestyle_CSSAnimations hint); then when we gather rule processors in
> nsStyleSet::GatherRuleProcessors we pick up the AnimationStyleRuleProcessor
> that calls EffectCompositor::GetAnimationRule that ends up calling
> MaybeUpdateAnimationRule -> ComposeAnimationRule? That is we don't actually
> compose style until we go to get the animation rule.
> 
> When I had a brief look, I thought the handling of the style attribute in
> Servo/Stylo was similar and we could probably try following that pattern for
> adding animation rules to the cascade.

Thanks, Brian. I am going to check the flow you mentioned to understand what happened.
Hi Manish,

I'm not familiar with the FFI part in Stylo, could you please give me some advises about this question? I'd like to call a Gecko_XXXX() from Rust side, and this Gecko_XXXX() should output a PropertyDeclarationBlock object (which is the uncomputed interpolation values).

e.g.
The interface of the Gecko_XXXX may be:
  Gecko_GetAnimationRule(RawGeckoElementBorrowed elem,
                         CSSPseudoElementType type,
                         RawServoDeclarationBlockBorrowed decl);  // |decl| is my output

This function uncomputes the AnimationValues, and put the answer to |decl|, so I might do something like this in this function:

  void
  Gecko_GetAnimationRule(RawGeckoElementBorrowed elem,
                         CSSPseudoElementType type,
                         RawServoDeclarationBlockBorrowed decl)
  {
    ...
    // check EffectSet on (|elem| & |type|)
    ...

    if (has animation) {
      // Get a nsTArray<RawServoAnimationValue>, animationValues.
      Servo_AnimationValues_Uncomputed(animationValues, decl); // This will put the output into |decl|.
    }
  }

My question is: How do I convert an empty |PropertyDeclarationBlock| object into |RawServoDeclarationBlockBorrowed| from Rust? I tried to do something like this (in a method of TElement):

  let answer = Arc::new(RwLock::new(PropertyDeclarationBlock { declaratoins: vec![], important_count: 0 }));
  unsafe{ Gecko_GetAnimationRule(elem, transmute(&*answer)) };
  ... // put anwser back to cascade in match_element.

I'm not sure which method I can use, so use transmute() directly. I think the FFI type of RwLock<PropertyDeclarationBlock> is RawServoDeclarationBlock, so deref |answer| and than get its shared ref. However, this is not correct, I got a crash while writing values into |answer| in Rust side.


Another way is to let Gecko_GetAnimationRule() returns a RawServoDeclarationBlockStrong (is it correct?) directly:

  RawServoDeclarationBlockStrong Gecko_GetAnimationRule(...)
  {
    ...
    RefPtr<RawServoDeclarationBlock> ret = Servo_AnimationValues_Uncomputed(animationValues).Consume();
    return reinterpret_cast<RawServoDeclarationBlock*>(ret.forget().take()); // not sure this part, because I still got a crash.
  }

What do you think about this FFI? Or should I revise its interface? Sorry to bother you about this FFI problem. Thank you.
Flags: needinfo?(manishearth)
(In reply to Boris Chiou [:boris] from comment #7)
>   let answer = Arc::new(RwLock::new(PropertyDeclarationBlock { declaratoins:
> vec![], important_count: 0 }));
>   unsafe{ Gecko_GetAnimationRule(elem, transmute(&*answer)) };
>   ... // put anwser back to cascade in match_element.

Looks like my usage of transmute is not correct. After applying "arc_as_borrowed", I can convert the &Arc<ServoType> into &GeckoType (i.e. XXXXBorrowed).
Flags: needinfo?(manishearth)
Please don't transmute on the Servo side :)

arc_as_borrowed is correct. glue.rs has lots of examples of types being converted from gecko form to servo form, usually searching through that helps find the operation you need.
(In reply to Manish Goregaokar [:manishearth] from comment #9)
> Please don't transmute on the Servo side :)
> 
> arc_as_borrowed is correct. glue.rs has lots of examples of types being
> converted from gecko form to servo form, usually searching through that
> helps find the operation you need.

Thanks for the reminder. Now Element.animate() works for some simple test cases. However, I have to figure out how to pass the pseudo element type from Rust to Gecko because we need to the pair, CSSPseudoElementType and dom::Element, to locate an AnimationTarget.
(In reply to Boris Chiou [:boris] from comment #10)
> Now Element.animate() works for some simple test
> cases. However, I have to figure out how to pass the pseudo element type
> from Rust to Gecko because we need to the pair, CSSPseudoElementType and
> dom::Element, to locate an AnimationTarget.


That's great to hear! :)

Passing the pseudo from Rust to Gecko should be easy. In Rust the PseudoElementType contains an `Atom`, which in turn just wraps `*mut nsIAtom`. You can pass that atom down to Gecko, and check whether that atom is nsCSSPseudoElements::before or nsCSSPseudoElements::after for animation purposes :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Passing the pseudo from Rust to Gecko should be easy. In Rust the
> PseudoElementType contains an `Atom`, which in turn just wraps `*mut
> nsIAtom`. You can pass that atom down to Gecko, and check whether that atom
> is nsCSSPseudoElements::before or nsCSSPseudoElements::after for animation
> purposes :)

Cool! Thanks, Emilio. Let me try it. :)
Hi Brian,

I have a question about the owning/non-owning type for PseudoElementHashEntry::mElement. What do you think if I make PseudoElementHashEntry::mElement not ref-counted [1]? Is it possible the dom::Element is destroyed before we finish the restyle for animations? (e.g. The hashtable used by nsSMILAnimationController also doesn't use owning SVGAnimationElement. For the case of dom::Element, I guess someone may still hold it. However, I'm not sure whether it is possible that it becomes a dangling pointer in the throttle case)

I ask this because dom::Element is not thread-safe ref-counted. I'd like to use the similar flow of EffectCompositor::GetAnimationRule and EffectCompositor::RequestRestyle:
    We put the |Element| and |CSSPseudoElementType| into EffectCompositor::mElementsToRestyle in RequestRestyle() [2], and after calling ComposeStyle(), we remove the entry [3]. Therefore, we will not call PostRestyleForAnimation too many times. In my implementation, we pass GeckoElement and call Animation::ComposeStyle() from Servo code, so we have thread-safe issue if we want to remove the entry.

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/animation/PseudoElementHashEntry.h#52
[2] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/animation/EffectCompositor.cpp#272
[3] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/animation/EffectCompositor.cpp#373
Flags: needinfo?(bbirtles)
Yes, I'm pretty sure the dom::Element can be destroyed before we finish the restyle. The call to RequestRestyle where we add the entry might even happen in a separate microtask. So I don't think you want to make that a non-owning reference (or, if you do, it should be some other type of reference that won't dangle, like a weak pointer).

I don't know much about the Stylo restyling process but I thought there was some kind of second step where we update data on the Gecko side. Is it possible to leave mElementsToRestyle as-is when calling ComposeStyle from Servo, then, in that second, step clear the entries from the mElementsToRestyle hashtable that we just restyled?

I guess that depends on what sort of information we have. If we always restyled all elements then we could just clear mElementsToRestyle, but I guess we restyle subtrees and skip elements that are running on the compositor etc. So perhaps we need to generate a list of elements whose animation style we updated and then, back on the Gecko side, iterate over that list and drop any matching entries from mElementsToRestyle (that sounds inefficient, but I think it has the some complexity as what we currently do, i.e. one hashtable lookup per restyled element).
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #15)
> I don't know much about the Stylo restyling process but I thought there was
> some kind of second step where we update data on the Gecko side. Is it
> possible to leave mElementsToRestyle as-is when calling ComposeStyle from
> Servo, then, in that second, step clear the entries from the
> mElementsToRestyle hashtable that we just restyled?

Thanks, Brian. I think I could use the "some kind of second step where we update data on the Gecko side" to clear the elements from mElementsToRestyle. Cameron suggests that I can check ServoRestyleManager::RecreateStyleContexts(), so I tried to clear mElementsToRestyle hashtable in that function (or its caller), and it looks good to me now. We don't support animations on compositor, so let's fix it later. i.e. just clear the entire hashtable without checking another list.
Attachment #8828728 - Flags: review?(manishearth)
Attachment #8828729 - Flags: review?(manishearth)
Attachment #8828730 - Flags: review?(hikezoe)
Attachment #8828731 - Flags: review?(cam)
Attachment #8828732 - Flags: review?(cam)
Attachment #8828733 - Flags: review?(emilio+bugs)
Attachment #8828733 - Flags: review?(cam)
Attachment #8828734 - Flags: review?(emilio+bugs)
Attachment #8828734 - Flags: review?(cam)
(In reply to Boris Chiou [:boris] from comment #20)
> We ref-count nsDOMNavigationTiming in DocumentTimeline, and those
> functions may be called from Servo side, so we need to make it
> thread-safe.

Which functions do we call on DocumentTimeline from Servo?  It looks like the ones that grab the DocumentTimeline and put it in a RefPtr don't really need to hold a strong reference, since the Document keeps a strong reference, and we don't do anything that would cause the Document to release it.
Comment on attachment 8828733 [details]
Bug 1317209 - Part 7: Skip some crash tests due to unimplemented features.

https://reviewboard.mozilla.org/r/105084/#review107032

::: servo/components/style/stylist.rs:461
(Diff revision 1)
>      pub fn push_applicable_declarations<E, V>(
>                                          &self,
>                                          element: &E,
>                                          parent_bf: Option<&BloomFilter>,
>                                          style_attribute: Option<&Arc<RwLock<PropertyDeclarationBlock>>>,
> +                                        anim_attribue: Option<Arc<RwLock<PropertyDeclarationBlock>>>,

Let's call this (here and everywhere else) `animation_rule` instead of `animation_attribute`?

::: servo/components/style/stylist.rs:534
(Diff revision 1)
>  
>              debug!("style attr: {:?}", relations);
>  
> -            // Step 5: Author-supplied `!important` rules.
> +            // Step 5: Animation attributes.
> +            if let Some(anim) = anim_attribue {
> +                relations |= AFFECTED_BY_STYLE_ATTRIBUTE;

Why? We need an `AFFECTED_BY_ANIMATIONS`, I think. This is ok for now because the effect is the same (disabling sharing), but please at least write a `TODO` comment here to add such a flag.
Attachment #8828733 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8828734 [details]
Bug 1317209 - Part 7: Support transition cascade level.

https://reviewboard.mozilla.org/r/105086/#review107034

::: layout/style/ServoBindings.h:166
(Diff revision 1)
>  // Animations
>  RawServoDeclarationBlockStrong
>  Gecko_GetAnimationRuleAttribute(RawGeckoElementBorrowed aElement,
> -                                nsIAtom* aPseudoTag);
> +                                nsIAtom* aPseudoTag,
> +                                mozilla::EffectCompositor::CascadeLevel
> +                                  aCascade);

Let's call this argument `aCascadeLevel`, or `aLevel` at least.

::: servo/components/style/dom.rs:254
(Diff revision 1)
>      fn style_attribute(&self) -> Option<&Arc<RwLock<PropertyDeclarationBlock>>>;
>  
>      /// Get this element's animation rule attribute.
>      fn get_animation_rule_attribute(&self, _pseudo: Option<&PseudoElement>)
> -                                    -> Option<Arc<RwLock<PropertyDeclarationBlock>>> {
> -        None
> +                                    -> (Option<Arc<RwLock<PropertyDeclarationBlock>>>,
> +                                        Option<Arc<RwLock<PropertyDeclarationBlock>>>) {

Let's make this a struct (`AnimationRules`?), and rename the method to `get_animation_rules`.

::: servo/components/style/stylist.rs:592
(Diff revision 1)
>          debug!("UA important: {:?}", relations);
>  
> +        // Step 10: Transition attributes.
> +        // The transitions sheet (CSS transitions that are tied to CSS markup)
> +        if let Some(anim) = anim_attribue.1 {
> +            relations |= AFFECTED_BY_STYLE_ATTRIBUTE;

Same here regarding `AFFECTED_BY_STYLE_ATTRIBUTE`. Since we potentially want to differenciate between animations and transitions in the future (for example, replacing transition rules is way more straight-forward) let's add `AFFECTED_BY_TRANSITIONS` too to rust-selectors).
Attachment #8828734 - Flags: review?(emilio+bugs) → review+
I made the PR to rust-selectors to support the new AFFECTING flags: https://github.com/servo/rust-selectors/pull/102
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #25)
> (In reply to Boris Chiou [:boris] from comment #20)
> > We ref-count nsDOMNavigationTiming in DocumentTimeline, and those
> > functions may be called from Servo side, so we need to make it
> > thread-safe.
> 
> Which functions do we call on DocumentTimeline from Servo?  It looks like
> the ones that grab the DocumentTimeline and put it in a RefPtr don't really
> need to hold a strong reference, since the Document keeps a strong
> reference, and we don't do anything that would cause the Document to release
> it.

The call stack is like this:

Hit MOZ_CRASH(nsDOMNavigationTiming not thread-safe) at /dom/base/nsDOMNavigationTiming.h:31
#01: mozilla::dom::DocumentTimeline::ToTimelineTime(mozilla::TimeStamp const&)
#02: mozilla::dom::DocumentTimeline::GetCurrentTime() const
#03: mozilla::dom::Animation::GetCurrentTime() const
#04: mozilla::dom::Animation::PlayState() const
#05: mozilla::dom::Animation::ComposeStyle(mozilla::AnimationRuleType&, nsCSSPropertyIDSet const&)

I call Animation::ComposeStyle() in part 5, and DocumentTimeline::ToTimelineTime() ref-counts nsDOMNavigationTiming [1]. I think I could revise that to only hold a raw pointer of nsDOMNavigationTiming object.

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/animation/DocumentTimeline.cpp#96,121
(In reply to Emilio Cobos Álvarez [:emilio] from comment #28)
> I made the PR to rust-selectors to support the new AFFECTING flags:
> https://github.com/servo/rust-selectors/pull/102

Oh, Thanks a lot. I will revise my code to use that. (I believe the PR will be merged sooner.)
Attachment #8828733 - Flags: review?(cam)
Attachment #8828728 - Flags: review?(cam)
Attachment #8828729 - Flags: review?(cam)
Comment on attachment 8828730 [details]
Bug 1317209 - Part 3: Add constness to GetEffectSet.

https://reviewboard.mozilla.org/r/104458/#review107182
Attachment #8828730 - Flags: review?(hikezoe) → review+
Comment on attachment 8828729 [details]
Bug 1317209 - Part 2: Call Servo's Interpolation from Gecko.

https://reviewboard.mozilla.org/r/104456/#review107196

servo part looks fine
Attachment #8828729 - Flags: review?(manishearth) → review+
Comment on attachment 8828728 [details]
Bug 1317209 - Part 1: Introduce ServoAnimationRule and implement uncompute FFI.

https://reviewboard.mozilla.org/r/104454/#review107194

servo part looks fine
Attachment #8828728 - Flags: review?(manishearth) → review+
(In reply to Boris Chiou [:boris] from comment #29)
> I call Animation::ComposeStyle() in part 5, and
> DocumentTimeline::ToTimelineTime() ref-counts nsDOMNavigationTiming [1]. I
> think I could revise that to only hold a raw pointer of
> nsDOMNavigationTiming object.

OK great, let's change that to use a raw pointer.
Comment on attachment 8828728 [details]
Bug 1317209 - Part 1: Introduce ServoAnimationRule and implement uncompute FFI.

https://reviewboard.mozilla.org/r/104454/#review107372

::: dom/animation/ServoAnimationRule.h:30
(Diff revision 1)
> +  ServoAnimationRule() = default;
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ServoAnimationRule)
> +
> +  void AddValue(nsCSSPropertyID aProperty,
> +                const RefPtr<RawServoAnimationValue>& aValue);

Make this RawServoAnimationValue* instead?

::: dom/animation/ServoAnimationRule.h:36
(Diff revision 1)
> +  nsDataHashtable<nsUint32HashKey, RefPtr<RawServoAnimationValue>>
> +    mAnimationValues;

You should use nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue> instead, which is designed for values being reference counted objects.  I'm not sure what the consequences are of using nsDataHashtable with RefPtr<...> as a value -- it might do more AddRef/Release calls than it needs to.

::: dom/animation/ServoAnimationRule.h:41
(Diff revision 1)
> +  nsDataHashtable<nsUint32HashKey, RefPtr<RawServoAnimationValue>>
> +    mAnimationValues;
> +};
> +
> +// A wrapper for animation rules.
> +struct AnimationRuleType {

To me, AnimationRuleType sounds like an enum that could be either "Gecko" or "Servo".  How about just AnimationRule, and make the two members mGecko and mServo.

Nit: "{" on new line.

Also, ServoAnimationRule.h doesn't seem to be the best place for this, but apart from a new file AnimationRule.h I'm not sure where else it could go.

::: dom/animation/ServoAnimationRule.cpp:23
(Diff revision 1)
> +  // FIXME: Pass the hash table into the FFI directly.
> +  nsTArray<RefPtr<RawServoAnimationValue>> values(mAnimationValues.Count());
> +  auto iter = mAnimationValues.ConstIter();
> +  for (; !iter.Done(); iter.Next()) {
> +    values.AppendElement(iter.Data());
> +  }
> +  return Servo_AnimationValues_Uncompute(&values);

Yeah, it would be nice at least to avoid the extra AddRef/Release on each of the RawServoAnimationValues.  Since we know that mAnimationValues keeps those strong references, we could just change the RawServoAnimationValueListBorrowed to something like RawServoAnimationValueBorrowedListBorrowed, where we store raw pointers in the nsTArray.

::: servo/ports/geckolib/glue.rs:169
(Diff revision 1)
> +pub extern "C" fn Servo_AnimationValues_Uncompute(value: RawServoAnimationValueListBorrowed)
> +     -> RawServoDeclarationBlockStrong
> +{
> +    let uncomputed_values = value.into_iter()
> +                                 .map(|v| {
> +                                     let raw_anim = unsafe{ v.mRawPtr.as_ref().unwrap() };

Nit: space before "{".
Attachment #8828728 - Flags: review?(cam) → review+
Comment on attachment 8828729 [details]
Bug 1317209 - Part 2: Call Servo's Interpolation from Gecko.

https://reviewboard.mozilla.org/r/104456/#review107374

::: dom/animation/KeyframeEffectReadOnly.cpp:547
(Diff revision 1)
> +                                         positionInSegment,
> +                                         computedTiming.mBeforeFlag);
> +
> +    MOZ_ASSERT(IsFinite(valuePosition), "Position value should be finite");
> +
> +    if (isServoBackend) {

It would be good to remove the duplicated code between the two branches.  For example, we have the zero-length segment handling and the interpolate/<0.5/>=0.5 cases, which look very similar in the two branches.

I don't know how easy it would be to factor this code out to something that worked on both backend types.  Maybe a template that can work on both types.

For now I'm OK with keeping this, but once we add the accumulate and addition support, I think we'll need to deal with the duplication somehow.

::: dom/animation/KeyframeEffectReadOnly.cpp:550
(Diff revision 1)
> +      RefPtr<RawServoAnimationValue> servoFromValue = segment->mServoFromValue;
> +      RefPtr<RawServoAnimationValue> servoToValue = segment->mServoToValue;

We don't need to store another strong reference to these, since |segment| will hold on to them.

::: dom/animation/KeyframeEffectReadOnly.cpp:585
(Diff revision 1)
> +      continue;
> +    }

I think it would be clearer to turn this into an "else" and put all of the Gecko backend work into the else branch.
Attachment #8828729 - Flags: review?(cam) → review+
Comment on attachment 8828731 [details]
Bug 1317209 - Part 4: Don't ref-count nsDOMNavigationTiming in DocumentTimeline.

https://reviewboard.mozilla.org/r/105080/#review107378

Let's remove the use of RefPtr in the relevant DocumentTimeline method instead.
Attachment #8828731 - Flags: review?(cam)
High level question: why are we storing both the Gecko AnimValuesStyleRule and the Servo ServoAnimationRule?  Won't we only ever use one in a given document?
Flags: needinfo?(boris.chiou)
Attached file PR to bump selectors
Flags: needinfo?(boris.chiou)
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #38)
> High level question: why are we storing both the Gecko AnimValuesStyleRule
> and the Servo ServoAnimationRule?  Won't we only ever use one in a given
> document?

Yes, I think we only need one of them. I just follow the same idea of AnimationProperty, we declare members for both gecko and servo, but only use one of them. How about use an union, so it's more clear that we only use one in a given document?
(In reply to Boris Chiou [:boris] from comment #40)
> Yes, I think we only need one of them. I just follow the same idea of
> AnimationProperty, we declare members for both gecko and servo, but only use
> one of them. How about use an union, so it's more clear that we only use one
> in a given document?

I think either a struct with a union inside it (a tagged union), or an AnimationRule base class that both AnimValuesStyleRule and ServoAnimationRule inherit from, would be OK.  The AnimationRule base class would be similar to what we do for StyleSheet, StyleSet, RestyleManagerBase, so it might be better to keep that using the same pattern.  Then we can just use RefPtr<AnimationRule>, and call AsGecko() or AsServo() on it.

We can do that in a followup bug.
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #41)

> I think either a struct with a union inside it (a tagged union), or an
> AnimationRule base class that both AnimValuesStyleRule and
> ServoAnimationRule inherit from, would be OK.  The AnimationRule base class
> would be similar to what we do for StyleSheet, StyleSet, RestyleManagerBase,
> so it might be better to keep that using the same pattern.  Then we can just
> use RefPtr<AnimationRule>, and call AsGecko() or AsServo() on it.
> 
> We can do that in a followup bug.

Thanks, heycam. They sound great. I will fix that in the followup bug.
Comment on attachment 8828732 [details]
Bug 1317209 - Part 5: Trigger composeStyle if there is an animation.

https://reviewboard.mozilla.org/r/105082/#review107384

::: dom/animation/EffectCompositor.cpp:502
(Diff revision 1)
> +}
> +
> +void
> +EffectCompositor::ClearElementsToRestyle()
> +{
> +  const auto iterEnd = mElementsToRestyle.end();

Please MOZ_ASSERT(NS_IsMainThread()) in here.

::: dom/animation/EffectCompositor.cpp:504
(Diff revision 1)
> +    if (iter->Count() != 0) {
> +      iter->Clear();
> +    }

Just call iter->Clear() without checking Count().

::: dom/animation/KeyframeEffectReadOnly.cpp:555
(Diff revision 1)
> +        NS_ERROR("Compose style for unsupported or non-animatable property, "
> +                 "so get invalid RawServoAnimationValues");

Move this to the earlier patch?

::: layout/base/ServoRestyleManager.cpp:497
(Diff revision 1)
> +void
> +ServoRestyleManager::ClearPendingAnimationList(nsIDocument* aDoc)
> +{
> +  if (!aDoc || !aDoc->GetShell()) {
> +    return;
> +  }
> +  nsPresContext* presContext = aDoc->GetShell()->GetPresContext();
> +  if (!presContext) {
> +    return;
> +  }
> +  presContext->EffectCompositor()->ClearElementsToRestyle();
> +}

Before we call ClearPendingAnimationList, we rely on the document having a pres shell / pres context.  So I think we can just do:
  PresContext()->EffectCompositor()->ClearElementsToRestyle();

up in ProcessPendingRestyles, instead of having this method.

::: layout/style/ServoBindings.h:161
(Diff revision 1)
> +RawServoDeclarationBlockStrong
> +Gecko_GetAnimationRuleAttribute(RawGeckoElementBorrowed aElement,

Agree with Emilio that we shouldn't call this an "attribute".

::: layout/style/ServoBindings.cpp:329
(Diff revision 1)
>    }
>    return reinterpret_cast<const RawServoDeclarationBlockStrong*>
>      (decl->AsServo()->RefRaw());
>  }
>  
> +RawServoDeclarationBlockStrong

Should this be RawServoDeclarationBlockStrongBorrowedOrNull instead?  That's what Gecko_GetServoDeclarationBlock uses.  Then you can just return nullptr instead of emptyDeclarationBlock.
Attachment #8828732 - Flags: review?(cam) → review+
Comment on attachment 8828734 [details]
Bug 1317209 - Part 7: Support transition cascade level.

https://reviewboard.mozilla.org/r/105086/#review107404
Attachment #8828734 - Flags: review?(cam) → review+
Comment on attachment 8828728 [details]
Bug 1317209 - Part 1: Introduce ServoAnimationRule and implement uncompute FFI.

https://reviewboard.mozilla.org/r/104454/#review107372

> You should use nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue> instead, which is designed for values being reference counted objects.  I'm not sure what the consequences are of using nsDataHashtable with RefPtr<...> as a value -- it might do more AddRef/Release calls than it needs to.

OK, I will change it.

> To me, AnimationRuleType sounds like an enum that could be either "Gecko" or "Servo".  How about just AnimationRule, and make the two members mGecko and mServo.
> 
> Nit: "{" on new line.
> 
> Also, ServoAnimationRule.h doesn't seem to be the best place for this, but apart from a new file AnimationRule.h I'm not sure where else it could go.

OK, I will put it into another file(i.e. AnimationRule.h), and rename to mGecko and mServo.

> Yeah, it would be nice at least to avoid the extra AddRef/Release on each of the RawServoAnimationValues.  Since we know that mAnimationValues keeps those strong references, we could just change the RawServoAnimationValueListBorrowed to something like RawServoAnimationValueBorrowedListBorrowed, where we store raw pointers in the nsTArray.

Sounds great. I will RawServoAnimationValueBorrowedListBorrowed as the function argument type.
Comment on attachment 8828729 [details]
Bug 1317209 - Part 2: Call Servo's Interpolation from Gecko.

https://reviewboard.mozilla.org/r/104456/#review107374

> It would be good to remove the duplicated code between the two branches.  For example, we have the zero-length segment handling and the interpolate/<0.5/>=0.5 cases, which look very similar in the two branches.
> 
> I don't know how easy it would be to factor this code out to something that worked on both backend types.  Maybe a template that can work on both types.
> 
> For now I'm OK with keeping this, but once we add the accumulate and addition support, I think we'll need to deal with the duplication somehow.

Yes, it's better to remove the duplicated code. Let's do it in the followup bug.
Comment on attachment 8828732 [details]
Bug 1317209 - Part 5: Trigger composeStyle if there is an animation.

https://reviewboard.mozilla.org/r/105082/#review107384

> Should this be RawServoDeclarationBlockStrongBorrowedOrNull instead?  That's what Gecko_GetServoDeclarationBlock uses.  Then you can just return nullptr instead of emptyDeclarationBlock.

This is what I think we may use at first, but I think RawServoDeclarationBlockStrongBorrowedOrNull is kind of a raw pointer to RawServoDeclarationBlockStrong, and I think there is no one to hold RawServoDeclarationBlockStrong after this function returns (is it safe?), so I pass RawServoDeclarationBlockStrong directly (a strong pointer), and let Servo side to free it.
Comment on attachment 8828732 [details]
Bug 1317209 - Part 5: Trigger composeStyle if there is an animation.

https://reviewboard.mozilla.org/r/105082/#review107384

> This is what I think we may use at first, but I think RawServoDeclarationBlockStrongBorrowedOrNull is kind of a raw pointer to RawServoDeclarationBlockStrong, and I think there is no one to hold RawServoDeclarationBlockStrong after this function returns (is it safe?), so I pass RawServoDeclarationBlockStrong directly (a strong pointer), and let Servo side to free it.

You are right.  I mis-read the very long type name and missed that it says "Borrowed" in there. :-)  Gecko_GetServoDeclarationBlock does rely on the object having a strong reference (inside the nsAttrValue).

I don't know whether returning a RefPtr<...> directly from this function is OK.  I thought I remember hearing that this is not OK for the C FFI functions.  I also see that various other construction functions, like Gecko_NewStyleQuoteValues, just return a raw pointer that has already had AddRef called on it, and then on the Rust side we can call RefPtr::from_addrefed.
Manish/Emilio, see comment 48: is returning a raw pointer (that has already had AddRef called on it) from Gecko to Servo, and then constructing a Servo RefPtr around it with RefPtr::from_addrefed, still the recommended way to return newly created refcounted objects from C++ to Rust?
Flags: needinfo?(manishearth)
> I don't know whether returning a RefPtr<...> directly from this function is OK.

These are wrappers over RefPtr, so it is safe.

That said you should probably just return RawSDBBorrowedOrNull, with Null representing the empty case.
Flags: needinfo?(manishearth)
Blocks: 1333310
Blocks: 1333311
(In reply to Manish Goregaokar [:manishearth] from comment #50)
> > I don't know whether returning a RefPtr<...> directly from this function is OK.
> 
> These are wrappers over RefPtr, so it is safe.
> 
> That said you should probably just return RawSDBBorrowedOrNull, with Null
> representing the empty case.

I don't think a Borrowed type is right.  Servo calls Gecko_GetAnimationRule, which returns the result of calling ServoAnimationRule::GetValues(), which returns the result of calling Servo_AnimationValues_Uncompute.  At no point do we store a strong reference to the ServoDeclarationBlock anywhere in a C++ object.  Just to confirm: all of these functions are declared to return a RawServoDeclarationBlockStrong -- does that sound fine, and we can also return null with that type?
Flags: needinfo?(manishearth)
>  At no point do we store a strong reference to the ServoDeclarationBlock anywhere in a C++ object. 

Ah, okay, use a strong ref then.

> does that sound fine, and we can also return null with that type?

yes and yes. The Borrowed types need an explicit OrNull, but Strong is implicitly OrNull since all refptrs are nullable.
Flags: needinfo?(manishearth)
Thanks, heycam, Manish, so I'd keep the current implementation (i.e. return RawServoDeclarationBlockStrong). Btw, I will rebase these again after selector update is merged.
Attached file Servo PR, #15175
Comment on attachment 8828731 [details]
Bug 1317209 - Part 4: Don't ref-count nsDOMNavigationTiming in DocumentTimeline.

https://reviewboard.mozilla.org/r/105080/#review107790
Attachment #8828731 - Flags: review?(cam) → review+
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03fb3196016f29cba5831bc13e11ab3b6a5d7450

After applying this bug, we start to run script-generated animations, so get more assertions in the tests:

e.g.
1. Related to transform animation
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1272475-1.html | assertion count 18 is more than expected 3 to 10 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1272475-2.html | assertion count 22 is more than expected 4 to 10 assertions

2. Related to bug 1311257
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]

Anyway, I didn't see other problems now, so I'm ready to push these patches.
For those crashtests where we now start to crash / fatally assert, can you make them skip-if(stylo) and then mention them in the bug for handling that crash?
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #65)
> For those crashtests where we now start to crash / fatally assert, can you
> make them skip-if(stylo) and then mention them in the bug for handling that
> crash?

Sure, I will upload an extra patch for them.
Attachment #8828734 - Attachment is obsolete: true
Comment on attachment 8828733 [details]
Bug 1317209 - Part 7: Skip some crash tests due to unimplemented features.

https://reviewboard.mozilla.org/r/105084/#review108152
Attachment #8828733 - Flags: review?(cam) → review+
heycam, Sorry for spam, I remove one patch accidentally.
Comment on attachment 8830160 [details]
Bug 1317209 - Part 6: Support transition cascade level.

https://reviewboard.mozilla.org/r/107054/#review108156
Attachment #8830160 - Flags: review?(cam) → review+
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7a4bf59791e
Part 1: Introduce ServoAnimationRule and implement uncompute FFI. r=heycam,manishearth
https://hg.mozilla.org/integration/autoland/rev/04041935086a
Part 2: Call Servo's Interpolation from Gecko. r=heycam,manishearth
https://hg.mozilla.org/integration/autoland/rev/553302167590
Part 3: Add constness to GetEffectSet. r=hiro
https://hg.mozilla.org/integration/autoland/rev/280a3456f532
Part 4: Don't ref-count nsDOMNavigationTiming in DocumentTimeline. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e40482064f56
Part 5: Trigger composeStyle if there is an animation. r=heycam
https://hg.mozilla.org/integration/autoland/rev/030d4996715a
Part 6: Support transition cascade level. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cfabf38bcffe
Part 7: Skip some crash tests due to unimplemented features. r=emilio,heycam
Bravo Boris! Now I can see animations!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #83)
> Bravo Boris! Now I can see animations!

Cool. There are still many bugs, so please file bugs so we can fix them ASAP. Now I'm trying to make the tests work (for script-generated animations).
Blocks: 1337693
Blocks: 1340445
Blocks: 1353202
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: