According to Web Animation spec, AnimationEffectReadOnly have two member: interface AnimationEffectReadOnly { readonly attribute AnimationEffectTimingReadOnly timing; ComputedTimingProperties getComputedTiming(); }; 1. "ComputedTimingProperties" is being handled by Bug 1108055. 2. "AnimationEffectTimingReadOnly" is not yet implemented. In this bug, we should add this new interface, AnimationEffectTimingReadOnly, and try to merge it into KeyframeEffectReadOnly.

We want to store the original value from KeyframeEffectOptions whose iterations is unrestricted double. Therefore, we can get the original value of iterations by AnimationEffectTimingReadOnly.

We store the original value of duration, so AnimationEffectReadOnly could get it. Add some APIs to convert the original duraion into a valid real number, so the Timing model can use it to calculate other values correctly.

Add AnimationEffectTimingReadOnly.webidl

Override TimingFromJS in KeyframeEffectReadOnly.

FillMode could be 'auto', and we should treat it as 'none' in the timing model. However, AnimationEffectTimingReadOnly should get its original value. Fix typos.

For part 1, instead of adding AnimationTiming.AdjustedIterations(), why don't we just calculate the correct iteration value in GetComputedTimingAt? Unless there's a performance reason to do otherwise, I think it's simplest to just let GetComputedTimingAt be responsible for converting specified values into computed values.

Actually, never mind, I should follow up on the list to check whether this behavior is really necessary or not.

(In reply to Brian Birtles (:birtles) from comment #22) > Actually, never mind, I should follow up on the list to check whether this > behavior is really necessary or not. https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0038.html

Comment on attachment 8690654 [details] [diff] [review] Part 1: Use unrestricted double for iterations (v4) Review of attachment 8690654 [details] [diff] [review]: ----------------------------------------------------------------- Explaining comment 21, what I have in mind is this: ::: dom/animation/KeyframeEffect.cpp @@ +46,5 @@ > { > // AnimationEffectTimingProperties > aRetVal.mDelay = aTiming.mDelay.ToMilliseconds(); > aRetVal.mFill = aTiming.mFillMode; > + aRetVal.mIterations = aTiming.AdjustedIterations(); aRetVal.mIterations = aComputedTiming.mIterations; @@ +176,5 @@ > if (aLocalTime.IsNull()) { > return result; > } > const TimeDuration& localTime = aLocalTime.Value(); > + const double iterations = aTiming.AdjustedIterations(); result.mIterations = IsNaN(mIterationCount) || mIterationCount < 0.0f ? 1.0f : mIterationCount; @@ +197,5 @@ > // Note that infinity == floor(infinity) so this will also be true when we > // have finished an infinitely repeating animation of zero duration. > isEndOfFinalIteration = > + iterations != 0.0 && > + iterations == floor(iterations); s/iterations/result.mIterations/ here and through the rest of this function. @@ +287,5 @@ > > StickyTimeDuration > KeyframeEffectReadOnly::ActiveDuration(const AnimationTiming& aTiming) > { > + const double iterations = aTiming.AdjustedIterations(); This is a bit tricky, I guess we could pass in the adjusted iteration count as a separate parameter? ::: dom/animation/KeyframeEffect.h @@ +48,5 @@ > * Eventually this will represent all the input timing parameters specified > * by content but for now it encapsulates just the subset of those > * parameters passed to GetPositionInIteration. > */ > struct AnimationTiming I was thinking we should expand ComputedTiming and add the following member: double mIterations; @@ +53,5 @@ > { > TimeDuration mIterationDuration; > TimeDuration mDelay; > + // mIterationCount could be a real number, NaN, or +/-Infinity > + double mIterationCount; I think we should rename this to mIterations but that can be done in a separate patch if it makes this patch unclear. @@ +61,5 @@ > + // Return a valid iteration count which is a real number > + // greater than or equal to 0 (including positive infinity). > + double AdjustedIterations() const { > + return (IsNaN(mIterationCount) || mIterationCount < 0.0f) ? 1.0f : mIterationCount; > + } I think we should drop this function. ::: layout/base/nsDisplayList.cpp @@ +381,5 @@ > StickyTimeDuration(timing.mDelay)); > animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value() > - timing.mDelay; > animation->duration() = timing.mIterationDuration; > + animation->iterationCount() = timing.AdjustedIterations(); We should add a call here to: ComputedTiming computedTiming = aAnimation->GetEffect()->GetComputedTiming(); And use computedTiming.mIterationCount;

What do you think of doing it this way?

(In reply to Brian Birtles (:birtles) from comment #24) > @@ +287,5 @@ > > > > StickyTimeDuration > > KeyframeEffectReadOnly::ActiveDuration(const AnimationTiming& aTiming) > > { > > + const double iterations = aTiming.AdjustedIterations(); > > This is a bit tricky, I guess we could pass in the adjusted iteration count > as a separate parameter? On further though, I think we could make ActiveDuration() just take two parameters: aIterations and aIterationDuration. These would be the "adjusted" values.

Maybe the original patch is ok, so long as we rename AdjustedIterations to ComputedIterations.

Comment on attachment 8690654 [details] [diff] [review] Part 1: Use unrestricted double for iterations (v4) Sorry for the back and forth on this one. Ideally, it would be nice if our internal model reflected the Web Animations model--i.e. you store your specified times and then you have a "compute timing" step and the result is that all the values are normalized. However, I think the approach you have is ok, as long as we use the name "computed" rather than "adjusted" to reflect this step. If you don't mind reworking this to match comment 24, I'd prefer that, but if you think this is better, I'm ok with it as long as you make the following changes. >@@ -172,16 +172,17 @@ KeyframeEffectReadOnly::GetComputedTimingAt( > result.mActiveDuration = ActiveDuration(aTiming); > > // The default constructor for ComputedTiming sets all other members to > // values consistent with an animation that has not been sampled. > if (aLocalTime.IsNull()) { > return result; > } > const TimeDuration& localTime = aLocalTime.Value(); >+ const double iterations = aTiming.AdjustedIterations(); Perhaps call the local variable |computedIterations| ? > StickyTimeDuration > KeyframeEffectReadOnly::ActiveDuration(const AnimationTiming& aTiming) > { >- if (aTiming.mIterationCount == mozilla::PositiveInfinity<float>()) { >+ const double iterations = aTiming.AdjustedIterations(); >+ if (IsInfinite(iterations)) { I think this method would actually be better taking aIterations and aIterationDuration members and asserting that they are sensible values, but this is ok for now. >diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h >index 493db89..5966e00 100644 >--- a/dom/animation/KeyframeEffect.h >+++ b/dom/animation/KeyframeEffect.h >@@ -48,20 +48,27 @@ enum class CompositeOperation : uint32_t; > * Eventually this will represent all the input timing parameters specified > * by content but for now it encapsulates just the subset of those > * parameters passed to GetPositionInIteration. > */ > struct AnimationTiming > { > TimeDuration mIterationDuration; > TimeDuration mDelay; >- float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite >+ // mIterationCount could be a real number, NaN, or +/-Infinity >+ double mIterationCount; Let's call this mIterations and change the comment to something like: double mIterations; // Can be NaN, negative, +/-Infinity // Use ComputedIterations() to get the normalized value > dom::PlaybackDirection mDirection; > dom::FillMode mFillMode; > >+ // Return a valid iteration count which is a real number >+ // greater than or equal to 0 (including positive infinity). >+ double AdjustedIterations() const { >+ return (IsNaN(mIterationCount) || mIterationCount < 0.0f) ? 1.0f : mIterationCount; >+ } Let's call this ComputedIterations(); >diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp >index e1121c2..ee5e2a3 100644 >--- a/layout/base/nsDisplayList.cpp >+++ b/layout/base/nsDisplayList.cpp >@@ -377,17 +377,17 @@ AddAnimationForProperty(nsIFrame* aFrame, const AnimationProperty& aProperty, > Nullable<TimeDuration> startTime = aAnimation->GetCurrentOrPendingStartTime(); > animation->startTime() = startTime.IsNull() > ? TimeStamp() > : aAnimation->AnimationTimeToTimeStamp( > StickyTimeDuration(timing.mDelay)); > animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value() > - timing.mDelay; > animation->duration() = timing.mIterationDuration; >- animation->iterationCount() = timing.mIterationCount; >+ animation->iterationCount() = timing.AdjustedIterations(); It would be nice to update the definition in gfx/layers/ipc/LayersMessages.ipdlh as well to use 'iterations' but we can do that in a separate bug.

Comment on attachment 8691730 [details] [diff] [review] Part 2: Store the original value of fill (v3) Again, I'd prefer we added mFillMode to ComputedTiming and set it in GetComputedTimingAt. However, if you want to keep this approach, please rename AdjustedFill() to ComputedFill(). Also, we should rename mFillMode to mFill to match AnimationEffectTimingProperties.

Comment on attachment 8690694 [details] [diff] [review] Part 3: Use OwingUnrestrictedDoubleOrString for duration (v2) Review of attachment 8690694 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with the following changes. Again, I think I'd still prefer we do the computation in GetComputedTimingAt, but perhaps we can do that later as a separate step. ::: dom/animation/KeyframeEffect.cpp @@ +24,5 @@ > namespace mozilla { > > +TimeDuration > +AnimationTiming::AdjustedIterationDuration() const > +{ Let's call this ComputedDuration(). Also, I think this should return a StickyTimeDuration since it can be infinity and will often be combined with other values. @@ +178,5 @@ > const Nullable<TimeDuration>& aLocalTime, > const AnimationTiming& aTiming) > { > const TimeDuration zeroDuration; > + const TimeDuration duration = aTiming.AdjustedIterationDuration(); I suspect you'll want to make both zeroDuration and duration have type StickyTimeDuration. @@ +242,3 @@ > iterationTime = isEndOfFinalIteration > + ? StickyTimeDuration(duration) > + : activeTime % duration; You should be able to drop the StickyTimeDuration() here. @@ +321,2 @@ > } > + return StickyTimeDuration(duration.MultDouble(iterations)); You might be able to drop the StickTimeDuration wrapper here since duration should be a StickyTimeDuration. @@ +585,5 @@ > animationTiming.mFillMode = opt.mFill; > } else { > + double dur = aOptions.IsUnrestrictedDouble() ? > + aOptions.GetAsUnrestrictedDouble() : > + 0.0f; I think you should line up these last two lines like: double dur = aOptions.IsUnrestrictedDouble() ? aOptions.GetAsUnrestrictedDouble() : 0.0; ::: dom/animation/KeyframeEffect.h @@ +51,5 @@ > * parameters passed to GetPositionInIteration. > */ > struct AnimationTiming > { > + dom::OwningUnrestrictedDoubleOrString mIterationDuration; Call it mDuration to match AnimationEffectTimingProperties? @@ +62,5 @@ > + // Return a valid iteration duration as a TimeDuration object. > + TimeDuration AdjustedIterationDuration() const; > + void SetIterationDuration(const double aDuration) { > + mIterationDuration.SetAsUnrestrictedDouble() = aDuration; > + } s/AdjustedIterationDuration()/ComputedDuration()/ s/SetIterationDuration()/SetDuration()/ Also, no need for 'const double'. It's passed by-value. It seems to me like it would be easier to read this if we grouped the methods together with the members they modify, e.g. // iterations double mIterations; double ComputedIterations() const { return (IsNaN(mIterationCount) || mIterationCount < 0.0f) ? 1.0f : mIterationCount; } // duration dom::OwningUnrestrictedDoubleOrString mDuration; StickyTimeDuration ComputedDuration() const; void SetDuration(double aDuration) { mDuration.SetAsUnrestrictedDouble() = aDuration; } // fill dom::FillMode mFill; dom::FillMode ComputedFill() const; bool FillsForwards() const; bool FillsBackwards() const; @@ +90,5 @@ > + !mIterationDuration.IsString() && > + !aOther.mIterationDuration.IsUnrestrictedDouble() && > + !aOther.mIterationDuration.IsString(); > + } > + return durEqual && Why not write this as: bool durationEqual; if (mDuration.IsUnrestrictedDouble()) { durationEqual = aOther.mDuration.IsUnrestrictedDouble() && (mDuration.GetAsUnrestrictedDouble() == aOther.mDuration.GetAsUnrestrictedDouble()); } else if (mDuration.IsString()) { durationEqual = aOther.mDuration.IsString() && (mDuration.GetAsString() == aOther.mDuration.GetAsString()); } else { // Check if both are uninitialized durationEqual = !aOther.mDuration.IsUnrestrictedDouble() && !aOther.mDuration.IsString(); } return durationEqual && mDelay == aOther.mDelay && mIterationCount == aOther.mIterationCount && mDirection == aOther.mDirection && mFillMode == aOther.mFillMode; Optionally, you could factor the first part out into a separate method, CompareUnrestrictedDoubleOrString.

Comment on attachment 8690654 [details] [diff] [review] Part 1: Use unrestricted double for iterations (v4) > struct AnimationTiming > { > TimeDuration mIterationDuration; > TimeDuration mDelay; >- float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite >+ // mIterationCount could be a real number, NaN, or +/-Infinity >+ double mIterationCount; Any reason to change this to double?

(In reply to Brian Birtles (:birtles) from comment #31) > Comment on attachment 8690654 [details] [diff] [review] > Part 1: Use unrestricted double for iterations (v4) > > > struct AnimationTiming > > { > > TimeDuration mIterationDuration; > > TimeDuration mDelay; > >- float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite > >+ // mIterationCount could be a real number, NaN, or +/-Infinity > >+ double mIterationCount; > > Any reason to change this to double? Just want to make sure mIterationCount has the same type as iterations in AnimationEffectTimingProperties (unresricted double). BTW, I will try to use your solution to make sure which one has clearer code. Thanks for your review.

(In reply to Brian Birtles (:birtles) from comment #25) > What do you think of doing it this way? Your solution is also good to me. I think the overhead is that we have to call GetEffect()->GetComputedTiming() in other places (for example: nsDisplayList.cpp) and I'm not sure the overhead of GetComputedTiming(). If it's fast, I think your solution is great (which keeps all computed values in one struct), and I am trying to revise them. Thanks.

We want to store the original value from KeyframeEffectOptions whose iterations is unrestricted double. Therefore, we can get the original value of iterations by AnimationEffectTimingReadOnly. By the way, replace mIterationCount with mIterations.

Replace mIterationCount with mIteraions in StyleAnimation and layers::Animation

FillMode could be 'auto', and we should treat it as 'none' in the timing model. However, AnimationEffectTimingReadOnly should get its original value. By the way, replace mFillMode with mFill.

We store the original value of duration in AnimationTiming, and add computed duration in ComputedTiming, so both the Timing model and AnimationEffectTimingReadOnly can get what they want. By the way, replace mIterationDuration with mDuration.

Add AnimationEffectTimingReadOnly.webidl

Comment on attachment 8692418 [details] [diff] [review] Part 1: Use unrestricted double for iterations (v5) Review of attachment 8692418 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I changed the design, so need your review again. Thanks.

Override TimingFromJS in KeyframeEffectReadOnly.

Comment on attachment 8692418 [details] [diff] [review] Part 1: Use unrestricted double for iterations (v5) Review of attachment 8692418 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.h @@ +88,5 @@ > Nullable<double> mProgress; > // Zero-based iteration index (meaningless if mProgress is null). > uint64_t mCurrentIteration = 0; > + // Computed iterations > + double mIterations = 1.0; No need to say "computed", we're in the "ComputedTiming" struct. Perhaps instead: // Unlike AnimationTiming::mIterations, this value is guaranteed to be in the // range [0, Infinity]. ::: layout/style/nsTransitionManager.cpp @@ +656,5 @@ > > AnimationTiming timing; > timing.mIterationDuration = TimeDuration::FromMilliseconds(duration); > timing.mDelay = TimeDuration::FromMilliseconds(delay); > + timing.mIterations = 1.0; As suggested in bug 1215406 comment 56, we should probably add a default constructor for AnimationTiming that sets these values to sensible defaults (probably using the dictionary defaults defined for AnimationEffectTimingProperties)--then we could simplify this code and ConvertKeyframeEffectOptions. That's a separate patch/bug however.

Comment on attachment 8692419 [details] [diff] [review] Part 2: Replace mIterationCount with mIterations (v1) Review of attachment 8692419 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we should change StyleAnimation since it maps to the animation-iteration-count property. The changes to AsyncCompositionManager.cpp and LayersMessages.ipdlh look fine however.

Comment on attachment 8692423 [details] [diff] [review] Part 3: Store the original value of fill (v4) Review of attachment 8692423 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't seem right. If we are going to store dom::FillMode::Auto on AnimationTiming then AnimationTiming::FillsForwards() and FillBackwards() need to handle it. It's probably better to move FillsForwards and FillsBackwards to ComputedTiming and, inside ComputedTiming::FillsForwards/Backwards(), assert that mFill is not Auto. We should also add a default value for mFill in ComputedTiming (probably none would be ok) so that when FillsFowards/FillsBackwards is called mFill is guaranteed to be set to a known value.

Comment on attachment 8692435 [details] [diff] [review] Part 4: Use OwingUnrestrictedDoubleOrString for duration (v4) Review of attachment 8692435 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed ::: dom/animation/KeyframeEffect.cpp @@ +161,4 @@ > > + // Always return the same object to benefit from return-value optimization. > + ComputedTiming result; > + if (aTiming.mDuration.IsUnrestrictedDouble()) { Add a space before this last line otherwise it appears that the comment, "Always return..." applies to this line too. @@ +163,5 @@ > + ComputedTiming result; > + if (aTiming.mDuration.IsUnrestrictedDouble()) { > + double durationMs = aTiming.mDuration.GetAsUnrestrictedDouble(); > + if (!IsNaN(durationMs) && durationMs >= 0.0f) { > + result.mDuration = TimeDuration::FromMilliseconds(durationMs); Shouldn't this by StickyTimeDuration::FromMilliseconds? @@ +176,1 @@ > "Expecting iteration duration >= 0"); We can drop this assertion and the comment now since we ensure this is true in the lines above. @@ +269,2 @@ > "In the active phase of a zero-duration animation?"); > + double progress = result.mDuration == TimeDuration::Forever() Nit: It might be slightly more natural to use StickyTimeDuration::Forever()? @@ +313,2 @@ > } > + return StickyTimeDuration(aComputedDuration.MultDouble(aComputedIterations)); Do we need the StickyTimeDuration() here? Doesn't MultDouble return the right kind of object for us? @@ +583,5 @@ > animationTiming.mDelay = TimeDuration(0); > animationTiming.mIterations = 1.0f; > animationTiming.mDirection = PlaybackDirection::Normal; > animationTiming.mFill = FillMode::None; > } Again, if we give AnimationTiming a default constructor we can change this whole branch to just: } else if (aOptions.IsUnrestrictedDouble()) { animationTiming.mDuration.SetAsUnrestrictedDouble() = aOptions.GetAsUnrestrictedDouble(); } ::: dom/animation/KeyframeEffect.h @@ +107,5 @@ > double mIterations = 1.0; > // Computed fill > dom::FillMode mFill; > + // Computed duration > + StickyTimeDuration mDuration; Once again, no need to repeat "computed" here. The type tells you that the duration is not "auto" so I think we can simply leave this without a comment. @@ +271,5 @@ > // Return the duration of the active interval for the given timing parameters. > static StickyTimeDuration > ActiveDuration(const AnimationTiming& aTiming, > + double aComputedIterations, > + const StickyTimeDuration& aComputedDuration); I don't think we need the aTiming parameter any more, right? (I also think it would be slightly more natural to put the duration parameter first.) And perhaps we can update the comment too, e.g.: // Return the duration of the active interval for the given duration and // iteration count. ActiveDuration(const StickyTimeDuration& aIterationDuration, double aIterationCount); ::: layout/style/nsTransitionManager.cpp @@ +148,5 @@ > > nsTransitionManager* manager = presContext->TransitionManager(); > manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType, > TransitionProperty(), > + mEffect->GetComputedTiming().mDuration, I think this line may be over 80 characters long.

Comment on attachment 8692436 [details] [diff] [review] Part 5: Add AnimationEffectTimingReadOnly interface (v1) Review of attachment 8692436 [details] [diff] [review]: ----------------------------------------------------------------- AnimationEffectTimingReadOnly is supposed to be a live object, i.e. should reflect the current values. We should just use AnimationEffectTimingReadOnly in place of AnimationTiming.

Comment on attachment 8692441 [details] [diff] [review] Part 6: Implement TimingFromJS (v2) Review of attachment 8692441 [details] [diff] [review]: ----------------------------------------------------------------- Based on my comments for part 5, this patch no longer makes sense.

(In reply to Brian Birtles (:birtles) from comment 42 > ::: layout/style/nsTransitionManager.cpp > As suggested in bug 1215406 comment 56, we should probably add a default > constructor for AnimationTiming that sets these values to sensible defaults > (probably using the dictionary defaults defined for > AnimationEffectTimingProperties)--then we could simplify this code and > ConvertKeyframeEffectOptions. > > That's a separate patch/bug however. OK. I will handle this later.

(In reply to Brian Birtles (:birtles) from comment #44) > Comment on attachment 8692423 [details] [diff] [review] > Part 3: Store the original value of fill (v4) > > Review of attachment 8692423 [details] [diff] [review]: > ----------------------------------------------------------------- > > This doesn't seem right. If we are going to store dom::FillMode::Auto on > AnimationTiming then AnimationTiming::FillsForwards() and FillBackwards() > need to handle it. > > It's probably better to move FillsForwards and FillsBackwards to > ComputedTiming and, inside ComputedTiming::FillsForwards/Backwards(), assert > that mFill is not Auto. We should also add a default value for mFill in > ComputedTiming (probably none would be ok) so that when > FillsFowards/FillsBackwards is called mFill is guaranteed to be set to a > known value. Oops, I didn't notice this part. Thanks for your rectification.

We want to store the original value from KeyframeEffectOptions whose iterations is unrestricted double. Therefore, we can get the original value of iterations by AnimationEffectTimingReadOnly. By the way, replace mIterationCount with mIterations.

Replace mIterationCount with mIterations.

FillMode could be 'auto', and we should treat it as 'none' in the timing model. However, AnimationEffectTimingReadOnly should get its original value. By the way, replace mFillMode with mFill.

Comment on attachment 8692738 [details] [diff] [review] Part 3: Store the original value of fill (v5) Review of attachment 8692738 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed ::: dom/animation/KeyframeEffect.cpp @@ +26,5 @@ > +ComputedTiming::ComputedTiming() > + : mCurrentIteration(0) > + , mIterations(0.0) > + , mFill(dom::FillMode::None) > + , mPhase(AnimationPhase::Null) Instead of adding a non-inline constructor, I think the member-initializer format is fine (and more likely to be inlined). Just add the initialization of mFill. @@ +28,5 @@ > + , mIterations(0.0) > + , mFill(dom::FillMode::None) > + , mPhase(AnimationPhase::Null) > +{ > + mProgress.SetNull(); This is not needed, Nullable initializes to a null value. @@ +37,2 @@ > { > + MOZ_ASSERT(mFill == dom::FillMode::Auto, "mFill couldn't be Auto."); "mFill should not be Auto in ComputedTiming" @@ +45,2 @@ > { > + MOZ_ASSERT(mFill == dom::FillMode::Auto, "mFill couldn't be Auto."); Ditto. ::: dom/animation/KeyframeEffect.h @@ +91,5 @@ > // Unlike AnimationTiming::mIterations, this value is guaranteed to be in the > // range [0, Infinity]. > + double mIterations; > + > + // This is guaranteed to be None, Forwards, Backwards, or Both. "This is the computed fill mode so it is number auto"

(In reply to Brian Birtles (:birtles) from comment #53) > Comment on attachment 8692738 [details] [diff] [review] > Part 3: Store the original value of fill (v5) > > Review of attachment 8692738 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=birtles with comments addressed > > ::: dom/animation/KeyframeEffect.cpp > @@ +26,5 @@ > > +ComputedTiming::ComputedTiming() > > + : mCurrentIteration(0) > > + , mIterations(0.0) > > + , mFill(dom::FillMode::None) > > + , mPhase(AnimationPhase::Null) > > Instead of adding a non-inline constructor, I think the member-initializer > format is fine (and more likely to be inlined). Just add the initialization > of mFill. Actually, the reason is: I don't want to include the definition of dom::FillMode in KeyframeEffect.h to workaround some compilation errors on Linux. Therefore, add a non-inline constructor here. > > @@ +28,5 @@ > > + , mIterations(0.0) > > + , mFill(dom::FillMode::None) > > + , mPhase(AnimationPhase::Null) > > +{ > > + mProgress.SetNull(); > > This is not needed, Nullable initializes to a null value. OK. I will remove this.

(In reply to Boris Chiou [:boris] from comment #54) > > Instead of adding a non-inline constructor, I think the member-initializer > > format is fine (and more likely to be inlined). Just add the initialization > > of mFill. > > Actually, the reason is: I don't want to include the definition of > dom::FillMode in KeyframeEffect.h to workaround some compilation errors on > Linux. Therefore, add a non-inline constructor here. You can't just #undef None?

(In reply to Brian Birtles (:birtles) from comment #55) > (In reply to Boris Chiou [:boris] from comment #54) > > > Instead of adding a non-inline constructor, I think the member-initializer > > > format is fine (and more likely to be inlined). Just add the initialization > > > of mFill. > > > > Actually, the reason is: I don't want to include the definition of > > dom::FillMode in KeyframeEffect.h to workaround some compilation errors on > > Linux. Therefore, add a non-inline constructor here. > > You can't just #undef None? I can #undef None if it's necessary to include the definition in KeyframeEffect.h. I will upload a new patch soon to let you review again. Thanks.

FillMode could be 'auto', and we should treat it as 'none' in the timing model. However, AnimationEffectTimingReadOnly should get its original value. By the way, replace mFillMode with mFill.

Comment on attachment 8692786 [details] [diff] [review] Part 3: Store the original value of fill (v6) Review of attachment 8692786 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed ::: dom/animation/KeyframeEffect.h @@ +24,5 @@ > #include "mozilla/dom/Nullable.h" > > +// Fix X11 header brain damage that conflicts with dom::FillMode::None > +#undef None > +#include "mozilla/dom/AnimationEffectReadOnlyBinding.h" Just something like: // X11 has a #define for None #ifdef None #undef None #endif would be fine :) (Similar to what we have in Animation.h) @@ +92,5 @@ > // Unlike AnimationTiming::mIterations, this value is guaranteed to be in the > // range [0, Infinity]. > double mIterations = 1.0; > > + // This is the computed fill mode so it is number auto Sorry, I made a mistake in my original comment. I meant "never auto" (not "number auto"). @@ +96,5 @@ > + // This is the computed fill mode so it is number auto > + dom::FillMode mFill = dom::FillMode::None; > + bool FillsForwards() const { > + MOZ_ASSERT(mFill == dom::FillMode::Auto, > + "mFill should not be Auto in ComputedTiming"); mFill != dom::FillMode::Auto @@ +101,5 @@ > + return mFill == dom::FillMode::Both || > + mFill == dom::FillMode::Forwards; > + } > + bool FillsBackwards() const { > + MOZ_ASSERT(mFill == dom::FillMode::Auto, Same here

FillMode could be 'auto', and we should treat it as 'none' in the timing model. However, AnimationEffectTimingReadOnly should get its original value. By the way, replace mFillMode with mFill.

We store the original value of duration in AnimationTiming, and add computed duration in ComputedTiming, so both the Timing model and AnimationEffectTimingReadOnly can get what they want. By the way, replace mIterationDuration with mDuration.

(In reply to Brian Birtles (:birtles) from comment #46) > Comment on attachment 8692436 [details] [diff] [review] > Part 5: Add AnimationEffectTimingReadOnly interface (v1) > > Review of attachment 8692436 [details] [diff] [review]: > ----------------------------------------------------------------- > > AnimationEffectTimingReadOnly is supposed to be a live object, i.e. should > reflect the current values. We should just use AnimationEffectTimingReadOnly > in place of AnimationTiming. Should I try to replace AnimationTiming with AnimationEffectTimingReadOnly directly or let AnimationEffectTimingReadOnly has a data member of AnimationTiming? I'm not sure whether we should keep AnimationTiming or not.

(In reply to Brian Birtles (:birtles) from comment #42) > As suggested in bug 1215406 comment 56, we should probably add a default > constructor for AnimationTiming that sets these values to sensible defaults > (probably using the dictionary defaults defined for > AnimationEffectTimingProperties)--then we could simplify this code and > ConvertKeyframeEffectOptions. > > That's a separate patch/bug however. If we replace AnimationTiming with AnimationEffectTimingReadOnly, we could add a constructor in AnimoationEffectTimingReadOnly to simplify these code.

(In reply to Boris Chiou [:boris] from comment #61) > Should I try to replace AnimationTiming with AnimationEffectTimingReadOnly > directly or let AnimationEffectTimingReadOnly has a data member of > AnimationTiming? I'm not sure whether we should keep AnimationTiming or not. At first I thought we should replace AnimationTiming with AnimationEffectTimingReadOnly. However, I think we create AnimationTiming objects on the compositor in order to pass to GetComputedTimingAt. It would be unfortunate to have to create a new heap object for every animation on the compositor for every tick just to calculate the current time. So probably we want to keep a lightweight object AnimationTiming and pass that to GetComputedTimingAt. Then AnimationEffectTimingReadOnly just needs to (non-owning) pointer back to its effect and return values from its effect. The difficulty is orphaned objects. AnimationEffectTimingReadOnly objects should probably not keep their associated effect alive but script can keep a handle to one of these objects even after the Animation is garbage-collected. (Furthermore, in future it should be possible to construct AnimationEffectTimingReadOnly objects without an effect so the same situation can arise there). There are probably a few different approaches: 1. Make AnimationEffectTimingReadOnly work in two different modes: one where it reads timing properties from an associated effect; and one, an orphan mode, where it uses local timing properties. This is what we did a lot in the SVG DOM and I think we decided it was too complex in the end. The advantage is we would only create the AnimationEffectTimingReadOnly object on-demand: most of the time it would not be needed. 2. Make AnimationEffectTimingReadOnly store all its timing properties locally (i.e. it has an mTiming member of type AnimationTiming) and when we call GetComputedTimingAt, we basically look up mTiming->mTiming. There are probably a number of other ways. For some approaches we can probably use the WebIDL bindings to take care of tracking the tear-off object for us.

Just to outline the first approach a bit more, we'd basically make an timing tear-off object with a raw-pointer to an AnimationTiming object and an orphan flag. When the timing object is attached to an effect, it will be passed a pointer to the effect's AnimationTiming object. When the effect is destroyed, it tells the tear-off object which allocates a new AnimationTiming object on the heap with a copy of the effect's AnimationTiming, updates it pointer to point to the new heap object, and set the orphan flag. When the tear-off object is destroyed, it checks the orphan flag, and if it is set, frees the memory for the heap-allocated timing object. That's pretty much how we do the SVG DOM stuff but in this case it's quite simple because we already have the AnimationTiming object and the tear-off object is read-only.

(In reply to Brian Birtles (:birtles) from comment #64) > Just to outline the first approach a bit more, we'd basically make an timing > tear-off object with a raw-pointer to an AnimationTiming object and an > orphan flag. > > When the timing object is attached to an effect, it will be passed a pointer > to the effect's AnimationTiming object. When the effect is destroyed, it > tells the tear-off object which allocates a new AnimationTiming object on > the heap with a copy of the effect's AnimationTiming, updates it pointer to > point to the new heap object, and set the orphan flag. When the tear-off > object is destroyed, it checks the orphan flag, and if it is set, frees the > memory for the heap-allocated timing object. > > That's pretty much how we do the SVG DOM stuff but in this case it's quite > simple because we already have the AnimationTiming object and the tear-off > object is read-only. Your explanation is helpful. However, I'm not familiar with SVG DOM stuffs, so I want to make sure if I catch on this idea: 1. When someone calls KeyframeEffectReadOnly::Timing(), we new an AnimationEffectTimingReadOnly object which accesses the AnimationTiming object in KeyframeEffectReadOnly. (Now, the timing object is attached to an effect.) 2. KeyframeEffectReadOnly asks this AnimationEffectTimingReadOnly object to allocates a new AnimationTiming object on the heap in the destroctor (i.e. in ~KeyframeEffectReadOnly()). (Now, the timing object is an orphan.) I'm not sure who should own this AnimationEffectTimingReadOnly object. When we create a new different effect, should it find an orphan timing object and let it to be attached? Thanks.

There are a lot of ways you could do it. One way might be something like: class KeyframeEffectReadOnly { ~KeyframeEffectReadOnly() { if (mTiming) { mTiming.Orphan(); } } RefPtr<AnimationEffectTimingReadOnly> Timing() { // (might need some const-cast / mutable annotations if Timing() is const) if (!mTiming) { mTiming = new AnimationEffectTimingReadOnly(mTimingParams); } return mTiming; } AnimationTiming mTimingParams; RefPtr<AnimationEffectTimingReadOnly> mTiming; } class AnimationEffectTimingReadOnly { public: explicit AnimationEffectTimingReadOnly(AnimationTiming& aTiming) : mTiming(&aTiming) { } void Orphan() { if (mLocalTiming && !mTiming) { mLocalTiming = new AnimationTiming(*mLocalTiming); mTiming = &mLocalTiming; } } // e.g. PlaybackDirection Direction() const { return mTiming->mDirection; } protected: AnimationTiming* mTimingParams; nsAutoPtr<AnimationTiming> mLocalTiming; } You might be able to use something like [Constant, Cached] on the WebIDL and re-use that to track the mTiming object? I'm not sure. Alternatively, you could create the tear-off object from the start, something like: class KeyframeEffectReadOnly { KeyframeEffectReadOnly() : mTiming(new AnimationEffectTimingReadOnly()) { } RefPtr<AnimationEffectTimingReadOnly> Timing() { return mTiming; } RefPtr<AnimationEffectTimingReadOnly> mTiming; } class AnimationEffectTimingReadOnly { public: // e.g. PlaybackDirection Direction() const { return mTiming.mDirection; } AnimationTiming mTiming; } The second approach seems more simple. The main disadvantage is an extra allocation for every animation. I think when we implement writeable timing objects, and replaceable timing objects, the second approach is going to be more simple. So I think perhaps the second approach is better here.

(In reply to Brian Birtles (:birtles) from comment #66) > The second approach seems more simple. The main disadvantage is an extra > allocation for every animation. > > I think when we implement writeable timing objects, and replaceable timing > objects, the second approach is going to be more simple. So I think perhaps > the second approach is better here. Very clear :). Yes, second one is more intuitive. If the memory allocation is not critical, I would try to implement 2nd idea. Thanks.

(In reply to Brian Birtles (:birtles) from comment #66) > The second approach seems more simple. The main disadvantage is an extra > allocation for every animation. > > I think when we implement writeable timing objects, and replaceable timing > objects, the second approach is going to be more simple. So I think perhaps > the second approach is better here. In the second approach, mTiming is a read-only object, but we still need to provide an API to reset the timing object (SetTiming(...)[1]). If we just wrap AnimationTiming by AnimationEffectTimingReadOnly, we can not revise it as before. I think We should use a workaround method to handle this case or remove the API. [1]https://dxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#493

(In reply to Boris Chiou [:boris] (away 12/1~12/4) from comment #68) > In the second approach, mTiming is a read-only object, but we still need to > provide an API to reset the timing object (SetTiming(...)[1]). If we just > wrap AnimationTiming by AnimationEffectTimingReadOnly, we can not revise it > as before. I think We should use a workaround method to handle this case or > remove the API. I'm not sure I understand the concern. I think the simplest way to organize this is as follows: * KeyframeEffectReadOnly (or AnimationEffectReadOnly if you decide to do it there) *always* has a valid mTiming object. It is a non-nullable readonly attribute in the IDL so I think it makes sense to initialize mTiming to a new object when you create an AnimationEffectReadOnly object. * KeyframeEffectReadOnly::Timing() always returns that object (we can use [Cached, Constant] in the WebIDL to enforce this, I believe) * In nsAnimationManager etc., when you want to update the timing properties, call effect->Timing().mTiming = timingParams * You could even implement the assignment operator or something similar so that when we copy timing from one effect to the other you can do: *oldEffect->Timing() = *newEffect->Timing() (* You might find it easier to rename AnimationTiming to AnimationEffectTimingStruct? or something so that the names don't get too confusing?) * You could also move some of the code for handling KeyframeEffectOptions to this object in order to simplify KeyframeEffectReadOnly. For example, something like: struct AnimationEffectTimingStruct { // Technically, we should initialize mDuration to "auto", but that will mean // allocating an extra string buffer for every object, even if that value is // to be destroyed straight away. // // For now we know that every code path that generates an // AnimationEffectTimingStruct will also end up setting mDuration so we // can just leave it unitialized. In future, when we implement a constructor // AnimationEffectTimingReadOnly we might investigate other ways of storing // duration so that we don't need a string to represent "auto". // Perhaps we could even use the unitialized state to represent "auto"? dom::OwningUnrestrictedDoubleOrString mDuration; TimeDuration mIterationDuration; TimeDuration mDelay; double mIterations = 1.0; dom::PlaybackDirection mDirection = dom::PlaybackDirection::Normal; dom::FillMode mFill = dom::FillMode::Auto; bool operator==(const AnimationTiming& aOther) const { ... } bool operator!=(const AnimationTiming& aOther) const { ... } }; class AnimationEffectTimingReadOnly { public: SetTiming(const AnimationEffectTimingProperties& aTiming); /* The following is not needed if we leave mTiming as public */ SetTiming(const AnimationEffectTimingStruct& aTiming); AnimationEffectTimingReadOnly& operator= (const AnimationEffectTimingReadOnly& aOther); AnimationEffectTimingStruct mTiming; };

(In reply to Brian Birtles (:birtles) from comment #69) > (In reply to Boris Chiou [:boris] (away 12/1~12/4) from comment #68) > > In the second approach, mTiming is a read-only object, but we still need to > > provide an API to reset the timing object (SetTiming(...)[1]). If we just > > wrap AnimationTiming by AnimationEffectTimingReadOnly, we can not revise it > > as before. I think We should use a workaround method to handle this case or > > remove the API. > > I'm not sure I understand the concern. > > I think the simplest way to organize this is as follows: > > * KeyframeEffectReadOnly (or AnimationEffectReadOnly if you decide to do it > there) *always* has a valid mTiming object. It is a non-nullable readonly > attribute in the IDL so I think it makes sense to initialize mTiming to a > new object when you create an AnimationEffectReadOnly object. > * KeyframeEffectReadOnly::Timing() always returns that object (we can use > [Cached, Constant] in the WebIDL to enforce this, I believe) > * In nsAnimationManager etc., when you want to update the timing properties, > call effect->Timing().mTiming = timingParams Sorry for replying lately. Thanks for your instructions step by step. It's really helpful. I think my question is about this part: what does "readonly" mean in the webidl language. Could we revise the data member of this "readonly" attribute by any assignment? Thanks. > * You could even implement the assignment operator or something similar so > that when we copy timing from one effect to the other you can do: > *oldEffect->Timing() = *newEffect->Timing() > (* You might find it easier to rename AnimationTiming to > AnimationEffectTimingStruct? or something so that the names don't get too > confusing?) > * You could also move some of the code for handling KeyframeEffectOptions to > this object in order to simplify KeyframeEffectReadOnly.

(In reply to Boris Chiou [:boris] (away 12/14~12/17) from comment #70) > (In reply to Brian Birtles (:birtles) from comment #69) > > * KeyframeEffectReadOnly (or AnimationEffectReadOnly if you decide to do it > > there) *always* has a valid mTiming object. It is a non-nullable readonly > > attribute in the IDL so I think it makes sense to initialize mTiming to a > > new object when you create an AnimationEffectReadOnly object. > > * KeyframeEffectReadOnly::Timing() always returns that object (we can use > > [Cached, Constant] in the WebIDL to enforce this, I believe) > > * In nsAnimationManager etc., when you want to update the timing properties, > > call effect->Timing().mTiming = timingParams > > Sorry for replying lately. Thanks for your instructions step by step. It's > really helpful. > I think my question is about this part: what does "readonly" mean in the > webidl language. Could we revise the data member of this "readonly" > attribute by any assignment? Thanks. In WebIDL, readonly means you can't replace the attribute's value with a new object. For example, if you have: interface A { readonly attribute B b; }; interface B { attribute DOMString string; }; Then, if you have an object |a|, that implements interface A, you can do: a.b.string = "test"; // ok However, you can't do: a.b = a.b; // TypeError exception thrown in strict mode So in this case you can *never* do: effectA.timing = effectB.timing // TypeError! But, if effectA is a KeyframeEffect object, then its 'timing' attribute will be a AnimationEffectTiming object so you *can* do: effectA.timing.duration = 3000; // Ok if 'timing' is an AnimationEffectTiming object

Add AnimationEffectTimingReadOnly.webidl

We store the original value of duration in AnimationTiming, and add computed duration in ComputedTiming, so both the Timing model and AnimationEffectTimingReadOnly can get what they want. By the way, replace mIterationDuration with mDuration. Rebased.

Add AnimationEffectTimingReadOnly.h/cpp and move AnimationTiming into AnimatinoRffectTimingReadOnly.h. Rename AnimationTiming as AnimationEffectTimingStruct.

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into AnimationEffectTimingReadOnly.

Replace AnimationEffectTimingStruct with AnimationEffectTimingReadOnly in KeyframeEffectReadOnly and implement KeyframeEffectReadOnly::Timing().

Comment on attachment 8699824 [details] [diff] [review] Part 5: Add AnimationEffectTimingReadOnly interface (v3) Looks fine to me but obviously you need a DOM peer to review this. Does this patch compile, however? Don't you need at least a stub implementation?

(In reply to Brian Birtles (:birtles) from comment #90) > Comment on attachment 8699824 [details] [diff] [review] > Part 5: Add AnimationEffectTimingReadOnly interface (v3) > > Looks fine to me but obviously you need a DOM peer to review this. > > Does this patch compile, however? Don't you need at least a stub > implementation? I put the other part in Part 9. I can move them back to this part and then send a review request to DOM peer. Thanks.

(In reply to Boris Chiou [:boris] from comment #91) > (In reply to Brian Birtles (:birtles) from comment #90) > > Comment on attachment 8699824 [details] [diff] [review] > > Part 5: Add AnimationEffectTimingReadOnly interface (v3) > > > > Looks fine to me but obviously you need a DOM peer to review this. > > > > Does this patch compile, however? Don't you need at least a stub > > implementation? > > I put the other part in Part 9. I can move them back to this part and then > send a review request to DOM peer. Thanks. Ideally, you should be able to apply each patch in turn and be able to compile and pass all tests. That makes finding regressions a lot easier.

Comment on attachment 8700502 [details] [diff] [review] Part 9: Implement Timing() in KeyframeEffectReadOnly (v2) Review of attachment 8700502 [details] [diff] [review]: ----------------------------------------------------------------- I will move the changes in AnimationEffectReadOnly.h/cpp into part 5. Thanks.

Add AnimationEffectTimingReadOnly.webidl Move stuff implementation into this part.

Comment on attachment 8701676 [details] [diff] [review] Part 5: Add AnimationEffectTimingReadOnly interface (v4) >diff --git a/dom/animation/AnimationEffectReadOnly.cpp b/dom/animation/AnimationEffectReadOnly.cpp >index 2363b31..673bc7d 100644 >--- a/dom/animation/AnimationEffectReadOnly.cpp >+++ b/dom/animation/AnimationEffectReadOnly.cpp >@@ -15,10 +15,16 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationEffectReadOnly, mParent) > NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationEffectReadOnly) > NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationEffectReadOnly) > > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationEffectReadOnly) > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > NS_INTERFACE_MAP_ENTRY(nsISupports) > NS_INTERFACE_MAP_END > >+already_AddRefed<AnimationEffectTimingReadOnly> >+AnimationEffectReadOnly::Timing() const >+{ >+ return RefPtr<AnimationEffectTimingReadOnly>().forget(); I don't understand how this works. Where do we add the connection to KeyframeEffect's mTiming member? Also, does this compile? Or are you going to rework all the other patches?

(In reply to Brian Birtles (:birtles) from comment #96) > Comment on attachment 8701676 [details] [diff] [review] > Part 5: Add AnimationEffectTimingReadOnly interface (v4) > > >diff --git a/dom/animation/AnimationEffectReadOnly.cpp b/dom/animation/AnimationEffectReadOnly.cpp > >index 2363b31..673bc7d 100644 > >--- a/dom/animation/AnimationEffectReadOnly.cpp > >+++ b/dom/animation/AnimationEffectReadOnly.cpp > >@@ -15,10 +15,16 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationEffectReadOnly, mParent) > > NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationEffectReadOnly) > > NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationEffectReadOnly) > > > > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationEffectReadOnly) > > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > > NS_INTERFACE_MAP_ENTRY(nsISupports) > > NS_INTERFACE_MAP_END > > > >+already_AddRefed<AnimationEffectTimingReadOnly> > >+AnimationEffectReadOnly::Timing() const > >+{ > >+ return RefPtr<AnimationEffectTimingReadOnly>().forget(); > > I don't understand how this works. Where do we add the connection to > KeyframeEffect's mTiming member? > > Also, does this compile? Or are you going to rework all the other patches? OK. I rework all the other patches and make sure each patch can be compiled. Thanks.

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into AnimationEffectTimingReadOnly.

1. Add AnimationEffectTimingReadOnly.webidl. 2. Add AnimationEffectTimingReadOnly cpp files. 3. Replace AnimationTiming with AnimationEffectTimingReadOnly in KeyframeEffectReadOnly