Closed
Bug 1340916
Opened 7 years ago
Closed 7 years ago
Clean up AnimationCollection, nsAnimationManager
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
Details
Attachments
(3 files)
I've decided to release some problem that I've found while I've been working on bug 1340322 (but not related to stylo). https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5375d7922661fdf3b77e2be0be72f083e136f54
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8838971 [details] Bug 1340916 - Part 1: Drop AnimationCollection::PseudoElementType(). https://reviewboard.mozilla.org/r/113726/#review115290 Nice. (I was going to do this in bug 1239945 but I should probably just give up on finishing that bug.)
Attachment #8838971 -
Flags: review?(bbirtles) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8838972 [details] Bug 1340916 - Part 2: Move keyframs array for old animations. https://reviewboard.mozilla.org/r/113728/#review115292
Attachment #8838972 -
Flags: review?(bbirtles) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8838973 [details] Bug 1340916 - Part 3: Add a helper function to create TimingParams CSS animation/transition properties. https://reviewboard.mozilla.org/r/113730/#review115294 This feels a bit odd to do this in TimingParams.h but maybe I just don't understand the comment, "This constructor will be also used in FFI for stylo." Dealing with floats is a CSS-specific thing, right? Can we either: a) Make this a helper function in layout/style/AnimationCommon.h, or b) Make this a named constructor TimingParams::FromCSSParams? I think I prefer (a). What do you think?
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #6) > Dealing with floats is a CSS-specific thing, right? Can we either: > > a) Make this a helper function in layout/style/AnimationCommon.h, or > b) Make this a named constructor TimingParams::FromCSSParams? > > I think I prefer (a). What do you think? Ah, indeed. I will add a helper function named TimingParamsFromCSSParams in AnimationCommon.h. Thanks!
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8838973 [details] Bug 1340916 - Part 3: Add a helper function to create TimingParams CSS animation/transition properties. https://reviewboard.mozilla.org/r/113730/#review115302 ::: layout/style/nsAnimationManager.cpp:589 (Diff revision 2) > - TimingParams timing; > - > - timing.mDuration.emplace(StickyTimeDuration::FromMilliseconds( > - aStyleAnimation.GetDuration())); > - timing.mDelay = TimeDuration::FromMilliseconds(aStyleAnimation.GetDelay()); > - timing.mIterations = aStyleAnimation.GetIterationCount(); > + TimingParams timing = > + TimingParamsFromCSSParams(aStyleAnimation.GetDuration(), > + aStyleAnimation.GetDelay(), > + aStyleAnimation.GetIterationCount(), > + aStyleAnimation.GetDirection(), > + aStyleAnimation.GetFillMode()); > - MOZ_ASSERT(timing.mIterations >= 0.0 && !IsNaN(timing.mIterations), > - "mIterations should be nonnegative & finite, as ensured by " > - "CSSParser"); > - timing.mDirection = aStyleAnimation.GetDirection(); > - timing.mFill = aStyleAnimation.GetFillMode(); > > return timing; Just make this: return TimingParamsFromCSSParams(aStyleAnimation.GetDuration(), aStyleAnimation.GetDelay(), aStyleAnimation.GetIterationCount(), aStyleAnimation.GetDirection(), aStyleAnimation.GetFillMode()); ?
Attachment #8838973 -
Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab9b5d45020ffcc58b37dbb38d9b7fa48ff0d8a2
Comment 12•7 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3556f62bcba3 Part 1: Drop AnimationCollection::PseudoElementType(). r=birtles https://hg.mozilla.org/integration/autoland/rev/b68b600c854b Part 2: Move keyframs array for old animations. r=birtles https://hg.mozilla.org/integration/autoland/rev/94a6e5edb075 Part 3: Add a helper function to create TimingParams CSS animation/transition properties. r=birtles
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3556f62bcba3 https://hg.mozilla.org/mozilla-central/rev/b68b600c854b https://hg.mozilla.org/mozilla-central/rev/94a6e5edb075
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•