Closed Bug 1004871 Opened 10 years ago Closed 10 years ago

Factor out a common method for calculating the time portion of an animation

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(11 files, 41 obsolete files)

part 1 - Factor event queuing out of EnsureStyleRuleFor;
18.43 KB, patch
Details | Diff | Splinter Review
part 2 - Add AnimationTiming struct, to encapsulate animation timing parameters;
15.68 KB, patch
Details | Diff | Splinter Review
part 3 - Replace parameters to GetPositionInIteration with an AnimationTiming object;
12.04 KB, patch
Details | Diff | Splinter Review
part 4 - Move FillsForwards/FillsBackwards to AnimationTiming;
3.59 KB, patch
Details | Diff | Splinter Review
part 5 - Add ComputedTiming data structure;
2.53 KB, patch
Details | Diff | Splinter Review
part 8 - Simplify ElementAnimations::GetEventsAt;
4.15 KB, patch
Details | Diff | Splinter Review
part 10 - Stop passing a fill mode of "both" to GetComputedTimingAt for throttled samples;
1.76 KB, patch
Details | Diff | Splinter Review
part 7 - Remove event queueing from GetPositionInIteration and do it in GetEventsAt;
9.07 KB, patch
Details | Diff | Splinter Review
part 11 - Move ElementAnimation members closer together for readability and packing
3.56 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 9 - Move ElementAnimations::GetPositionInIteration to ElementAnimation::GetComputedTimingAt;
15.18 KB, patch
Details | Diff | Splinter Review
part 6 - Make GetPositionInIteration return a ComputedTiming object;
17.66 KB, patch
Details | Diff | Splinter Review 
Currently ElementAnimations::GetPositionInIteration does two jobs:

a) Gets the time portion for the given time and timing parameters
b) Queues up events and changes flags on passed on animations accordingly

In order to share more code between CSS animations and transitions and have something resembling a generic animation architecture we should separate these two behaviours and create a generic "GetTimePortionAt" method (or actually "GetComputingTimingAt" since we'll end up wanting to return other properties like the current iteration etc.)

Separating out event dispatch also allows us to better handle situations where we want to skip applying animation effects but still dispatch events (e.g. when we have an empty keyframe rule, see bug 1004377).

This bug tracks the steps necessary to make that happen. It corresponds to most of "Phase 2" described in bug 999922.
This patch moves event queuing out of EnsureStyleRuleFor into a separate method.
This is a preparatory step towards making GetPositionInIteration into a more
generic method for calculating the current time fraction.

In order to achieve this, GetPositionInIteration needs to be able to calculate
the correct time portion for times outside the range [0, 1] even when it is not
passed a StyleAnimation object. Specifically, it needs the fill mode of the
animation to be passed in.

(Rather than using FillForwards/FillBackwards this patch just compares the
NS_STYLE_ANIMATION_FILL_MODE_* values directly but FillForwards/FillBackwards
are restored in a subsequent patch when they are added to the struct used to
lump the timing parameters together.)

There are a number of places where positionInIteration is used to determine if
the current sample occurs in the active phase or after. This is sub-optimal but
is fixed in a subsequent patch in this series.

The actual work of removing event queueing from GetPositionInIteration is
deferred to a subsequent patch in order to keep the changes as small as
possible. This patch simply makes separate calls to GetPositionInIteration for
interpolating and for event queueing.
Attached patch part 2 - Add AnimationTiming (obsolete) — — Splinter Review 
Introduces a struct to store timing parameters for passing to
GetPositionInIteration. In future this struct is expected to be expanded to
include other timing parameters as well (based roughly on Web Animations'
"Timing" interface, hence the name AnimationTiming).
This patch makes use of the AnimationTiming struct introduced in the previous
patch to simplify calls to ElementAnimations::GetPositionInIteration.
This patch moves the FillsForwards/FillsBackwards methods previously defined on
ElementAnimations to the structure contain the fill mode: AnimationTiming. It
also changes GetPositionInIteration to use these methods.
This patch adds a ComputedTiming struct for storing the results of calculating
the timing properties of an animation at a given sample time.
Attachment #8416305 - Attachment is obsolete: true
This patch makes ElementAnimations::GetPositionInIteration return
a ComputedTiming object instead of just a time portion (time fraction).

Since the ComputedTiming object includes phase information, we can fix those
parts of EnsureStyleRule and GetEventsAt that were temporarily using the time
portion to guess if the animation might have finished or not.
This patch simply shifts the event-related code from GetPositionInIteration to
GetEventsAt. Although there are simplifications that could be done to
GetEventsAt, they are deferred to a subsequent patch so as not to obscure the
translation of code from one function to another.

As a result of moving event-related handling from GetPositionInIteration it no
longer needs to support different main-thread vs off-thread modes.
This patch shuffles the code in ElementAnimations::GetEventsAt to make it easier
to follow.

It also removes a check for whether or not the animation is paused.
Previously we would not dispatch events if the animation was paused and in its
active phase (but we would if the animation had finished). There doesn't seem to
be any reason for this. If the animation was paused between the last sample and
the current sample and the boundary of an iteration also occurred in that time
then I expect we should dispatch that event. Removing this check for the pause
state does not cause any tests fail.

Separating out the event logic here makes it clear that we do not dispatch start
events in the situation where one sample falls before the active interval and
one sample falls after it (filed as bug 1004361). This patch adds a comment to
this effect.
Attachment #8416315 - Attachment is obsolete: true
This patch simply moves the code from ElementAnimations to StyleAnimation so
that it can later be used in transitions code and so we can later move
EnsureStyleRuleFor to StyleAnimation.
This was only needed when we were inspecting the returned time fraction but now
that we inspect the phase it's not necessary to force the fill mode to "both".
David, I don't want to overload you with review requests. Can you suggest anyone else who might be suitable for reviewing these patches?
Flags: needinfo?(dbaron)
Attachment #8416311 - Attachment is obsolete: true
Maybe dholbert can review them; sorry, should have responded sooner.
Flags: needinfo?(dbaron)
Attachment #8416270 - Attachment is obsolete: true
Attached patch part 2 - Add AnimationTiming (obsolete) — — Splinter Review 

        Attachment #8423613 -
        Flags: review?(dholbert)

        Attachment #8416302 -
        Attachment is obsolete: true

        Attachment #8416324 -
        Attachment is obsolete: true

        Attachment #8416306 -
        Attachment is obsolete: true

        Attachment #8416307 -
        Attachment is obsolete: true

        Attachment #8416314 -
        Attachment is obsolete: true

        Attachment #8416318 -
        Attachment is obsolete: true

        Attachment #8416317 -
        Attachment is obsolete: true

        Attachment #8416319 -
        Attachment is obsolete: true

        Attachment #8416321 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8423612 [details] [diff] [review]
part 1 - Factor event queuing out of EnsureStyleRuleFor

This is basically the first time I've looked at this code, so bear with me if I ask some stupid questions. :)

> ElementAnimations::GetPositionInIteration(TimeDuration aElapsedDuration,
[...]
>+    if (aFillMode != NS_STYLE_ANIMATION_FILL_MODE_BOTH &&
>+        aFillMode != NS_STYLE_ANIMATION_FILL_MODE_FORWARDS) {
>+      // No animation data.
>+      return -1;

Could you clarify this "No animation data" comment, while you're here? (Perhaps it means "The animation isn't active or filling at this time"? If so, say something like that. "No animation data" sounds a bit like data is just missing, or something.)

(There's another copy of this comment lower down, too, which could use the same clarification.)

>+  } else if (currentIterationCount < 0.0) {
[...]
>-      NS_ASSERTION(aAnimation, "Should not run animation that hasn't started yet on the compositor");

Are you sure we want to drop this assertion from the < 0.0 case? It looks like it might still be relevant.

>-      GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime),
>-                             anim->mIterationDuration, anim->mIterationCount,
>-                             anim->mDirection, anim, this, &aEventsToDispatch);
[...]
>+        GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime),
>+                               anim->mIterationDuration, anim->mIterationCount,
>+                               anim->mDirection,
>+                               NS_STYLE_ANIMATION_FILL_MODE_BOTH);

Why are we hardcoding in FILL_MODE_BOTH here? Shouldn't we be looking this up off of the animation or something? (If not, maybe add a comment explaining why)

(This happens in two places that I noticed -- in AsyncCompositionManager.cpp and in nsAnimationManager.cpp.)

>+      // XXX This shouldn't be checking for positionInIteration >= 1.0, but
>+      // rather, checking that we're in the after phase (since we can get
>+      // positionInIteration == 1.0 when we have a direction that goes
>+      // backwards). We'll fix this in a subsequent patch when we introduce
>+      // phases but for now the result is we'll sometimes skip throttling when
>+      // we could have throttled the sample.
>       if (!anim->mIsRunningOnCompositor ||
>-          (anim->mLastNotification != oldLastNotification &&
>-           anim->mLastNotification ==
>-           ElementAnimation::LAST_NOTIFICATION_END)) {
>+          (positionInIteration >= 1.0 &&
>+           anim->mLastNotification != ElementAnimation::LAST_NOTIFICATION_END))

Why is the mLastNotification-vs-LAST_NOTIFICATION_END comparison here changing from == to !=  ?

(Superficially, it looks like that's flipping some bit of logic, but I'm probably misunderstanding.)

>+void
>+ElementAnimations::GetEventsAt(TimeStamp aRefreshTime,
>+                               EventArray& aEventsToDispatch)
>+{
>+    // We should *not* skip animations with zero duration (bug 1004365) or those
>+    // with no keyframes (bug 1004377).
>+    // We will fix this separately but for now this is necessary since
>+    // GetPositionInIteration does not yet handle zero iteration durations.

I'm not sure what "zero iteration durations" means. I think this wants to say "zero-duration iterations"?

>+    if (anim->mProperties.Length() == 0 ||
>+        anim->mIterationDuration.ToMilliseconds() <= 0.0) {
>+      // No animation data.
>+      continue;
>+    }
>+
>+    GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime),
>+                           anim->mIterationDuration, anim->mIterationCount,
>+                           anim->mDirection, anim->mFillMode,
>+                           anim, this, &aEventsToDispatch);
>+  }

Three things:
 (1) Use .IsEmpty() instead of Length() == 0
 (2) Could you flip the logic so that you don't need "continue" here?  Better to avoid 'continue' unless you absolutely need it, IMHO. (And it doesn't really add any value here.)
 (3) As above, I'm not sure what "No animation data" means, so perhaps clarify that (if the comment is still around, after the continue-removal).

        Attachment #8423612 -
        Attachment is obsolete: true

        Attachment #8423612 -
        Flags: review?(dholbert)

    
    

    
  
Hi Daniel,

Thanks for looking into this!

Would it help to post a roll-up patch so you can see what the final result looks like?

(In reply to Daniel Holbert [:dholbert] from comment #27)
> > ElementAnimations::GetPositionInIteration(TimeDuration aElapsedDuration,
> [...]
> >+    if (aFillMode != NS_STYLE_ANIMATION_FILL_MODE_BOTH &&
> >+        aFillMode != NS_STYLE_ANIMATION_FILL_MODE_FORWARDS) {
> >+      // No animation data.
> >+      return -1;
> 
> Could you clarify this "No animation data" comment, while you're here?
> (Perhaps it means "The animation isn't active or filling at this time"? If
> so, say something like that. "No animation data" sounds a bit like data is
> just missing, or something.)

Sure. I've updated this everywhere it occurs in this file.

> >+  } else if (currentIterationCount < 0.0) {
> [...]
> >-      NS_ASSERTION(aAnimation, "Should not run animation that hasn't started yet on the compositor");
> 
> Are you sure we want to drop this assertion from the < 0.0 case? It looks
> like it might still be relevant.

The sum of these patches moves us to a point where GetPositionInIteration (later GetComputedTimingAt) no longer has different modes depending on whether it is being called from the main thread (in which case aAnimation is provided) or the compositor (in which case aAnimation is null).

This method itself is intended to be generic. It's really not the responsibility of this method to check that the compositor isn't loaded with animations that are yet to start so I think this assertion should be removed.

(And, for the record, calling this for animations that have yet to start should work just fine and in fact we intend to put such animations on the compositor in future. Currently they don't call this method however and that is checked in http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?from=AsyncCompositionManager.cpp#407)
 
> >-      GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime),
> >-                             anim->mIterationDuration, anim->mIterationCount,
> >-                             anim->mDirection, anim, this, &aEventsToDispatch);
> [...]
> >+        GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime),
> >+                               anim->mIterationDuration, anim->mIterationCount,
> >+                               anim->mDirection,
> >+                               NS_STYLE_ANIMATION_FILL_MODE_BOTH);
> 
> Why are we hardcoding in FILL_MODE_BOTH here? Shouldn't we be looking this
> up off of the animation or something? (If not, maybe add a comment
> explaining why)

For animations on the compositor, that's the way they're currently designed to work. We put them on the compositor when they are due to start. If they happen to end up there beforehand we just don't sample them. But that should only ever happen under test conditions where the refresh driver time can go backwards. Backwards fill happens on the main thread.

When the animation finishes on the compositor, it keeps filling forwards until it gets removed (which is hopefully very soon after finishing). Then any forwards fill is performed on the main thread.

So that's the current behavior. In future I want to pass the fill mode to the compositor but currently we don't. These patches are intended to preserve the current behavior. We can change it in a future bug.

I've added a comment describing this.

> (This happens in two places that I noticed -- in AsyncCompositionManager.cpp
> and in nsAnimationManager.cpp.)

The instance in nsAnimationManager.cpp is different. In that case we want to make sure the returned positionInIteration is not -1 so we can tell if the animation has finished for the purpose of skipping throttling. That use of a hard-coded fill mode is removed in part 10 so I'm not sure how important it is to add a comment for this second instance. I'd rather leave it out for now.

> >+      // XXX This shouldn't be checking for positionInIteration >= 1.0, but
> >+      // rather, checking that we're in the after phase (since we can get
> >+      // positionInIteration == 1.0 when we have a direction that goes
> >+      // backwards). We'll fix this in a subsequent patch when we introduce
> >+      // phases but for now the result is we'll sometimes skip throttling when
> >+      // we could have throttled the sample.
> >       if (!anim->mIsRunningOnCompositor ||
> >-          (anim->mLastNotification != oldLastNotification &&
> >-           anim->mLastNotification ==
> >-           ElementAnimation::LAST_NOTIFICATION_END)) {
> >+          (positionInIteration >= 1.0 &&
> >+           anim->mLastNotification != ElementAnimation::LAST_NOTIFICATION_END))
> 
> Why is the mLastNotification-vs-LAST_NOTIFICATION_END comparison here
> changing from == to !=  ?

That's because previously the call to GetPositionInIteration updated mLastNotification on anim and so we would check that it had changed and was now the end notification. Now, however, GetPositionInIteration does just what it says and doesn't update mLastNotification so we check that that we're past the end of the active interval but the last notification hasn't been set to end yet (that happens in GetEventsAt).

> >+void
> >+ElementAnimations::GetEventsAt(TimeStamp aRefreshTime,
> >+                               EventArray& aEventsToDispatch)
> >+{
> >+    // We should *not* skip animations with zero duration (bug 1004365) or those
> >+    // with no keyframes (bug 1004377).
> >+    // We will fix this separately but for now this is necessary since
> >+    // GetPositionInIteration does not yet handle zero iteration durations.
> 
> I'm not sure what "zero iteration durations" means. I think this wants to
> say "zero-duration iterations"?

Yes! Thank you!

> >+    if (anim->mProperties.Length() == 0 ||
> >+        anim->mIterationDuration.ToMilliseconds() <= 0.0) {
> >+      // No animation data.
> >+      continue;
> >+    }
> >+
> >+    GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime),
> >+                           anim->mIterationDuration, anim->mIterationCount,
> >+                           anim->mDirection, anim->mFillMode,
> >+                           anim, this, &aEventsToDispatch);
> >+  }
> 
> Three things:
>  (1) Use .IsEmpty() instead of Length() == 0

Yes. Good idea. I've changed this here and the other places in this file (where this code was copied from).

>  (2) Could you flip the logic so that you don't need "continue" here? 
> Better to avoid 'continue' unless you absolutely need it, IMHO. (And it
> doesn't really add any value here.)

I'd somewhat prefer to keep it for now so the relationship with the other parts of the code that this is supposed to parallel is obvious.

We actually use continue quite a bit in nsAnimationManager. I don't mind it since it keeps the level of indentation manageable but perhaps in a follow-up patch I can factor out the inner parts of these loops and then remove the 'continue' useage without deep indentation.

In any case I think that should happen in a separate patch so as not to obscure how code is being shifted around in this patch.

Do you want me to add a part 11 for this? If not, I have 4 bugs filed on changes needed to GetEventsAt (due to existing bugs in our AnimationEvent handling) and I can probably remove the use of 'continue' in one of those.

>  (3) As above, I'm not sure what "No animation data" means, so perhaps
> clarify that (if the comment is still around, after the continue-removal).

Fixed.

    
    

    
  


  
This time after refreshing

        Attachment #8424573 -
        Flags: review?(dholbert)

        Attachment #8424547 -
        Attachment is obsolete: true

        Attachment #8424547 -
        Flags: review?(dholbert)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 2 - Add AnimationTiming (obsolete)
        — 
          — Splinter Review
      

    


  
Fix bitrot

        Attachment #8424588 -
        Flags: review?(dholbert)

        Attachment #8423613 -
        Attachment is obsolete: true

        Attachment #8423613 -
        Flags: review?(dholbert)

        Attachment #8423615 -
        Attachment is obsolete: true

        Attachment #8423615 -
        Flags: review?(dholbert)

        Attachment #8423616 -
        Attachment is obsolete: true

        Attachment #8423616 -
        Flags: review?(dholbert)

        Attachment #8423617 -
        Attachment is obsolete: true

        Attachment #8423617 -
        Flags: review?(dholbert)

        Attachment #8423619 -
        Attachment is obsolete: true

        Attachment #8423619 -
        Flags: review?(dholbert)

        Attachment #8423621 -
        Attachment is obsolete: true

        Attachment #8423621 -
        Flags: review?(dholbert)

        Attachment #8423623 -
        Attachment is obsolete: true

        Attachment #8423623 -
        Flags: review?(dholbert)

        Attachment #8423624 -
        Attachment is obsolete: true

        Attachment #8423624 -
        Flags: review?(dholbert)

        Attachment #8423620 -
        Attachment is obsolete: true

        Attachment #8423620 -
        Flags: review?(dholbert)

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #29)
> Would it help to post a roll-up patch so you can see what the final result
> looks like?

Probably not, but thanks! I've got the patches imported locally into a patch queue, and I'm reviewing them with the assistance of a merge tool.

> (In reply to Daniel Holbert [:dholbert] from comment #27)
 > >+      // No animation data.
> > >+      return -1;
> > 
> > Could you clarify this "No animation data" comment, while you're here?
> 
> Sure. I've updated this everywhere it occurs in this file.

Thanks!

One nit: it looks like the new versions of this comment are could use a period at the end. If you can fix that without bitrotting the world, great; if not, no big deal.

> >  (2) Could you flip the logic so that you don't need "continue" here? 
> > Better to avoid 'continue' unless you absolutely need it, IMHO. (And it
> > doesn't really add any value here.)
> 
> I'd somewhat prefer to keep it for now so the relationship with the other
> parts of the code that this is supposed to parallel is obvious.
> 
> We actually use continue quite a bit in nsAnimationManager.

Ah, didn't realize this was mirroring other existing code. OK, not a big deal then. I'll ignore the nagging voice in the back of my head from one of my CS professors. :)

> In any case I think that should happen in a separate patch so as not to
> obscure how code is being shifted around in this patch.
> 
> Do you want me to add a part 11 for this?

I wouldn't bother; it's something we can always do later, and probably avoid scope-creep on this bug here, since it's already quite large number-of-patches-wise. :)

    
    

    
  
Comment on attachment 8424573 [details] [diff] [review]
part 1 - Factor event queuing out of EnsureStyleRuleFor

>Bug 1004871 part 1 - Factor event queuing out of EnsureStyleRuleFor
[...]
>The actual work of removing event queueing from GetPositionInIteration is

Nit: s/queueing/queuing/ in later line of the commit-message.

>-      // If aAnimation is null, that means we're on the compositor
>-      // thread.  We want to just keep filling forwards until the main
>-      // thread gets around to updating the compositor thread (which
>-      // might take a little while).  So just assume we fill fowards and
>-      // move on.
>+    }
>+    if (aFillMode != NS_STYLE_ANIMATION_FILL_MODE_BOTH &&
>+        aFillMode != NS_STYLE_ANIMATION_FILL_MODE_FORWARDS) {
>+      // The animation isn't active or filling at this time

Nit: per comment 40, this comment (& its other instances) should end with a period, if that doesn't bitrot things too horribly.

r=me

        Attachment #8424573 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8424588 [details] [diff] [review]
part 2 - Add AnimationTiming

># HG changeset patch
># User Brian Birtles <birtles@gmail.com>
># Date 1400472306 -32400
># Node ID da95a2388c933f867d3b54c1350fb0c97b99581f
># Parent  d51fee26afbc9175b4bbf810d38964d630e88505
>Bug 1004871 part 2 - Add AnimationTiming

This commit message (first line) is a bit short/vague.

Maybe append something like (taken from the subsequent lines):
"...struct, to encapsulate animation timing parameters".

>+++ b/layout/style/AnimationCommon.h
> 
>+struct AnimationTiming
>+{
>+  mozilla::TimeDuration mIterationDuration;
>+  float mIterationCount; // NS_IEEEPositiveInfinity() means infinite
>+  uint8_t mDirection;
>+  uint8_t mFillMode;
>+};
>+

Add a brief comment above this struct, explaining what it's for. (And to hint about why these particular things are bundled together)

(e.g. maybe say that this encapsulates the inputs to GetPositionInIteration(), even though that's only half-true until the next patch)

>@@ -236,28 +244,26 @@ struct ElementAnimation
[...]
>   nsString mName; // empty string for 'none'
>-  float mIterationCount; // NS_IEEEPositiveInfinity() means infinite
>-  uint8_t mDirection;
>-  uint8_t mFillMode;
>+  AnimationTiming mTiming;
>   uint8_t mPlayState;
> 
[...]
>   bool HasAnimationOfProperty(nsCSSProperty aProperty) const;
>   bool IsRunningAt(mozilla::TimeStamp aTime) const;
[...]
>   mozilla::TimeStamp mStartTime;
>   mozilla::TimeStamp mPauseStart;
>   mozilla::TimeDuration mDelay;
>-  mozilla::TimeDuration mIterationDuration;
>   bool mIsRunningOnCompositor;

This leaves these member-vars in a slightly odd order. In particular:
  - mPlayState (a single-byte value) is now listed in between structs, whereas previously it was packed with other uint8_t's. That seems undesirable. (Technically it's up against a uint8_t inside of AnimationTiming, but that's not clear to the reader and probably (?) not useful to the compiler for packing purposes.)
  - I'm not clear on why the member-vars are split apart by tens of lines here in the first place. The separation made a bit more sense before this patch, when it was "bunch of uint8_ts + string" vs. "bunch Timestamps & bool", but now it seems a bit more haphazard.

So: in a followup (or here if you'd prefer), could you perhaps make all of these member-vars adjacent, and reorder them so that the uint8_t is at the bottom with the bool (since they stand a better chance of being packed than a uint8_t and a struct, I'd think).  Unless there's some rhyme/reason to the current ordering that I'm not seeing.

>diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
>@@ -749,30 +754,30 @@ nsAnimationManager::BuildAnimations(nsSt
>-    dest->mIterationCount = src.GetIterationCount();
>-    dest->mDirection = src.GetDirection();
>-    dest->mFillMode = src.GetFillMode();
>+    dest->mTiming.mIterationCount = src.GetIterationCount();
>+    dest->mTiming.mDirection = src.GetDirection();
>+    dest->mTiming.mFillMode = src.GetFillMode();
>     dest->mPlayState = src.GetPlayState();
> 
[...]
> 
>-    dest->mIterationDuration =
>+    dest->mTiming.mIterationDuration =
>       TimeDuration::FromMilliseconds(src.GetDuration());

Could you shift these last 2 lines up up to just before where we set mIterationCount, so that we're initializing all of mTiming's fields at once (and in the proper order)?

r=me with that

        Attachment #8424588 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8424589 [details] [diff] [review]
part 3 - Replace parameters to GetPositionInIteration with an AnimationTiming object

>+++ b/gfx/layers/composite/AsyncCompositionManager.cpp
>@@ -426,29 +426,30 @@ SampleAnimations(Layer* aLayer, TimeStam
>-    double numIterations = animation.numIterations() != -1 ?
>-      animation.numIterations() : NS_IEEEPositiveInfinity();
>+    AnimationTiming timing;
>+    timing.mIterationDuration = animation.duration();
>+    timing.mIterationCount = animation.numIterations() != -1 ?
>+                             animation.numIterations() :
>+                             NS_IEEEPositiveInfinity();

Hmm, it's unfortunate that we have this inconsistency about how to represent "forever" (-1 vs +infinity) and consequently need this conditional stuff here.

I'd like to make this consistent. (Maybe standardizing on infinity, since it probably works out better math-wise.)  Could you file a followup bug on that, if you agree & if you don't know of that already being tracked/fixed elsewhere?

r=me on this part, with the followup filed (or if the followup isn't applicable/doable, with more info on why that is)

        Attachment #8424589 -
        Flags: review?(dholbert) → review+

    
    

    
  
Also, it looks like animation.numIterations() is a float, per:
mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh?rev=f8ba182364b1#181
whereas AnimationTiming::mIterationCount is a double.

Should we make those types consistent (particularly because we copy one to the other)?  (And perhaps make their names consistent, while we're at it?)

        Attachment #8424590 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8424591 [details] [diff] [review]
part 5 - Add ComputedTiming data structure

>diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
>+struct ComputedTiming
>+{

Add a brief comment explaining the purpose of this struct (maybe something like the following, based on your extended commit message: "Stores the results of calculating the timing properties of an animation at a given sample time.")

>+  // Zero-based iteration index (0 when mTimeFraction is kNullTimeFraction)
>+  uint64_t mCurrentIteration;

The "0 when mTimeFraction is kNullTimeFraction" comment here is a bit misleading; it implies that there's an important connection between those two variable-settings. But really, I'd expect that...
 - This value will *also* be 0 when we're simply in the first iteration.
 - When mTimeFraction is kNullTimeFraction, this value is really probably just meaningless & should be ignored. (though it will happen to be set to 0)

If I'm understanding correctly, maybe update the documentation to say "meaningless when mTimeFraction is kNullTimeFraction"? (or "irrelevant", or something to that effect)  (maybe also mentioning that it can be expected to be 0, if you like)

(This meaninglessness probably also applies to the phase-enum as well, right? i.e. if mTimeFraction is kNullTimeFraction, then all of the other member-data here is kind of meaningless?)

>+  enum {
>+    // Not sampled or sampled with a null time
>+    AnimationPhase_Null,

I don't understand what this comment is saying (particularly the "sampled with a null time" part)

I think this enum value is really serving as a sentinel "uninitialized" value, though I'm not sure we even need it -- in the next patch, it looks like GetPositionInIteration() sets the enum member-var to one of the *non* _Null values in all code-paths before it returns.  So if I'm not mistaken, GetPositionInIteration() never actually returns a ComputedTiming() with the _Null enum value.

So I'd prefer we either
 - clearly document this value as an "uninitialized" sentinel (and maybe also add some assertions about it in some relevant places in other patches, for code outside of GetPositionInIteration() that deals with ComputedTiming)
or...
 - just remove the enum value altogether.

r=me with the above addressed.

        Attachment #8424591 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8424592 [details] [diff] [review]
part 6 - Make GetPositionInIteration return a ComputedTiming object

>diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp
>+  // Always return the same object in order to benefit from return-value
>+  // optimization
>+  ComputedTiming result;
>+

nit: if you drop "in order", you can make this a one-liner comment.

>   double whichIteration = floor(currentIterationCount);
>   if (whichIteration == aTiming.mIterationCount && whichIteration != 0.0) {
>@@ -138,17 +145,19 @@ ElementAnimations::GetPositionInIteratio
>         ? NS_ANIMATION_START : NS_ANIMATION_ITERATION;
> 
>     aAnimation->mLastNotification = whichIteration;
>     AnimationEventInfo ei(aEa->mElement, aAnimation->mName, message,
>                           aElapsedDuration, aEa->PseudoElement());
>     aEventsToDispatch->AppendElement(ei);
>   }
> 
>-  return positionInIteration;
>+  result.mTimeFraction = positionInIteration;
>+  result.mCurrentIteration = uint64_t(currentIterationCount); // floor

Note that we already do an explicit "floor" operation with 'whichIteration' up above, and then we convert *that* to a uint64_t in a few clauses up above.

Can/should this share code with that, so we're not floor'ing / converting to uint64_t independently too many times?

(Also, do we need to care about the possibility that currentIterationCount is greater than the maximum uint64_t value?)

>-      // XXX Only set mNeedsRefreshes to true when we are are in the active
>-      // phase (the reason we test for <= 1 rather than <1 is to cover cases
>-      // where we have an alternating direction which can produce a value of
>-      // 1 while we're still in the active phase). We will fix this when we
>-      // introduce phases
>-      if (positionInIteration <= 1 && !anim->IsPaused()) {
>+
>+      if ((computedTiming.mPhase == ComputedTiming::AnimationPhase_Before ||
>+           computedTiming.mPhase == ComputedTiming::AnimationPhase_Active) &&
>+          !anim->IsPaused()) {
>         mNeedsRefreshes = true;

Note: The comment that's being deleted here seems to disagree with the code that's replacing it, RE the conditions under which we should set mNeedsRefreshes. (The comment says "only[...]when we are in the active phase", but the new code sets it in the "_Before" phase as well).

I'm guessing the comment is wrong, and not the code? (If so, since this comment is added in an earlier patch, it might be worth tweaking it for HG-archeology's sake, but it probably doesn't matter.)

>-      // The position is -1 when we don't have fill data for the current time,
>-      // so we shouldn't animate.
>-      if (positionInIteration == -1)
>+      // If the time fraction is null we don't have fill data for the current
>+      // time so we shouldn't animate

Add comma after "null", and keep the period at the end. (Both aid in readability.)

>diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h
>--- a/layout/style/nsAnimationManager.h
>+++ b/layout/style/nsAnimationManager.h
>@@ -56,31 +56,32 @@ struct ElementAnimations MOZ_FINAL
>   // This function takes as input the timing parameters of an animation and
>-  // returns the position in the current iteration.
>+  // returns the computed timing at the given moment.

This change makes the summary less clear, IMHO. (It's not really clear what "computed timing at the given moment" means.)

Maybe "...returns a ComputedTiming struct representing the animation's progress at the given moment"? (or something along those lines)


Not setting r+ on this one yet since I'm curious about your thoughts on the floor()ing and the AnimationPhase_Before thing.

    
    

    
  
Comment on attachment 8424597 [details] [diff] [review]
part 7 - Remove event queueing from GetPositionInIteration and do it in GetEventsAt

>+++ b/layout/style/nsAnimationManager.h
>   // This function takes as input the timing parameters of an animation and
>   // returns the computed timing at the given moment.
>-  //
>-  // When this function is called from the main thread with an actual
>-  // ElementAnimation* pointer, animation events are also queued.
>-  // This function returns ComputedTiming::kNullTimeFraction for the
>-  // mTimeFraction member of the return value if the animation should not be
>-  // run (because it is not currently active and has no fill behavior).
>-  //
>-  // After calling GetPositionInIteration with non-null aAnimation and aEa, be
>-  // sure to call CheckNeedsRefresh on the animation manager afterwards.
>   static mozilla::ComputedTiming GetPositionInIteration(

The documentation about kNullTimeFraction (& through the end of that paragraph) probably needs to stay, right?

Also: I'm pretty sure "has no fill behavior" wants to say "is not filling at this time".  ("no fill behavior" sounds like FILL_MODE_NONE, which I don't think is what you wanted to say here.) (This line of documentation is added in an earlier patch here -- I didn't comment on it in the earlier patch because I noticed that it went away eventually, but now I'm realizing it probably *shouldn't* go away, hence commenting on it now.)

Feel free to just fix that fill-behavior typo in this patch, if that's easiest bitrot-wise (rather than fixing it in the patch that added this language).

>+  // After calling this be sure to call CheckNeedsRefresh on the animation
>+  // manager afterwards.
>   void EnsureStyleRuleFor(TimeStamp aRefreshTime, bool aIsThrottled);

Nit: add comma after "calling this".

r=me with the above.

        Attachment #8424597 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8424593 [details] [diff] [review]
part 8 - Simplify ElementAnimations::GetEventsAt

>diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
>+    switch (computedTiming.mPhase) {
>+      case ComputedTiming::AnimationPhase_Active:
[...]
>+      case ComputedTiming::AnimationPhase_After:
[...]
>+      default:
>+        // Do nothing
>+        break;
>     }

So in the "default" code-path here, we're really just expecting AnimationPhase_Before. (That and _Null are the only not-explicitly-listed-here values for this enum. And, per comment 45, I think we never expect to see _Null in the wild.)

I'd suggest getting rid of "default", and replacing it with:

  case ComputedTiming::AnimationPhase_Null:
    MOZ_ASSERT(false, "Not expecting ComputedTiming values in null phase");
    // fall through:
  case ComputedTiming::AnimationPhase_Begin:
    // do nothing
    break;

(If you decide not to have a _Null phase after all, as semi-suggested in comment 45, then that case wouldn't be here of course.)

This structure has the following benefits:
 - It lets you cleanly assert about Null being unexpected.
 - If you add a new phase (new enum value) in the future, the compiler will make sure you update this 'switch' statement to explicitly consider that new value. (which might be important, if it needs special behavior here). With 'default', the compiler can't keep you honest like that.

r=me with something along those lines.

        Attachment #8424593 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8424594 [details] [diff] [review]
part 9 - Move ElementAnimations::GetPositionInIteration to ElementAnimation::GetComputedTimingAt

>+++ b/layout/style/AnimationCommon.cpp
>+ComputedTiming
>+ElementAnimation::GetComputedTimingAt(TimeDuration aElapsedDuration,
>+                                      const AnimationTiming &aTiming)
>+{

Make the "&" type-hugging there. (shift it to the left)

>+++ b/layout/style/nsAnimationManager.cpp
>@@ -142,18 +72,18 @@ ElementAnimations::EnsureStyleRuleFor(Ti
>           anim->mTiming.mIterationDuration.ToMilliseconds() <= 0.0) {
>         continue;
>       }
> 
>       // The ElapsedDurationAt() call here handles pausing.  But:
>       // FIXME: avoid recalculating every time when paused.
>       AnimationTiming timing = anim->mTiming;
>       timing.mFillMode = NS_STYLE_ANIMATION_FILL_MODE_BOTH;
>-      ComputedTiming computedTiming =
>-        GetPositionInIteration(anim->ElapsedDurationAt(aRefreshTime), timing);
>+      ComputedTiming computedTiming = ElementAnimation::GetComputedTimingAt(
>+        anim->ElapsedDurationAt(aRefreshTime), timing);

I'm not so sure about the extreme-deindentation style here... Maybe use a local-var, like you do in GetEventsAt?

(I guess you end up re-using the var in GetEventsAt(), so it's more obviously-necessary there. But even here where it'd be used just this once, it makes the function-call easier to visually parse, IMO, and better-matches our coding style.)

Like so:
    TimeDuration elapsedDuration = anim->ElapsedDurationAt(aRefreshTime);
    ComputedTiming computedTiming =
      ElementAnimation::GetComputedTimingAt(elapsedDuration, mTiming);

(This pattern happens twice.)

>@@ -287,26 +216,27 @@ ElementAnimations::GetEventsAt(TimeStamp
>-    // GetPositionInIteration does not yet handle zero-duration iterations.
>+    // ElementAnimation::GetComputedTimingAt does not yet handle zero iteration
>+    // durations.

The "zero iteration durations" (at the end there) wants to be "zero-duration iterations", per comment 27. ;)

r=me with the above (assuming of course that changes to GetPositionInIteration() mentioned in earlier patches are still preserved in the conversion to GetComputedTimingAt in this patch.)

        Attachment #8424594 -
        Flags: review?(dholbert) → review+

        Attachment #8424595 -
        Flags: review?(dholbert) → review+

        Attachment #8424573 -
        Attachment is obsolete: true

        Attachment #8424588 -
        Attachment is obsolete: true

        Attachment #8424589 -
        Attachment is obsolete: true

        Attachment #8424590 -
        Attachment is obsolete: true

        Attachment #8424591 -
        Attachment is obsolete: true

        Attachment #8424592 -
        Attachment is obsolete: true

        Attachment #8424592 -
        Flags: review?(dholbert)

        Attachment #8424597 -
        Attachment is obsolete: true

        Attachment #8424593 -
        Attachment is obsolete: true

        Attachment #8424594 -
        Attachment is obsolete: true

        Attachment #8424595 -
        Attachment is obsolete: true

    
    

    
  
Hi Daniel, thanks for all the reviews!

(In reply to Daniel Holbert [:dholbert] from comment #41)
> Comment on attachment 8424573 [details] [diff] [review]
> part 1 - Factor event queuing out of EnsureStyleRuleFor
[...]
> Nit: s/queueing/queuing/ in later line of the commit-message.

Fixed.

> Nit: per comment 40, this comment (& its other instances) should end with a
> period, if that doesn't bitrot things too horribly.

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #42)
> part 2 - Add AnimationTiming
[...]
> This commit message (first line) is a bit short/vague.

Fixed.

> >+struct AnimationTiming
[...]
> Add a brief comment above this struct, explaining what it's for. (And to
> hint about why these particular things are bundled together)

Fixed.

> >@@ -236,28 +244,26 @@ struct ElementAnimation
> So: in a followup (or here if you'd prefer), could you perhaps make all of
> these member-vars adjacent, and reorder them so that the uint8_t is at the
> bottom with the bool (since they stand a better chance of being packed than
> a uint8_t and a struct, I'd think).  Unless there's some rhyme/reason to the
> current ordering that I'm not seeing.

Added as part 11.

> >diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
> >-    dest->mIterationDuration =
> >+    dest->mTiming.mIterationDuration =
> >       TimeDuration::FromMilliseconds(src.GetDuration());
> 
> Could you shift these last 2 lines up up to just before where we set
> mIterationCount, so that we're initializing all of mTiming's fields at once
> (and in the proper order)?

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #43)
> part 3 - Replace parameters to GetPositionInIteration with an
> AnimationTiming object
[...]
> >-    double numIterations = animation.numIterations() != -1 ?
> >-      animation.numIterations() : NS_IEEEPositiveInfinity();
> >+    AnimationTiming timing;
> >+    timing.mIterationDuration = animation.duration();
> >+    timing.mIterationCount = animation.numIterations() != -1 ?
> >+                             animation.numIterations() :
> >+                             NS_IEEEPositiveInfinity();
> 
> Hmm, it's unfortunate that we have this inconsistency about how to represent
> "forever" (-1 vs +infinity) and consequently need this conditional stuff
> here.
> 
> I'd like to make this consistent. (Maybe standardizing on infinity, since it
> probably works out better math-wise.)  Could you file a followup bug on
> that, if you agree & if you don't know of that already being tracked/fixed
> elsewhere?

Filed as bug 1015803

(In reply to Daniel Holbert [:dholbert] from comment #44)
> Also, it looks like animation.numIterations() is a float, per:
> mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.
> ipdlh?rev=f8ba182364b1#181
> whereas AnimationTiming::mIterationCount is a double.
> 
> Should we make those types consistent (particularly because we copy one to
> the other)?  (And perhaps make their names consistent, while we're at it?)

I'm not certain about the types (we seem to use float exclusively in layer messages?) but definitely the names.

(In reply to Daniel Holbert [:dholbert] from comment #45)
> part 5 - Add ComputedTiming data structure
[...]
> >+struct ComputedTiming
> >+{
> 
> Add a brief comment explaining the purpose of this struct (maybe something
> like the following, based on your extended commit message

Fixed.


> The "0 when mTimeFraction is kNullTimeFraction" comment here is a bit
> misleading; it implies that there's an important connection between those
> two variable-settings. But really, I'd expect that...

Fixed.

> >+  enum {
> >+    // Not sampled or sampled with a null time
> >+    AnimationPhase_Null,
> 
> I don't understand what this comment is saying (particularly the "sampled
> with a null time" part)
> 
> I think this enum value is really serving as a sentinel "uninitialized"
> value, though I'm not sure we even need it

Removed.

(In reply to Daniel Holbert [:dholbert] from comment #46)
> part 6 - Make GetPositionInIteration return a ComputedTiming object
[...]
> nit: if you drop "in order", you can make this a one-liner comment.

Fixed.

> >   double whichIteration = floor(currentIterationCount);
> >   if (whichIteration == aTiming.mIterationCount && whichIteration != 0.0) {
> >@@ -138,17 +145,19 @@ ElementAnimations::GetPositionInIteratio
> >         ? NS_ANIMATION_START : NS_ANIMATION_ITERATION;
> > 
> >     aAnimation->mLastNotification = whichIteration;
> >     AnimationEventInfo ei(aEa->mElement, aAnimation->mName, message,
> >                           aElapsedDuration, aEa->PseudoElement());
> >     aEventsToDispatch->AppendElement(ei);
> >   }
> > 
> >-  return positionInIteration;
> >+  result.mTimeFraction = positionInIteration;
> >+  result.mCurrentIteration = uint64_t(currentIterationCount); // floor
> 
> Note that we already do an explicit "floor" operation with 'whichIteration'
> up above, and then we convert *that* to a uint64_t in a few clauses up above.
> 
> Can/should this share code with that, so we're not floor'ing / converting to
> uint64_t independently too many times?
> 
> (Also, do we need to care about the possibility that currentIterationCount
> is greater than the maximum uint64_t value?)

I'm a bit unsure about all this but here's what I've done:
* mLastNotification in ElementAnimation was uint32_t. It seems like if we're going to use uint64_t to represent iteration counts at all we need to use them consistently so I switch ElementAnimation to use uint64_t.
* I've calculated positionInIteration directly from currentIterationCount. This seems simpler and keeps the floating-point math together.
* I've made whichIteration an integer. It seems to make more sense to me that whichIteration always represents an integral count and not a double acting as an integer.
* Regarding the range limits, the limit is actually further restricted by the fact that when we assign whichIteration to ElementAnimation::mLastNotification we have a couple of sentinel values in the upper range. I've added logic to specifically check for this and wrap the range at that point.

Let me know what you think about all this.

[...]
> Note: The comment that's being deleted here seems to disagree with the code
> that's replacing it, RE the conditions under which we should set
> mNeedsRefreshes. (The comment says "only[...]when we are in the active
> phase", but the new code sets it in the "_Before" phase as well).
> 
> I'm guessing the comment is wrong, and not the code? (If so, since this
> comment is added in an earlier patch, it might be worth tweaking it for
> HG-archeology's sake, but it probably doesn't matter.)

I went back and fixed it in the earlier patch.

> Add comma after "null", and keep the period at the end. (Both aid in
> readability.)

Fixed.

> >diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h
[...]
> This change makes the summary less clear, IMHO. (It's not really clear what
> "computed timing at the given moment" means.)

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #47)
> part 7 - Remove event queueing from GetPositionInIteration and do it in
> GetEventsAt
[...]
> The documentation about kNullTimeFraction (& through the end of that
> paragraph) probably needs to stay, right?

Good catch, fixed.

> Also: I'm pretty sure "has no fill behavior" wants to say "is not filling at
> this time".

Fixed.

> Feel free to just fix that fill-behavior typo in this patch, if that's
> easiest bitrot-wise (rather than fixing it in the patch that added this
> language).

Yes, I fixed it in this patch.

> Nit: add comma after "calling this".

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #48)
> part 8 - Simplify ElementAnimations::GetEventsAt
[...]
> So in the "default" code-path here, we're really just expecting
> AnimationPhase_Before. (That and _Null are the only
> not-explicitly-listed-here values for this enum. And, per comment 45, I
> think we never expect to see _Null in the wild.)
> 
> I'd suggest getting rid of "default", and replacing it with:
> 
>   case ComputedTiming::AnimationPhase_Null:
>     MOZ_ASSERT(false, "Not expecting ComputedTiming values in null phase");
>     // fall through:
>   case ComputedTiming::AnimationPhase_Begin:
>     // do nothing
>     break;

Fixed and moved AnimationPhase_Before to the start of the series of case statements so it reads before-active-after.

(In reply to Daniel Holbert [:dholbert] from comment #49)
> part 9 - Move ElementAnimations::GetPositionInIteration to
> ElementAnimation::GetComputedTimingAt
[...]
> Make the "&" type-hugging there. (shift it to the left)

Fixed.

> I'm not so sure about the extreme-deindentation style here... Maybe use a
> local-var, like you do in GetEventsAt?

Fixed.

> (This pattern happens twice.)

Fixed both occurrences.

> The "zero iteration durations" (at the end there) wants to be "zero-duration
> iterations", per comment 27. ;)

Fixed.

    
    

    
  
Looks like I broke things pretty badly along the way: https://tbpl.mozilla.org/?tree=Try&rev=325340514f22
(Not surprising given that I've probably rebased these patches about 2 dozen times :)

I'll look into it tomorrow.

    
    

    
  


  
Fix a bug I introduced. Clearing review for now though because there is still a random assertion failure.

        Attachment #8428534 -
        Attachment is obsolete: true

        Attachment #8428534 -
        Flags: review?(dholbert)

        Attachment #8428535 -
        Attachment is obsolete: true

        Attachment #8428537 -
        Attachment is obsolete: true

        Attachment #8428540 -
        Attachment is obsolete: true

        Attachment #8428540 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8428578 [details] [diff] [review]
part 6 - Make GetPositionInIteration return a ComputedTiming object

(In reply to Brian Birtles (:birtles) from comment #63)
> Created attachment 8428578 [details] [diff] [review]
> part 6 - Make GetPositionInIteration return a ComputedTiming object
> 
> Fix a bug I introduced. Clearing review for now though because there is
> still a random assertion failure.

I fixed the bug by updating positionInIteration when we're at the end of the last iteration. I'm not sure if this is actually how I want to fix it or if I should go back to using whichIteration.

I'll have a bit more of a think about it but feel free to review in the meantime.

        Attachment #8428578 -
        Flags: review?(dholbert)

    
    

    
  


  
I figured that since it is a public holiday in the US you wouldn't mind if
I slipped in another update to part 6.

Having thought about how to approach the timing calculations in
GetPositionInIteration, I think it's ok for now. In future I plan to rework this
method significantly as described in bug 999922 comment 2. I hope to be able to
remove the redundancy introduced by this patch then.

Also, I've removed the checks for overflow on the iteration count. Given an
animation that iterates once every *microsecond*, we could still represent the
current iteration for about 580,000 years.

        Attachment #8428961 -
        Flags: review?(dholbert)

        Attachment #8428578 -
        Attachment is obsolete: true

        Attachment #8428578 -
        Flags: review?(dholbert)

        Attachment #8428584 -
        Attachment is obsolete: true

        Attachment #8428586 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8428961 [details] [diff] [review]
part 6 - Make GetPositionInIteration return a ComputedTiming object

>+ComputedTiming
> ElementAnimations::GetPositionInIteration(TimeDuration aElapsedDuration,
[...]
>+  ComputedTiming result;
>     if (!aTiming.FillsForwards()) {
>-      // The animation isn't active or filling at this time.
>-      return -1;
>+      result.mTimeFraction = ComputedTiming::kNullTimeFraction;
>+      return result;

I think it's probably worth keeping the comment, to explain what the significance of that FillsForwards() check was, & why we're returning kNullTimeFraction.

>     currentIterationCount = aTiming.mIterationCount;
>   } else if (currentIterationCount < 0.0) {
>+    result.mPhase = ComputedTiming::AnimationPhase_Before;
>     if (!aTiming.FillsBackwards()) {
>-      // The animation isn't active or filling at this time.
>-      return -1;
>+      result.mTimeFraction = ComputedTiming::kNullTimeFraction;
>+      return result;

Same here.

>+  double positionInIteration = fmod(currentIterationCount, 1);
>+
>+  // Set |whichIteration| to the integral index of the current iteration.
>+  // Casting to an integer here gives us floor(currentIterationCount).
>+  uint64_t whichIteration = static_cast<uint64_t>(currentIterationCount);

Add a comment explaining why we're not bothering to check for overflow (per comment 68).

>+  // Check for the end of the last iteration.
>+  if (whichIteration == aTiming.mIterationCount && whichIteration != 0) {
>     // When the animation's iteration count is an integer (as it
>     // normally is), we need to end at 100% of its last iteration
>     // rather than 0% of the next one (unless it's zero).

Two things about this "if" check:
 (1) Probably better to do the 0 comparison first, since it's cheaper (no integer-to-float type-conversion necessary)
 (2) The other comparison is between a uint64_t and a float, which feels slightly iffy, because converting an uint64_t to a float is lossy, once we've surpassed the smallest integer that's not representable in a float. (which I think is approximately 2^N where N is the number of bits of mantissa.)  Is the worst that can happen here just that we'll run the animation too many or too few times? If so, maybe this doesn't matter, when we get to that number of iterations.

Also, while you're here -- please make this comment-tweak inside the "if" body:
 s/last iteration/final iteration/
(since "last" can easily (& incorrectly) be interpreted as "previous")

r=me with the above addressed (except maybe (2))

        Attachment #8428961 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #71)
> Comment on attachment 8428961 [details] [diff] [review]
> part 6 - Make GetPositionInIteration return a ComputedTiming object
[...]
>  (2) The other comparison is between a uint64_t and a float, which feels
> slightly iffy, because converting an uint64_t to a float is lossy, once
> we've surpassed the smallest integer that's not representable in a float.
> (which I think is approximately 2^N where N is the number of bits of
> mantissa.)  Is the worst that can happen here just that we'll run the
> animation too many or too few times? If so, maybe this doesn't matter, when
> we get to that number of iterations.

The worst that can happen is:

a) We'll get the wrong fill value
b) We'll get some funny animation values (i.e. jumping to the end of the animation range)
c) We'll end up with the wrong alternating behavior

Typically we'll only reach this point for an animation that iterates very quickly over a long but finite period of time in which case (b) and (c) will probably be hard to notice. (a) will only matter once we finish the animation. Basically we're saying if you write 'animation-iteration-count: 8388608' or greater you might end up with the wrong fill value. Even for the unlikely case of an animation with an interval of 1 millisecond, we won't reach the end for 2h20m so you're not likely to see the fill value.

However, I think we can fix this by making the condition:

if (whichIteration != 0 &&
    result.mPhase == ComputedTiming::AnimationPhase_After && 
    aTiming.mIterationCount == floor(aTiming.mIterationCount)) {
  ...
}

Which also happens to line up with the comment that follows that refers to checking if the iteration count is an integer.

What do you think?
Flags: needinfo?(dholbert)

        Attachment #8428961 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #72)

> I think we can fix this by making the condition:
> 
> if (whichIteration != 0 &&
>     result.mPhase == ComputedTiming::AnimationPhase_After && 
>     aTiming.mIterationCount == floor(aTiming.mIterationCount)) {
>   ...
> }
> 
> Which also happens to line up with the comment that follows that refers to
> checking if the iteration count is an integer.
> 
> What do you think?

That sounds good!
Flags: needinfo?(dholbert)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: