Closed Bug 1341985 Opened 7 years ago Closed 7 years ago

stylo: Mark element which needs to create/remove/modify CSS animations during traversal and create/remove/modify the animations after 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

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(12 files, 3 obsolete files)

Bug 1341985 - We call EnsureTimerStarted on the main-thread after the traversal.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - Split nsStyleAutoArray into a new header to avoid including nsStyleStruct.h in ServoBindingTypes.h.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1341985 - Implement Gecko_StyleAnimationsEquals for checking nsStyleAutoArray<StyleAnimation> equality in servo side.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - Trigger the second traversal for updating CSS animations.
59 bytes, text/x-review-board-request
heycam
: review+
birtles
: review+
Details
Bug 1341985 - Trigger the second traversal for updating CSS animations in the case of Servo_ResolveStyleLazily.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - Update CSS animations in a SequentialTask.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - GetAnimationCollection() takes const Element*.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1341985 - Skip update_animations if we have no running animations and the element becomes display:none.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - Call UpdateAnimations even if the element has no computed values.
59 bytes, text/x-review-board-request
birtles
: review+
heycam
: review+
Details
Bug 1341985 - Use registerCleanupFunction to restore to normal refresh mode.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - Update CSS animation's reftest.list for stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341985 - Update mochitest expectation.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
And after creating/removing/modifying the CSS animations, we do kick the second traversal for the target element and descendants.

https://public.etherpad-mozilla.org/p/stylo-animation#lineNumber=182
Depends on: 1340322
Blocks: stylo-css-transitions
Blocks: 1334036
Blocks: 1335545
Some patches need to be rebased after bug 1343362 lands.
Depends on: 1344581
Comment on attachment 8843765 [details]
Bug 1341985 - Split nsStyleAutoArray into a new header to avoid including nsStyleStruct.h in ServoBindingTypes.h.

https://reviewboard.mozilla.org/r/117328/#review118974

This is a patch split off from bug 1340322 because in that bug we didn't expose nsStyleAutoArray in FFI.
Comment on attachment 8843765 [details]
Bug 1341985 - Split nsStyleAutoArray into a new header to avoid including nsStyleStruct.h in ServoBindingTypes.h.

https://reviewboard.mozilla.org/r/117328/#review119008
Attachment #8843765 - Flags: review?(bbirtles) → review+
Comment on attachment 8843771 [details]
Bug 1341985 - GetAnimationCollection() takes const Element*.

https://reviewboard.mozilla.org/r/117340/#review119014
Attachment #8843771 - Flags: review?(bbirtles) → review+
I don't really understand how this works. If we are calling EffectCompositor::PostRestyleForAnimation during parallel traversal, why is it ok to mutate EffectCompositor::mElementsForDeferredRestyle?

Also, won't this affect regular animation restyles when we only want to do the second pass when we have new animations?
(In reply to Brian Birtles (:birtles) from comment #22)
> I don't really understand how this works. If we are calling
> EffectCompositor::PostRestyleForAnimation during parallel traversal, why is
> it ok to mutate EffectCompositor::mElementsForDeferredRestyle?

The PostRestyleForAnimation is called on the main-thread (actually it's processed during the SequentialTask). I thought I did add an assertion there.

> Also, won't this affect regular animation restyles when we only want to do
> the second pass when we have new animations?

Is there a case that we do request a restyle during parallel traversal?  We should avoid the case apart from this bug.
(In reply to Brian Birtles (:birtles) from comment #22)
> I don't really understand how this works.

This works something like below:

1. Create a SequentialTask if an animation property is changed during the parallel traversal
2. Process the SequentialTask after the parallel traversal
3. Update CSS animations from the SequentialTask
4. PostRestyleForAnimation() is called as a result of 3 if we need to update animation styles
5. Store the element in mElementsForDeferredRestyle that needs to be updated animation styles 
6. Kick the second traversal if we have element in mElementsForDeferredRestyle after the SequentialTask.

All of 2-6 are processed on the main-thread.
Oh, ok. I thought ServoStyleSet::IsInServoTraversal() referred to the parallel traversal but I guess it covers the phase where we're running sequential tasks too.

I haven't gone through all the patches in this series so I don't know how it all fits together but I suspect we might end up needing a two phase approach for Gecko anyway to handle transitions properly. Ideally we should coordinate that work.

I do wonder, however, why we can't just use mElementsForRestyle and why we need to introduce mElementsForDeferredRestyle?

Also, marginally related, I've noticed we often trigger restyles in methods named "XXXNoUpdate" (e.g. CancelFromStyle, PlayFromStyle etc.). I suspect that in Gecko we might want to drop the animation rule node replacement step and just allow those methods to trigger restyles (perhaps coalescing the changes though if that doesn't already happen).
(In reply to Brian Birtles (:birtles) from comment #25)
> Oh, ok. I thought ServoStyleSet::IsInServoTraversal() referred to the
> parallel traversal but I guess it covers the phase where we're running
> sequential tasks too.
> 
> I haven't gone through all the patches in this series so I don't know how it
> all fits together but I suspect we might end up needing a two phase approach
> for Gecko anyway to handle transitions properly. Ideally we should
> coordinate that work.
> 
> I do wonder, however, why we can't just use mElementsForRestyle and why we
> need to introduce mElementsForDeferredRestyle?

That's because we have no chance to clear mElementsForRestyle before we start to process the SequentialTask.
If we created another SequentialTask to remove each element from mElementsForRestyle, I think we can re-use mElementsForRestyle for this issue too.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)

> That's because we have no chance to clear mElementsForRestyle before we
> start to process the SequentialTask.
> If we created another SequentialTask to remove each element from
> mElementsForRestyle, 

*and* the another SequentialTask was surely processed before the SequentialTask for CSS animations.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> (In reply to Brian Birtles (:birtles) from comment #25)
> > I do wonder, however, why we can't just use mElementsForRestyle and why we
> > need to introduce mElementsForDeferredRestyle?
> 
> That's because we have no chance to clear mElementsForRestyle before we
> start to process the SequentialTask.
> If we created another SequentialTask to remove each element from
> mElementsForRestyle, I think we can re-use mElementsForRestyle for this
> issue too.

Why does it need to be another SequentialTask? Why can't we just clobber mElementsForRestyle at the start of the task that updates animations?

Better still, why don't we do the pre-traversal step we discussed and clear the table then?
(In reply to Brian Birtles (:birtles) from comment #28)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> > (In reply to Brian Birtles (:birtles) from comment #25)
> > > I do wonder, however, why we can't just use mElementsForRestyle and why we
> > > need to introduce mElementsForDeferredRestyle?
> > 
> > That's because we have no chance to clear mElementsForRestyle before we
> > start to process the SequentialTask.
> > If we created another SequentialTask to remove each element from
> > mElementsForRestyle, I think we can re-use mElementsForRestyle for this
> > issue too.
> 
> Why does it need to be another SequentialTask? Why can't we just clobber
> mElementsForRestyle at the start of the task that updates animations?

The task is kicked when we drop ThreadLocalStyleContext [1].  I am not sure we can clobber mElementsForRestyle there..

[1] https://hg.mozilla.org/mozilla-central/file/8d026c601510/servo/components/style/context.rs#l274

> Better still, why don't we do the pre-traversal step we discussed and clear
> the table then?

I am sorry, I don't understand about the pre-traversal step. What does it exactly mean?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> (In reply to Brian Birtles (:birtles) from comment #28)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> > > (In reply to Brian Birtles (:birtles) from comment #25)
> > > > I do wonder, however, why we can't just use mElementsForRestyle and why we
> > > > need to introduce mElementsForDeferredRestyle?
> > > 
> > > That's because we have no chance to clear mElementsForRestyle before we
> > > start to process the SequentialTask.
> > > If we created another SequentialTask to remove each element from
> > > mElementsForRestyle, I think we can re-use mElementsForRestyle for this
> > > issue too.
> > 
> > Why does it need to be another SequentialTask? Why can't we just clobber
> > mElementsForRestyle at the start of the task that updates animations?
> 
> The task is kicked when we drop ThreadLocalStyleContext [1].  I am not sure
> we can clobber mElementsForRestyle there..

Ah, OK. We could probably clobber mElementsForRestyle at the end of traverse_subtree(), but I am not 100% sure.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> (In reply to Brian Birtles (:birtles) from comment #28)
> > Better still, why don't we do the pre-traversal step we discussed and clear
> > the table then?
> 
> I am sorry, I don't understand about the pre-traversal step. What does it
> exactly mean?

There are a few notes in bug 1341987 comment 13 and the message I sent to stylo-team but the idea is basically we do:

1. EffectCompositor::WillComposeAnimations
   - Iterate over all elements in mElementsToRestyle with a 'true' flag (i.e. posted restyle)
     - Call WillComposeStyle on each animation in the element's EffectSet and have it update its
       mProgressOnLastCompose / mCurrentIterationOnLastCompose / mFinishedAtLastComposeStyle etc. state
   - Clear from mElementsToRestyle all the elements we visited (i.e. leave the ones with only a throttled
     restyle)
(2. Do animation restyle)
3. Do parallel traversal
4. Generate/update/drop animations and add them to mElementsToRestyle
If needed:
5. Do another pre-traversal step as with (1)
6. Do traversal where we apply animations
No longer depends on: 1344581
Ah, OK. But it's for all of animations, is it scoped in this bug? I thought we will handle those in bug 1340958 (or some other bug related to).
I'd like to avoid adding mElementsForDeferredRestyle if possible. If we can add a slimmed down version of the pre-traversal that simply removes elements we're about to visit from mElementsToRestyle that seems like it would be a step in the right direction.
Do you have a half-baked patch for the pre-traversal? Or have clear vision for that?  I've never thought it in detail so I have no clear vision for that now.  Please file a bug for it and block this bug, and assigned to me.  I think it will take some time because I have no clear idea for now.
Attachment #8843767 - Flags: review?(cam)
Attachment #8843767 - Flags: review?(bbirtles)
Attachment #8843769 - Flags: review?(cam)
Cleared review request against patches that will be affected by the pre-traversal changes.  I think other patches will not affected by the changes at all.
Depends on: 1344619
Filed bug 1344619, and have started to working on that.
Give me a little more time to work on this. I'd like to line up bug 1192592 with this and it may yet be that a second hashmap is appropriate.
OK. It's fine, bug 1344619 will be useful to avoid any threading issues during composing styles in either way.  Leaving the second hash map is not a problem for bug 1344619. ;-)
Comment on attachment 8843762 [details]
Bug 1341985 - Part 1: Use borrow() instead of borrow_mut() for PerDocumentStyleData.

https://reviewboard.mozilla.org/r/117322/#review119172
Attachment #8843762 - Flags: review?(cam) → review+
Comment on attachment 8843763 [details]
Bug 1341985 - Part 7: Add a utility function to convert PseudoElement to nsIAtom*.

https://reviewboard.mozilla.org/r/117324/#review119174
Attachment #8843763 - Flags: review?(cam) → review+
Comment on attachment 8843764 [details]
Bug 1341985 - We call EnsureTimerStarted on the main-thread after the traversal.

https://reviewboard.mozilla.org/r/117326/#review119176

::: commit-message-59fbc:1
(Diff revision 1)
> +Bug 1341985 - Part 3: We call EntureTimerStarted on the main-thread after the traversal. r?heycam

EnsureTimerStarted

::: layout/base/nsRefreshDriver.cpp:1330
(Diff revision 1)
> -  MOZ_ASSERT(!ServoStyleSet::IsInServoTraversal(),
> +  MOZ_ASSERT(!ServoStyleSet::IsInServoTraversal() || NS_IsMainThread(),
>               "EnsureTimerStarted should be called only when we are not "
> -             "in servo traversal");
> +             "in servo traversal or on the main-thread");

It would be nice if the assertion could fail if we are in the Servo traversal but running on the main thread (such as when we heuristically choose to run sequentially, or when STYLO_THREADS=1).  I'm not sure what the cleanest way of exposing that so that we can assert it would be, though.
Attachment #8843764 - Flags: review?(cam) → review+
Comment on attachment 8843766 [details]
Bug 1341985 - Implement Gecko_StyleAnimationsEquals for checking nsStyleAutoArray<StyleAnimation> equality in servo side.

https://reviewboard.mozilla.org/r/117330/#review119186
Attachment #8843766 - Flags: review?(cam) → review+
Comment on attachment 8843768 [details]
Bug 1341985 - Part 8: Split off animation related process in a function.

https://reviewboard.mozilla.org/r/117334/#review119516

This has been moved into bug 1344619.
Attachment #8843767 - Flags: review?(cam)
Attachment #8843768 - Flags: review?(cam)
Attachment #8843769 - Flags: review?(cam)
Comment on attachment 8843767 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations.

https://reviewboard.mozilla.org/r/117332/#review119520

r=me on the layout/style/ changes with these two issues fixed.  birtles should review the other changes.

::: layout/style/ServoStyleSet.cpp:209
(Diff revision 1)
>    bool postTraversalRequired =
>      Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior);

Can you assert before the Servo_TraverseSubtree call that there are no deferred restyles to post?

::: layout/style/ServoStyleSet.cpp:214
(Diff revision 1)
> +  if (mPresContext->EffectCompositor()->PostDeferredRestyles()) {
> +    postTraversalRequired =
> +      Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior);

You should do something like:

  if (Servo_TraverseSubtree(...)) {
    postTraversalRequired = true;
  }

otherwise we could clobber a previous "true" value.  (I'm not certain that the Servo_TraverseSubtree must always return true here -- maybe it could return false if the updated animations didn't cause any restyling to happen.)
Attachment #8843767 - Flags: review?(cam) → review+
Comment on attachment 8843767 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations.

https://reviewboard.mozilla.org/r/117332/#review119524

::: dom/animation/EffectCompositor.cpp:976
(Diff revision 1)
> +  // Store the element only if any ancestors of the element have not stored yet.
> +  // XXX: SequentialTask for child element is surely processed after
> +  // SequentialTask for its parent? If its indeterminate, we need to figure out
> +  // other ways.
> +  nsIContent* parent = aElement->GetParent();
> +  bool contains = false;
> +  while (parent) {
> +    if (!parent->IsElement()) {

I wonder if we really have to do this optimization.  If we don't do it, then the PostRestyleEvent call later will just do similar work -- it will traverse up the tree setting dirty bits until it finds an element that already had a restyle posted for it.

So I think we can just simplify MarkDeferredRestyle down to storing the element in the table.
Comment on attachment 8843769 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations in the case of Servo_ResolveStyleLazily.

https://reviewboard.mozilla.org/r/117336/#review119522

I think I'd like to see this a bit simpler, in conjunction with the comment I just made on the previous patch.

::: dom/animation/EffectCompositor.cpp:339
(Diff revision 1)
> +    } else if (mPresContext->StyleSet()->AsServo()->IsLazyResolving()) {
> +      // FIXME: Bug 1302946: We will handle eRestyle_CSSTransitions.
> +      MOZ_ASSERT(hint == eRestyle_CSSAnimations);
> +      MarkDeferredRestyleForLazyResolving(element);
> +      return;

If we do the simplification in the previous patch, which would make MarkDeferredRestyle and MarkDeferredRestyleForLazyResolving exactly the same (i.e., just storing the element in the table), then we shouldn't need to expose the IsLazyResolving() function or store that state on the ServoStyleSet, since we'll call the same function in either case.

::: dom/animation/EffectCompositor.cpp:349
(Diff revision 1)
>      } else if (!mPresContext->RestyleManager()->IsInStyleRefresh()) {
>        // FIXME: stylo only supports Self and Subtree hints now, so we override
>        // it for stylo if we are not in process of restyling.
>        hint = eRestyle_Self | eRestyle_Subtree;
> +    } else {
> +      MOZ_ASSERT_UNREACHABLE("Should not reqeust restyle");

request

::: layout/style/ServoStyleSet.cpp:673
(Diff revision 1)
> -  return Servo_ResolveStyleLazily(aElement, aPseudoTag, mRawSet.get()).Consume();
> +  mIsLazyResolving = true;
> +  RefPtr<ServoComputedValues> computedValues =
> +    Servo_ResolveStyleLazily(aElement, aPseudoTag, mRawSet.get()).Consume();
> +  mIsLazyResolving = false;
> +
> +  if (mPresContext->EffectCompositor()->PostDeferredRestyle(aElement)) {

In the case of ResolveStyleLazily, we should only ever get a single deferred restyle to process, right?  Instead of having a separate PostDeferredRestyle() call, should we instead just use PostDeferredRestyles() (and it will just iterate over that single entry), and pass in the restyle hint to post (i.e. eRestyle_Self here, and Self|Subtree from the other call site)?
Attachment #8843769 - Flags: review?(cam) → review-
Comment on attachment 8843770 [details]
Bug 1341985 - Update CSS animations in a SequentialTask.

https://reviewboard.mozilla.org/r/117338/#review119530

r=me with these things addressed.

::: layout/style/ServoBindings.cpp:435
(Diff revision 1)
> +{
> +  MOZ_ASSERT(aElement);

We should assert we're on the main thread here.

::: servo/components/style/context.rs:193
(Diff revision 1)
>      /// Sets selector flags. This is used when we need to set flags on an
>      /// element that we don't have exclusive access to (i.e. the parent).
>      SetSelectorFlags(SendElement<E>, ElementSelectorFlags),
> +
> +    /// Marks that we need to create/remove/update CSS animations after the
> +    /// first travesal.

traversal

::: servo/components/style/gecko/wrapper.rs:512
(Diff revision 1)
>          let node_flags = selector_flags_to_node_flags(flags);
>          (self.flags() & node_flags) == node_flags
>      }
> +
> +    unsafe fn update_animations(&self, pseudo: Option<PseudoElement>) {
> +        let atom_ptr = pseudo.map(|p| p.as_atom().as_ptr()).unwrap_or(ptr::null_mut());

Might be nice to have a convenience function for this somewhere, since we also do it in get_animation_rules.  Not sure where, though.

::: servo/components/style/gecko/wrapper.rs:517
(Diff revision 1)
> +        let parent_element = self.parent_element();
> +        let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data());
> +        let parent_values = parent_data.as_ref().map(|d| d.styles().primary.values());
> +        let parent_values_opt = parent_values.map(|v|
> +            *HasArcFFI::arc_as_borrowed(v)
> +        );

Is the choice of parent values correct for ::before and ::after?  Don't they inherit from the element, rather than the element's parent?

::: servo/components/style/matching.rs:605
(Diff revision 1)
> +        if booleans.animate && cfg!(feature = "gecko") {
> +            let ref new_box_style = new_values.get_box();

I think it might be cleaner to factor out the two animations handling blocks (the Servo and Gecko ones) into two identically named helper functions, with different #[cfg(feature = "...")]s on them.

::: servo/components/style/matching.rs:626
(Diff revision 1)
> +                      has_new_animation_style) ||
> +                     (old_display_style != display::T::none &&
> +                      new_display_style == display::T::none)
> +                });
> +            if needs_update_animations {
> +                let task = SequentialTask::update_animations(self.as_node().as_element().unwrap(),

The argument can't just be "self"?
Comment on attachment 8843770 [details]
Bug 1341985 - Update CSS animations in a SequentialTask.

https://reviewboard.mozilla.org/r/117338/#review119536

::: servo/components/style/dom.rs:339
(Diff revision 1)
> +    /// Creates a task to update CSS Animations on a given (pseudo-)element.
> +    /// Note: Gecko only.
> +    unsafe fn update_animations(&self, pseudo: Option<PseudoElement>);

By the way, you'll need to provide an empty implementation of this function in the script crate so that Servo still builds.
Comment on attachment 8843772 [details]
Bug 1341985 - Skip update_animations if we have no running animations and the element becomes display:none.

https://reviewboard.mozilla.org/r/117342/#review119534

::: layout/style/ServoBindings.cpp:464
(Diff revision 1)
> +  nsAnimationManager::CSSAnimationCollection* collection =
> +    nsAnimationManager::CSSAnimationCollection
> +                      ::GetAnimationCollection(aElement, pseudoType);

This reads from the document's main property table, which should be fine since it doesn't do any writes. Let's hope the static analysis realises this. :)

::: servo/components/style/dom.rs:343
(Diff revision 1)
> +    /// Returns true if the element has a CSS animation.
> +    unsafe fn has_css_animations(&self, pseudo: Option<&PseudoElement>) -> bool;

Same here, you'll need an implementation of this method for Servo elements.
Attachment #8843772 - Flags: review?(cam) → review+
Comment on attachment 8843773 [details]
Bug 1341985 - Call UpdateAnimations even if the element has no computed values.

https://reviewboard.mozilla.org/r/117344/#review119556

::: servo/components/style/gecko/wrapper.rs:515
(Diff revision 1)
>      }
>  
>      unsafe fn update_animations(&self, pseudo: Option<PseudoElement>) {
>          let atom_ptr = pseudo.map(|p| p.as_atom().as_ptr()).unwrap_or(ptr::null_mut());
>  
> -        let computed_data = self.borrow_data().unwrap();
> +        // We have to update animation even if the element has no computed style

*animations

::: servo/components/style/gecko/wrapper.rs:516
(Diff revision 1)
> -        let computed_values = computed_data.styles().primary.values();
> +        // since it means the element is in display:none subtree, we should destroy
> +        // all CSS animations in display:none subtree.

s/in display:none subtree/in a display:none subtree/
Attachment #8843773 - Flags: review?(cam) → review+
Comment on attachment 8843774 [details]
Bug 1341985 - Use registerCleanupFunction to restore to normal refresh mode.

https://reviewboard.mozilla.org/r/117346/#review119560

::: commit-message-a9dfa:1
(Diff revision 1)
> +Bug 1341985 - Part 13: Use regsterCleanupFunction to restore to normal refresh mode. r?heycam

*registerCleanupFunction

::: commit-message-a9dfa:3
(Diff revision 1)
> +Bug 1341985 - Part 13: Use regsterCleanupFunction to restore to normal refresh mode. r?heycam
> +
> +Othewise, the refresh driver left under test mode when a javascript error

*Otherwise
Attachment #8843774 - Flags: review?(cam) → review+
Comment on attachment 8843775 [details]
Bug 1341985 - Update CSS animation's reftest.list for stylo.

https://reviewboard.mozilla.org/r/117348/#review119562
Attachment #8843775 - Flags: review?(cam) → review+
Comment on attachment 8843776 [details]
Bug 1341985 - Update mochitest expectation.

https://reviewboard.mozilla.org/r/117350/#review119564
Attachment #8843776 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #46)

> ::: layout/style/ServoStyleSet.cpp:673
> (Diff revision 1)
> > -  return Servo_ResolveStyleLazily(aElement, aPseudoTag, mRawSet.get()).Consume();
> > +  mIsLazyResolving = true;
> > +  RefPtr<ServoComputedValues> computedValues =
> > +    Servo_ResolveStyleLazily(aElement, aPseudoTag, mRawSet.get()).Consume();
> > +  mIsLazyResolving = false;
> > +
> > +  if (mPresContext->EffectCompositor()->PostDeferredRestyle(aElement)) {
> 
> In the case of ResolveStyleLazily, we should only ever get a single deferred
> restyle to process, right?  Instead of having a separate
> PostDeferredRestyle() call, should we instead just use
> PostDeferredRestyles() (and it will just iterate over that single entry),
> and pass in the restyle hint to post (i.e. eRestyle_Self here, and
> Self|Subtree from the other call site)?

A cumbersome point is that ResolveStyleLazily is not in sInServoTravesal state. Without mIsLazyResolving flag, we can't differentiate whether a request comes from outside this updating CSS animation process in EffectCompositor::PostRestyleForAnimation.

Can we set sInServoTravesal true in ResolveStyleLazily too?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> A cumbersome point is that ResolveStyleLazily is not in sInServoTravesal
> state. Without mIsLazyResolving flag, we can't differentiate whether a
> request comes from outside this updating CSS animation process in
> EffectCompositor::PostRestyleForAnimation.
> 
> Can we set sInServoTravesal true in ResolveStyleLazily too?

Yeah, I think conceptually it is a similar situation -- we are doing restyling, although it will happen to be on the main thread, and we won't actually traverse.  So that sounds OK to me.
(In reply to Cameron McCormack (:heycam) from comment #55)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> > A cumbersome point is that ResolveStyleLazily is not in sInServoTravesal
> > state. Without mIsLazyResolving flag, we can't differentiate whether a
> > request comes from outside this updating CSS animation process in
> > EffectCompositor::PostRestyleForAnimation.
> > 
> > Can we set sInServoTravesal true in ResolveStyleLazily too?
> 
> Yeah, I think conceptually it is a similar situation -- we are doing
> restyling, although it will happen to be on the main thread, and we won't
> actually traverse.  So that sounds OK to me.

Thanks! I will do it.
Comment on attachment 8843773 [details]
Bug 1341985 - Call UpdateAnimations even if the element has no computed values.

https://reviewboard.mozilla.org/r/117344/#review119918

::: layout/style/nsAnimationManager.cpp:1131
(Diff revision 1)
>               "document tree");
>  
>    CSSPseudoElementType pseudoType =
>      nsCSSPseudoElements::GetPseudoType(aPseudoTagOrNull,
>                                         CSSEnabledState::eForAllContent);
> +  if (!aComputedValues) {

Should this have a comment saying something like "If we are in a display:none subtree we will have no computed values. Since CSS animations should not run in display:none subtrees we should stop (actually, destroy) any animations on this element here."
Attachment #8843773 - Flags: review?(bbirtles) → review+
Comment on attachment 8843770 [details]
Bug 1341985 - Update CSS animations in a SequentialTask.

https://reviewboard.mozilla.org/r/117338/#review119530

> Is the choice of parent values correct for ::before and ::after?  Don't they inherit from the element, rather than the element's parent?

This parent values are for getting ineritance value in Servo_GetComputedKeyframeValues.

> The argument can't just be "self"?

No, at least I can't. :/
I got this error:
the trait bound `&Self: dom::TElement` is not satisfied
Though there are still a couple of issues what I need to figure out how to do it, I think these patch can be reviewed as the second round.
Comment on attachment 8843763 [details]
Bug 1341985 - Part 7: Add a utility function to convert PseudoElement to nsIAtom*.

This needs to be reviewed by Cameron.
Attachment #8843763 - Flags: review+ → review?(cam)
I did a chat with Brian on IRC this morning about mElementsForDeferredRestyle, the hashtable storing elements for the second traversal.  And we've decided to drop it for now.  I will post updated patches soon.
Comment on attachment 8843769 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations in the case of Servo_ResolveStyleLazily.

https://reviewboard.mozilla.org/r/117336/#review120296

::: dom/animation/EffectCompositor.cpp:1023
(Diff revision 3)
>      if (!elementsToRestyle.Get(key)) {
>        // Ignore throttled restyle and no restyle request.
>        continue;
>      }
>  
> +    Servo_NoteExplicitHints(aElement, eRestyle_Self, nsChangeHint(0));

Are we calling this instead of PostRestyleEvent deliberately, because we don't need to the work that that does (registering as a refresh observer, flagging that a style flush is needed)?  If not, then let's just call PostRestyleEvent.
Attachment #8843769 - Flags: review?(cam) → review+
Comment on attachment 8843768 [details]
Bug 1341985 - Part 8: Split off animation related process in a function.

https://reviewboard.mozilla.org/r/117334/#review120302
Attachment #8843768 - Flags: review?(cam) → review+
Comment on attachment 8843770 [details]
Bug 1341985 - Update CSS animations in a SequentialTask.

https://reviewboard.mozilla.org/r/117338/#review120306

::: servo/components/style/dom.rs:339
(Diff revision 3)
> +    /// Creates a task to update CSS Animations on a given (pseudo-)element.
> +    /// Note: Gecko only.
> +    fn update_animations(&self, _pseudo: Option<&PseudoElement>) {
> +    }

Ah, I changed my mind, sorry.  I think it would better to add the implementation to the script crate for Servo, and make it panic instead with a message saying that this should only be called for Gecko.
Attachment #8843770 - Flags: review?(cam) → review+
Comment on attachment 8843763 [details]
Bug 1341985 - Part 7: Add a utility function to convert PseudoElement to nsIAtom*.

https://reviewboard.mozilla.org/r/117324/#review120310
Attachment #8843763 - Flags: review?(cam) → review+
Comment on attachment 8843767 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations.

https://reviewboard.mozilla.org/r/117332/#review119010

Sorry, I'm missing some context here so I don't quite follow this. What is the order of steps here?

I assumed it was something like:

1. PreTraverse
2. First parallel traversal
3. Update animations
4. Second parallel traversal, if needed

Is that right?

::: commit-message-8b6a9:1
(Diff revision 1)
> +Bug 1341985 - Part 6: Kick the second traversal for updating CSS animations. r?heycam,birtles

Nit: s/Kick/Trigger/

::: dom/animation/EffectCompositor.h:232
(Diff revision 3)
>      const AnimationPerformanceWarning& aWarning);
>  
>    // Do a bunch of stuff that we should avoid doing during the parallel
>    // traversal (e.g. changing member variables) for all elements that we expect
>    // to restyle on the next traversal.
> -  void PreTraverse();
> +  // Returns true we had elements for the stuff.

I have no idea what this means. I'm guessing this is a placeholder comment? :)

::: layout/style/ServoStyleSet.cpp:224
(Diff revision 3)
>    bool postTraversalRequired =
>      Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior);
> +
> +  // Kick the second traversal to make CSS animations' styles up-to-date as
> +  // needed.
> +  if (mPresContext->EffectCompositor()->PreTraverse()) {
> +    if (Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior)) {
> +      postTraversalRequired = true;
> +    }
> +  }

How does this work? I thought the pre-traversal step was before the first traversal?

::: layout/style/ServoStyleSet.cpp:229
(Diff revision 3)
> +  if (mPresContext->EffectCompositor()->PreTraverse()) {
> +    if (Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior)) {
> +      postTraversalRequired = true;
> +    }
> +  }

Why isn't this just:

if (mPresContext->... &&
    Servo_TraverseSubtree...) {
  postTraversalRequired = true;
}

Is it because we add extra logic in the middle in later patches?
(In reply to Cameron McCormack (:heycam) from comment #88)
> ::: dom/animation/EffectCompositor.cpp:1023
> (Diff revision 3)
> >      if (!elementsToRestyle.Get(key)) {
> >        // Ignore throttled restyle and no restyle request.
> >        continue;
> >      }
> >  
> > +    Servo_NoteExplicitHints(aElement, eRestyle_Self, nsChangeHint(0));
> 
> Are we calling this instead of PostRestyleEvent deliberately, because we
> don't need to the work that that does (registering as a refresh observer,
> flagging that a style flush is needed)?  If not, then let's just call
> PostRestyleEvent.

Indeed. This sounds correct to me. So, I tried, but unfortunately we have an assertion of !ServoStyleSet::IsInServoTraversal() check in PostRestyleEvent().

As far as I can tell this PreTraverse() is called in a style flush. And at least for the second traversal that will be kicked as a result of this request, it seems to me that we don't need to register a refresh observer.  So I am going to use Servo_NoteExplicitHints() directly here and I will revisit this problem later if we have an issue.
(In reply to Brian Birtles (:birtles) from comment #92)
> 1. PreTraverse
> 2. First parallel traversal
> 3. Update animations
> 4. Second parallel traversal, if needed
> 
> Is that right?

We have the second pre-traversal before 4.

> if (mPresContext->... &&
>     Servo_TraverseSubtree...) {
>   postTraversalRequired = true;
> }
> 
> Is it because we add extra logic in the middle in later patches?

Thanks. I will do this.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #93)
> Indeed. This sounds correct to me. So, I tried, but unfortunately we have an
> assertion of !ServoStyleSet::IsInServoTraversal() check in
> PostRestyleEvent().
> 
> As far as I can tell this PreTraverse() is called in a style flush. And at
> least for the second traversal that will be kicked as a result of this
> request, it seems to me that we don't need to register a refresh observer. 
> So I am going to use Servo_NoteExplicitHints() directly here and I will
> revisit this problem later if we have an issue.

OK, I think it's OK to call Servo_NoteExplicitHints then.  I would prefer this to be a method on ServoRestyleManager, so that we can abstract away the FFI functions from code like this, though.  (I guess it can be a static method since we don't actually need to do anything with the restyle manager itself.)  Maybe name it "PostRestyleEventForAnimations", or something like that?  And add a comment to the declaration of that method that explains why it's needed (and why we can't call PostRestyleEvent).
(In reply to Cameron McCormack (:heycam) from comment #95)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #93)
> > Indeed. This sounds correct to me. So, I tried, but unfortunately we have an
> > assertion of !ServoStyleSet::IsInServoTraversal() check in
> > PostRestyleEvent().
> > 
> > As far as I can tell this PreTraverse() is called in a style flush. And at
> > least for the second traversal that will be kicked as a result of this
> > request, it seems to me that we don't need to register a refresh observer. 
> > So I am going to use Servo_NoteExplicitHints() directly here and I will
> > revisit this problem later if we have an issue.
> 
> OK, I think it's OK to call Servo_NoteExplicitHints then.  I would prefer
> this to be a method on ServoRestyleManager, so that we can abstract away the
> FFI functions from code like this, though.  (I guess it can be a static
> method since we don't actually need to do anything with the restyle manager
> itself.)  Maybe name it "PostRestyleEventForAnimations", or something like
> that?  And add a comment to the declaration of that method that explains why
> it's needed (and why we can't call PostRestyleEvent).

That's really a nice idea. I will do it. Thank you!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #94)
> (In reply to Brian Birtles (:birtles) from comment #92)
> > 1. PreTraverse
> > 2. First parallel traversal
> > 3. Update animations
> > 4. Second parallel traversal, if needed
> > 
> > Is that right?
> 
> We have the second pre-traversal before 4.

Why is that? I thought step (3) was where we updated animations when there were new/deleted/updated animations but for animations that tick normally we update them in step (2)? And so if it's a pre-traversal, it should happen before then?
(In reply to Brian Birtles (:birtles) from comment #97)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #94)
> > (In reply to Brian Birtles (:birtles) from comment #92)
> > > 1. PreTraverse
> > > 2. First parallel traversal
> > > 3. Update animations
> > > 4. Second parallel traversal, if needed
> > > 
> > > Is that right?
> > 
> > We have the second pre-traversal before 4.
> 
> Why is that? I thought step (3) was where we updated animations when there
> were new/deleted/updated animations but for animations that tick normally we
> update them in step (2)? And so if it's a pre-traversal, it should happen
> before then?

We need to call Servo_NoteExplicitHints() somewhere outside the SequentialTask of updating animations.
When we had mElementsForDeferredRestyle, it's PostDeferredRestyles(), but now we use mElementsToRestyle instead of mElementsForDeferredRestyle, so I did add it in PreTraverse().  Do you prefer adding another function for this purpose?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> (In reply to Brian Birtles (:birtles) from comment #97)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #94)
> > > (In reply to Brian Birtles (:birtles) from comment #92)
> > > > 1. PreTraverse
> > > > 2. First parallel traversal
> > > > 3. Update animations
> > > > 4. Second parallel traversal, if needed
> > > > 
> > > > Is that right?
> > > 
> > > We have the second pre-traversal before 4.
> > 
> > Why is that? I thought step (3) was where we updated animations when there
> > were new/deleted/updated animations but for animations that tick normally we
> > update them in step (2)? And so if it's a pre-traversal, it should happen
> > before then?
> 
> We need to call Servo_NoteExplicitHints() somewhere outside the
> SequentialTask of updating animations.
> When we had mElementsForDeferredRestyle, it's PostDeferredRestyles(), but
> now we use mElementsToRestyle instead of mElementsForDeferredRestyle, so I
> did add it in PreTraverse().  Do you prefer adding another function for this
> purpose?

Also note that, apart from Servo_NoteExplicitHints, we call PreTraverse() before the second traversal since we need to update mProgressOnLastCompose, or other stuff for the second traversal.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> We need to call Servo_NoteExplicitHints() somewhere outside the
> SequentialTask of updating animations.
> When we had mElementsForDeferredRestyle, it's PostDeferredRestyles(), but
> now we use mElementsToRestyle instead of mElementsForDeferredRestyle, so I
> did add it in PreTraverse().  Do you prefer adding another function for this
> purpose?

What does Servo_NoteExplicitHints() do? Why does it need to be called outside the SequentialTask for updating animations?
(In reply to Brian Birtles (:birtles) from comment #100)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> > We need to call Servo_NoteExplicitHints() somewhere outside the
> > SequentialTask of updating animations.
> > When we had mElementsForDeferredRestyle, it's PostDeferredRestyles(), but
> > now we use mElementsToRestyle instead of mElementsForDeferredRestyle, so I
> > did add it in PreTraverse().  Do you prefer adding another function for this
> > purpose?
> 
> What does Servo_NoteExplicitHints() do? Why does it need to be called
> outside the SequentialTask for updating animations?

I did leave a comment in PostRestyleForAnimation.

// We can't call Servo_NoteExplicitHints here since AtomicRefCell does not allow us to mutate ElementData of the |aElement| in SequentialTask.

I am not sure why AtomicRefCell blocks us in detail though.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #101)
> (In reply to Brian Birtles (:birtles) from comment #100)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> > > We need to call Servo_NoteExplicitHints() somewhere outside the
> > > SequentialTask of updating animations.
> > > When we had mElementsForDeferredRestyle, it's PostDeferredRestyles(), but
> > > now we use mElementsToRestyle instead of mElementsForDeferredRestyle, so I
> > > did add it in PreTraverse().  Do you prefer adding another function for this
> > > purpose?
> > 
> > What does Servo_NoteExplicitHints() do? Why does it need to be called
> > outside the SequentialTask for updating animations?
> 
> I did leave a comment in PostRestyleForAnimation.
> 
> // We can't call Servo_NoteExplicitHints here since AtomicRefCell does not
> allow us to mutate ElementData of the |aElement| in SequentialTask.
> 
> I am not sure why AtomicRefCell blocks us in detail though.

I guess the reason is that the SequentialTask does not have mut Element. Oh, can we have a mut one?
Maybe I am wrong mut is not related to AtomicRefCell.
What does Servo_NoteExplicitHints() do? What is the expected sequence here? For which parts is ServoStyleSet::IsInServoTraversal() true?

I'll look into this tomorrow but it would help if you can provide some context here.
It sets restyle hints and change hints to element data.

https://hg.mozilla.org/mozilla-central/file/19289cc8bf6f/servo/ports/geckolib/glue.rs#l1219

servo's styling processes based on those hints.
(In reply to Brian Birtles (:birtles) from comment #104)
> What does Servo_NoteExplicitHints() do? What is the expected sequence here?
> For which parts is ServoStyleSet::IsInServoTraversal() true?

The sequence is:

1. PreTraverse
2. the first parallel traversal
 2 - 1. create SequentialTask(s) for each element which updated animation properties
3. the SequentialTask
 3 - 1. PostRestyleForAnimation might be called if necessary
4. Second PreTraverse
5. the second parallel traversal if necessary

ServoStyleSet::IsInServoTraversal() is true in all the cases other than 1. Also 1, 3, 4 run on the main-thread.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Priority: -- → P1
I think this is a P1 bug (according to Bobby's mail).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #106)
> (In reply to Brian Birtles (:birtles) from comment #104)
> > What does Servo_NoteExplicitHints() do? What is the expected sequence here?
> > For which parts is ServoStyleSet::IsInServoTraversal() true?
> 
> The sequence is:
> 
> 1. PreTraverse
> 2. the first parallel traversal
>  2 - 1. create SequentialTask(s) for each element which updated animation
> properties
> 3. the SequentialTask
>  3 - 1. PostRestyleForAnimation might be called if necessary
> 4. Second PreTraverse
> 5. the second parallel traversal if necessary
> 
> ServoStyleSet::IsInServoTraversal() is true in all the cases other than 1.
> Also 1, 3, 4 run on the main-thread.

Thank you! That's exactly the information I was looking for. It's especially helpful when reviewing a partial patch series.
Comment on attachment 8843767 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations.

https://reviewboard.mozilla.org/r/117332/#review120762

::: dom/animation/EffectCompositor.h:232
(Diff revision 3)
>      const AnimationPerformanceWarning& aWarning);
>  
>    // Do a bunch of stuff that we should avoid doing during the parallel
>    // traversal (e.g. changing member variables) for all elements that we expect
>    // to restyle on the next traversal.
> -  void PreTraverse();
> +  // Returns true we had elements for the stuff.

Why did this issue get dropped?

"Returns true we had elements for the stuff" doesn't make any sense.
(In reply to Brian Birtles (:birtles) from comment #109)
> Comment on attachment 8843767 [details]
> Bug 1341985 - Part 5: Kick the second traversal for updating CSS animations.
> 
> https://reviewboard.mozilla.org/r/117332/#review120762
> 
> ::: dom/animation/EffectCompositor.h:232
> (Diff revision 3)
> >      const AnimationPerformanceWarning& aWarning);
> >  
> >    // Do a bunch of stuff that we should avoid doing during the parallel
> >    // traversal (e.g. changing member variables) for all elements that we expect
> >    // to restyle on the next traversal.
> > -  void PreTraverse();
> > +  // Returns true we had elements for the stuff.
> 
> Why did this issue get dropped?
> 
> "Returns true we had elements for the stuff" doesn't make any sense.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #106)
> 4. Second PreTraverse
> 5. the second parallel traversal if necessary

I thought this explained the reason.  
If we had elements that need to be updated in the second parallel traversal, then PreTraverse() returns true.
Comment on attachment 8843767 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations.

https://reviewboard.mozilla.org/r/117332/#review120768

::: dom/animation/EffectCompositor.h:232
(Diff revision 3)
>      const AnimationPerformanceWarning& aWarning);
>  
>    // Do a bunch of stuff that we should avoid doing during the parallel
>    // traversal (e.g. changing member variables) for all elements that we expect
>    // to restyle on the next traversal.
> -  void PreTraverse();
> +  // Returns true we had elements for the stuff.

How about just, "Returns true if we there are elements needing a restyle for animation." ?

::: dom/animation/EffectCompositor.cpp:326
(Diff revision 3)
> +      // We can't call Servo_NoteExplicitHints here since AtomicRefCell does not
> +      // allow us to mutate ElementData of the |aElement| in SequentialTask.
> +      // We call Servo_NoteExplicitHints for the element in PreTraverse() right
> +      // before the second traversal for CSS animations.
> +      // Don't post any restyle requests to the pres shell. This request will be
> +      // processed during the second traversal.

// We can't call Servo_NoteExplicitHints here since AtomicRefCell does not
// allow us to mutate ElementData of the |aElement| in SequentialTask.
// Instead we call Servo_NoteExplicitHints for the element in PreTraverse()
// which will be called right before the second traversal that we do for
// updating CSS animations.
// In that case PreTraverse() will return true so that we know to do the
// second traversal so we don't need to post any restyle requests to the
// PresShell.

::: dom/animation/EffectCompositor.cpp:958
(Diff revision 3)
>  EffectCompositor::PreTraverse()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mPresContext->RestyleManager()->IsServo());
>  
> +  bool found = false;

Call this foundElementsNeedingRestyle ?

::: dom/animation/EffectCompositor.cpp:979
(Diff revision 3)
> +      found = true;
> +
>        EffectSet* effects =
>          EffectSet::GetEffectSet(target.mElement, target.mPseudoType);
>        if (!effects) {
>          // Drop the EffectSets that have been destroyed.

(Nit: As per bug 1344619 comment 36, drop the 'the' here.)

::: layout/style/ServoStyleSet.cpp:227
(Diff revision 3)
> +  // Kick the second traversal to make CSS animations' styles up-to-date as
> +  // needed.

// If there are still animation restyles needed, trigger a second traversal to
  // update CSS animations' styles.

?
Attachment #8843767 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #111)
> Comment on attachment 8843767 [details]
> Bug 1341985 - Part 5: Kick the second traversal for updating CSS animations.
> 
> https://reviewboard.mozilla.org/r/117332/#review120768
> 
> ::: dom/animation/EffectCompositor.h:232
> (Diff revision 3)
> >      const AnimationPerformanceWarning& aWarning);
> >  
> >    // Do a bunch of stuff that we should avoid doing during the parallel
> >    // traversal (e.g. changing member variables) for all elements that we expect
> >    // to restyle on the next traversal.
> > -  void PreTraverse();
> > +  // Returns true we had elements for the stuff.
> 
> How about just, "Returns true if we there are elements needing a restyle for
> animation." ?

Returns true if there are elements needing a restyle for animation.
Depends on: 1346065
Comment on attachment 8843764 [details]
Bug 1341985 - We call EnsureTimerStarted on the main-thread after the traversal.

https://reviewboard.mozilla.org/r/117326/#review119176

> It would be nice if the assertion could fail if we are in the Servo traversal but running on the main thread (such as when we heuristically choose to run sequentially, or when STYLO_THREADS=1).  I'm not sure what the cleanest way of exposing that so that we can assert it would be, though.

I did't come up with a clear idea for now, so I filed bug 1346065.
Comment on attachment 8843767 [details]
Bug 1341985 - Trigger the second traversal for updating CSS animations.

https://reviewboard.mozilla.org/r/117332/#review119520

> Can you assert before the Servo_TraverseSubtree call that there are no deferred restyles to post?

Now this Servo_TraverseSubtree is called only if we have elements that needs to be updated in the second traversal. So, I don't think the assertion is necessary here. Thanks.
Attachment #8843762 - Attachment is obsolete: true
Attachment #8843763 - Attachment is obsolete: true
Attachment #8843768 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6ab48ced3b8
We call EnsureTimerStarted on the main-thread after the traversal. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c6b334aa177c
Split nsStyleAutoArray into a new header to avoid including nsStyleStruct.h in ServoBindingTypes.h. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d018acf339b3
Implement Gecko_StyleAnimationsEquals for checking nsStyleAutoArray<StyleAnimation> equality in servo side. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ffe615ac2f20
Trigger the second traversal for updating CSS animations. r=birtles,heycam
https://hg.mozilla.org/integration/autoland/rev/ce96353dd040
Trigger the second traversal for updating CSS animations in the case of Servo_ResolveStyleLazily. r=heycam
https://hg.mozilla.org/integration/autoland/rev/03f4ba0942c7
Update CSS animations in a SequentialTask. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a6765b4cdf73
GetAnimationCollection() takes const Element*. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9d0b76d22267
Skip update_animations if we have no running animations and the element becomes display:none. r=heycam
https://hg.mozilla.org/integration/autoland/rev/5adbe41c0e30
Call UpdateAnimations even if the element has no computed values. r=birtles,heycam
https://hg.mozilla.org/integration/autoland/rev/13ad5103ea62
Use registerCleanupFunction to restore to normal refresh mode. r=heycam
https://hg.mozilla.org/integration/autoland/rev/da8b2149d6a2
Update CSS animation's reftest.list for stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/500becbd127a
Update mochitest expectation. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: