1015803 - Animation.numIterations should use infinity to represent infinity

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Align Layers' Animation.numIterations with AnimationTiming.mIterationCount

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'.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

(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.)

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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).

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1f0e87bbad

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff1f0e87bbad