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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(4 files)
12.65 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.52 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
10.19 KB,
patch
|
Details | Diff | Splinter Review | |
11.41 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
This patch removes some redundant calculation of timing phases in FlushTransitions by calling GetComputedTiming and using the definitions there.
Attachment #8445005 -
Flags: review?(dholbert)
Assignee | ||
Comment 3•10 years ago
|
||
This is just for my sake to show how we could support clamping iteration duration inside GetComputedTimingAt
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
(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
Assignee | ||
Comment 7•10 years ago
|
||
The code in question is already inside a namespace mozilla { ... } block so this reference is largely redundant.
Attachment #8445570 -
Flags: review?(dholbert)
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b31a50d545f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/695c4dc0efc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/26d6c067ee0d (this extra commit and the end is the comment fix from comment 4)
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b31a50d545f7 https://hg.mozilla.org/mozilla-central/rev/695c4dc0efc7 https://hg.mozilla.org/mozilla-central/rev/26d6c067ee0d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•