Closed Bug 1339704 Opened 8 years ago Closed 8 years ago

stylo: Filter out the CascadeLevel which doesn't need to be styled in GetServoAnimationRule

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

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

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(3 files)

Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.
59 bytes, text/x-review-board-request
hiro
: review+
birtles
: review+
Details
Bug 1339704 - Part 1 - Rename nsCSSPropertyIDSet::Invert as Inverse.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1339704 - Part 3 - Update mochitest expectation and reftest status.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Example:
  div {
    margin-left: 300px !important;
  }

  div.animate([ { marginLeft: "10px" },
                { marginLeft: "100px" } ], 3000);

The expected result: we shouldn't see the animation because margin-left has an value with !important level, 300px, which overrides the value with animation level.

The actual result: I still can see the animation.

I checked the cascade result, applicable_declarations, in match_element [1], which has something like this:

1. animation level
ApplicableDeclarationBlock
{
  source: Declarations(RwLock {
    data: PropertyDeclarationBlock {
      declarations: [(margin-left: 79.48333px, Normal)], important_count: 0
    }}),
  level: Animations, source_order: 0, specificity: 0
},

2. author important level
ApplicableDeclarationBlock
{
  source: Style(RwLock {
    data: StyleRule {
      selectors: SelectorList([Selector(#target, specificity = 0x100000)]),
      block: RwLock {
        data: PropertyDeclarationBlock {
          declarations: [(margin-left: 300px, Important)], important_count: 1 } } }}), 
  level: AuthorImportant, source_order: 265, specificity: 1048576
},

We push all information into applicable_delcation, and they looks ok to me. Maybe we haven't implement Animation level yet in other places, so there are something wrong. Although we can skip to do animation for those !important properties, the cascade should still work correctly.

[1] http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/servo/components/style/matching.rs#661-668
Blocks: stylo
Depends on: 1302945
Oh, maybe the source_order and specificity are not correct in Animation level.
I don't expect source-order and specifity to matter, they should only matter within the same cascade level. I can look at this.
Flags: needinfo?(emilio+bugs)
This is because we don't set propertiesToSkip at all[1], especially for transition level.  As a result we composed transition level values from animation level effect so that the transition values override important rules.

[1] https://hg.mozilla.org/mozilla-central/file/d11c29c1db3a/dom/animation/EffectCompositor.cpp#l488

I think this issue will be solved in bug 1334036, once we introduce the same mechanism of UpdateCascadeResults() in stylo.
Flags: needinfo?(emilio+bugs)
Depends on: 1334036
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> This is because we don't set propertiesToSkip at all[1], especially for
> transition level.  As a result we composed transition level values from
> animation level effect so that the transition values override important
> rules.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/d11c29c1db3a/dom/animation/
> EffectCompositor.cpp#l488
> 
> I think this issue will be solved in bug 1334036, once we introduce the same
> mechanism of UpdateCascadeResults() in stylo.

But by the example in comment 0, I didn't see any transition level values in applicable_declarations after doing cascading. Only author important level, animation level, and other lower levels. Bug 1334036 can skip the animations with !important level for OMTA, but not in parallel styling because we still need a thread-safe way to do something like UpdateCascadeResults() in GetServoAnimationRule(). I think even though propertiesToSkip is empty, if there are only Animation level and Author important level (and other lower levels), Author important values should override others. Do we still need to compose transition level also in comment 0?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> As a result we composed transition level values from
> *animation* level effect so that the transition values override important
> rules.

Without propertiesToSkip we compose both of cascade level's effects.  You can easily confirm it to see transition level animation rule.  Or another evidence is that we don't pass |aCascadeLevel| to KeyframeEffectReadOnly::CompsoseStyle().
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > As a result we composed transition level values from
> > *animation* level effect so that the transition values override important
> > rules.
> 
> Without propertiesToSkip we compose both of cascade level's effects.  You
> can easily confirm it to see transition level animation rule.  Or another
> evidence is that we don't pass |aCascadeLevel| to
> KeyframeEffectReadOnly::CompsoseStyle().

Oop. Sorry, it's my bad. Yes, you are right. I didn't notice that. However, UpdateCascadeResults doesn't put the transition properties into the propertiesToSkip [1], and I think the root cause is: I forget to check mElementsToRestyle[CascadeLevel] before doing composeStyle()... I'm so sorry... I will fix that in the bug. Sorry to bother you guys.

[1] http://searchfox.org/mozilla-central/rev/59cd73fbfa14384a81a4e3eb17d3881b372702c4/layout/style/nsRuleNode.cpp#10714
Assignee: nobody → boris.chiou
Summary: stylo: The property value with !important level doesn't override that with animation level → stylo: Filter out the CascadeLevel which doesn't need to be styled in GetServoAnimationRule
Depends on: 1340938
Attachment #8839803 - Flags: review?(hikezoe)
No longer depends on: 1334036
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115816

::: dom/animation/EffectCompositor.cpp:461
(Diff revision 1)
> +  // talos numbers when we introduced it (Tart tests from memory).
> +  // mElementsToRestyle is a document-level data structure but we only need
> +  // to read it during during the parallel traversal. This only possible problem
> +  // is: someone may put a new element into it from RequestRestyle(), but
> +  // it should request another restyle to handle it, not during the current
> +  // restyle, I think.

I am not sure this whole of comment is neccesary. Brian?

::: dom/animation/EffectCompositor.cpp:465
(Diff revision 1)
> +  if (!mElementsToRestyle[aCascadeLevel].Contains(key)) {
> +    return nullptr;
> +  }

With this change, can we get correct value by getComputedStyle() for script animations running on the compositor?
Attachment #8839803 - Flags: review?(bbirtles)
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115816

> With this change, can we get correct value by getComputedStyle() for script animations running on the compositor?

I will try it, but I think the the answer is yes, because we only check if the key exists. For throttle animations, the mElementsToRestyle[aCascadeLevel].get(key) is false, but mElementsToRestyle[aCascadeLevel].Contains(key) should still return true, right?
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115816

> I will try it, but I think the the answer is yes, because we only check if the key exists. For throttle animations, the mElementsToRestyle[aCascadeLevel].get(key) is false, but mElementsToRestyle[aCascadeLevel].Contains(key) should still return true, right?

getComputedStyle() can get correct values for scripted animations runnning on compositor. :)
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115826

::: dom/animation/EffectCompositor.cpp:465
(Diff revision 1)
> +  if (!mElementsToRestyle[aCascadeLevel].Contains(key)) {
> +    return nullptr;
> +  }

Ah, sorry. I was confused.
But I don't still understand how we get computed styles for compositor animations when we flush styles..
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115826

> Ah, sorry. I was confused.
> But I don't still understand how we get computed styles for compositor animations when we flush styles..

I am not familiar with the detailed flow, and all I know is: when we call getComputedStyle(), we mark |mNeedThrottledAnimationFlush|, and call nsComputedDOMStyle::UpdateCurrentStyleSources [1], which will flush pending notifications. So finally, we call PostRestyleForThrottledAnimations() [2] to post restyle for those throttled animations.

[1] http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/layout/style/nsComputedDOMStyle.cpp#800
[2] http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/layout/base/PresShell.cpp#4130
Thank you. Now I understand completely.
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115836

::: dom/animation/EffectCompositor.cpp:461
(Diff revision 1)
> +  // talos numbers when we introduced it (Tart tests from memory).
> +  // mElementsToRestyle is a document-level data structure but we only need
> +  // to read it during during the parallel traversal. This only possible problem
> +  // is: someone may put a new element into it from RequestRestyle(), but
> +  // it should request another restyle to handle it, not during the current
> +  // restyle, I think.

Yes, we don't need all this comment.

Also, I'm going to take a while to get to this review too. We need to work out what the correct behavior here is, not just guess.

::: dom/animation/EffectCompositor.cpp:465
(Diff revision 1)
> +  if (!mElementsToRestyle[aCascadeLevel].Contains(key)) {
> +    return nullptr;
> +  }

Drive-by comment...

When we post that restyle, however, we update the hashmap to indicate that we posted a restyle for the element such that !!Get() will return something true.

Isn't that what we want to check here? i.e. we only want to run ComposeStyle if we've posted a restyle for the element?

(I don't know why returning nullptr is ok for an animation that is running but doesn't need updating though...)
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review115836

> Drive-by comment...
> 
> When we post that restyle, however, we update the hashmap to indicate that we posted a restyle for the element such that !!Get() will return something true.
> 
> Isn't that what we want to check here? i.e. we only want to run ComposeStyle if we've posted a restyle for the element?
> 
> (I don't know why returning nullptr is ok for an animation that is running but doesn't need updating though...)

Using !!Get() is also ok because we change its bool value to true after calling the PostRestyleForAnimation() in both throttle and normal animations. Oops, we should return the current rule! Sorry, I will update it.
(In reply to Boris Chiou [:boris] from comment #15)
> Comment on attachment 8839803 [details]
> Bug 1339704 - Use mElementsToRestyle to filter out unwanted updates.
> 
> https://reviewboard.mozilla.org/r/114360/#review115836
> 
> > Drive-by comment...
> > 
> > When we post that restyle, however, we update the hashmap to indicate that we posted a restyle for the element such that !!Get() will return something true.
> > 
> > Isn't that what we want to check here? i.e. we only want to run ComposeStyle if we've posted a restyle for the element?
> > 
> > (I don't know why returning nullptr is ok for an animation that is running but doesn't need updating though...)
> 
> Using !!Get() is also ok because we change its bool value to true after
> calling the PostRestyleForAnimation() in both throttle and normal
> animations. Oops, we should return the current rule! Sorry, I will update it.

In stylo restyling system, I am wondering when we call GetServoAnimationRule() without RequestRestyle.
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review117016

::: dom/animation/EffectCompositor.cpp:454
(Diff revision 2)
>    if (!mPresContext || !mPresContext->IsDynamic()) {
>      // For print or print preview, ignore animations.
>      return nullptr;
>    }
>  
> +  // FIXME: bug 1340958: Use |const EffectSet*| to make sure we only read it?

Let's not try to fix bug 1340958 here. We don't know how we're going to fix it yet, so let's just drop this comment.

::: dom/animation/EffectCompositor.cpp:460
(Diff revision 2)
>    EffectSet* effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);
>    if (!effectSet) {
>      return nullptr;
>    }
>  
> +  // FIXME: Is it possible to use the |const Element*| as the key?

The trouble is the ctor for PseudoElementHashEntry takes a PseudoElementHashEntry::KeyType and  PseudoElementHashEntry needs a mutable Element since it calls addref/release on it.

We could do that same thing as nsRefPtrHashKey and const_cast in the PseudoElementHashEntry ctor[1], but so long as we have const_casts anywhere, we can't really rely on constness as an indication that we're not mutating the data.

Given that we're probably only constructor entry objects when we modify the hashmap, that's probably a better place to do the const_cast than here, I guess. So perhaps we should do that as a separate patch first.

[1] http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/xpcom/ds/nsHashKeys.h#334

::: dom/animation/EffectCompositor.cpp:463
(Diff revision 2)
>    }
>  
> +  // FIXME: Is it possible to use the |const Element*| as the key?
> +  const PseudoElementHashEntry::KeyType key = { const_cast<Element*>(aElement),
> +                                                aPseudoType };
> +  if (!mElementsToRestyle[aCascadeLevel].Contains(key)) {

As mentioned, if we haven't posted a restyle for this (pseudo-)element then presumably we think the rule is still valid. So we should use !!Get() here instead.

::: dom/animation/EffectCompositor.cpp:464
(Diff revision 2)
>  
> +  // FIXME: Is it possible to use the |const Element*| as the key?
> +  const PseudoElementHashEntry::KeyType key = { const_cast<Element*>(aElement),
> +                                                aPseudoType };
> +  if (!mElementsToRestyle[aCascadeLevel].Contains(key)) {
> +    // This is a read-only op, so it should be thread-safe.

Not sure we need this comment.

::: dom/animation/EffectCompositor.cpp:475
(Diff revision 2)
> +  // FIXME: bug 1340958: We should use SequentialTask to write the value into
> +  //        EffectSet.

Again, let's leave solving this to bug 1340958 here.

::: dom/animation/EffectCompositor.cpp:493
(Diff revision 2)
> +  // FIXME: bug 1340958: We should use SequentialTask to write the value into
> +  //        EffectSet.

Ditto.
Attachment #8839803 - Flags: review?(bbirtles)
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review117016

> The trouble is the ctor for PseudoElementHashEntry takes a PseudoElementHashEntry::KeyType and  PseudoElementHashEntry needs a mutable Element since it calls addref/release on it.
> 
> We could do that same thing as nsRefPtrHashKey and const_cast in the PseudoElementHashEntry ctor[1], but so long as we have const_casts anywhere, we can't really rely on constness as an indication that we're not mutating the data.
> 
> Given that we're probably only constructor entry objects when we modify the hashmap, that's probably a better place to do the const_cast than here, I guess. So perhaps we should do that as a separate patch first.
> 
> [1] http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/xpcom/ds/nsHashKeys.h#334

Thanks, Brian. Let's move the const_cast into PseudoElementHashEntry. Actually, I have another concern for using RefPtr<Element> (in PseudoElementHashEntry) in this function: dom::Elmenet is not thread-safe ref-counted, so using RefPtr<Element> here might not be safe. However, we addref/release on it all in this funtion and someone must still hold it, so it may be ok now.

> As mentioned, if we haven't posted a restyle for this (pseudo-)element then presumably we think the rule is still valid. So we should use !!Get() here instead.

OK. Let's use !!Get().
(In reply to Boris Chiou [:boris] from comment #19)
> Comment on attachment 8839803 [details]
> Bug 1339704 - Use mElementsToRestyle to filter out unwanted updates.
> 
> https://reviewboard.mozilla.org/r/114360/#review117016
> 
> > The trouble is the ctor for PseudoElementHashEntry takes a PseudoElementHashEntry::KeyType and  PseudoElementHashEntry needs a mutable Element since it calls addref/release on it.
> > 
> > We could do that same thing as nsRefPtrHashKey and const_cast in the PseudoElementHashEntry ctor[1], but so long as we have const_casts anywhere, we can't really rely on constness as an indication that we're not mutating the data.
> > 
> > Given that we're probably only constructor entry objects when we modify the hashmap, that's probably a better place to do the const_cast than here, I guess. So perhaps we should do that as a separate patch first.
> > 
> > [1] http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/xpcom/ds/nsHashKeys.h#334
> 
> Thanks, Brian. Let's move the const_cast into PseudoElementHashEntry.
> Actually, I have another concern for using RefPtr<Element> (in
> PseudoElementHashEntry) in this function: dom::Elmenet is not thread-safe
> ref-counted, so using RefPtr<Element> here might not be safe. However, we
> addref/release on it all in this funtion and someone must still hold it, so
> it may be ok now.

We definitely cannot AddRef or Release Elements from the parallel traversal - that has the potential to corrupt the non-atomic refcounts.

> 
> > As mentioned, if we haven't posted a restyle for this (pseudo-)element then presumably we think the rule is still valid. So we should use !!Get() here instead.
> 
> OK. Let's use !!Get().
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #20)
> We definitely cannot AddRef or Release Elements from the parallel traversal
> - that has the potential to corrupt the non-atomic refcounts.

We're not, it's fine.
Depends on: 1344619
Oops, now I realized bug 1344619 breaks this... I am confused.  We should not clear mElementsToRestyle in the pre-traversal?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> Oops, now I realized bug 1344619 breaks this... I am confused.  We should
> not clear mElementsToRestyle in the pre-traversal?

Oh yes. Or do we have another way to let parallel styling know this |element, pseudoType| shouldn't be cascaded?
What I can think of is that using EffectSet::mPropertiesForAnimationsLevel which is a result of UpdateCacadeResult(). Hmm now I guess we should call UpdateCacadeResult() in the pre-traversal as well.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> What I can think of is that using EffectSet::mPropertiesForAnimationsLevel
> which is a result of UpdateCacadeResult(). Hmm now I guess we should call
> UpdateCacadeResult() in the pre-traversal as well.

OK, I see. I will try to call MaybeUpdateCascadeResult in the pre-traversal in Bug 1334036.
(In reply to Boris Chiou [:boris] from comment #25)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> > What I can think of is that using EffectSet::mPropertiesForAnimationsLevel
> > which is a result of UpdateCacadeResult(). Hmm now I guess we should call
> > UpdateCacadeResult() in the pre-traversal as well.
> 
> OK, I see. I will try to call MaybeUpdateCascadeResult in the pre-traversal
> in Bug 1334036.

My guess is just only for mPropertiesForAnimationsLevel, I am not sure that calling UpdateCacadeResult() in the pre-traversal is effective for compositor animations.  mPropertiesForAnimationsLevel does not need nsStyleContext at all, so we can update it without nsStyleContext (i.e. before computing styles).
P1, since Boris appears to be actively working on this.
Priority: -- → P1
Mark this as P2 temporary because I think we should focus on CSS Transition first.
Priority: P1 → P2
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review121674

It's bit tricky but I am OK with this change for now.
You will need to update two entries in reftest-stylo.list in layout/reftests/css-transitions.
Also you will need to update stylo-failures.md.  I did not check which test will be passed in this file.

::: dom/animation/EffectCompositor.cpp:692
(Diff revision 3)
> +  EffectSet* effects = EffectSet::GetEffectSet(aElement, aPseudoType);
> +  if (!effects || !effects->CascadeNeedsUpdate()) {
> +    return;
> +  }

I think we can add MOZ_ASSERT(effects) here.
That's because we will not call this function without EffectSet.  For compositor animations I am thinking we will create a SequentialTask to update cascade results only if we have compositor running animations on the target element.

::: dom/animation/EffectCompositor.cpp:1008
(Diff revision 3)
>          // Drop EffectSets that have been destroyed.
>          iter.Remove();
>          continue;
>        }
>  
> +      MaybeUpdateCascadeResults(target.mElement, target.mPseudoType);

I think we also need to call this function in another PreTraverse().

::: dom/animation/EffectSet.h:198
(Diff revision 3)
> +  const nsCSSPropertyIDSet& PropertiesForAnimationsLevel() const
> +  {
> +    return mPropertiesForAnimationsLevel;
> +  }

I am not sure this is necessary. You are concerned with the static analysis tool?
Attachment #8839803 - Flags: review?(hikezoe) → review+
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review121776

Does this fix the bug described in comment 0? It seems like it doesn't actually look up !important rules, right?

::: dom/animation/EffectCompositor.h:199
(Diff revision 3)
> +  // The variant of the above function. This is for Servo backend because we
> +  // don't use nsStyleContext to get the rule node to traverse the style tree
> +  // to find the !important rules. Instead, we can get the rule node by
> +  // |aElement|.

// Variant of MaybeUpdateCascadeResults for the Servo backend.
// The Servo backend doesn't use an nsStyleContext to get the rule node
// to traverse the style tree to find !important rules and instead
// gets the rule node from |aElement|.

::: dom/animation/EffectCompositor.h:203
(Diff revision 3)
>  
> +  // The variant of the above function. This is for Servo backend because we
> +  // don't use nsStyleContext to get the rule node to traverse the style tree
> +  // to find the !important rules. Instead, we can get the rule node by
> +  // |aElement|.
> +  // FIXME: Implement the rule node traversal for stylo in Bug 1334036.

This FIXME should probably go in the body of MaybeUpdateCascadeResults? Next to where we pass the null nsStyleContext?

::: dom/animation/EffectCompositor.cpp:507
(Diff revision 3)
> -  // stylo: we don't support animations on compositor now, so propertiesToSkip
> -  // is an empty set.
> -  const nsCSSPropertyIDSet propertiesToSkip;
> +  const nsCSSPropertyIDSet& propertiesToSkip =
> +    aCascadeLevel == CascadeLevel::Animations
> +      ? effectSet->PropertiesForAnimationsLevel().Invert()
> +      : effectSet->PropertiesForAnimationsLevel();

(For my own notes--and I think I've noted this before--this works because C++ extends the lifetime of a temporary (the result of Invert) when we bind a reference to it.

Also, I realize now we should have named nsCSSPropertyIDSet::Invert as 'Inverse' instead since that makes it more obvious that the object is not being modified but a new object is being returned.)

::: dom/animation/EffectSet.h:198
(Diff revision 3)
> +  const nsCSSPropertyIDSet& PropertiesForAnimationsLevel() const
> +  {
> +    return mPropertiesForAnimationsLevel;
> +  }

If we do need this, we should just make it return an nsCSSPropertyIDSet by value (instead of a const ref).
Attachment #8839803 - Flags: review?(bbirtles)
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review121776

> It seems like it doesn't actually look up !important rules, right?

This patch can fix the bug descriped in comment 0. I realized that we don't really need to look up !important rules in this case. The root case is that we always update css transitions and css animations even if we only have scripted/css animations [1], so the unwanted/redundant transition overrides the !important rules by cascade ordering. Now, we can use mPropertiesForAnimationsLevel to know that we have animations on this target, and shouldn't update the unwanted/redundant transition, so no one will override the !important rules, and the !important rules will override the animations we created, so everything looks good now.

MaybeUpdateCascadeResults will try to update mPropertiesForAnimationsLevel and mPropertiesWithImportantRules, and I think mPropertiesWithImportantRules is only used for OMTA now to check which animations are overriden by !important rules, so we shouldn't send them to the compositor.

[1] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/servo/components/style/gecko/wrapper.rs#424-426

> This FIXME should probably go in the body of MaybeUpdateCascadeResults? Next to where we pass the null nsStyleContext?

OK, I will put it there.

> If we do need this, we should just make it return an nsCSSPropertyIDSet by value (instead of a const ref).

OK, Let's use call by value.
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review121674

>You will need to update two entries in reftest-stylo.list in layout/reftests/css-transitions.
Also you will need to update stylo-failures.md.  I did not check which test will be passed in this file.

I saw some UNEXPECTED_PASS in my try, and I think those are what you mentioned. I will update patches to fix them. Thanks!

> I think we can add MOZ_ASSERT(effects) here.
> That's because we will not call this function without EffectSet.  For compositor animations I am thinking we will create a SequentialTask to update cascade results only if we have compositor running animations on the target element.

Sure, I will add it!

> I think we also need to call this function in another PreTraverse().

I forgot to add it in another PreTraverse()!
Mark P1 because this may block other P1 bug (bug 1344966)
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8846979 [details]
Bug 1339704 - Part 3 - Update mochitest expectation and reftest status.

https://reviewboard.mozilla.org/r/120020/#review121836
Attachment #8846979 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8846978 [details]
Bug 1339704 - Part 1 - Rename nsCSSPropertyIDSet::Invert as Inverse.

https://reviewboard.mozilla.org/r/120018/#review121838

Thanks!
Attachment #8846978 - Flags: review?(hikezoe) → review+
Comment on attachment 8839803 [details]
Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions.

https://reviewboard.mozilla.org/r/114360/#review121850
Attachment #8839803 - Flags: review?(bbirtles) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9bdecbf729db -d 6066c5ca8f70: rebasing 381884:9bdecbf729db "Bug 1339704 - Part 1 - Rename nsCSSPropertyIDSet::Invert as Inverse. r=hiro"
merging dom/animation/EffectCompositor.cpp
rebasing 381885:6ac88b434b32 "Bug 1339704 - Part 2 - Filter out unwanted CascadeLevel::Transitions. r=birtles,hiro"
merging dom/animation/EffectCompositor.cpp
rebasing 381886:2ef663fb9460 "Bug 1339704 - Part 3 - Update mochitest expectation and reftest status. r=xidorn" (tip)
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Boris, I just landed a change against stylo-failures.md, you need to modify it again. Sorry for the inconvenience.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> Boris, I just landed a change against stylo-failures.md, you need to modify
> it again. Sorry for the inconvenience.

Haha, it's ok, I can wait one more merge. :)
(In reply to Boris Chiou [:boris] from comment #43)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> > Boris, I just landed a change against stylo-failures.md, you need to modify
> > it again. Sorry for the inconvenience.
> 
> Haha, it's ok, I can wait one more merge. :)

Oh, I think I can cherry-pick your patch and rebase it. Anyway, thanks for the reminder. :)
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0c113ba3978
Part 1 - Rename nsCSSPropertyIDSet::Invert as Inverse. r=hiro
https://hg.mozilla.org/integration/autoland/rev/bd0d3ac13d7a
Part 2 - Filter out unwanted CascadeLevel::Transitions. r=birtles,hiro
https://hg.mozilla.org/integration/autoland/rev/adcc2ad6c0a1
Part 3 - Update mochitest expectation and reftest status. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/a0c113ba3978
https://hg.mozilla.org/mozilla-central/rev/bd0d3ac13d7a
https://hg.mozilla.org/mozilla-central/rev/adcc2ad6c0a1
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: