Closed Bug 1340916 Opened 7 years ago Closed 7 years ago

Clean up AnimationCollection, nsAnimationManager

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

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: nobody → hikezoe
Status: NEW → ASSIGNED
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 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 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?
(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 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+
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
You need to log in before you can comment on or make changes to this bug.