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)
Core
DOM: Animation
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 | ||
Updated•7 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
(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
Comment 3•7 years ago
|
||
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).
Assignee | ||
Comment 4•7 years ago
|
||
(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. :(
Reporter | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
(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)
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Comment 11•7 years ago
|
||
(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 :)
Assignee | ||
Comment 12•7 years ago
|
||
(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. :)
Comment hidden (spam) |
Assignee | ||
Comment 14•7 years ago
|
||
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)
Reporter | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Comment 16•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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)
Comment hidden (obsolete) |
Comment 25•7 years ago
|
||
(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 26•7 years ago
|
||
mozreview-review |
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 27•7 years ago
|
||
mozreview-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+
Comment 28•7 years ago
|
||
I made the PR to rust-selectors to support the new AFFECTING flags: https://github.com/servo/rust-selectors/pull/102
Assignee | ||
Comment 29•7 years ago
|
||
(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
Assignee | ||
Comment 30•7 years ago
|
||
(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.)
Assignee | ||
Updated•7 years ago
|
Attachment #8828733 -
Flags: review?(cam)
Assignee | ||
Updated•7 years ago
|
Attachment #8828728 -
Flags: review?(cam)
Attachment #8828729 -
Flags: review?(cam)
Comment 31•7 years ago
|
||
mozreview-review |
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 32•7 years ago
|
||
mozreview-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 33•7 years ago
|
||
mozreview-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+
Comment 34•7 years ago
|
||
(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 35•7 years ago
|
||
mozreview-review |
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 36•7 years ago
|
||
mozreview-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 37•7 years ago
|
||
mozreview-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)
Comment 38•7 years ago
|
||
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)
Assignee | ||
Comment 39•7 years ago
|
||
Flags: needinfo?(boris.chiou)
Assignee | ||
Comment 40•7 years ago
|
||
(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?
Comment 41•7 years ago
|
||
(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.
Assignee | ||
Comment 42•7 years ago
|
||
(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 43•7 years ago
|
||
mozreview-review |
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 44•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 45•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 46•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 47•7 years ago
|
||
mozreview-review-reply |
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 48•7 years ago
|
||
mozreview-review-reply |
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.
Comment 49•7 years ago
|
||
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)
Comment 50•7 years ago
|
||
> 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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 58•7 years ago
|
||
(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)
Comment 59•7 years ago
|
||
> 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)
Assignee | ||
Comment 60•7 years ago
|
||
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.
Assignee | ||
Comment 61•7 years ago
|
||
Comment 62•7 years ago
|
||
mozreview-review |
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+
Comment hidden (obsolete) |
Assignee | ||
Comment 64•7 years ago
|
||
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.
Comment 65•7 years ago
|
||
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?
Assignee | ||
Comment 66•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8828734 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Comment 74•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 77•7 years ago
|
||
heycam, Sorry for spam, I remove one patch accidentally.
Comment 78•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 79•7 years ago
|
||
Try: stylo: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a0097ffecdda8b39384971c4cfa7226f87a6b76&selectedJob=71765516 gecko: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b83b0213a2d515f4a5d818a28ba04278e9e44e0&selectedJob=71812392 Looks like there is no new issue.
Comment hidden (mozreview-request) |
Comment 81•7 years ago
|
||
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
Comment 82•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f7a4bf59791e https://hg.mozilla.org/mozilla-central/rev/04041935086a https://hg.mozilla.org/mozilla-central/rev/553302167590 https://hg.mozilla.org/mozilla-central/rev/280a3456f532 https://hg.mozilla.org/mozilla-central/rev/e40482064f56 https://hg.mozilla.org/mozilla-central/rev/030d4996715a https://hg.mozilla.org/mozilla-central/rev/cfabf38bcffe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 83•7 years ago
|
||
Bravo Boris! Now I can see animations!
Assignee | ||
Comment 84•7 years ago
|
||
(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).
Updated•7 years ago
|
status-firefox52:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•