Closed
Bug 1430924
Opened 6 years ago
Closed 6 years ago
Minor cleanups for CSS animations/transitions event handling
Categories
(Core :: DOM: Animation, enhancement)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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 7•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Comment 9•6 years ago
|
||
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.)
Comment 10•6 years ago
|
||
(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 11•6 years ago
|
||
mozreview-review |
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+
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/59fe694b71b5 https://hg.mozilla.org/mozilla-central/rev/43280c5e33c5 https://hg.mozilla.org/mozilla-central/rev/55a05cde7eb7 https://hg.mozilla.org/mozilla-central/rev/0d52b29a58e7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•