Closed Bug 1340958 Opened 8 years ago Closed 8 years ago

stylo: Gecko_GetAnimationRule mutates the effect set during the parallel traversal

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

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

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(9 files, 6 obsolete files)

Bug 1340958 - Do not call get_animation_rules for pseudo elements other than ::before and ::after.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1340958 - Do not call EffectCompositor::GetServoAnimationRule for print preview.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1340958 - Separate ComposeStyle() into servo and gecko versions.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340958 - Templatize ComposeStyle.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340958 - Allocate StyleRule only if we need to compose styles.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340958 - Add AnimationValueMap and expose it in FFI.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1340958 - Put computed values into AnimationValueMap instead of hashtable in gecko.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1340958 - Drop AnimationRule and ServoAnimationRule.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1340958 - Drop Servo_AnimationValues_Uncompute.
59 bytes, text/x-review-board-request
boris
: review+
Details
+++ This bug was initially created as a clone of Bug #1340340 +++ Static analysis caught this in bug 1294915 comment 43. Error: Dereference write __temp_1 Location: void mozilla::EffectSet::UpdateAnimationRuleRefreshTime(uint32, mozilla::TimeStamp*) @ ff-dbg/dist/include/mozilla/EffectSet.h:181 ### SafeArguments: arg1 Stack Trace: mozilla::ServoAnimationRule* mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385 We have a fix for nsRefreshDriver::mActiveTimer case in bug 1340340. In this bug we will handle the EffectSet case.
Working on this now. I'm trying to add one or two arms into enum SequentialTask for this bug.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Depends on: 1341987
MozReview-Commit-ID: AOp1qTnltlm
Not sure if this bug is obsoleted by other work - feel free to resolve if it is.
Priority: -- → P1
I have an idea to solve this and bug 1337693. I will try the idea this week.
The idea is simple, we can just put each composed styles into servo's PropertyDeclarationBlock instead of gecko's hashtable. After investigation for bug 1344966 comment 8, I am convinced we don't need to cache the animation value, so we can pass an empty PropertyDeclarationBlock to Gecko and put each composed values in the PropertyDeclarationBlock. But this patch does not work yet, since we need to convert RawServoDeclarationBlockBorrowed back to mutable PropertyDeclarationBlock. I did add such transmute in this patch, but yeah, rust is really clever, it is not allowed.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > But this patch does not work yet, since we need to convert > RawServoDeclarationBlockBorrowed back to mutable PropertyDeclarationBlock. > I did add such transmute in this patch, but yeah, rust is really clever, it > is not allowed. I don't yet understand FFI and ownership model in rust. I did confirm we can put the computed values without the transmute. yay!
Assignee: boris.chiou → hikezoe
Seems hiro's patch works, so re-assign to hiro. Thanks for help. :)
Attachment #8840335 - Attachment is obsolete: true
I'm holding this for now, since after some thought about animatino-only restyle I think we might need to cache the animation rules for normal restyle after the animation-only restyle.
OK. We can get old animation rules from ElementData when we do normal restyle. We ensure the old animation rules is generated in the animation-only restyle.
Attachment #8846910 - Attachment is obsolete: true
These patch will fix bug 1333310, bug 1337693 and this.
Comment on attachment 8847397 [details] Bug 1340958 - Do not call get_animation_rules for pseudo elements other than ::before and ::after. https://reviewboard.mozilla.org/r/120348/#review122328 ::: layout/style/ServoBindings.cpp:411 (Diff revision 1) > - : CSSPseudoElementType::NotPseudo; > - if (pseudoType != CSSPseudoElementType::NotPseudo && > - pseudoType != CSSPseudoElementType::before && > + MOZ_ASSERT(pseudoType == CSSPseudoElementType::NotPseudo || > + pseudoType == CSSPseudoElementType::before || > + pseudoType == CSSPseudoElementType::after); Nit: I would slightly prefer the assertion to be at the top of the function (and so then just compare aPseudoTag to nsGkAtoms::before/after and nullptr).
Attachment #8847397 - Flags: review?(cam) → review+
Comment on attachment 8847398 [details] Bug 1340958 - Do not call EffectCompositor::GetServoAnimationRule for print preview. https://reviewboard.mozilla.org/r/120350/#review122330
Attachment #8847398 - Flags: review?(cam) → review+
Comment on attachment 8847399 [details] Bug 1340958 - Split get_animation_rules into get_animation_rule and get_transition_rule. https://reviewboard.mozilla.org/r/120352/#review122332
Attachment #8847399 - Flags: review?(cam) → review+
Comment on attachment 8847402 [details] Bug 1340958 - Allocate StyleRule only if we need to compose styles. https://reviewboard.mozilla.org/r/120358/#review122334 I don't understand the purpose of this patch. Can you explain why we are doing this (and what the patch is doing, at a high level), ideally in the commit message?
Attachment #8847402 - Flags: review?(cam) → review-
Comment on attachment 8847403 [details] Bug 1340958 - Drop AnimationRule and ServoAnimationRule. https://reviewboard.mozilla.org/r/120360/#review122338 Dropping ServoAnimationRule is ok to me, but I would like to wait other's reviews. BTW, I trid to make ServoAnimationRule look like AnimValuesStyleRule because I think we might still need its hashtable to retrieve the animation value [1] in GetUnderlyingStyle(). PropertyDeclarationBlock uses a vector to store its properties, so we have to make sure GetUnderlyingStyle() has a better/other way to get the value. [1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/animation/KeyframeEffectReadOnly.cpp#446
(In reply to Boris Chiou [:boris] from comment #31) > Comment on attachment 8847403 [details] > Bug 1340958 - Drop AnimationRule and ServoAnimationRule. > > https://reviewboard.mozilla.org/r/120360/#review122338 > > Dropping ServoAnimationRule is ok to me, but I would like to wait other's > reviews. BTW, I trid to make ServoAnimationRule look like > AnimValuesStyleRule because I think we might still need its hashtable to > retrieve the animation value [1] in GetUnderlyingStyle(). > PropertyDeclarationBlock uses a vector to store its properties, so we have > to make sure GetUnderlyingStyle() has a better/other way to get the value. Ah, right. We might need to add a new struct in servo side when servo has additive or accumulative animations for some day.
Comment on attachment 8847402 [details] Bug 1340958 - Allocate StyleRule only if we need to compose styles. https://reviewboard.mozilla.org/r/120358/#review122344 I thought PropertyDeclarationBlock does override an existent property, but actually doesn't.
Comment on attachment 8847402 [details] Bug 1340958 - Allocate StyleRule only if we need to compose styles. MozReview did not clear review request.
Attachment #8847402 - Flags: review?(cam)
Attachment #8847403 - Flags: review?(boris.chiou)
Attachment #8847404 - Flags: review?(boris.chiou)
Oh wait. The comment of PropertyDeclarationBlock says "Overridden declarations are skipped" but it doesn't mean for push()..
I've decided that I will land part 1 to templatization patch here.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36) > I've decided that I will land part 1 to templatization patch here. The refactoring parts are good, and I think we can temporarily _not_ store ServoAnimationRule back to EffectSet to fix the thread-safe issue. After we decide that ServoAnimationRule is still needed for other purpose, then we can add it back with SequentialTask or other ways?
Instead I am thinking that we pass a servo's hash map to GetServoAnimationRules() and put the computed values with an FFI to call insert() for the hash map from ComposedStyle(), after GetServoAnimationRules() create a vec from the hash map, then put them in PropertyDeclarationBlock without calling PropertyDeclarationBlock::put(). With this way we have no overhead to put each time when we add a PropertyDeclaration into the block. But anyway I don't do it soon. I have no effective idea to convert the hash map into the vec.
Comment on attachment 8847400 [details] Bug 1340958 - Separate ComposeStyle() into servo and gecko versions. https://reviewboard.mozilla.org/r/120354/#review122748 r=me with the following addressed although the change to the order of the early return and creation of animation rule probably deserves a separate patch. ::: dom/animation/KeyframeEffectReadOnly.h:467 (Diff revision 2) > + void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule, > + const AnimationProperty& aProperty, > + const AnimationPropertySegment& aSegment, > + const ComputedTiming& aComputedTiming); > + > + void ComposeStyle(RefPtr<ServoAnimationRule>& aStyleRule, Should we call these ComposeStyleRule, perhaps? I think it's confusing to have three methods call ComposeStyle where one of them does something quite different? Even after the template changes in the next patch I think having a different name makes it clearer that these methods do a subset of the work of ComposeStyle. ::: dom/animation/KeyframeEffectReadOnly.cpp:520 (Diff revision 2) > + const AnimationProperty& aProperty, > + const AnimationPropertySegment& aSegment, > + const ComputedTiming& aComputedTiming) > +{ > + if (!aStyleRule) { > + // Allocate the style rule now that we know we have animation data. We should possibly rewrite this to make more sense now: // We only call this method if we know we have animation data. If we don't // have a style rule yet, allocate it now since we know we'll need it. Although, looking further into this method, we have an early return of fromValue or toValue is null. We shouldn't allocate the rule until after that early return right? If so, that looks like a bug introduced by bug 1331704. And if we fix that, the original comment problem makes sense again.
Attachment #8847400 - Flags: review?(bbirtles) → review+
Comment on attachment 8847401 [details] Bug 1340958 - Templatize ComposeStyle. https://reviewboard.mozilla.org/r/120356/#review122756 Sorry, but I don't understand what this patch buys us. It seems only to make the code more complex? If there's a need for it, we should explain it in the commit message. ::: dom/animation/Animation.h:324 (Diff revision 2) > - * Updates |aStyleRule| with the animation values of this animation's effect, > - * if any. > + * Updates |aResultContainer| with the animation values of this animation's > + * effect, if any. > * Any properties contained in |aPropertiesToSkip| will not be added or > - * updated in |aStyleRule|. > + * updated in |aResultContainer|. > */ > - void ComposeStyle(AnimationRule& aStyleRule, > + template<typename ResultContainer> > + void ComposeStyle(ResultContainer&& aRestultContainer, Why ResultContainer instead of StyleRule? Perhaps lated in this patch series we no longer use style rules. If so, that should be explained in the commit message. (It's also really helpful when reviewing a partial patch series!) ::: dom/animation/KeyframeEffectReadOnly.cpp:706 (Diff revision 2) > - // Bug 1333311 - We use two branches for Gecko and Stylo. However, it's > + ComposeStyle(Forward<ResultContainer>(aResultContainer), > - // better to remove the duplicated code. We haven't really fixed the bug though, have we? It seems like we still have: * Two all but identical blocks for handling zero-length segments * Two identical calculations of |positionInSegment| * Two identical calculations of |valuePosition| * Two identical null checks for |valuePosition| * Two almost identical blocks for handling the result of interpolate and doing the 50% flip when it fails It seems like that bug still remains
Attachment #8847401 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #39) > ::: dom/animation/KeyframeEffectReadOnly.cpp:520 > (Diff revision 2) > > + const AnimationProperty& aProperty, > > + const AnimationPropertySegment& aSegment, > > + const ComputedTiming& aComputedTiming) > > +{ > > + if (!aStyleRule) { > > + // Allocate the style rule now that we know we have animation data. > > We should possibly rewrite this to make more sense now: > > // We only call this method if we know we have animation data. If we don't > // have a style rule yet, allocate it now since we know we'll need it. > > Although, looking further into this method, we have an early return of > fromValue or toValue is null. We shouldn't allocate the rule until after > that early return right? If so, that looks like a bug introduced by bug > 1331704. No. This was introduced in this patch. I will fix it. Before this patch there was no early return in such case, we just did continue there.
Ah, but yes, this was introduced in that bug.
(In reply to Brian Birtles (:birtles) from comment #40) > Comment on attachment 8847401 [details] > Bug 1340958 - Templatize ComposeStyle. > > https://reviewboard.mozilla.org/r/120356/#review122756 > > Sorry, but I don't understand what this patch buys us. It seems only to make > the code more complex? If there's a need for it, we should explain it in the > commit message. > > ::: dom/animation/Animation.h:324 > (Diff revision 2) > > - * Updates |aStyleRule| with the animation values of this animation's effect, > > - * if any. > > + * Updates |aResultContainer| with the animation values of this animation's > > + * effect, if any. > > * Any properties contained in |aPropertiesToSkip| will not be added or > > - * updated in |aStyleRule|. > > + * updated in |aResultContainer|. > > */ > > - void ComposeStyle(AnimationRule& aStyleRule, > > + template<typename ResultContainer> > > + void ComposeStyle(ResultContainer&& aRestultContainer, > > Why ResultContainer instead of StyleRule? > > Perhaps lated in this patch series we no longer use style rules. If so, that > should be explained in the commit message. (It's also really helpful when > reviewing a partial patch series!) > > ::: dom/animation/KeyframeEffectReadOnly.cpp:706 > (Diff revision 2) > > - // Bug 1333311 - We use two branches for Gecko and Stylo. However, it's > > + ComposeStyle(Forward<ResultContainer>(aResultContainer), > > - // better to remove the duplicated code. > > We haven't really fixed the bug though, have we? I did misread it's bug 1333310. But anyway we don't fix either bug.
Attachment #8847403 - Attachment is obsolete: true
Attachment #8847404 - Attachment is obsolete: true
Comment on attachment 8847402 [details] Bug 1340958 - Allocate StyleRule only if we need to compose styles. https://reviewboard.mozilla.org/r/120358/#review122798 ::: dom/animation/KeyframeEffectReadOnly.cpp:637 (Diff revision 3) > > + if (!aStyleRule.mGecko) { Are there two blank lines here? I guess we only want one.
Attachment #8847402 - Flags: review?(bbirtles) → review+
Comment on attachment 8847899 [details] Bug 1340958 - Put computed values into AnimationValueMap instead of hashtable in gecko. https://reviewboard.mozilla.org/r/120824/#review122800 ::: servo/components/style/gecko/wrapper.rs:430 (Diff revision 1) > -> Option<Arc<RwLock<PropertyDeclarationBlock>>> { > let atom_ptr = PseudoElement::ns_atom_or_null_from_opt(pseudo); > - unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Animations).into_arc_opt() } > + let animation_values = Arc::new(RwLock::new(AnimationValueMap::new())); > + if unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Animations, > + HasArcFFI::arc_as_borrowed(&animation_values)) } { > + Some(Arc::new(RwLock::new(animation_values.read().clone().into()))) Without clone() I got a fllowing error: 0:44.25 443 | Some(Arc::new(RwLock::new(animation_values.read().into()))) 0:44.25 | ^^^^ the trait `std::convert::From<parking_lot::RwLockReadGuard<'_, std::collections::HashMap<properties::animated_properties::TransitionProperty, properties::animated_properties::AnimationValue>>>` is not implemented for `properties::declaration_block::PropertyDeclarationBlock` 0:44.25 | I think we can avoid this clone() but I don't find a way for it.
Comment on attachment 8847897 [details] Bug 1340958 - Add AnimationValueMap and expose it in FFI. https://reviewboard.mozilla.org/r/120820/#review122818 ::: commit-message-e46dd:1 (Diff revision 1) > +Bug 1340958 - Add AnimationValuemap and expose it in FFI. r?heycam *AnimationValueMap
Attachment #8847897 - Flags: review?(cam) → review+
Comment on attachment 8847899 [details] Bug 1340958 - Put computed values into AnimationValueMap instead of hashtable in gecko. https://reviewboard.mozilla.org/r/120824/#review122820 Looks good, but I'd like to see another version of the patch with these things addressed. ::: commit-message-f4f1d:8 (Diff revision 1) > +Before this patch, we store each computed values in a hashtable, > +nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>, for all > +KeyframeEffectReadOnly on an element, and convert the ServoAnimationValues of > +the hashtable into an nsTArray<ServoAnimationValue*> and then convert > +the ServoAnimationValues of the nsTArray into PropertyDeclarationBlock > +in rust. This way was really ineffective. s/ineffective/inefficient/, I guess? ::: dom/animation/EffectCompositor.h:163 (Diff revision 1) > // modification because it may case some thread-safe issues. > - ServoAnimationRule* GetServoAnimationRule(const dom::Element* aElement, > + bool GetServoAnimationRule( > + const dom::Element* aElement, > - CSSPseudoElementType aPseudoType, > + CSSPseudoElementType aPseudoType, > - CascadeLevel aCascadeLevel); > + CascadeLevel aCascadeLevel, > + const RawServoAnimationValueMap* aAnimationValues); It's not obvious that aAnimationValues is that the object that will have the values stored in it, especially since it's |const RawServoAnimationValueMap*|. (Which I guess doesn't matter, due to the way the bindings / types work.) So please adjust the comment about to say that the values are stored in the RawServoAnimationValueMap. Can you also write it as RawServoAnimationValueMapBorrowed, to match the .cpp file. ::: dom/animation/KeyframeEffectReadOnly.cpp:643 (Diff revision 1) > Servo_AnimationValues_Interpolate(servoFromValue, > servoToValue, > valuePosition).Consume(); > > if (interpolated) { > - aAnimationRule->AddValue(aProperty.mProperty, interpolated); > + // XXX: FIXME We can *move* this interpolated value. I'm not sure how easy it is to move across the FFI boundary safely (i.e. without just using raw pointers)... ::: servo/components/style/gecko/wrapper.rs:430 (Diff revision 1) > -> Option<Arc<RwLock<PropertyDeclarationBlock>>> { > let atom_ptr = PseudoElement::ns_atom_or_null_from_opt(pseudo); > - unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Animations).into_arc_opt() } > + let animation_values = Arc::new(RwLock::new(AnimationValueMap::new())); > + if unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Animations, > + HasArcFFI::arc_as_borrowed(&animation_values)) } { > + Some(Arc::new(RwLock::new(animation_values.read().clone().into()))) It's because you have `From` implemented for AnimationValueMap, so you can't call into() on a reference to an AnimationValueMap. `From` is designed to consume the value you are converting from. To avoid that, you could add a separate `fn to_property_declaration_block(&self)` method. I think we should do that. ::: servo/components/style/gecko/wrapper.rs:436 (Diff revision 1) > fn get_transition_rule(&self, pseudo: Option<&PseudoElement>) > -> Option<Arc<RwLock<PropertyDeclarationBlock>>> { > let atom_ptr = PseudoElement::ns_atom_or_null_from_opt(pseudo); > - unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Transitions).into_arc_opt() } > + let animation_values = Arc::new(RwLock::new(AnimationValueMap::new())); It might be nice to factor out the common code from get_animation_rule and get_transition_rule into a helper function.
Attachment #8847899 - Flags: review?(cam) → review-
Comment on attachment 8847898 [details] Bug 1340958 - Add a function to convert AnimationValueMap to PropertyDeclarationBlock. https://reviewboard.mozilla.org/r/120822/#review122824 Per the comments on the final patch, let's have a method that doesn't need to clone the AnimationValueMap.
Attachment #8847898 - Flags: review?(cam) → review-
(In reply to Cameron McCormack (:heycam) from comment #56) > ::: dom/animation/KeyframeEffectReadOnly.cpp:643 > (Diff revision 1) > > Servo_AnimationValues_Interpolate(servoFromValue, > > servoToValue, > > valuePosition).Consume(); > > > > if (interpolated) { > > - aAnimationRule->AddValue(aProperty.mProperty, interpolated); > > + // XXX: FIXME We can *move* this interpolated value. > > I'm not sure how easy it is to move across the FFI boundary safely (i.e. > without just using raw pointers)... Yeah, this comment was somewhat confusing. I will drop it. When we implemented additive or accumulative in stylo, these function calls will be moved in rust, at that time we can move this value, I think. > ::: servo/components/style/gecko/wrapper.rs:430 > (Diff revision 1) > > -> Option<Arc<RwLock<PropertyDeclarationBlock>>> { > > let atom_ptr = PseudoElement::ns_atom_or_null_from_opt(pseudo); > > - unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Animations).into_arc_opt() } > > + let animation_values = Arc::new(RwLock::new(AnimationValueMap::new())); > > + if unsafe { Gecko_GetAnimationRule(self.0, atom_ptr, CascadeLevel::Animations, > > + HasArcFFI::arc_as_borrowed(&animation_values)) } { > > + Some(Arc::new(RwLock::new(animation_values.read().clone().into()))) > > It's because you have `From` implemented for AnimationValueMap, so you can't > call into() on a reference to an AnimationValueMap. Oh, thanks. It's a reference? > `From` is designed to > consume the value you are converting from. To avoid that, you could add a > separate `fn to_property_declaration_block(&self)` method. I think we > should do that. I will do it. thanks.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #58) > > `From` is designed to > > consume the value you are converting from. To avoid that, you could add a > > separate `fn to_property_declaration_block(&self)` method. I think we > > should do that. I've decided add from_animation_value_map() into PropertyDeclarationBlock since all members of PropertyDeclarationBlock are private and Rust does not allow us impl for type alias. So if we add the function to AnimationValueMap, we need to make AnimationValueMap as a tuple or a struct.
Comment on attachment 8847898 [details] Bug 1340958 - Add a function to convert AnimationValueMap to PropertyDeclarationBlock. https://reviewboard.mozilla.org/r/120822/#review122876 ::: commit-message-72c8c:1 (Diff revision 2) > +Bug 1340958 - Add a functio to convert AnimationValueMap to PropertyDeclarationBlock. r?heycam functions
Attachment #8847898 - Flags: review?(cam) → review+
Comment on attachment 8847899 [details] Bug 1340958 - Put computed values into AnimationValueMap instead of hashtable in gecko. https://reviewboard.mozilla.org/r/120824/#review122878
Attachment #8847899 - Flags: review?(cam) → review+
Comment on attachment 8847897 [details] Bug 1340958 - Add AnimationValueMap and expose it in FFI. https://reviewboard.mozilla.org/r/120820/#review123258 ::: commit-message-e46dd:1 (Diff revision 2) > +Bug 1340958 - Add AnimationValuemap and expose it in FFI. r?heycam AnimationValueMap ::: commit-message-e46dd:3 (Diff revision 2) > +AnimationValueMap will be used for storing computed values of lower effect > +when an effect is added or accumulated on the lower effect. > +This AnimationValueMap can be also used for normal replace animation. > +Because current Gecko composes all of effects in the composite order at once > +even if there is no additive or accumulative animations. We can put each > +computed value into this AnimationValueMap every time composing an effect. Sorry, I don't understand this comment. At first it sounds like a type we're introducing to represent the underlying value, then it sounds like a type we're introducing to represent the result of compositing all the different effects on an element. Which is it? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:260 (Diff revision 2) > +/// This HashMap stores the underlying values that are the last AnimationValue > +/// to be composed for each TransitionProperty. Again, is this just trying to say it is the underlying value? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:261 (Diff revision 2) > +/// to be composed for each TransitionProperty. > +#[cfg(feature = "gecko")] > +pub type AnimationValueMap = HashMap<TransitionProperty, AnimationValue>; Just curious, why is this called TransitionProperty? Is that a servo thing? Can we change it?
Comment on attachment 8847899 [details] Bug 1340958 - Put computed values into AnimationValueMap instead of hashtable in gecko. https://reviewboard.mozilla.org/r/120824/#review123254 ::: dom/animation/EffectCompositor.h:159 (Diff revision 2) > CascadeLevel aCascadeLevel, > nsStyleContext* aStyleContext); > > // Get animation rule for stylo. This is an equivalent of GetAnimationRule > - // and will be called from servo side. We need to be careful while doing any > - // modification because it may case some thread-safe issues. > + // and will be called from servo side. > + // The animation rule is stored into |RawServoAnimationValueMapBorrowed|. s/into/in/ ::: dom/animation/EffectCompositor.h:160 (Diff revision 2) > nsStyleContext* aStyleContext); > > // Get animation rule for stylo. This is an equivalent of GetAnimationRule > - // and will be called from servo side. We need to be careful while doing any > - // modification because it may case some thread-safe issues. > - ServoAnimationRule* GetServoAnimationRule(const dom::Element* aElement, > + // and will be called from servo side. > + // The animation rule is stored into |RawServoAnimationValueMapBorrowed|. > + // We need to be careful while doing any modification because it may case some s/case/cause/ ::: dom/animation/EffectCompositor.cpp:488 (Diff revision 2) > - CSSPseudoElementType aPseudoType, > + CSSPseudoElementType aPseudoType, > - CascadeLevel aCascadeLevel) > + CascadeLevel aCascadeLevel, > + RawServoAnimationValueMapBorrowed aAnimationValues) > { > + MOZ_ASSERT(aAnimationValues); > + Nit: No need for this blank line?
(In reply to Brian Birtles (:birtles) from comment #78) > Comment on attachment 8847897 [details] > Bug 1340958 - Add AnimationValueMap and expose it in FFI. > > https://reviewboard.mozilla.org/r/120820/#review123258 > > ::: commit-message-e46dd:1 > (Diff revision 2) > > +Bug 1340958 - Add AnimationValuemap and expose it in FFI. r?heycam > > AnimationValueMap > > ::: commit-message-e46dd:3 > (Diff revision 2) > > +AnimationValueMap will be used for storing computed values of lower effect > > +when an effect is added or accumulated on the lower effect. > > +This AnimationValueMap can be also used for normal replace animation. > > +Because current Gecko composes all of effects in the composite order at once > > +even if there is no additive or accumulative animations. We can put each > > +computed value into this AnimationValueMap every time composing an effect. > > Sorry, I don't understand this comment. > > At first it sounds like a type we're introducing to represent the underlying > value, then it sounds like a type we're introducing to represent the result > of compositing all the different effects on an element. Which is it? > > ::: servo/components/style/properties/helpers/animated_properties.mako.rs:260 > (Diff revision 2) > > +/// This HashMap stores the underlying values that are the last AnimationValue > > +/// to be composed for each TransitionProperty. > > Again, is this just trying to say it is the underlying value? OK. I can drop the comment about underlying value here. It was confusing. > ::: servo/components/style/properties/helpers/animated_properties.mako.rs:261 > (Diff revision 2) > > +/// to be composed for each TransitionProperty. > > +#[cfg(feature = "gecko")] > > +pub type AnimationValueMap = HashMap<TransitionProperty, AnimationValue>; > > Just curious, why is this called TransitionProperty? Is that a servo thing? > Can we change it? Hmm, I am not sure we can change it. In my understandings the TransitionProperty means animatable longhand property and 'all' for transitions.
Comment on attachment 8848342 [details] Bug 1340958 - Drop Servo_AnimationValues_Uncompute. https://reviewboard.mozilla.org/r/121248/#review123268
Attachment #8848342 - Flags: review?(boris.chiou) → review+
Comment on attachment 8848341 [details] Bug 1340958 - Drop AnimationRule and ServoAnimationRule. https://reviewboard.mozilla.org/r/121246/#review123270
Attachment #8848341 - Flags: review?(boris.chiou) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #80) > OK. I can drop the comment about underlying value here. It was confusing. Thanks. please update the commit message too. > > ::: servo/components/style/properties/helpers/animated_properties.mako.rs:261 > > (Diff revision 2) > > > +/// to be composed for each TransitionProperty. > > > +#[cfg(feature = "gecko")] > > > +pub type AnimationValueMap = HashMap<TransitionProperty, AnimationValue>; > > > > Just curious, why is this called TransitionProperty? Is that a servo thing? > > Can we change it? > > Hmm, I am not sure we can change it. In my understandings the > TransitionProperty means animatable longhand property and 'all' for > transitions. We don't need to change it in this bug, but I'm curious about what this means. How can it represent both 'all' and longhand properties? 'all' is basically just a shorthand. Do we really want to store 'all' in this hashmap? If not perhaps we should be using a different type?
Comment on attachment 8847401 [details] Bug 1340958 - Templatize ComposeStyle. https://reviewboard.mozilla.org/r/120356/#review123266 ::: commit-message-29878:3 (Diff revision 4) > +In a later bug we will replace ServoAnimationRule with a servo's hash map and > +put directly computed values into the hashmap instead of storing > +the computed values into ServoAnimationRule and converting the rule > +into nsTArray<> and passing the array to an FFI. > +At that time, ComposeStyle() will take the hash map pointer instead of > +ServoAnimationRule. Not a later bug but later in this patch series, right? "Later in this patch series we will replace ServoAnimationRule with a hashmap. At that point, we will would like to pass the hashmap to ComposeStyle. In order to achieve that, this patch templatizes the 'animation rule' parameter of ComposeStyle in both Animation and KeyframeEffectReadOnly so that it can represent a hashmap instead." ::: dom/animation/Animation.h:324 (Diff revision 4) > - * Updates |aStyleRule| with the animation values of this animation's effect, > - * if any. > + * Updates |aResultContainer| with the animation values of this animation's > + * effect, if any. > * Any properties contained in |aPropertiesToSkip| will not be added or > - * updated in |aStyleRule|. > + * updated in |aResultContainer|. > */ > - void ComposeStyle(AnimationRule& aStyleRule, > + template<typename ResultContainer> > + void ComposeStyle(ResultContainer&& aRestultContainer, (a)ResultContainer seems too generic to me. How about: s/ResultContainer/ComposedAnimationStyle/ s/aResultContainer/aComposeResult/ ? ::: dom/animation/Animation.h:329 (Diff revision 4) > - void ComposeStyle(AnimationRule& aStyleRule, > + template<typename ResultContainer> > + void ComposeStyle(ResultContainer&& aRestultContainer, Do we still need AnimationRule after this bug? Or ServoAnimationRule?
Attachment #8847401 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #83) > > > ::: servo/components/style/properties/helpers/animated_properties.mako.rs:261 > > > (Diff revision 2) > > > > +/// to be composed for each TransitionProperty. > > > > +#[cfg(feature = "gecko")] > > > > +pub type AnimationValueMap = HashMap<TransitionProperty, AnimationValue>; > > > > > > Just curious, why is this called TransitionProperty? Is that a servo thing? > > > Can we change it? > > > > Hmm, I am not sure we can change it. In my understandings the > > TransitionProperty means animatable longhand property and 'all' for > > transitions. > > We don't need to change it in this bug, but I'm curious about what this > means. How can it represent both 'all' and longhand properties? 'all' is > basically just a shorthand. Do we really want to store 'all' in this > hashmap? If not perhaps we should be using a different type? Right, but unfortunately servo has no such enum at this moment. It would be good if we could create the enum for properties that animatable is True in macro. I am really not sure we can do it.
(In reply to Brian Birtles (:birtles) from comment #84) > ::: dom/animation/Animation.h:324 > (Diff revision 4) > > - * Updates |aStyleRule| with the animation values of this animation's effect, > > - * if any. > > + * Updates |aResultContainer| with the animation values of this animation's > > + * effect, if any. > > * Any properties contained in |aPropertiesToSkip| will not be added or > > - * updated in |aStyleRule|. > > + * updated in |aResultContainer|. > > */ > > - void ComposeStyle(AnimationRule& aStyleRule, > > + template<typename ResultContainer> > > + void ComposeStyle(ResultContainer&& aRestultContainer, > > (a)ResultContainer seems too generic to me. How about: > > s/ResultContainer/ComposedAnimationStyle/ > s/aResultContainer/aComposeResult/ Nice names! Thanks! > ::: dom/animation/Animation.h:329 > (Diff revision 4) > > - void ComposeStyle(AnimationRule& aStyleRule, > > + template<typename ResultContainer> > > + void ComposeStyle(ResultContainer&& aRestultContainer, > > Do we still need AnimationRule after this bug? Or ServoAnimationRule? No. Boris reviewed those patches in this bug. Thank you!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #85) > (In reply to Brian Birtles (:birtles) from comment #83) > > > > ::: servo/components/style/properties/helpers/animated_properties.mako.rs:261 > > > > (Diff revision 2) > > > > > +/// to be composed for each TransitionProperty. > > > > > +#[cfg(feature = "gecko")] > > > > > +pub type AnimationValueMap = HashMap<TransitionProperty, AnimationValue>; > > > > > > > > Just curious, why is this called TransitionProperty? Is that a servo thing? > > > > Can we change it? > > > > > > Hmm, I am not sure we can change it. In my understandings the > > > TransitionProperty means animatable longhand property and 'all' for > > > transitions. > > > > We don't need to change it in this bug, but I'm curious about what this > > means. How can it represent both 'all' and longhand properties? 'all' is > > basically just a shorthand. Do we really want to store 'all' in this > > hashmap? If not perhaps we should be using a different type? > > Right, but unfortunately servo has no such enum at this moment. It would be > good if we could create the enum for properties that animatable is True in > macro. I am really not sure we can do it. Doh! Idiot! Current TransitionProperty does do so.
Attachment #8847399 - Attachment is obsolete: true
Attachment #8847898 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/10ec4edc3c63 Do not call get_animation_rules for pseudo elements other than ::before and ::after. r=heycam https://hg.mozilla.org/integration/autoland/rev/d94b4a1a9c22 Do not call EffectCompositor::GetServoAnimationRule for print preview. r=heycam https://hg.mozilla.org/integration/autoland/rev/efc0e046a5c1 Allocate StyleRule only if we need to compose styles. r=birtles https://hg.mozilla.org/integration/autoland/rev/f2fdcee9cb8f Separate ComposeStyle() into servo and gecko versions. r=birtles https://hg.mozilla.org/integration/autoland/rev/e9870de9fa97 Templatize ComposeStyle. r=birtles https://hg.mozilla.org/integration/autoland/rev/205110b44b69 Add AnimationValueMap and expose it in FFI. r=heycam https://hg.mozilla.org/integration/autoland/rev/82de9aee6922 Put computed values into AnimationValueMap instead of hashtable in gecko. r=heycam https://hg.mozilla.org/integration/autoland/rev/71af5dbc19e2 Drop AnimationRule and ServoAnimationRule. r=boris https://hg.mozilla.org/integration/autoland/rev/a01bbd72a8e4 Drop Servo_AnimationValues_Uncompute. r=boris
See Also: → 1333310
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: