Closed Bug 1430924 Opened 3 years ago Closed 3 years ago

Minor cleanups for CSS animations/transitions event handling

Categories

(Core :: DOM: Animation, enhancement)

55 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(4 files)

While I was thinking how to solve bug 1415780, I noticed we can do the event handling bit smarter.  Main two points what I am thinking are

1) we don't need to call nsTransitionManager::QueueEvent() or nsAnimationmanager::QueueEvent() for each event, we can make the call for several events (i.e. QueueEvents()) instead
2) we don't need to create AnimationEventParams or TransitionEventParams, we can create AnimationEventInfo or TransitionEventInfo directly

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9231a706945c525d6bd22fa40fb95c391504d7d9
Comment on attachment 8943061 [details]
Bug 1430924 - Move mEventDispatcher in nsAnimationManager and nsTransitionManager into the common template class.

https://reviewboard.mozilla.org/r/213334/#review219178

Moving the common part makes sense. Nice.
Attachment #8943061 - Flags: review?(boris.chiou) → review+
Comment on attachment 8943062 [details]
Bug 1430924 - AnimationEventInfo and TransitionEventInfo take NonOwningAnimationTarget instead of Element and CSSPseudoElementType pair.

https://reviewboard.mozilla.org/r/213336/#review219180
Attachment #8943062 - Flags: review?(boris.chiou) → review+
Comment on attachment 8943063 [details]
Bug 1430924 - Queue a bunch of animation events at once.

https://reviewboard.mozilla.org/r/213338/#review219184

::: layout/style/nsAnimationManager.cpp:313
(Diff revision 1)
>  
> +  if (events.IsEmpty()) {
> +    return;
> +  }
> +
> +  AutoTArray<AnimationEventInfo, 2> animationEvents;

So in most cases, we have only one or two events, right?
Attachment #8943063 - Flags: review?(boris.chiou) → review+
(In reply to Boris Chiou [:boris] from comment #7)
> Comment on attachment 8943063 [details]
> Bug 1430924 - Queue a bunch of animation events at once.
> 
> https://reviewboard.mozilla.org/r/213338/#review219184
> 
> ::: layout/style/nsAnimationManager.cpp:313
> (Diff revision 1)
> >  
> > +  if (events.IsEmpty()) {
> > +    return;
> > +  }
> > +
> > +  AutoTArray<AnimationEventInfo, 2> animationEvents;
> 
> So in most cases, we have only one or two events, right?

As far as I can tell, in most cases it's zero.  I don't quite understand the reason why the number was chosen originally, my best guess is that it's the maximum number of events other than cancel event, but.. I don't know.
Yeah, two is the maximum we ever dispatch at once.

See: https://drafts.csswg.org/css-animations-2/#event-dispatch

(I don't quite follow what the benefit is of the third patch--queueing multiple events at once--but it's up to you.)
(In reply to Brian Birtles (:birtles) from comment #9)
> Yeah, two is the maximum we ever dispatch at once.
> 
> See: https://drafts.csswg.org/css-animations-2/#event-dispatch
> 
> (I don't quite follow what the benefit is of the third patch--queueing
> multiple events at once--but it's up to you.)

Oh! Yes. After checking the 4th patch, I think one of the benefit may be: we don't have to manually write a for-loop the push all events into the event queue, even though the number of maximum events is only 2 for animations, and only 3 for transitions.
Comment on attachment 8943064 [details]
Bug 1430924 - Create animation event struct directly.

https://reviewboard.mozilla.org/r/213340/#review219200

Looks good to me because we don't have to construct one more temporary type.
Attachment #8943064 - Flags: review?(boris.chiou) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59fe694b71b5
Move mEventDispatcher in nsAnimationManager and nsTransitionManager into the common template class. r=boris
https://hg.mozilla.org/integration/autoland/rev/43280c5e33c5
AnimationEventInfo and TransitionEventInfo take NonOwningAnimationTarget instead of Element and CSSPseudoElementType pair. r=boris
https://hg.mozilla.org/integration/autoland/rev/55a05cde7eb7
Queue a bunch of animation events at once. r=boris
https://hg.mozilla.org/integration/autoland/rev/0d52b29a58e7
Create animation event struct directly. r=boris
You need to log in before you can comment on or make changes to this bug.