Closed Bug 1029370 Opened 10 years ago Closed 10 years ago

Encapsulate processing of AnimationTiming.mIterationDuration a little better

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(4 files)

In debugging bug 1028514 I initially thought the problem was because we weren't handling negative durations correctly. As a result I reworked the timing calculations so that we weren't directly reading AnimationTiming.mIterationDuration in so many places but only in GetComputedTimingAt where we would perform any negative handling before returning a sanitized version.

The cause of bug 1028514 turned out to be overflow within TimeStamp but some of those patches still make sense so I thought I'd land them anyway. They remove redundant code and will probably be necessary once we implement this part of the Web Animations API since it can provide negative durations.
This patch make the active duration a property of the ComputedTiming struct and
returns this as part of calculting GetComputedTimingAt. GetComputedTimingAt was
already calling the method to calculate the ActiveDuration and the only other
callers of ActiveDuration() were also calling GetComputedTimingAt so this
doesn't make us do any unnecessary calculation.

I've left ActiveDuration as a public method on ElementAnimation for now since
it's a struct and just about everything there is public. At some point in the
future we'll probably make this more class-like hide some details but that can
happen as a separate step. This patch does, however, move the definition of
ActiveDuration inside .cpp file.

In tidying up GetComputedTimingAt we also replace all the references to
TimeDuration() and TimeDuration(0) with a single local variable representing
zero duration. This should be easier to read and possibly a little faster.

We don't use a function static variable since this method is called from
different threads and the initialization of function statics is not guaranteed
to be thread-safe until C++0x.
Attachment #8445004 - Flags: review?(dholbert)
This patch removes some redundant calculation of timing phases in
FlushTransitions by calling GetComputedTiming and using the definitions there.
Attachment #8445005 - Flags: review?(dholbert)
This is just for my sake to show how we could support clamping iteration duration inside GetComputedTimingAt
Comment on attachment 8445004 [details] [diff] [review]
part 1 - Move active duration calculation to GetComputedTimingAt

>Bug 1029370 part 1 - Move active duration calculation to GetComputedTimingAt
>
>This patch make the active duration a property of the ComputedTiming struct and

s/make/makes/

>returns this as part of calculting GetComputedTimingAt. GetComputedTimingAt was

s/calculting/calculating/

>I've left ActiveDuration as a public method on ElementAnimation for now since
>it's a struct and just about everything there is public. At some point in the
>future we'll probably make this more class-like hide some details but that can

s/hide/to hide/?  (or ", hiding some details,")

>+++ b/layout/style/AnimationCommon.cpp
> ComputedTiming
> ElementAnimation::GetComputedTimingAt(TimeDuration aLocalTime,
>                                       const AnimationTiming& aTiming)
> {
>+  const TimeDuration zeroDuration;
>+
>   // Currently we expect negative durations to be picked up during CSS
>   // parsing but when we start receiving timing parameters from other sources
>   // we will need to clamp negative durations here.
>   // For now, if we're hitting this it probably means we've overflowing
>   // integer arithmetic in mozilla::TimeStamp.
>-  MOZ_ASSERT(aTiming.mIterationDuration >= TimeDuration(0),
>+  MOZ_ASSERT(aTiming.mIterationDuration >= zeroDuration,
>              "Expecting iteration duration >= 0");

(This comment/assertion don't seem to exist in this function in mozilla-central. Are they added in a pending patch on another bug? This bug should perhaps be marked as depending on that bug)

>+mozilla::TimeDuration
>+ElementAnimation::ActiveDuration(const AnimationTiming& aTiming) {

Drop the "mozilla::" prefix -- it's unneeded, since this code is inside of namespace mozilla.

Also: move the { down to its own line.
>diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
>+  // The total duration of the animation including any iterations.
>+  // Will equal TimeDuration::Forever() if the animation repeats indefinitely.
>+  mozilla::TimeDuration mActiveDuration;

I think this would be clearer with:
  s/any iterations/all iterations/
or perhaps:
  s/any iterations/any repetitions/

("any" makes it sound like iterations are something that may very well not be present, which is confusing, despite being sort of true (you can have an animation with 0 iterations, but that's rare))

Also: As above, this code is inside of namespace mozilla {}, so I think we can drop the mozilla:: prefix here.

>+  // Return the duration of the active interval for the given timing parameters.
>+  static mozilla::TimeDuration ActiveDuration(const AnimationTiming& aTiming);

As above, this code is inside of namespace mozilla {}, so I think we can drop the mozilla:: prefix here.

Also, a general comment: It looks like the only valid way to get a ComputedTiming struct is to call GetComputedTimingAt().  (If you just directly instantiate a ComputedTiming struct, it'll have zeroed-out member-vars, and a uninitialized "mPhase" variable.)  I think it's worth mentioning GetComputedTimingAt() in the ComputedTiming documentation, perhaps in a standalone patch (rs=me if you want to push that on its own as a DONTBUILD comment-tweak)

r=me with the above addressed
Attachment #8445004 - Flags: review?(dholbert) → review+
Comment on attachment 8445005 [details] [diff] [review]
part 2 - Make nsTransitionManager::FlushTransitions reuse GetComputedTimingAt

>+++ b/layout/style/nsTransitionManager.cpp
>-        } else if (anim->mStartTime + anim->mTiming.mDelay +
>-                   anim->mTiming.mIterationDuration <= now) {
>-          MOZ_ASSERT(anim->mProperties.Length() == 1,
>-                     "Should have one animation property for a transition");
>-          nsCSSProperty prop = anim->mProperties[0].mProperty;
>-          if (nsCSSProps::PropHasFlags(prop, CSS_PROPERTY_REPORT_OTHER_NAME))
>-          {
>-            prop = nsCSSProps::OtherNameFor(prop);
>+        } else {
>+          TimeDuration localTime = anim->GetLocalTimeAt(now);
>+          ComputedTiming computedTiming =
>+            ElementAnimation::GetComputedTimingAt(localTime, anim->mTiming);
>+          if (computedTiming.mPhase == ComputedTiming::AnimationPhase_After) {

So previously, we were treating
    anim->mStartTime + anim->mTiming.mDelay +
    anim->mTiming.mIterationDuration <= now
as a proxy for "Is this animation over?", which is *only* correct if anim->mTiming.mIterationCount is 1.0 (I think).

I'm betting that we can rely on mIterationCount being 1.0f here, since this is a transition, and transitions only run once.  Still, might be worth asserting/documenting that invariant somewhere, if we do in fact expect mIterationCount to be 1.0f.

>+          } else if (
>+            computedTiming.mPhase == ComputedTiming::AnimationPhase_Active &&
>+            canThrottleTick &&
>+            !anim->mIsRunningOnCompositor) {
>+            // Start a transition with a delay where we should start the
>+            // transition proper.
>+            et->UpdateAnimationGeneration(mPresContext);
>+            transitionStartedOrEnded = true;

Style nit: Let's bump "computedTiming.mPhase == " up to the previous line here, maybe with some parens around that equality-comparison, so that this looks more like:

          } else if ((computedTiming.mPhase ==
                      ComputedTiming::AnimationPhase_Active) &&
                     canThrottleTick &&
                    !anim->mIsRunningOnCompositor) {
            // Start a transition with a delay where we should start the
            // transition proper.
            et->UpdateAnimationGeneration(mPresContext);
            transitionStartedOrEnded = true;

That makes the condition easier to visually separate from the code.

r=me with the latter part (the style nit) addressed. (The first part, about mIterationCount being 1.0f, might want its own patch/bug... but if you think it makes sense to add an assertion about this to FlushTransitions() (the function touched by this patch), feel free to do so here.)
Attachment #8445005 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 8445004 [details] [diff] [review]
> part 1 - Move active duration calculation to GetComputedTimingAt
...
> >   // Currently we expect negative durations to be picked up during CSS
> >   // parsing but when we start receiving timing parameters from other sources
> >   // we will need to clamp negative durations here.
> >   // For now, if we're hitting this it probably means we've overflowing
> >   // integer arithmetic in mozilla::TimeStamp.
> >-  MOZ_ASSERT(aTiming.mIterationDuration >= TimeDuration(0),
> >+  MOZ_ASSERT(aTiming.mIterationDuration >= zeroDuration,
> >              "Expecting iteration duration >= 0");
> 
> (This comment/assertion don't seem to exist in this function in
> mozilla-central. Are they added in a pending patch on another bug? This bug
> should perhaps be marked as depending on that bug)

Sorry, that's bug 1028514. Fixed.

> Also: As above, this code is inside of namespace mozilla {}, so I think we
> can drop the mozilla:: prefix here.

I dropped the "mozilla::" in the .cpp file but in the .h file there is a lot of "mozilla::" going on so I'd rather file a separate patch that drops them all at once.

> Also, a general comment: It looks like the only valid way to get a
> ComputedTiming struct is to call GetComputedTimingAt().  (If you just
> directly instantiate a ComputedTiming struct, it'll have zeroed-out
> member-vars, and a uninitialized "mPhase" variable.)  I think it's worth
> mentioning GetComputedTimingAt() in the ComputedTiming documentation,
> perhaps in a standalone patch (rs=me if you want to push that on its own as
> a DONTBUILD comment-tweak)

Will do.

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Comment on attachment 8445005 [details] [diff] [review]
> part 2 - Make nsTransitionManager::FlushTransitions reuse GetComputedTimingAt
> 
> >+++ b/layout/style/nsTransitionManager.cpp
> >-        } else if (anim->mStartTime + anim->mTiming.mDelay +
> >-                   anim->mTiming.mIterationDuration <= now) {
> >-          MOZ_ASSERT(anim->mProperties.Length() == 1,
> >-                     "Should have one animation property for a transition");
> >-          nsCSSProperty prop = anim->mProperties[0].mProperty;
> >-          if (nsCSSProps::PropHasFlags(prop, CSS_PROPERTY_REPORT_OTHER_NAME))
> >-          {
> >-            prop = nsCSSProps::OtherNameFor(prop);
> >+        } else {
> >+          TimeDuration localTime = anim->GetLocalTimeAt(now);
> >+          ComputedTiming computedTiming =
> >+            ElementAnimation::GetComputedTimingAt(localTime, anim->mTiming);
> >+          if (computedTiming.mPhase == ComputedTiming::AnimationPhase_After) {
> 
> So previously, we were treating
>     anim->mStartTime + anim->mTiming.mDelay +
>     anim->mTiming.mIterationDuration <= now
> as a proxy for "Is this animation over?", which is *only* correct if
> anim->mTiming.mIterationCount is 1.0 (I think).
> 
> I'm betting that we can rely on mIterationCount being 1.0f here, since this
> is a transition, and transitions only run once.  Still, might be worth
> asserting/documenting that invariant somewhere, if we do in fact expect
> mIterationCount to be 1.0f.

That's correct. The previous code did rely on that and it is a reasonable assumption because, as you point out, transitions don't repeat. However, the code I'm replacing this with (GetComputedTimingAt) correctly handles repetitions. So even if we decide to make transitions repeatable in future this code should be correct. Hence, I don't think we want the assertion.
Depends on: 1028514
The code in question is already inside a namespace mozilla { ... } block so this
reference is largely redundant.
Attachment #8445570 - Flags: review?(dholbert)
RE the assertion -- you're right that we're removing the dependency on mIterationCount being 1 in this code, so the assertion doesn't matter as much for the logic here anymore.  I think it still might make sense to assert about that somewhere, though, as a sanity-check that we're not somehow letting CSS animations leak into nsTransitionManager, and that we're not somehow getting bogus transitions. (And if we someday decide to allow transitions to be repeated, we could drop the assertion at that point.)

Not a high priority, though, and maybe not worth it.
Comment on attachment 8445570 [details] [diff] [review]
part 3 - Remove unneeded reference to "mozilla::" inside AnimationCommon.h

Cancelling the review request for this. As discussed with dholbert on IRC, I'm concerned that removing the 'mozilla::' prefix could lead to conflicts due to the unified build system. Specifically, the concern is:

* If AnimationCommon.h has a reference to just, say, 'TimeStamp', and
* Some A.cpp file in layout/style has a declaration such as 'using mozilla::gfx::TimeStamp' (or 'using namespace mozilla::gfx' which happens to include TimeStamp), and
* This A.cpp file gets unified with another file, B.cpp, that includes AnimationCommon.h

then, we'll end up with a conflict when we get to including AnimationCommon.h since there will be two possibilities for resolving 'TimeStamp'.

On the other hand, if we have 'mozilla::TimeStamp' in the header file there won't be any ambiguity.

So it seems safer to leave the 'mozilla::' prefix in for now. This may change, if, for example, we require putting 'using namespace mozilla' at the start of every .cpp file since that would mean we'd probably hit the conflict in A.cpp instead of it mysteriously cropping up in B.cpp.
Attachment #8445570 - Flags: review?(dholbert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: