Closed Bug 1015803 Opened 7 years ago Closed 7 years ago

Animation.numIterations should use infinity to represent infinity

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(1 file)

As mentioned in Bug 1004871 comment 43, in layers code we use numIterations = -1 to represent infinity whereas elsewhere we use infinity (i.e. NS_IEEEPositiveInfinity()). We should make this consistent.

While we're at it we should probably rename numIterations to iterationCount.

See:

  http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh#182

And:

  http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#332
In the IPC Animation struct used in layers code we have a member called
'numIterations' where 'iterate forever' is represented by -1.

In layout/style however we have an AnimationTiming struct with an
mIterationCount member where 'iterate forever' is represented by
NS_IEEEPositiveInfinity().

This patch renames 'numIterations' to 'iterationCount' and uses infinity to
represent 'iterate forever'.
Attachment #8429019 - Flags: review?(dholbert)
Sorry about all the review requests but this one should be easy.

I didn't change the type to double (despite the suggestion in Bug 1004871 comment 44) since AnimationTiming::mIterationCount is actually not a double but a float. I think a float is sufficient for representing the number of iterations desired.

(One place where we might have range considerations is representing the current iteration however since that may be reporting on an animation that repeats indefinitely. For an animation that repeats every millisecond, a 32-bit integer would be enough for 50 days. That's probably enough since I can't think of any practical uses of an animation that repeats every millisecond, but we're using 64-bit integers there anyway.)

I wondered if the use of -1 to represent "forever" in layers code was because our IPC code can't serialize the value "infinity" but I made up a test case and ran through it with a debugger and didn't see any issues there.
Comment on attachment 8429019 [details] [diff] [review]
Align Layers' Animation.numIterations with AnimationTiming.mIterationCount

>+++ b/gfx/layers/ipc/LayersMessages.ipdlh
>-  // How many times to repeat the animation.  -1 means "forever".
>-  float numIterations;
>+  // Number of times to repeat the animation, including positive infinity.
>+  float iterationCount;

While you're here, it's probably worth also documenting:
  ...what 0 represents (don't play at all? or play but don't repeat?)
  ...that negative values are invalid (if that's the case) (we probably should have some assertions to that effect, if we don't already. That doesn't have to happen here though.)

r=me with that.
Attachment #8429019 - Flags: review?(dholbert) → review+
(In reply to Brian Birtles (:birtles) from comment #2)
> I didn't change the type to double (despite the suggestion in Bug 1004871
> comment 44) since AnimationTiming::mIterationCount is actually not a double
> but a float.

(Sorry, not sure why I thought it was a double there. I must've been looking at another variable.)
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8429019 [details] [diff] [review]
> Align Layers' Animation.numIterations with AnimationTiming.mIterationCount
> 
> >+++ b/gfx/layers/ipc/LayersMessages.ipdlh
> >-  // How many times to repeat the animation.  -1 means "forever".
> >-  float numIterations;
> >+  // Number of times to repeat the animation, including positive infinity.
> >+  float iterationCount;
> 
> While you're here, it's probably worth also documenting:
>   ...what 0 represents (don't play at all? or play but don't repeat?)
>   ...that negative values are invalid (if that's the case) (we probably
> should have some assertions to that effect, if we don't already. That
> doesn't have to happen here though.)
> 
> r=me with that.

I added the following:

  // Values <= 0 mean the animation will not play (although events are still
  // dispatched on the main thread).
https://hg.mozilla.org/mozilla-central/rev/ff1f0e87bbad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.