1415780 - Fix timeout in test_event-dispatch.html with conformant Promise handling

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

Below is the test cases that cause the timeout:

 'Active -> Idle, setting Animation.timeline = null'
 'Set timeline and play transition after clearing the timeline.'

It seems setting null timeline caused it?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

In case of transitions tests:
 'Before -> Idle (Animation.timeline = null)'
 'Active -> Idle, no delay (Animation.timeline = null)'
 'Active -> Idle, with positive delay (Animation.timeline = null)'
 'Active -> Idle, with negative delay (Animation.timeline = null)'

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I will start looking this.  As far as I cal tell, this is the last failure caused by unknown reason.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Hmm, it seems to me that cancel event will never be fired if the target animation is the last one since we unregister refresh driver in DocumentTimeline::RemoveAnimation() if the removing animation is the last one.

I have no idea why the failure cases don't fail on Stylo though.

Comment 4

[Brian Birtles (:birtles)]

Is this bug about css-animations/test_event-dispatch.html or css-transitions/test_event-dispatch.html ?

Cancel events should end up being dispatched on the next tick due to:

  http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817

i.e. that should make us the document timeline re-register with the refresh driver. Perhaps the ordering is different now and we end up re-registering but then unregistering.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Is this bug about css-animations/test_event-dispatch.html or
> css-transitions/test_event-dispatch.html ?
> 
> Cancel events should end up being dispatched on the next tick due to:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817

Thanks! It helped me to understand what's wrong.  The test case set null timeline, not using cancel().

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > Is this bug about css-animations/test_event-dispatch.html or
> > css-transitions/test_event-dispatch.html ?
> > 
> > Cancel events should end up being dispatched on the next tick due to:
> > 
> >  
> > http://searchfox.org/mozilla-central/rev/
> > c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817
> 
> Thanks! It helped me to understand what's wrong.  The test case set null
> timeline, not using cancel().

Oh wait. SetTimelineNoUpdate() also calls UpdateTiming() there..

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > (In reply to Brian Birtles (:birtles) from comment #4)
> > > Is this bug about css-animations/test_event-dispatch.html or
> > > css-transitions/test_event-dispatch.html ?
> > > 
> > > Cancel events should end up being dispatched on the next tick due to:
> > > 
> > >  
> > > http://searchfox.org/mozilla-central/rev/
> > > c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817
> > 
> > Thanks! It helped me to understand what's wrong.  The test case set null
> > timeline, not using cancel().
> 
> Oh wait. SetTimelineNoUpdate() also calls UpdateTiming() there..

I guess re-registering refresh driver fails if we have no timeline.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

It seems to me that we don't need to eagerly remove the animation from the old timeline in Animation::SetTimelineNoUpdate().  We end up removing it in DocumentTimeline::WillRefresh() in the next tick.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> It seems to me that we don't need to eagerly remove the animation from the
> old timeline in Animation::SetTimelineNoUpdate().  We end up removing it in
> DocumentTimeline::WillRefresh() in the next tick.

Gah, it breaks bug 1277272.  So nsAnimationManager or nsTransitionManager observes refresh driver directly?

Comment 10

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > It seems to me that we don't need to eagerly remove the animation from the
> > old timeline in Animation::SetTimelineNoUpdate().  We end up removing it in
> > DocumentTimeline::WillRefresh() in the next tick.
> 
> Gah, it breaks bug 1277272.  So nsAnimationManager or nsTransitionManager
> observes refresh driver directly?

No, they are called from within nsRefreshDriver::Tick:

  http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/layout/base/nsRefreshDriver.cpp#1614-1620

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > > It seems to me that we don't need to eagerly remove the animation from the
> > > old timeline in Animation::SetTimelineNoUpdate().  We end up removing it in
> > > DocumentTimeline::WillRefresh() in the next tick.
> > 
> > Gah, it breaks bug 1277272.  So nsAnimationManager or nsTransitionManager
> > observes refresh driver directly?
> 
> No, they are called from within nsRefreshDriver::Tick:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> c99d035f00dd894feff38e4ad28a73fb679c63a6/layout/base/nsRefreshDriver.
> cpp#1614-1620

What I meant is that nsAnimationManager or nsTransitionManager should keep the nsRefreshDriver awake, here in nsRefreshDriver::Tick

 for (uint32_t i = 0; i < ArrayLength(mObservers); ++i) {
  ..
  ..
  DispatchAnimationEvents();

If mObservers is empty the refresh driver never calls DispatchAnimationEvents(), that's the problem of this bug.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

To be more clear, when setting null timeline for an animation, the DocumentTimeline unregisters itself from nsRefreshDriver (mObservers there).

And in this test case we fail to re-register document timeline to the refresh driver since the animation has no timeline.  But even if we succeeded to re-register document timeline to the refresh driver, we will hit by bug 1277272 in some cases.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> To be more clear, when setting null timeline for an animation, the
> DocumentTimeline unregisters itself from nsRefreshDriver (mObservers there).
> 
> And in this test case we fail to re-register document timeline to the
> refresh driver since the animation has no timeline.  But even if we
> succeeded to re-register document timeline to the refresh driver, we will
> hit by bug 1277272 in some cases.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > > > It seems to me that we don't need to eagerly remove the animation from the
> > > > old timeline in Animation::SetTimelineNoUpdate().  We end up removing it in
> > > > DocumentTimeline::WillRefresh() in the next tick.
> > > 
> > > Gah, it breaks bug 1277272.  So nsAnimationManager or nsTransitionManager
> > > observes refresh driver directly?
> > 
> > No, they are called from within nsRefreshDriver::Tick:
> > 
> >  
> > http://searchfox.org/mozilla-central/rev/
> > c99d035f00dd894feff38e4ad28a73fb679c63a6/layout/base/nsRefreshDriver.
> > cpp#1614-1620
> 
> What I meant is that nsAnimationManager or nsTransitionManager should keep
> the nsRefreshDriver awake, here in nsRefreshDriver::Tick

keep awake *if the manager has queued events, of course*.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I noticed that observing refresh driver by nsAnimationManager also solves bug 1356533 since we don't need to enumerate documents in each tick,  but as a side effect, we can't dispatch transition events prior to animation events. :/

Comment 15

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> I noticed that observing refresh driver by nsAnimationManager also solves
> bug 1356533 since we don't need to enumerate documents in each tick,  but as
> a side effect, we can't dispatch transition events prior to animation
> events. :/

Yeah, and apart from that I think there are a number of ordering requirements in the HTML spec that we currently don't get right. We should see how much Chrome follows that ordering and find a way to align where we need to.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #15)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> > I noticed that observing refresh driver by nsAnimationManager also solves
> > bug 1356533 since we don't need to enumerate documents in each tick,  but as
> > a side effect, we can't dispatch transition events prior to animation
> > events. :/
> 
> Yeah, and apart from that I think there are a number of ordering
> requirements in the HTML spec that we currently don't get right. We should
> see how much Chrome follows that ordering and find a way to align where we
> need to.

It seems to me that they don't care about the document order[1] nor the event order[2] so far.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/PageAnimator.cpp?type=cs&q=ServiceScriptedAnimations&l=31
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/ScriptedAnimationController.cpp?l=96

As for fixing this bug, I think deferring to remove animation from timeline to the next tick when setting null timeline and unregistering refresh driver in DocumentTimeline dtor is a feasible way for now.

This way works fine on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46c0c71ffb7f72b559b87effa6a1f01fef2ff07b

Also I tried to write a reftest that animationcancel event failed to be fired reliably.  But after the try, I noticed that we can't write reftest  since reftest harness uses setTimeout(0) repeatedly so refresh driver keeps ticking.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

The reason why this failure doesn't appear on stylo is that PostRestyleEventForAnimations call [1] in EffectCompositor::PreTraverseInSubtree() and the call makes nsRefreshDriver::AddStyleFlushObserver() happen.

What happens in the case of stylo is;

1) Set timeline for the animation to null inside the callback of animationstart event (precisely it's inside a Promise callback)
2) Call RequestRestyle(Layer)
3) The animated element is stored in EffectCompositor::mElementsToRestyle
 (Above 1 - 3 processed in DispatchAnimationEvents())
4) Clear nsRefreshDriver::mStyleFlushObservers just before processing restyle
5) Start restyling process
6) Call PostRestyleEventForAnimations in EffectCompositor::PreTraverseInSubtree()
7) PostRestyleEventForAnimations ends up calling nsIPresShell::EnsureStyleFlush() in NoteDirtyElement(), it leads to nsRefreshDriver::AddStyleFlushObserver()
8) So, in the next tick the refresh driver still have a style flush observer, then cancel event happens

So, if we don't drop the PostRestyleEventForAnimations call, this test will never fail.  Actually, IIRC, the call is for processing the final styling for CSS animations/transitions that have been removed in the first traversal (e.g. setting animation-name is none, etc.), so it's not so good for this failure, but anyway, it's not a critical issue.  Downgrading to P3 priority.  

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

In the latest try bug 1193394 comment 81, this test failed on stylo too.  Something might have been changed.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> In the latest try bug 1193394 comment 81, this test failed on stylo too. 
> Something might have been changed.

The change is this <https://hg.mozilla.org/mozilla-central/rev/dae099622091#l1.17>.  We bail out in the failure case (setting null timeline case).  So we can no longer rely on the PostRestyleEventForAnimations call, we have to observe refresh driver,  it's a big deal.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

My basic idea here is to create a new instance that handles CSS animation/transition events (and eventually web animation events as well) per each document (I plan to let nsPresContext have the instance), and when we queue any animation events, nsRefreshDriver adds it into an array just like we do for mStyleFlushObservers and calls EnsureTimerStarted().

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Mostly done. :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d024d36eaf64dcbcdf0039f80ce0267f738c86ec

Need to write more commit messages there.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Don't count all observers for nsRefreshDriver.

We just need information whether there is still an observer or not in most
cases.  The only case we need to know the count is in an assertion in the
dtor of nsRefreshDriver.  In the dtor we are checking there remains no
observers other than early runners.

Note that the order in HasObserver() was adjusted to reflect that we check
boolean flag first (mViewManagerFlushIsPending) and subsequently check observer
which is likey happen to.  mFrameRequestCallbackDocs should have checked prior
to mResizeEventFlushObservers though.

Review commit: https://reviewboard.mozilla.org/r/215502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215502/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements.

It's not need to be a template function since the function is independent from
AnimationType.

Review commit: https://reviewboard.mozilla.org/r/215504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215504/

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents().

The comment is no longer correct since bug 1180125.

Review commit: https://reviewboard.mozilla.org/r/215506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215506/

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher.

We plan to use this class to dispatch web animation API events as well
(bug 1354501), but don't plan to integrate nsRefreshDriver::mPendingEvent
which is for full-screen event, into this in near future.  Though we should
integrate it in the end.

Review commit: https://reviewboard.mozilla.org/r/215508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215508/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Split AnimationEventDipatcher into an independent file.

Review commit: https://reviewboard.mozilla.org/r/215510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215510/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - De-templatize AnimationEventDispatcher.

Now single AnimationEventDispatcher can handle both CSS animation/transition
events simultaneously.  To do this we had to change AnimationEventInfo and
TransitionEventInfo into a single struct, the struct is also named
AnimationEventInfo.  And the new AnimationEventInfo has a union field which
has InternalTransitionEvent and InternalAnimationEvent respectively so that
we can use the new AnimationEventInfo in a single nsTArray<>.

It results we can sort both CSS animation/transition events altogether.  Thus
we make sure CSS animation/transition events are sorted by scheduled event time
prior to their composite order (i.e. transitions is prior to animations).

At this moment, we don't sort both events altogether since nsAnimationManager
and nsTransitionManager has an AnimationEventDispatcher respectively.  In the
next patch we move AnimationEventDispatcher into nsPresContext, i.e. each
document has an AnimationEventDispatcher without the distinction between
CSS animations and transitions.

Review commit: https://reviewboard.mozilla.org/r/215512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215512/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Make AnimationEventDispatcher refcountable.

In a subsequent patch in this patch series, we want to make nsPresContext
have an AnimationEventDispatcher as RefPtr<>.

Instead, if we were trying to make nsPresContext have the
AnimationEventDispatcher as data object (not RefPtr<>) just like we did in
CommonAnimationManager, we will fall into header inclusion hell since Element.h
includes nsPresContext.h and AnimationEventDispatcher.h ends up including
Element.h.  Even if we could solve the inclusion hell, we will suffer from Rust
bindgen issues for some reasons.

Review commit: https://reviewboard.mozilla.org/r/215514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215514/

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

Now we sort CSS animation/transition events by scheduled event time prior
to compositor order.

SortEvents() will be a private method in the next patch in this patch
series.

Review commit: https://reviewboard.mozilla.org/r/215516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215516/

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

Now we sort pending events only when dispatching the events, i.e.
only inside DispatchEvent().

Review commit: https://reviewboard.mozilla.org/r/215518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215518/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

So that we can now ensure nsRefreshDriver ticks (i.e. nsRefreshDriver doesn't
stop its timer) for all queued events.
Before this patch, dispatching CSS animation/transition events relied on the
fact that DocumentTimeline observes nsRefreshDriver.  For this fact,
animationcancel or transitioncancel event did not dispatch properly in some
cases, i.e. the case where the animation was dropped from the DocumentTimeline.

Review commit: https://reviewboard.mozilla.org/r/215520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215520/

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8945308 [details]
Bug 1415780 - Don't count all observers for nsRefreshDriver.

https://reviewboard.mozilla.org/r/215502/#review221252

::: layout/base/nsRefreshDriver.h:399
(Diff revision 1)
>      eNeverAdjustTimer = 1 << 2,
>    };
>    void EnsureTimerStarted(EnsureTimerStartedFlags aFlags = eNone);
>    void StopTimer();
>  
> +  bool HasObserver() const;

I think I'd call this HasObservers()

::: layout/base/nsRefreshDriver.h:401
(Diff revision 1)
>    void EnsureTimerStarted(EnsureTimerStartedFlags aFlags = eNone);
>    void StopTimer();
>  
> +  bool HasObserver() const;
>    uint32_t ObserverCount() const;
> -  uint32_t ImageRequestCount() const;
> +  bool HasImageRequest() const;

And this HasImageRequests()

Comment 33

[Brian Birtles (:birtles)]

Comment on attachment 8945309 [details]
Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements.

https://reviewboard.mozilla.org/r/215504/#review221516

::: layout/style/nsCSSPseudoElements.cpp:162
(Diff revision 1)
>    return PseudoElementHasFlags(aType,
>                                 CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE);
>  }
> +
> +/* static */ nsString
> +nsCSSPseudoElements::PseudoTypeAsString(CSSPseudoElementType aPseudoType)

Should this be s/CSSPseudoElementType/Type/ here for consistency with the rest of this file and the header file?

Comment 34

[Brian Birtles (:birtles)]

Comment on attachment 8945310 [details]
Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents().

https://reviewboard.mozilla.org/r/215506/#review221518

Comment 35

[Brian Birtles (:birtles)]

Comment on attachment 8945311 [details]
Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215508/#review221520

::: commit-message-d878e:5
(Diff revision 1)
> +which is for full-screen event, into this in near future.  Though we should
> +integrate it in the end.

(Nit: I'm not sure what this last sentence means. I suggest we could drop it.)

Comment 36

[Brian Birtles (:birtles)]

Comment on attachment 8945312 [details]
Bug 1415780 - Split AnimationEventDipatcher into an independent file.

https://reviewboard.mozilla.org/r/215510/#review221522

::: dom/animation/AnimationEventDispatcher.h:138
(Diff revision 1)
> +}
> +
> +} // namespace mozilla
> +
> +#endif // mozilla_AnimationEventDispatcher_h
> +

Nit: blank line here

Comment 37

[Brian Birtles (:birtles)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215512/#review221530

::: dom/animation/AnimationEventDispatcher.h:26
(Diff revision 1)
>  
> -template <class EventInfo>
> +struct AnimationEventInfo {
> +  enum class EventType : uint8_t {
> +    CSSTransition,
> +    CSSAnimation,
> +    WebAnimation,

I love that you're fixing the web animations bug at the same time!

However, it might be better to leave this enum value out until we actually start using this class for Web Animations too?

::: dom/animation/AnimationEventDispatcher.h:32
(Diff revision 1)
> +  union {
> +    InternalTransitionEvent mCSSTransitionEvent;
> +    InternalAnimationEvent mCSSAnimationEvent;
> +  };

Oh, interesting. I guess this works because C++11 allows non-POD unions (including types that have virtual functions like these). And furthermore, because we define constructors for the union itself.

The other option would be to use mozilla::Variant, but as far as I can tell, this looks to be ok.

It might be worth getting masayuki or smaug to double-check this patch too.

::: dom/animation/AnimationEventDispatcher.h:51
(Diff revision 1)
> +    , mAnimation(aAnimation)
> +    , mTimeStamp(aTimeStamp)
> +    , mCSSAnimationEvent(true, aMessage)
> +  {
> +    aAnimationName->ToString(mCSSAnimationEvent.mAnimationName);
> +    // XXX Looks like nobody initialize WidgetEvent::time

(We really need to do bug 1026806 :/)

::: dom/animation/AnimationEventDispatcher.h:109
(Diff revision 1)
> +  void FreeUnionValue()
> +  {
> +    switch (mType) {
> +      case EventType::CSSTransition:
> +        mCSSTransitionEvent.~InternalTransitionEvent();
> +        break;
> +      case EventType::CSSAnimation:
> +        mCSSAnimationEvent.~InternalAnimationEvent();
> +        break;
> +      case EventType::WebAnimation:
> +        MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("");
> +        break;
> +    }
> +  }

I think mozilla::Variant would give to use for free but I think it also assumes that its members types are copy-constructible and move-constructible. I wonder if there's anyway to specialize VariantImplementation::copyConstruct and have it call Duplicate / AssignXXXEventData as needed?

Just a thought.

Anyway, this looks good so far but I'll have a proper look after lunch.

Comment 38

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #37)
> ::: dom/animation/AnimationEventDispatcher.h:32
> (Diff revision 1)
> > +  union {
> > +    InternalTransitionEvent mCSSTransitionEvent;
> > +    InternalAnimationEvent mCSSAnimationEvent;
> > +  };
> 
> Oh, interesting. I guess this works because C++11 allows non-POD unions
> (including types that have virtual functions like these). And furthermore,
> because we define constructors for the union itself.

Yeah, "unrestricted unions".

> ::: dom/animation/AnimationEventDispatcher.h:109
> (Diff revision 1)
> > +  void FreeUnionValue()
> > +  {
> > +    switch (mType) {
> > +      case EventType::CSSTransition:
> > +        mCSSTransitionEvent.~InternalTransitionEvent();
> > +        break;
> > +      case EventType::CSSAnimation:
> > +        mCSSAnimationEvent.~InternalAnimationEvent();
> > +        break;
> > +      case EventType::WebAnimation:
> > +        MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("");
> > +        break;
> > +    }
> > +  }
> 
> I think mozilla::Variant would give to use for free but I think it also
> assumes that its members types are copy-constructible and
> move-constructible. I wonder if there's anyway to specialize
> VariantImplementation::copyConstruct and have it call Duplicate /
> AssignXXXEventData as needed?

I didn't know mozilla::Variant, will look. Thanks!

Comment 39

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #38)
> > ::: dom/animation/AnimationEventDispatcher.h:109
> > (Diff revision 1)
> > > +  void FreeUnionValue()
> > > +  {
> > > +    switch (mType) {
> > > +      case EventType::CSSTransition:
> > > +        mCSSTransitionEvent.~InternalTransitionEvent();
> > > +        break;
> > > +      case EventType::CSSAnimation:
> > > +        mCSSAnimationEvent.~InternalAnimationEvent();
> > > +        break;
> > > +      case EventType::WebAnimation:
> > > +        MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("");
> > > +        break;
> > > +    }
> > > +  }
> > 
> > I think mozilla::Variant would give to use for free but I think it also
> > assumes that its members types are copy-constructible and
> > move-constructible. I wonder if there's anyway to specialize
> > VariantImplementation::copyConstruct and have it call Duplicate /
> > AssignXXXEventData as needed?
> 
> I didn't know mozilla::Variant, will look. Thanks!

I did rewrite the code with Variant, it gets much simpler!  I will revise this patch.

Comment 40

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #37)
> I think mozilla::Variant would give to use for free but I think it also...

Wow, my English is so bad recently. I *meant* to say "I think mozilla::Variant would give us this for free..."

Anyway, I think you worked it out!

Comment 41

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #37)
> ::: dom/animation/AnimationEventDispatcher.h:109
> (Diff revision 1)
> > +  void FreeUnionValue()
> > +  {
> > +    switch (mType) {
> > +      case EventType::CSSTransition:
> > +        mCSSTransitionEvent.~InternalTransitionEvent();
> > +        break;
> > +      case EventType::CSSAnimation:
> > +        mCSSAnimationEvent.~InternalAnimationEvent();
> > +        break;
> > +      case EventType::WebAnimation:
> > +        MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("");
> > +        break;
> > +    }
> > +  }
> 
> I think mozilla::Variant would give to use for free but I think it also
> assumes that its members types are copy-constructible and
> move-constructible. I wonder if there's anyway to specialize
> VariantImplementation::copyConstruct and have it call Duplicate /
> AssignXXXEventData as needed?

Hmm, I can't solve this.  Variant requires the copy-contructor for InternalTransitionEvent and InternalAnimationEvent in any way.  We can, of course, use the default copy constructor for them, but it sesms not a bit cost wise (it looks negligible though).  That's said the cost will be eliminated once we make WidgetEvent move friendly (bug 1433008).  So I'd use the default copy-constructor and copy-assignment here.  Brian what do you think?

FWIW, here is a patch for bug 1433008. https://pastebin.mozilla.org/9076696

Comment 42

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> (In reply to Brian Birtles (:birtles) from comment #37)
> > I think mozilla::Variant would give to use for free but I think it also
> > assumes that its members types are copy-constructible and
> > move-constructible. I wonder if there's anyway to specialize
> > VariantImplementation::copyConstruct and have it call Duplicate /
> > AssignXXXEventData as needed?
> 
> Hmm, I can't solve this.  Variant requires the copy-contructor for
> InternalTransitionEvent and InternalAnimationEvent in any way.

Yeah, I thought you might be able to specialize VariantImplementation::copyConstruct somehow.

I guess not. Thanks for trying though.

> We can, of
> course, use the default copy constructor for them, but it sesms not a bit
> cost wise (it looks negligible though).  That's said the cost will be
> eliminated once we make WidgetEvent move friendly (bug 1433008).  So I'd use
> the default copy-constructor and copy-assignment here.  Brian what do you
> think?
> 
> FWIW, here is a patch for bug 1433008. https://pastebin.mozilla.org/9076696

I think Olli of Masayuki would be better able to comment on how we should approach that.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #42)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > (In reply to Brian Birtles (:birtles) from comment #37)
> > > I think mozilla::Variant would give to use for free but I think it also
> > > assumes that its members types are copy-constructible and
> > > move-constructible. I wonder if there's anyway to specialize
> > > VariantImplementation::copyConstruct and have it call Duplicate /
> > > AssignXXXEventData as needed?
> > 
> > Hmm, I can't solve this.  Variant requires the copy-contructor for
> > InternalTransitionEvent and InternalAnimationEvent in any way.
> 
> Yeah, I thought you might be able to specialize
> VariantImplementation::copyConstruct somehow.

That was using the default copy-constructor, copied every members and then AssignXXXEventData. :/

> I think Olli of Masayuki would be better able to comment on how we should
> approach that.

Thanks for the suggestion, I will ping Masayuki first.

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

https://reviewboard.mozilla.org/r/215518/#review221540

Comment 45

[Brian Birtles (:birtles)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215516/#review221524

::: dom/animation/AnimationEventDispatcher.h:176
(Diff revision 1)
>  };
>  
>  class AnimationEventDispatcher
>  {
>  public:
>    AnimationEventDispatcher() : mIsSorted(true) { }

Does this constructor need to initialize mPresContext to nullptr?

::: dom/animation/AnimationEventDispatcher.h:198
(Diff revision 1)
>      mIsSorted = false;
>    }
>  
> -  // This is exposed as a separate method so that when we are dispatching
> -  // *both* transition events and animation events we can sort both lists
> -  // once using the current state of the document before beginning any
> +  // Sort all pending CSS animation/transition events by scheduled event time
> +  // and composite order.
> +  // Section 3.3.5 in https://drafts.csswg.org/web-animations-1/#timelines

Probably better to link to the specific algorithm involved, drop the section number (since it will probably change), and use the unversioned spec link:

https://drafts.csswg.org/web-animations/#update-animations-and-send-events

::: layout/style/nsAnimationManager.cpp:321
(Diff revision 1)
>    Animation::UpdateTiming(aSeekFlag, aSyncNotifyFlag);
>  }
>  
>  ////////////////////////// nsAnimationManager ////////////////////////////
>  
> -NS_IMPL_CYCLE_COLLECTION(nsAnimationManager, mEventDispatcher)
> +NS_IMPL_CYCLE_COLLECTION_0(nsAnimationManager);

Is this needed? Shouldn't we just make nsAnimationManager stop participating in cycle collection and just be a regular ref-counted class?

See:
https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/xpcom/base/nsCycleCollectionParticipant.h#923-925

::: layout/style/nsTransitionManager.cpp:426
(Diff revision 1)
>    }
>  }
>  
>  ////////////////////////// nsTransitionManager ////////////////////////////
>  
> -NS_IMPL_CYCLE_COLLECTION(nsTransitionManager, mEventDispatcher)
> +NS_IMPL_CYCLE_COLLECTION_0(nsTransitionManager);

Likewise here.

Comment 46

[Brian Birtles (:birtles)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

https://reviewboard.mozilla.org/r/215514/#review221538

::: commit-message-0f7ce:6
(Diff revision 1)
> +Instead, if we were trying to make nsPresContext have the
> +AnimationEventDispatcher as data object (not RefPtr<>) just like we did in
> +CommonAnimationManager, we will fall into header inclusion hell since Element.h
> +includes nsPresContext.h and AnimationEventDispatcher.h ends up including
> +Element.h.  Even if we could solve the inclusion hell, we will suffer from Rust
> +bindgen issues for some reasons.

I guess Maybe<> requires a complete type too.

::: dom/animation/AnimationEventDispatcher.h:173
(Diff revision 1)
>      }
>      return nullptr;
>    }
>  };
>  
>  class AnimationEventDispatcher

We should declare this class `final`.

(Because we do decide to subclass it, we'll want to make the dtor virtual and hopefully dropping the `final` will remind us to do that.)

::: dom/animation/AnimationEventDispatcher.cpp:30
(Diff revision 1)
> +
> +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AnimationEventDispatcher, AddRef)
> +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AnimationEventDispatcher, Release)
> +
> +} // namespace mozilla
> +

Nit: blank line here

Comment 47

[Brian Birtles (:birtles)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

https://reviewboard.mozilla.org/r/215520/#review221548

I haven't reviewed this properly yet but just a few comments in passing.

::: dom/animation/AnimationEventDispatcher.h:177
(Diff revision 1)
>  };
>  
>  class AnimationEventDispatcher
>  {
>  public:
>    AnimationEventDispatcher() : mIsSorted(true) { }

Should we initialize mIsObserving here as well?

::: dom/animation/AnimationEventDispatcher.h:198
(Diff revision 1)
>    {
>      mPendingEvents.AppendElements(Move(aEvents));
>      mIsSorted = false;
> +    if (!mIsObserving) {
> +      mIsObserving =
> +        mPresContext->RefreshDriver()->ScheduleAnimatioinEventDispatch(this);

s/Animatioin/Animation/

::: layout/base/nsRefreshDriver.h:255
(Diff revision 1)
> +    bool appended =
> +      mAnimationEventFlushObservers.AppendElement(aDispatcher) != nullptr;

Will this ever be nullptr? Isn't AutoTArray infallible?

::: layout/base/nsRefreshDriver.cpp:2395
(Diff revision 1)
> +void
> +nsRefreshDriver::CancelPendingAnimationEvents(AnimationEventDispatcher* aDispatcher)
> +{
> +  MOZ_ASSERT(aDispatcher);
> +  aDispatcher->ClearEventQueue();
> +  mAnimationEventFlushObservers.RemoveElement(aDispatcher);

Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document?

Comment 48

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #42)

> That was using the default copy-constructor, copied every members and then
> AssignXXXEventData. :/
> 
> > I think Olli of Masayuki would be better able to comment on how we should
> > approach that.
> 
> Thanks for the suggestion, I will ping Masayuki first.

Discussed with Masayuki on IRC, he's fine with either way. So I'd use the default copy-constructor here and drop it in bug  1433008.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945308 [details]
Bug 1415780 - Don't count all observers for nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215502/diff/1-2/

Comment 50

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945309 [details]
Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215504/diff/1-2/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945310 [details]
Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215506/diff/1-2/

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945311 [details]
Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215508/diff/1-2/

Comment 53

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945312 [details]
Bug 1415780 - Split AnimationEventDipatcher into an independent file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215510/diff/1-2/

Comment 54

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215512/diff/1-2/

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215514/diff/1-2/

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215516/diff/1-2/

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215518/diff/1-2/

Comment 58

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215520/diff/1-2/

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215512/#review221554

::: dom/animation/AnimationEventDispatcher.h:23
(Diff revision 2)
>  
>  class nsPresContext;
>  
>  namespace mozilla {
>  
> -template <class EventInfo>
> +struct AnimationEventInfo {

nit: |{| should be in next line.

::: dom/animation/AnimationEventDispatcher.h:86
(Diff revision 2)
> +
> +  WidgetEvent* AsWidgetEvent()
> +  {
> +    if (mEvent.is<InternalTransitionEvent>()) {
> +      return &mEvent.as<InternalTransitionEvent>();
> +    } else if (mEvent.is<InternalAnimationEvent>()) {

nit: This can be just new |if| statement since the previous |if| block always return.

Comment 60

[Brian Birtles (:birtles)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215512/#review221556

Comment 61

[Brian Birtles (:birtles)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215516/#review221558

r=me with the issues addressed

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945312 [details]
Bug 1415780 - Split AnimationEventDipatcher into an independent file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215510/diff/2-3/

Comment 63

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215512/diff/2-3/

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215514/diff/2-3/

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215516/diff/2-3/

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215518/diff/2-3/

Comment 67

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215520/diff/2-3/

Comment 68

[Brian Birtles (:birtles)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

https://reviewboard.mozilla.org/r/215520/#review221562

I'm going to cancel the review request on this patch for now because I'd like to review it with the comments raised in comment 47 addressed.

I suspect if we don't need to null-check the result of AppendElement the logic will change a little, and I'd like to hear your thoughts about the performance considerations.

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215516/#review221524

> Does this constructor need to initialize mPresContext to nullptr?

I did forget to drop this constructor. :/

> Is this needed? Shouldn't we just make nsAnimationManager stop participating in cycle collection and just be a regular ref-counted class?
> 
> See:
> https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/xpcom/base/nsCycleCollectionParticipant.h#923-925

Indeed. Fixed.

Comment 70

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

https://reviewboard.mozilla.org/r/215520/#review221548

> Will this ever be nullptr? Isn't AutoTArray infallible?

I did innocently copy from AddStyleFlushObserver().  Fixed.

> Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document?

Yeah, indeed.  I didn't notice the fact, I copied this part from others as well. Anyway in the case of destroying the root document, that's not a good thing.  We should solve this somehow.  I'd like to defer this issue in a followup bug to fix also other part (i.e. style flush observers, etc.).  Is that fine with you?

Comment 71

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #70)
> > Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document?
> 
> Yeah, indeed.  I didn't notice the fact, I copied this part from others as
> well. Anyway in the case of destroying the root document, that's not a good
> thing.  We should solve this somehow.  I'd like to defer this issue in a
> followup bug to fix also other part (i.e. style flush observers, etc.).  Is
> that fine with you?

If we're already doing that (and it seems we are) then that seems ok.

Comment 72

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #71)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #70)
> > > Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document?
> > 
> > Yeah, indeed.  I didn't notice the fact, I copied this part from others as
> > well. Anyway in the case of destroying the root document, that's not a good
> > thing.  We should solve this somehow.  I'd like to defer this issue in a
> > followup bug to fix also other part (i.e. style flush observers, etc.).  Is
> > that fine with you?
> 
> If we're already doing that (and it seems we are) then that seems ok.

Filed bug 1433362.

Comment 73

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

MozReview did not allow me to add r? flag.

Comment 74

[Brian Birtles (:birtles)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

https://reviewboard.mozilla.org/r/215520/#review221566

Is there any code we no longer need now? I mean, code that only existed to make sure we ticked the refresh driver once more to dispatch cancel events?
(I know there used to be but I think we refactored things in such a way that we no longer needed it.)

I think I want to take one last look at this with the following comments addressed, where applicable.

::: dom/animation/AnimationEventDispatcher.h:120
(Diff revision 3)
>    void QueueEvents(nsTArray<AnimationEventInfo>&& aEvents)
>    {
>      mPendingEvents.AppendElements(Move(aEvents));
>      mIsSorted = false;
> +    if (!mIsObserving) {
> +      mPresContext->RefreshDriver()->ScheduleAnimationEventDispatch(this);

Shouldn't this null-check mPresContext?

And does it make sense to disassociate ourselves from the refresh driver in Disconnect()?

And if we're doing that, presumably we could have a debug-only dtor that asserts that mIsObserving is false? (Although perhaps that's unnecessary.)

::: layout/base/PresShell.cpp:1364
(Diff revision 3)
>    if (mPresContext) {
> -    mPresContext->AnimationEventDispatcher()->ClearEventQueue();
> +    mPresContext->CancelPendingAnimationEvents();
>    }

Would it be a bit less convoluted (and make my comment about the refresh driver below unnecessary) to just do:

if (mPresContext) {
  rd->CancelPendingAnimationEvents(AnimationEventDispatcher());
}

?

Then we wouldn't need to add CancelPendingAnimationEvents to nsPresContext.

::: layout/base/nsPresContext.cpp:3186
(Diff revision 3)
>  }
>  
> +void
> +nsPresContext::CancelPendingAnimationEvents()
> +{
> +  MOZ_ASSERT(mRefreshDriver);

This could probably do with a comment explaining why we're sure we'll have a refresh driver here, e.g.

"This is expected to be called when the corresponding pres shell is destroyed but before the refresh driver is destroyed."

::: layout/base/nsRefreshDriver.h:253
(Diff revision 3)
> +    NS_ASSERTION(!mAnimationEventFlushObservers.Contains(aDispatcher),
> +                 "Double-adding animation event flush observer");

(Any reason to prefer NS_ASSERTION here? Do we do that elsewhere in this file?)

::: layout/base/nsRefreshDriver.cpp:1624
(Diff revision 3)
> -  for (uint32_t i = 0; i < documents.Length(); ++i) {
> -    nsIDocument* doc = documents[i];
> +    RefPtr<AnimationEventDispatcher> kungFuDeathGrip(dispatcher);
> +    dispatcher->DispatchEvents();

What ensures that the next dispatcher in dispatchers is not destroyed while processing the previous dispatcher?

i.e. do we need to take an owning ref to each of these dispatchers *before* iterating over them?

Comment 75

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #74)
> Comment on attachment 8945317 [details]
> Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.
> 
> https://reviewboard.mozilla.org/r/215520/#review221566
> 
> Is there any code we no longer need now? I mean, code that only existed to
> make sure we ticked the refresh driver once more to dispatch cancel events?
> (I know there used to be but I think we refactored things in such a way that
> we no longer needed it.)

Ah right.  We do that in Animation::CancelNoUpdate().  I will add an additional patch to remove that hack.

> ::: dom/animation/AnimationEventDispatcher.h:120
> (Diff revision 3)
> >    void QueueEvents(nsTArray<AnimationEventInfo>&& aEvents)
> >    {
> >      mPendingEvents.AppendElements(Move(aEvents));
> >      mIsSorted = false;
> > +    if (!mIsObserving) {
> > +      mPresContext->RefreshDriver()->ScheduleAnimationEventDispatch(this);
> 
> Shouldn't this null-check mPresContext?

I think we shouldn't.  Instead I think we should add an assertion since QueueEvents() should only be called from DocumentTimeline::WillRefresh(), it means there is no possibility to destroy the pres context.

> And does it make sense to disassociate ourselves from the refresh driver in
> Disconnect()?

Disconnect() seems to be bit late for me.  Disconnect() is called from the dtor of nsPresContext.  But hmm, is there any chance to destroy nsPresContext before we disassociate this event dispatcher in PresShell::Destory()?  I am not sure but I understand your concern. So

> And if we're doing that, presumably we could have a debug-only dtor that
> asserts that mIsObserving is false? (Although perhaps that's unnecessary.)

Yep, I'd add the assertion in the dtor and disassociate the dispatcher in Disconnect() as well just in case.
Thanks for the suggestion!

> ::: layout/base/PresShell.cpp:1364
> (Diff revision 3)
> >    if (mPresContext) {
> > -    mPresContext->AnimationEventDispatcher()->ClearEventQueue();
> > +    mPresContext->CancelPendingAnimationEvents();
> >    }
> 
> Would it be a bit less convoluted (and make my comment about the refresh
> driver below unnecessary) to just do:
> 
> if (mPresContext) {
>   rd->CancelPendingAnimationEvents(AnimationEventDispatcher());
> }
> 
> ?
> 
> Then we wouldn't need to add CancelPendingAnimationEvents to nsPresContext.

Sounds nice. I will do it.

> ::: layout/base/nsRefreshDriver.h:253
> (Diff revision 3)
> > +    NS_ASSERTION(!mAnimationEventFlushObservers.Contains(aDispatcher),
> > +                 "Double-adding animation event flush observer");
> 
> (Any reason to prefer NS_ASSERTION here? Do we do that elsewhere in this
> file?)

Yes, we do it in other places as well.

> ::: layout/base/nsRefreshDriver.cpp:1624
> (Diff revision 3)
> > -  for (uint32_t i = 0; i < documents.Length(); ++i) {
> > -    nsIDocument* doc = documents[i];
> > +    RefPtr<AnimationEventDispatcher> kungFuDeathGrip(dispatcher);
> > +    dispatcher->DispatchEvents();
> 
> What ensures that the next dispatcher in dispatchers is not destroyed while
> processing the previous dispatcher?
> 
> i.e. do we need to take an owning ref to each of these dispatchers *before*
> iterating over them?

Ah right. indeed. This will lead a crash.  Good catch!

Comment 76

[Brian Birtles (:birtles)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

https://reviewboard.mozilla.org/r/215518/#review221572

::: commit-message-15596:1
(Diff revision 3)
> +Bug 1415780 - Make AnimationEventDispatcher::Sort() private. r?birtles

nit: s/Sort/SortEvents/

Comment 77

[Brian Birtles (:birtles)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

https://reviewboard.mozilla.org/r/215516/#review221574

::: commit-message-50321:1
(Diff revision 3)
> +Bug 1415780 - Let nsPresContext have AnimationEventDipatcher. r?birtles

s/AnimationEventDipatcher/AnimationEventDispatcher/

Comment 78

[Brian Birtles (:birtles)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

https://reviewboard.mozilla.org/r/215514/#review221576

::: commit-message-0f7ce:1
(Diff revision 3)
> +Bug 1415780  - Make AnimationEventDipatcher refcountable. r?birtles

Here too: AnimationEventDispatcher (missing 's')

Comment 79

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate.

Now AnimationEventDispatcher ensures that the refresh driver's next tick
for cancel event, so we don't need to re-observe the timeline (it happens
in UpdateTiming) once after removing the animation from the timeline.

Review commit: https://reviewboard.mozilla.org/r/215830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215830/

Comment 80

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945308 [details]
Bug 1415780 - Don't count all observers for nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215502/diff/2-3/

Comment 81

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945309 [details]
Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215504/diff/2-3/

Comment 82

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945310 [details]
Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215506/diff/2-3/

Comment 83

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945311 [details]
Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215508/diff/2-3/

Comment 84

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945312 [details]
Bug 1415780 - Split AnimationEventDipatcher into an independent file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215510/diff/3-4/

Comment 85

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215512/diff/3-4/

Comment 86

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215514/diff/3-4/

Comment 87

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215516/diff/3-4/

Comment 88

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215518/diff/3-4/

Comment 89

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215520/diff/3-4/

Comment 90

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945712 [details]
Bug 1415780 - Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215830/diff/1-2/

Comment 91

[Brian Birtles (:birtles)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

https://reviewboard.mozilla.org/r/215520/#review221590

::: dom/animation/AnimationEventDispatcher.cpp:46
(Diff revision 4)
> +}
> +
> +void
> +AnimationEventDispatcher::QueueEvents(nsTArray<AnimationEventInfo>&& aEvents)
> +{
> +  MOZ_ASSERT(mPresContext, "");

Any reason for the blank comment?

::: layout/base/nsPresContext.h:244
(Diff revision 4)
>  
>    mozilla::AnimationEventDispatcher* AnimationEventDispatcher()
>    {
>      return mAnimationEventDispatcher;
>    }
> +

(ReviewBoard doesn't tell me what changed here but it doesn't look intentional)

::: layout/base/nsRefreshDriver.h:517
(Diff revision 4)
> +  AutoTArray<RefPtr<mozilla::AnimationEventDispatcher>, 16>
> +    mAnimationEventFlushObservers;

Do we really want this to take an owning reference?

::: layout/base/nsRefreshDriver.cpp:1621
(Diff revision 4)
> -  AutoTArray<nsCOMPtr<nsIDocument>, 32> documents;
> -  CollectDocuments(mPresContext->Document(), &documents);
> +  AutoTArray<RefPtr<AnimationEventDispatcher>, 16> dispatchers;
> +  mAnimationEventFlushObservers.SwapElements(dispatchers);

Can we just take an owning reference here instead?

Comment 92

[Brian Birtles (:birtles)]

Comment on attachment 8945712 [details]
Bug 1415780 - Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate.

https://reviewboard.mozilla.org/r/215830/#review221598

Comment 93

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #91)

> ::: layout/base/nsRefreshDriver.cpp:1621
> (Diff revision 4)
> > -  AutoTArray<nsCOMPtr<nsIDocument>, 32> documents;
> > -  CollectDocuments(mPresContext->Document(), &documents);
> > +  AutoTArray<RefPtr<AnimationEventDispatcher>, 16> dispatchers;
> > +  mAnimationEventFlushObservers.SwapElements(dispatchers);
> 
> Can we just take an owning reference here instead?

We can. But do you mean doing AppendElement each AnimationEventDispatcher* in mAnimationEventFlushObservers to AutoTArray<RefPtr<AnimationEventDispatcher>> and then clearing mAnimationEventFlushObservers?  I am not sure it's better than SwapElements().

Comment 94

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #93)
> We can. But do you mean doing AppendElement each AnimationEventDispatcher*
> in mAnimationEventFlushObservers to
> AutoTArray<RefPtr<AnimationEventDispatcher>> and then clearing
> mAnimationEventFlushObservers?  I am not sure it's better than
> SwapElements().

Right. Otherwise it seems like nsRefreshDriver should participate in cycle collection since the AnimationEffectDispatchers are included in cycle collection via the nsPresContext.

Comment 95

[Brian Birtles (:birtles)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

https://reviewboard.mozilla.org/r/215520/#review221824

I'm going to be traveling from today, so r=me with the issues around the AnimationEventDispatcher lifetimes resolved.

I think it's fine to keep non-owning references in the nsRefreshDriver since we have a fairly straightforward out-of-band setup for disassociating the dispatchers before either side dies.

However, we should probably take an owning reference to each of them before iterating over the copy of list. That probably means we can't use swap which is a shame but the algorithm is already O(n) so iterating over the list once more is not an increase in complexity.

Comment 96

[Hiroyuki Ikezoe (:hiro)]

Thanks!

I've already revised the part of dispatching animation events, and pushed a try.  I planed to push a new review request once the try finished. 

https://hg.mozilla.org/try/rev/bd840383d46ce77adf206b60a13ccaeafd72e09e#l5.48
https://treeherder.mozilla.org/#/jobs?repo=try&revision=188dceb99d22bc2ce30b5ff6329c8d258f5e0283

I am hoping this works for you.  I have to run from now on as well, don't have time to push a new patch set unfortunately.

Comment 97

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #96)
> Thanks!
> 
> I've already revised the part of dispatching animation events, and pushed a
> try.  I planed to push a new review request once the try finished. 
> 
> https://hg.mozilla.org/try/rev/bd840383d46ce77adf206b60a13ccaeafd72e09e#l5.48
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=188dceb99d22bc2ce30b5ff6329c8d258f5e0283
> 
> I am hoping this works for you.  I have to run from now on as well, don't
> have time to push a new patch set unfortunately.

That looks great. Thanks!

Comment 98

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945308 [details]
Bug 1415780 - Don't count all observers for nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215502/diff/3-4/

Comment 99

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945309 [details]
Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215504/diff/3-4/

Comment 100

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945310 [details]
Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215506/diff/3-4/

Comment 101

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945311 [details]
Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215508/diff/3-4/

Comment 102

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945312 [details]
Bug 1415780 - Split AnimationEventDipatcher into an independent file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215510/diff/4-5/

Comment 103

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215512/diff/4-5/

Comment 104

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215514/diff/4-5/

Comment 105

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215516/diff/4-5/

Comment 106

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215518/diff/4-5/

Comment 107

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215520/diff/4-5/

Comment 108

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945712 [details]
Bug 1415780 - Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215830/diff/2-3/

Comment 109

[Hiroyuki Ikezoe (:hiro)]

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12c14145ecfc5e7f8ed611f4f9a21dc47974367e

Comment 110

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/433a6f0d0ac3
Don't count all observers for nsRefreshDriver. r=smaug
https://hg.mozilla.org/integration/autoland/rev/027a7ed3c948
Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e298f242ce7c
Drop the comment for nsAnimationManager::DispatchEvents(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/621b9aaf4a8f
Rename DelayedEventsDispatcher to AnimationEventDispatcher. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ae7be65f3c88
Split AnimationEventDipatcher into an independent file. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c6567e4196f6
De-templatize AnimationEventDispatcher. r=birtles,masayuki
https://hg.mozilla.org/integration/autoland/rev/f0cc29e7ccd7
Make AnimationEventDispatcher refcountable. r=birtles
https://hg.mozilla.org/integration/autoland/rev/79e980195ee8
Let nsPresContext have AnimationEventDispatcher. r=birtles
https://hg.mozilla.org/integration/autoland/rev/43d42ca7308f
Make AnimationEventDispatcher::SortEvents() private. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a88250ad7a3e
Let AnimationEventDispatcher observe nsRefreshDriver. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6bad89a17566
Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate. r=birtles

Comment 111

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fa2f138319e
Drop CollectDocuments in nsRefreshDriver.cpp. r=me CLOSED TREE

Comment 112

[Cosmin Sabou [:CosminS]]

Backed out for build bustages on nsRefreshDriver.cpp:1606:1 and AnimationCommon.h:168:51

Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6bad89a1756671ae8583c1c0e44b2254a13e45d1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158838487&repo=autoland&lineNumber=18301

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158838473&repo=autoland&lineNumber=19495

Backout link: https://hg.mozilla.org/integration/autoland/rev/71ac93eb3d4cdcdcbca8188dd827a36991ff54e6

Comment 113

[Hiroyuki Ikezoe (:hiro)]

I think on mingw winbase.h is included after dom/animation/Animaion.h.  I have no idea how to avoid this but it's a fundamental issue.

Comment 114

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #113)
> I think on mingw winbase.h is included after dom/animation/Animaion.h.  I
> have no idea how to avoid this but it's a fundamental issue.

To be clear, it's a fundamental issue regardless of these patches.

Comment 115

[Hiroyuki Ikezoe (:hiro)]

To eliminate these impacts by the macros defined in windows.h, we need to fix bug 834505, I think we should introduce WindowsUndefineMacros.h or some such just like we do in X11UndefineNone.h.

Anyway, for this bug I am trying add '#undef GetCurrentTime' in some places.

Comment 116

[Hiroyuki Ikezoe (:hiro)]

There were two places that include window.h and cause this build failure;

 layout/generic/nsPluginFrame.h
 ipc/chromium/src/base/process_util.h

In the former case we should add '#undef GetCurrentTime' in the undefining macro list [1].
But the latter case, I am not sure we should add the macro there.  So I decided to add the undef macro in the template function , GetAnimationPhaseWithoutEffect where the build error happened.

[1] https://hg.mozilla.org/mozilla-central/file/232dbd5acc3f/layout/generic/nsPluginFrame.h#l26

Comment 117

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Make AnimationEventDispatcher::SortEvents() private.

Now we sort pending events only when dispatching the events, i.e.
only inside DispatchEvent().

Review commit: https://reviewboard.mozilla.org/r/216076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216076/

Comment 118

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415780 - Drop CollectDocuments in nsRefreshDriver.cpp.

Review commit: https://reviewboard.mozilla.org/r/216078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216078/

Comment 119

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215518/diff/5-6/

Comment 120

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945308 [details]
Bug 1415780 - Don't count all observers for nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215502/diff/4-5/

Comment 121

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945309 [details]
Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215504/diff/4-5/

Comment 122

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945310 [details]
Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215506/diff/4-5/

Comment 123

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945311 [details]
Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215508/diff/4-5/

Comment 124

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945312 [details]
Bug 1415780 - Split AnimationEventDipatcher into an independent file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215510/diff/5-6/

Comment 125

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945313 [details]
Bug 1415780 - De-templatize AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215512/diff/5-6/

Comment 126

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945314 [details]
Bug 1415780 - Make AnimationEventDispatcher refcountable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215514/diff/5-6/

Comment 127

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945315 [details]
Bug 1415780 - Let nsPresContext have AnimationEventDispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215516/diff/5-6/

Comment 128

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945317 [details]
Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215520/diff/5-6/

Comment 129

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945712 [details]
Bug 1415780 - Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215830/diff/3-4/

Comment 130

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8946009 [details]
Bug 1415780 - Drop CollectDocuments in nsRefreshDriver.cpp.

https://reviewboard.mozilla.org/r/216078/#review221862

Comment 131

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

MozReview got confused.

Anyway this should work.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c245e53719c51e32beeb703bb024f597ccd22e85

Comment 132

[Tom Ritter [:tjr] (OOTO until 9/2)]

I have seen the GetCurrentTime issue before, transiently, and then it went away. My best guess is that it's a Unified build issue.

I don't care too much one way or the other about how it's fixed; but maybe the most permanent solution is renaming the method?

Comment 133

[Hiroyuki Ikezoe (:hiro)]

(In reply to Tom Ritter [:tjr] from comment #132)
> I have seen the GetCurrentTime issue before, transiently, and then it went
> away. My best guess is that it's a Unified build issue.

Yeah, I think so.

> I don't care too much one way or the other about how it's fixed; but maybe
> the most permanent solution is renaming the method?

Renaming sounds a good way, but in the animation case it will be awkward.  The Animation object has a Web API 'attribute double? currentTime', we implement it in C++ as 'GetCurrentTimeAsDouble'.  The Animation class also has 'Nullable<TimeDuration> GetCurrentTime()' for internal use in C++.  Unfortunately X11 defines CurrentTime as well [1], so we have to name the function GetCurrentTimeAsTimeDuration or something.  It sounds bit long...  Oh, I think GetCurrentTimeDuration might be better.  

Thanks for the suggestion!  The renaming will work for me. :)

[1] https://hg.mozilla.org/mozilla-central/file/9c0bcf801521/dom/animation/Animation.h#l25

Comment 134

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #133)
> Renaming sounds a good way, but in the animation case it will be awkward. 
> The Animation object has a Web API 'attribute double? currentTime', we
> implement it in C++ as 'GetCurrentTimeAsDouble'.  The Animation class also
> has 'Nullable<TimeDuration> GetCurrentTime()' for internal use in C++. 
> Unfortunately X11 defines CurrentTime as well [1], so we have to name the
> function GetCurrentTimeAsTimeDuration or something.  It sounds bit long... 
> Oh, I think GetCurrentTimeDuration might be better.  

Filed bug 1433705.

Comment 135

[Brian Birtles (:birtles)]

Comment on attachment 8945316 [details]
Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime.

https://reviewboard.mozilla.org/r/215518/#review221884

Feels a little odd to do this inside the function definition itself. It doesn't work if pushed above the function definition?

Comment 136

[Brian Birtles (:birtles)]

Comment on attachment 8946008 [details]
Bug 1415780 - Make AnimationEventDispatcher::SortEvents() private.

https://reviewboard.mozilla.org/r/216076/#review221886

Comment 137

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #135)
> Comment on attachment 8945316 [details]
> Bug 1415780 - Undef GetCurrentTime in the template function that uses
> Animation::GetCurrentTime.
> 
> https://reviewboard.mozilla.org/r/215518/#review221884
> 
> Feels a little odd to do this inside the function definition itself. It
> doesn't work if pushed above the function definition?

I haven't try it but if it worked, the build error on mingw didn't happen I guess.

Comment 138

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd9bab66f1f4
Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime. r=birtles
https://hg.mozilla.org/integration/autoland/rev/59606dd5c52a
Don't count all observers for nsRefreshDriver. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2c769e6d110b
Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements. r=birtles
https://hg.mozilla.org/integration/autoland/rev/346c95ffa745
Drop the comment for nsAnimationManager::DispatchEvents(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/5a4068a1cc40
Rename DelayedEventsDispatcher to AnimationEventDispatcher. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ca2c833ff617
Split AnimationEventDipatcher into an independent file. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3e89823dbf46
De-templatize AnimationEventDispatcher. r=birtles,masayuki
https://hg.mozilla.org/integration/autoland/rev/71e7e9e19c25
Make AnimationEventDispatcher refcountable. r=birtles
https://hg.mozilla.org/integration/autoland/rev/47160fce2944
Let nsPresContext have AnimationEventDispatcher. r=birtles
https://hg.mozilla.org/integration/autoland/rev/cebc1a9e1770
Make AnimationEventDispatcher::SortEvents() private. r=birtles
https://hg.mozilla.org/integration/autoland/rev/fbb98433e050
Let AnimationEventDispatcher observe nsRefreshDriver. r=birtles
https://hg.mozilla.org/integration/autoland/rev/600af8b23ec1
Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7e8c97401cb2
Drop CollectDocuments in nsRefreshDriver.cpp. r=hiro

Comment 139

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cd9bab66f1f4
https://hg.mozilla.org/mozilla-central/rev/59606dd5c52a
https://hg.mozilla.org/mozilla-central/rev/2c769e6d110b
https://hg.mozilla.org/mozilla-central/rev/346c95ffa745
https://hg.mozilla.org/mozilla-central/rev/5a4068a1cc40
https://hg.mozilla.org/mozilla-central/rev/ca2c833ff617
https://hg.mozilla.org/mozilla-central/rev/3e89823dbf46
https://hg.mozilla.org/mozilla-central/rev/71e7e9e19c25
https://hg.mozilla.org/mozilla-central/rev/47160fce2944
https://hg.mozilla.org/mozilla-central/rev/cebc1a9e1770
https://hg.mozilla.org/mozilla-central/rev/fbb98433e050
https://hg.mozilla.org/mozilla-central/rev/600af8b23ec1
https://hg.mozilla.org/mozilla-central/rev/7e8c97401cb2