The default bug view has changed. See this FAQ.

stylo: Gecko_GetAnimationRule mutates the effect set during the parallel traversal

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
a month ago
5 days ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
boris
: review+
Details | Review
59 bytes, text/x-review-board-request
boris
: review+
Details | Review
(Assignee)

Description

a month ago
+++ 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.

Comment 1

29 days ago
Working on this now. I'm trying to add one or two arms into enum SequentialTask for this bug.
Assignee: nobody → boris.chiou

Updated

29 days ago
Status: NEW → ASSIGNED
(Assignee)

Updated

28 days ago
Depends on: 1341987

Comment 2

28 days ago
Created attachment 8840335 [details] [diff] [review]
Don't store AnimationRule into EffectSet during parallel styling.

MozReview-Commit-ID: AOp1qTnltlm
Not sure if this bug is obsoleted by other work - feel free to resolve if it is.
Priority: -- → P1
(Assignee)

Comment 4

9 days ago
I have an idea to solve this and bug 1337693.  I will try the idea this week.
(Assignee)

Comment 5

9 days ago
Created attachment 8846910 [details] [diff] [review]
Put computed styles into servo's declaration block instead of gecko's hash table

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.
(Assignee)

Comment 6

9 days ago
(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!

Updated

9 days ago
Assignee: boris.chiou → hikezoe
Seems hiro's patch works, so re-assign to hiro. Thanks for help. :)

Updated

9 days ago
Attachment #8840335 - Attachment is obsolete: true
(Assignee)

Comment 8

9 days ago
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.
(Assignee)

Comment 9

9 days ago
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.
(Assignee)

Updated

8 days ago
Attachment #8846910 - Attachment is obsolete: true
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 hidden (mozreview-request)
(Assignee)

Comment 18

8 days ago
These patch will fix bug 1333310, bug 1337693 and this.

Comment 19

8 days ago
mozreview-review
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 20

8 days ago
mozreview-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 21

8 days ago
mozreview-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 22

8 days ago
mozreview-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 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 hidden (mozreview-request)

Comment 31

8 days ago
mozreview-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
(Assignee)

Comment 32

8 days ago
(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.
(Assignee)

Comment 33

8 days ago
mozreview-review
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.
(Assignee)

Comment 34

8 days ago
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)
(Assignee)

Updated

8 days ago
Attachment #8847403 - Flags: review?(boris.chiou)
(Assignee)

Updated

8 days ago
Attachment #8847404 - Flags: review?(boris.chiou)
(Assignee)

Comment 35

8 days ago
Oh wait.  The comment of PropertyDeclarationBlock says "Overridden declarations are skipped" but it doesn't mean for push()..
(Assignee)

Comment 36

8 days ago
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?
(Assignee)

Comment 38

8 days ago
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 39

7 days ago
mozreview-review
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 40

7 days ago
mozreview-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)
(Assignee)

Comment 41

7 days ago
(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.
(Assignee)

Comment 42

7 days ago
Ah, but yes, this was introduced in that bug.
(Assignee)

Comment 43

7 days ago
(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.
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 days ago
Attachment #8847403 - Attachment is obsolete: true
(Assignee)

Updated

7 days ago
Attachment #8847404 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 53

7 days ago
mozreview-review
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+
(Assignee)

Comment 54

7 days ago
mozreview-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 55

7 days ago
mozreview-review
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 56

7 days ago
mozreview-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 57

7 days ago
mozreview-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-
(Assignee)

Comment 58

7 days ago
(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.
(Assignee)

Comment 59

7 days ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 65

7 days ago
mozreview-review
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 66

7 days ago
mozreview-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 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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 78

6 days ago
mozreview-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 79

6 days ago
mozreview-review
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?
(Assignee)

Comment 80

6 days ago
(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 81

6 days ago
mozreview-review
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 82

6 days ago
mozreview-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 84

6 days ago
mozreview-review
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+
(Assignee)

Comment 85

6 days ago
(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.
(Assignee)

Comment 86

6 days ago
(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!
(Assignee)

Comment 87

6 days ago
(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.
(Assignee)

Comment 88

6 days ago
Final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bc8974509f5ddb886def13ce4536d7d109ae95

PR for servo:
https://github.com/servo/servo/pull/16005
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 days ago
Attachment #8847399 - Attachment is obsolete: true
(Assignee)

Updated

6 days ago
Attachment #8847898 - Attachment is obsolete: true

Comment 98

6 days ago
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

Updated

5 days ago
See Also: → bug 1333310

Comment 99

5 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10ec4edc3c63
https://hg.mozilla.org/mozilla-central/rev/d94b4a1a9c22
https://hg.mozilla.org/mozilla-central/rev/efc0e046a5c1
https://hg.mozilla.org/mozilla-central/rev/f2fdcee9cb8f
https://hg.mozilla.org/mozilla-central/rev/e9870de9fa97
https://hg.mozilla.org/mozilla-central/rev/205110b44b69
https://hg.mozilla.org/mozilla-central/rev/82de9aee6922
https://hg.mozilla.org/mozilla-central/rev/71af5dbc19e2
https://hg.mozilla.org/mozilla-central/rev/a01bbd72a8e4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.