Now we validate some of TimingParams members so we can avoid re-check them in KeyframeEffectReadOnly::GetComputedTimingAt(). And we can cache active duration and end time instead of calculation in GetComputedTimingAt. Optimizations what I am going to handle in this bug: 1. Remove std::max for iterationStart. http://hg.mozilla.org/mozilla-central/file/06678484909c/dom/animation/KeyframeEffect.cpp#l249 2. Remove validation for iterations with a pre-validation in nsAnimationManager. http://hg.mozilla.org/mozilla-central/file/06678484909c/dom/animation/KeyframeEffect.cpp#l246 3. Cache active duration 4. Cache end time 5. Use the cached end time in Animation::EffectEnd()

Review commit: https://reviewboard.mozilla.org/r/45211/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45211/

Review commit: https://reviewboard.mozilla.org/r/45213/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45213/

Review commit: https://reviewboard.mozilla.org/r/45215/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45215/

Review commit: https://reviewboard.mozilla.org/r/45217/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45217/

Review commit: https://reviewboard.mozilla.org/r/45219/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45219/

Review commit: https://reviewboard.mozilla.org/r/45221/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45221/

Comment on attachment 8739337 [details] MozReview Request: Bug 1263063 - Part 1: Remove unnecessary clamping of TimingParams::mIterationStart, since it's guaranteed to be nonnegative. r?dholbert https://reviewboard.mozilla.org/r/45211/#review41747 Two issues with commit message: > Bug 1263063 - Part 1: iterationStart is guranteed that it's greater than 0. r?dholbert 1. "greater than 0" is incorrect here -- that'd be ">", but your code is ">=". I'm assuming the code is correct, so the message should say "at least 0" or "nonnegative", instead of "greater than 0". 2. The commit message should be phrased as a description of the change. So, addressing both points, the commit message's prose should be something more like: > Remove unnecessary clamping of TimingParams::mIterationStart, since it's guaranteed to be nonnegative. ::: dom/animation/KeyframeEffect.cpp:249 (Diff revision 1) > } > > result.mIterations = IsNaN(aTiming.mIterations) || aTiming.mIterations < 0.0f ? > 1.0f : > aTiming.mIterations; > - result.mIterationStart = std::max(aTiming.mIterationStart, 0.0); > + MOZ_ASSERT(aTiming.mIterationStart >= 0.0f); A few things: 1. Nit: drop the "f" suffix here -- mIterationStart is a double, so we should be comparing it to the double version of 0.0, which is spelled simply "0.0" (no f). (This won't change the behavior, but makes it removes some ambiguity about what type we're dealing with.) 2. Please add a message to this assertion, hinting at our justification for asserting this. (How do we know this value should be nonnegative? From a quick skim, it looks like *maybe* we're expecting ValidateIterationStart to have caught this? Not sure.)

Comment on attachment 8739338 [details] MozReview Request: Bug 1263063 - Part 2: Add an assertion to ensure that iteration count is nonnegative and finite. r?dholbert https://reviewboard.mozilla.org/r/45213/#review41749 commit message nit: "Validate iteration" is vague -- you're missing the word "count". It should probably say "Validate iteration count". ::: layout/style/nsAnimationManager.cpp:586 (Diff revision 1) > > timing.mDuration.emplace(StickyTimeDuration::FromMilliseconds( > aStyleAnimation.GetDuration())); > timing.mDelay = TimeDuration::FromMilliseconds(aStyleAnimation.GetDelay()); > + ErrorResult rv; > + TimingParams::ValidateIterations(aStyleAnimation.GetIterationCount(), rv); Now that we're validating ranges for one member-var in this function, are there any any other member-vars that we should be validating as well? For example: it looks like TimingParams has logic to validate its duration[1]. Do we need to exercise that validation here as well in our mDuration assignment just above this? (or alternately, can we assert that timing.mDuration already ends up in the valid range without any explicit need for validation?) (That change probably wouldn't belong in this patch, but it's probably worth adding another patch here [or on a followup bug] to address this one way or another [with validation or with an assertion], for mDuration and anything else that might need validation here.) [1] http://mxr.mozilla.org/mozilla-central/source/dom/animation/TimingParams.h?rev=2bcb11fae866&mark=45-45,56-61#45

Comment on attachment 8739339 [details] MozReview Request: Bug 1263063 - Part 3: Change logic in KeyFrameEffect to assume that TimingParams' iteration-count has already been validated as nonnegative & finite. r?dholbert https://reviewboard.mozilla.org/r/45215/#review41759 As noted in part 1, the commit message needs a tweak here: > iteration is now guranteed that it's greater than or equals to 0 (Firstly, two typos here: s/guranteed/guaranteed/, s/or equals/or equal/) But setting those aside: this doesn't communicate that there's actually any change happening to the code. Please replace with something like: "Change logic in KeyFrameEffect to assume that TimingParams' iteration-count has already been validated" (possibly adding ..."as nonnegative & finite" at the end of this message, if you like) ::: dom/animation/KeyframeEffect.cpp:246 (Diff revision 1) > MOZ_ASSERT(aTiming.mDuration.ref() >= zeroDuration, > "Iteration duration should be positive"); > result.mDuration = aTiming.mDuration.ref(); > } > > - result.mIterations = IsNaN(aTiming.mIterations) || aTiming.mIterations < 0.0f ? > + MOZ_ASSERT(aTiming.mIterations >= 0.0f && !IsNaN(aTiming.mIterations)); As noted in part 1, please add an explanatory message here, to explain the justification. e.g. "mIterations should be nonnegative & finite, as ensured by ValidateIterations" (Assertions with zero explanation should be rare, & should only be allowed when it's superficially obvious from context why the asserted condition can be assumed to hold. Otherwise, they're just hand-wavy and are hard to verify & understand. And if & when such unjustified assertions fail, the person triaging the failure has to do extra work to figure out why someone at some point in the past thought this assertion might be a valid thing to assert. An explanatory message goes a long way in helping ease that process.)

https://reviewboard.mozilla.org/r/45211/#review41769 ::: dom/animation/KeyframeEffect.cpp:23 (Diff revision 1) > #include "nsComputedDOMStyle.h" // nsComputedDOMStyle::GetStyleContextForElement > #include "nsCSSPropertySet.h" > #include "nsCSSProps.h" // For nsCSSProps::PropHasFlags > #include "nsCSSPseudoElements.h" // For CSSPseudoElementType > #include "nsDOMMutationObserver.h" // For nsAutoAnimationMutationBatch > #include <algorithm> // For std::max One more nit on "part 1" -- since you're removing the only std::max() expression in this file, you should also remove this `#include <algorithm> // For std::max` line as well from the top of the file.

Comment on attachment 8739340 [details] MozReview Request: Bug 1263063 - Part 4: Move ActiveDuration() into TimingParams. r?dholbert https://reviewboard.mozilla.org/r/45217/#review41773 So, this seems like it could be a case of premature optimization to me... It is nice to remove unnecessary multiply operations, but I can easily imagine that this one multiply (per animation, per sample) is just insignificant in the face of all the other computations that we're doing, per animation per sample. From that perspective, it's not at all obvious that the cost here (growing TimingParams & adding logic/complexity to keep this new member up-to-date, introducing more room for bugs) is justified by the benefit of optimizing away one multiply. Anyway, I'll proceed assuming this is a worthwhile optimization, for the time being. (And perhaps it's not a big deal either way; I'm open to going ahead with this regardless, I think.) ::: dom/animation/KeyframeEffect.cpp:251 (Diff revision 1) > MOZ_ASSERT(aTiming.mIterations >= 0.0f && !IsNaN(aTiming.mIterations)); > result.mIterations = aTiming.mIterations; > MOZ_ASSERT(aTiming.mIterationStart >= 0.0f); > result.mIterationStart = aTiming.mIterationStart; > > - result.mActiveDuration = ActiveDuration(result.mDuration, result.mIterations); > + result.mActiveDuration = aTiming.mActiveDuration; Two concerns, followed by steps on how to address both of them. - We should somehow assert that mActiveDuration is correct here. (i.e. that we didn't forget to update it via some codepath that modiifed iteration-count or iteration duration) - We should ensure that nobody can accidentally modify mActiveDuration -- i.e. it should be read-only & only modified via UpdateActiveDuration. (unlike TimingParams' other member-vars, which are all public) To address these concerns, we should do the following: 1. Change mActiveDuration to be a private member-var. (not public) 2. Expose a getter, "TimingParams::GetActiveDuration() const", which you'd call here (in GetComputedTimingAt()) instead of directly reading the variable. 3. In this GetActiveDuration method, add some #ifdef DEBUG code that ensures the cached value is actually correct. (Simplest thing would be to save the current mActiveDuration value to a local variable, and call UpdateActiveDuration(), and then assert that the update didn't change anything -- that mActiveDuration is still equal to your local variable. Again, this would all be #ifdef DEBUG, and it would probably require a const_cast<TimingParams*>(this) so that you could call the non-const UpdateActiveDuration from inside of a const getter method.) ::: gfx/layers/composite/AsyncCompositionManager.cpp:603 (Diff revision 1) > timing.mFill = dom::FillMode::Both; > timing.mFunction = > AnimationUtils::TimingFunctionToComputedTimingFunction( > animation.easingFunction()); > + // FIXME: This active duration calculation should be cached in somewhere. > + timing.UpdateActiveDuration(); I'm not immediately clear where you're thinking this calculation can/should be cached... Maybe the |animation| object? If that's what you mean, maybe replace "in somewhere" with "on |animation|, perhaps".

Comment on attachment 8739341 [details] MozReview Request: Bug 1263063 - Part 5: Introduce TimingParams::EndTime(). r?dholbert https://reviewboard.mozilla.org/r/45219/#review41781 ::: dom/animation/TimingParams.h:105 (Diff revision 1) > // Cached value. This is re-calculated whenever mDuration or mIterations is > // changed. > StickyTimeDuration mActiveDuration; > + // Cached value. This is re-calculated whenever mActiveDuration, mDelay or > + // mEndDelay is changed. > + StickyTimeDuration mEndTime; As noted for the previous patch, this new member-variable should be private, and should be accessed via a const getter which does some #ifdef DEBUG work to make sure the cached value is actually correct. ::: dom/animation/TimingParams.h:118 (Diff revision 1) > mIterations == 0.0) { > mActiveDuration = zeroDuration; > - return; > + } else { > - } > - > - mActiveDuration = mDuration->MultDouble(mIterations); > + mActiveDuration = mDuration->MultDouble(mIterations); This change (replacing an early-return with an "if/else" branch) should probably just be made in the previous patch (part 4), where you created this UpdateActiveDuration function. i.e. we should just use "if/else" up-front when you add this code, instead if starting it out as "if/return" and then rewriting it as if/else in this later patch. That way, this patch here can just make a single targeted change to this method -- adding a single call to UpdateEndTime() -- which is 100% aligned with the overall goal of this patch. ::: dom/animation/TimingParams.h:122 (Diff revision 1) > - > - mActiveDuration = mDuration->MultDouble(mIterations); > + mActiveDuration = mDuration->MultDouble(mIterations); > - } > + } > + UpdateEndTime(); > + } > + // This should be called whenever mActiveDuration, mDelay or mEndDelay is Please add a blank line here, to separate the previous method from the documentation for this new method.

Comment on attachment 8739342 [details] MozReview Request: Bug 1263063 - Part 6: Use TimingParams::EndTime() instead of re-calculation ComputedTiming each time. r?dholbert https://reviewboard.mozilla.org/r/45221/#review41793 This part makes sense -- and it makes me less concerned that the other changes here are premature optimization (comment 11), since they enable this change which optimizes away a lot more logic than a simple multiply. r=me on this part, though maybe consider editing your commit message to replace "each time" with "in Animation::EffectEnd", to make this a little more concrete about what's changing. Not a big deal though.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > 3. Cache active duration Are you sure we want to do this? It's not an expensive calculation, right? Unless we have profiles that show that this is significant I don't think we should do this. It leads to more code (all the additional calls to UpdateActiveDuration) and more likelihood of bugs since if we forget to add UpdateActiveDuration to some code path we'll end up with a bug that we likely won't detect for quite a while. That's especially likely to happen if we later add an early return somewhere. > 4. Cache end time Likewise here, I don't think we should do this unless we have profiles demonstrating that this is significant.

(In reply to Brian Birtles (:birtles, away 2-10 April) from comment #14) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > 3. Cache active duration > > Are you sure we want to do this? > > It's not an expensive calculation, right? Unless we have profiles that show > that this is significant I don't think we should do this. It leads to more > code (all the additional calls to UpdateActiveDuration) and more likelihood > of bugs since if we forget to add UpdateActiveDuration to some code path > we'll end up with a bug that we likely won't detect for quite a while. > That's especially likely to happen if we later add an early return somewhere. > > > 4. Cache end time > > Likewise here, I don't think we should do this unless we have profiles > demonstrating that this is significant. I have a plan to write a talos test for animations but not start yet. How about adding a new function to calculate just effect end instead of caching it for now? The calculation of effect end is called several times (dozen times?) in a tick though IsInEffect() or IsCurrent(). Most of calculations in GetComputedTimingAt() should be avoided in case we want only effect end.

(In reply to Daniel Holbert [:dholbert] from comment #13) > Comment on attachment 8739342 [details] > MozReview Request: Bug 1263063 - Part 6: Use cached end time instead of > re-calculation each time. r?dholbert [...] > This part makes sense -- and it makes me less concerned that the other > changes here are premature optimization (comment 11), since they enable this > change which optimizes away a lot more logic than a simple multiply. Actually, I think was mistaken in thinking that caching optimizations were necessary for this part. It seems like we could just as easily enable part 6 with a lazy (non-cache-based) EndTime() accessor on the object that's returned by SpecifiedTiming(). And this would still let us optimize away the GetComputedTiming() call, if that's the goal here. I think that's what you're suggesting in your "How about [...]" in comment 15? If so, that sounds good to me!

(In reply to Daniel Holbert [:dholbert] from comment #8) > ::: layout/style/nsAnimationManager.cpp:586 > (Diff revision 1) > > > > timing.mDuration.emplace(StickyTimeDuration::FromMilliseconds( > > aStyleAnimation.GetDuration())); > > timing.mDelay = TimeDuration::FromMilliseconds(aStyleAnimation.GetDelay()); > > + ErrorResult rv; > > + TimingParams::ValidateIterations(aStyleAnimation.GetIterationCount(), rv); > > Now that we're validating ranges for one member-var in this function, are > there any any other member-vars that we should be validating as well? > > For example: it looks like TimingParams has logic to validate its > duration[1]. Do we need to exercise that validation here as well in our > mDuration assignment just above this? (or alternately, can we assert that > timing.mDuration already ends up in the valid range without any explicit > need for validation?) > > (That change probably wouldn't belong in this patch, but it's probably worth > adding another patch here [or on a followup bug] to address this one way or > another [with validation or with an assertion], for mDuration and anything > else that might need validation here.) > > [1] > http://mxr.mozilla.org/mozilla-central/source/dom/animation/TimingParams. > h?rev=2bcb11fae866&mark=45-45,56-61#45 Animation duration is already validated in CSS parser. http://hg.mozilla.org/mozilla-central/file/29d5a4175c8b/layout/style/nsCSSPropList.h#l460 And I did not realized it but Animation iteration is also validated in the parser. So part 2 patch is not needed at all. Thank you, Daniel for making me realize the fact! http://hg.mozilla.org/mozilla-central/file/29d5a4175c8b/layout/style/nsCSSPropList.h#l482 I am going to upload new patch set without part 2. (In reply to Daniel Holbert [:dholbert] from comment #16) > It seems like we could just as easily enable part 6 with a lazy > (non-cache-based) EndTime() accessor on the object that's returned by > SpecifiedTiming(). And this would still let us optimize away the > GetComputedTiming() call, if that's the goal here. > > I think that's what you're suggesting in your "How about [...]" in comment > 15? Yes, exactly.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17) > (In reply to Daniel Holbert [:dholbert] from comment #8) > > ::: layout/style/nsAnimationManager.cpp:586 > > (Diff revision 1) > > > > > > timing.mDuration.emplace(StickyTimeDuration::FromMilliseconds( > > > aStyleAnimation.GetDuration())); > > > timing.mDelay = TimeDuration::FromMilliseconds(aStyleAnimation.GetDelay()); > > > + ErrorResult rv; > > > + TimingParams::ValidateIterations(aStyleAnimation.GetIterationCount(), rv); > > > > Now that we're validating ranges for one member-var in this function, are > > there any any other member-vars that we should be validating as well? > > > > For example: it looks like TimingParams has logic to validate its > > duration[1]. Do we need to exercise that validation here as well in our > > mDuration assignment just above this? (or alternately, can we assert that > > timing.mDuration already ends up in the valid range without any explicit > > need for validation?) > > > > (That change probably wouldn't belong in this patch, but it's probably worth > > adding another patch here [or on a followup bug] to address this one way or > > another [with validation or with an assertion], for mDuration and anything > > else that might need validation here.) > > > > [1] > > http://mxr.mozilla.org/mozilla-central/source/dom/animation/TimingParams. > > h?rev=2bcb11fae866&mark=45-45,56-61#45 > > Animation duration is already validated in CSS parser. > http://hg.mozilla.org/mozilla-central/file/29d5a4175c8b/layout/style/ > nsCSSPropList.h#l460 Oops, I was wrong. In case of animation iteration the CSS parser does not ensure that its value is not NaN. We should use ValidateIterations for animation iteration.

Comment on attachment 8739337 [details] MozReview Request: Bug 1263063 - Part 1: Remove unnecessary clamping of TimingParams::mIterationStart, since it's guaranteed to be nonnegative. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45211/diff/1-2/

Comment on attachment 8739338 [details] MozReview Request: Bug 1263063 - Part 2: Add an assertion to ensure that iteration count is nonnegative and finite. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45213/diff/1-2/

Comment on attachment 8739339 [details] MozReview Request: Bug 1263063 - Part 3: Change logic in KeyFrameEffect to assume that TimingParams' iteration-count has already been validated as nonnegative & finite. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45215/diff/1-2/

Comment on attachment 8739340 [details] MozReview Request: Bug 1263063 - Part 4: Move ActiveDuration() into TimingParams. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45217/diff/1-2/

Comment on attachment 8739341 [details] MozReview Request: Bug 1263063 - Part 5: Introduce TimingParams::EndTime(). r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45219/diff/1-2/

Comment on attachment 8739342 [details] MozReview Request: Bug 1263063 - Part 6: Use TimingParams::EndTime() instead of re-calculation ComputedTiming each time. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45221/diff/1-2/

https://reviewboard.mozilla.org/r/45211/#review41769 > One more nit on "part 1" -- since you're removing the only std::max() expression in this file, you should also remove this `#include <algorithm> // For std::max` line as well from the top of the file. Great! Thanks! Good catch!

https://reviewboard.mozilla.org/r/45215/#review41759 Thanks always! Fixed it as well.

Two responses on "part 2": (In reply to Hiroyuki Ikezoe (:hiro) from comment #17) > > Now that we're validating ranges for one member-var in this function, are > > there any any other member-vars that we should be validating as well? > > > > For example: it looks like TimingParams has logic to validate its > > duration[1]. Do we need to exercise that validation here as well in our > > mDuration assignment just above this? (or alternately, can we assert that > > timing.mDuration already ends up in the valid range without any explicit > > need for validation?) > > Animation duration is already validated in CSS parser. > http://hg.mozilla.org/mozilla-central/file/29d5a4175c8b/layout/style/ > nsCSSPropList.h#l460 Please add an assertion that duration is valid here (in part 2), then - that makes it clearer why you're only doing partial validation here. (validating one member-var but not the other, for no clear reason) (In reply to Hiroyuki Ikezoe (:hiro) from comment #18) > In case of animation iteration the CSS parser does not > ensure that its value is not NaN. We should use ValidateIterations for > animation iteration. Hmm. Out of curiosity, how would we actually end up with NaN in this value? Surely we won't parse "NaN"... I could imagine "calc(0/0)" producing NaN, but we don't seem to accept calc() values for this property (animation-iteration-count) at all (e.g. "animation-iteration-count: calc(5/2)"). As a reference, Chrome does accept calc() values (e.g. calc but they reject calc(0/0) as invalid. So, I'm not clear how we get NaN here.

Comment on attachment 8739340 [details] MozReview Request: Bug 1263063 - Part 4: Move ActiveDuration() into TimingParams. r?dholbert https://reviewboard.mozilla.org/r/45217/#review42083 Commit message nit: > Part 4: Move ActiveDuration() in TimingParams s/in/into/ ::: dom/animation/TimingParams.h:102 (Diff revision 2) > dom::FillMode mFill = dom::FillMode::Auto; > Maybe<ComputedTimingFunction> mFunction; > > + // Return the duration of the active interval for the given duration and > + // iteration count. > + StickyTimeDuration ActiveDuration() const This documentation (copypasted from the old function-signature) doesn't quite make sense anymore -- it needs to be updated. (In particular: "...for the given duration and iteration count" no longer makes sense, since this function doesn't take any parameters now.)

Comment on attachment 8739341 [details] MozReview Request: Bug 1263063 - Part 5: Introduce TimingParams::EndTime(). r?dholbert https://reviewboard.mozilla.org/r/45219/#review42091 Minor commit message nit: > Part 5: Introduce TimingParams::EndTime. Consider adding parens here, "EndTime()", to make it clearer that this is a method rather than an enum or a nested class or something.

(In reply to Daniel Holbert [:dholbert] from comment #27) > Two responses on "part 2": > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #17) > > > Now that we're validating ranges for one member-var in this function, are > > > there any any other member-vars that we should be validating as well? > > > > > > For example: it looks like TimingParams has logic to validate its > > > duration[1]. Do we need to exercise that validation here as well in our > > > mDuration assignment just above this? (or alternately, can we assert that > > > timing.mDuration already ends up in the valid range without any explicit > > > need for validation?) > > > > Animation duration is already validated in CSS parser. > > http://hg.mozilla.org/mozilla-central/file/29d5a4175c8b/layout/style/ > > nsCSSPropList.h#l460 > > Please add an assertion that duration is valid here (in part 2), then - that > makes it clearer why you're only doing partial validation here. (validating > one member-var but not the other, for no clear reason) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #18) > > In case of animation iteration the CSS parser does not > > ensure that its value is not NaN. We should use ValidateIterations for > > animation iteration. > > Hmm. Out of curiosity, how would we actually end up with NaN in this value? > Surely we won't parse "NaN"... I could imagine "calc(0/0)" producing NaN, > but we don't seem to accept calc() values for this property > (animation-iteration-count) at all (e.g. "animation-iteration-count: > calc(5/2)"). As a reference, Chrome does accept calc() values (e.g. calc but > they reject calc(0/0) as invalid. So, I'm not clear how we get NaN here. I was somewhat confused duration, e.g. '100s', itself is a NaN. Sorry for the confusion.

Sorry, I don't think comment 30 answers my question. Back in comment 18, you'd said that we *do* need part 2 because NaN values can get through the CSS parser, for "animation-iteration-count" -- and I'm wondering how that can happen. (what sorts of specified values for this property would actually produce NaN there) (Your response in Comment 30 is about duration, not about iteration count.) Maybe the answer is: you were right in comment 17 and we don't actually need part 2 at all? In which case, I'd suggest we should populate "part 2" with assertions to verify that our values are in fact valid.

(My r+ on part 2 was conditional on the assumption that its validation is necessary -- but I'm less sure about that now.)

Comment on attachment 8739337 [details] MozReview Request: Bug 1263063 - Part 1: Remove unnecessary clamping of TimingParams::mIterationStart, since it's guaranteed to be nonnegative. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45211/diff/2-3/

Comment on attachment 8739338 [details] MozReview Request: Bug 1263063 - Part 2: Add an assertion to ensure that iteration count is nonnegative and finite. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45213/diff/2-3/

Comment on attachment 8739339 [details] MozReview Request: Bug 1263063 - Part 3: Change logic in KeyFrameEffect to assume that TimingParams' iteration-count has already been validated as nonnegative & finite. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45215/diff/2-3/

Comment on attachment 8739340 [details] MozReview Request: Bug 1263063 - Part 4: Move ActiveDuration() into TimingParams. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45217/diff/2-3/

Comment on attachment 8739341 [details] MozReview Request: Bug 1263063 - Part 5: Introduce TimingParams::EndTime(). r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45219/diff/2-3/

Comment on attachment 8739342 [details] MozReview Request: Bug 1263063 - Part 6: Use TimingParams::EndTime() instead of re-calculation ComputedTiming each time. r?dholbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45221/diff/2-3/

(In reply to Daniel Holbert [:dholbert] from comment #31) > Sorry, I don't think comment 30 answers my question. Back in comment 18, > you'd said that we *do* need part 2 because NaN values can get through the > CSS parser, for "animation-iteration-count" -- and I'm wondering how that > can happen. (what sorts of specified values for this property would actually > produce NaN there) > > (Your response in Comment 30 is about duration, not about iteration count.) > > Maybe the answer is: you were right in comment 17 and we don't actually need > part 2 at all? In which case, I'd suggest we should populate "part 2" with > assertions to verify that our values are in fact valid. I made another mistake in comment#30. (Actually I was reading the code related to *duration*...) It took me a while to check that CSSParser against animation-iteration-count surely checks NaN value and does not pass to. I did check it with 'element.style.animationIterationCount = 0 / 0;'. In case when NaN is passed to style.animationIterationCount, CSSParserImpl::ParseVariant is called with aVariantMask = VARIANT_KEYWORD | VARIANT_NUMBER and mToken.mType = *eCSSToken_Ident*, so the function returns CSSParseResult::NotFound at the end. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc024e7dd4a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6ecb8e9927 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c606174ee17 https://hg.mozilla.org/integration/mozilla-inbound/rev/4160ea9c6de3 https://hg.mozilla.org/integration/mozilla-inbound/rev/eeda1ce17055 https://hg.mozilla.org/integration/mozilla-inbound/rev/966cee83e979

https://hg.mozilla.org/mozilla-central/rev/dc024e7dd4a5 https://hg.mozilla.org/mozilla-central/rev/2a6ecb8e9927 https://hg.mozilla.org/mozilla-central/rev/2c606174ee17 https://hg.mozilla.org/mozilla-central/rev/4160ea9c6de3 https://hg.mozilla.org/mozilla-central/rev/eeda1ce17055 https://hg.mozilla.org/mozilla-central/rev/966cee83e979