This is: a) So we can keep testing our frame-to-property mapping after we update getFrames() to preserve the author-specified frames (bug 1245748) b) So DevTools can continue to use this information (in place of getFrames() once it changes) to expose *how* frames are being interpreted and run c) So, in future, we can expose more per-value information such as per-value composite modes etc. The idea is to build on the API exposed in bug 1196114, possible renaming getPropertyState() to simply getProperties().

Later in this patch series when we convert tests from web-platform tests to mochitest-chrome tests, some of the test cases that use zero-length segments (overlapping keyframes at certain offsets) would trigger failed assertions in KeyframeEffectReadOnly::ComposeStyle. This is because this method was originally written with CSS animations in mind where segments cannot be zero-length. Furthermore, when these same tests cases are run as web-platform-tests, the failed assertions are not visible. This patch adjusts the handling of segments to allow zero-length segments and adds a test to check that the handling matches that defined in Web Animations which is summarized in the following informative note, "this procedure permits overlapping keyframes. The behavior is that at the point of overlap the output value jumps to the value of the last defined keyframe at that offset. For overlapping frames at 0 or 1, the output value for iteration progress values less than 0 or greater than or equal to 1 is the value of the first keyframe or the last keyframe in keyframes respectively."[1] [1] https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect Review commit: https://reviewboard.mozilla.org/r/40097/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40097/

This better matches the order in the WebIDL and, once we rename GetPropertyState to GetProperties it will make sense for GetFrames and GetProperties to be side-by-side. Review commit: https://reviewboard.mozilla.org/r/40099/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40099/

We are now extending this API to include more than just metadata about each animated property but also the property values themselves. Note that we can't use the name AnimationProperty for the dictionary since we already use that name internally and [BinaryName] doesn't seem to apply to dictionaries. An alternative might be to rename the internal object to AnimationPropertyStruct like we already do for Keyframe. Review commit: https://reviewboard.mozilla.org/r/40101/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40101/

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

I think the reason we originally didn't do this is that the "isRunningOnCompositor" status might be misleading for animations that are being overridden. That is, there are some animations we don't send to the compositor because they are being overridden by another animation (e.g. a CSS animation touching the 'transform' animation will cause a CSS transition on the same property not to run, despite the fact that transitions apply higher in the cascade). This is not merely a performance optimization but means we don't have to do the cascade on the compositor. In the future, once we introduce additive animation, we won't be able to handle this so simply since it an animation will be able to be partially overridden. Instead, consumers of this API will need to look at the 'composite' member of the various animation values to see if an animation is being fully or partially overridden. As a result, this API really should return all running animations, even if they are currently being overridden. Review commit: https://reviewboard.mozilla.org/r/40105/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40105/

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

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

https://reviewboard.mozilla.org/r/40097/#review36809 ::: dom/animation/KeyframeEffect.cpp:598 (Diff revision 1) > + if (segment->mToKey == segment->mFromKey) { > + if (computedTiming.mProgress.Value() < 0) { > + *val = segment->mFromValue; > + } else { > + *val = segment->mToValue; > + } > + return; > + } I guess you might want to conitnue here to iterate other CSS properties.

Comment on attachment 8730706 [details] MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40097/diff/1-2/

Comment on attachment 8730707 [details] MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40099/diff/1-2/

Comment on attachment 8730708 [details] MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40101/diff/1-2/

Comment on attachment 8730709 [details] MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40103/diff/1-2/

Comment on attachment 8730710 [details] MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40105/diff/1-2/

Comment on attachment 8730711 [details] MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40107/diff/1-2/

Comment on attachment 8730712 [details] MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40109/diff/1-2/

Comment on attachment 8730707 [details] MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro https://reviewboard.mozilla.org/r/40099/#review36817

Comment on attachment 8730706 [details] MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam https://reviewboard.mozilla.org/r/40097/#review36829

Comment on attachment 8730708 [details] MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz https://reviewboard.mozilla.org/r/40101/#review36835 r=me though I'm not a big fan of including words like "Class", "Struct" and "Enum" in identifiers. (Maybe "Details" or something?)

Comment on attachment 8730709 [details] MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz https://reviewboard.mozilla.org/r/40103/#review36839

Comment on attachment 8730710 [details] MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro https://reviewboard.mozilla.org/r/40105/#review36845 I'd like to see a test case for this.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > https://reviewboard.mozilla.org/r/40097/#review36809 > > ::: dom/animation/KeyframeEffect.cpp:598 > (Diff revision 1) > > + if (segment->mToKey == segment->mFromKey) { > > + if (computedTiming.mProgress.Value() < 0) { > > + *val = segment->mFromValue; > > + } else { > > + *val = segment->mToValue; > > + } > > + return; > > + } > > I guess you might want to conitnue here to iterate other CSS properties. Yes, good catch!

Don't forget to get a DOM peer's review on the patches with .webidl changes (even though they're ChromeOnly).

Comment on attachment 8730711 [details] MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam https://reviewboard.mozilla.org/r/40107/#review36843 ::: dom/animation/KeyframeEffect.cpp:1811 (Diff revision 2) > + const StyleAnimationValue& aValue, > + AnimationPropertyValueDictionary& aResult) > +{ > + aResult.mOffset.Construct(aOffset); > + > + nsAutoString stringValue; If you use nsString, will the call to Construct be able to share the string rather than being forced to copy it from the stack? ::: dom/animation/KeyframeEffect.cpp:1863 (Diff revision 2) > + propertyDictionary.mValues.Value().AppendElement(fromValue, > + mozilla::fallible); Not sure whether it's better to silently fail on OOM here or to do something explicit.

Comment on attachment 8730712 [details] MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam https://reviewboard.mozilla.org/r/40109/#review36857

Comment on attachment 8730706 [details] MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40097/diff/2-3/

Comment on attachment 8730707 [details] MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40099/diff/2-3/

Comment on attachment 8730708 [details] MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40101/diff/2-3/

Comment on attachment 8730709 [details] MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40103/diff/2-3/

Comment on attachment 8730710 [details] MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40105/diff/2-3/

Comment on attachment 8730711 [details] MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40107/diff/2-3/

Comment on attachment 8730712 [details] MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40109/diff/2-3/

Comment on attachment 8730708 [details] MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz https://reviewboard.mozilla.org/r/40101/#review37011 r=me

Comment on attachment 8730709 [details] MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz https://reviewboard.mozilla.org/r/40103/#review37013 r=me assuming it's expected that the member will be missing in some cases. If it will always be present, just make it a required member.

Comment on attachment 8730706 [details] MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40097/diff/3-4/

Comment on attachment 8730707 [details] MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40099/diff/3-4/

Comment on attachment 8730708 [details] MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40101/diff/3-4/

Comment on attachment 8730709 [details] MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40103/diff/3-4/

Comment on attachment 8730710 [details] MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40105/diff/3-4/

Comment on attachment 8730711 [details] MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40107/diff/3-4/

Comment on attachment 8730712 [details] MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40109/diff/3-4/

At the same time we also make the 'warning' member of AnimationPropertyDetails no longer nullable and simply use the absence of the member to indicate "no warning" (which is what we were already doing -- we were never actually setting it to null). Review commit: https://reviewboard.mozilla.org/r/40675/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40675/

(In reply to Boris Zbarsky [:bz] from comment #33) > Comment on attachment 8730709 [details] > MozReview Request: Bug 1254419 - Add values member to > AnimationPropertyDictionary; r=heycam > > https://reviewboard.mozilla.org/r/40103/#review37013 > > r=me assuming it's expected that the member will be missing in some cases. > If it will always be present, just make it a required member. I didn't realise we used required for dictionary objects we're returning but I can see it simplifies the code and documents the expected output better. I've updated all the members of these two dictionaries accordingly in a follow-up patch.

Comment on attachment 8731517 [details] MozReview Request: Bug 1254419 - Make always-set members of AnimationProperty(Value)Details required https://reviewboard.mozilla.org/r/40675/#review37193 ::: dom/animation/KeyframeEffect.cpp:1843 (Diff revision 1) > } > > - propertyDetails.mValues.Construct(); > + if (!propertyDetails.mValues.SetCapacity( > - if (!propertyDetails.mValues.Value().SetCapacity( > property.mSegments.Length(), mozilla::fallible)) { > MOZ_CRASH("Out of memory allocating values array"); Why bother passing mozilla::fallible if you plan to MOZ_CRASH anyway? Either don't pass it or handle the failure without crashing, I'd think... (yes, I know, preexisting, probably fodder for a separate bug). r=me

(In reply to Boris Zbarsky [:bz] from comment #43) > Comment on attachment 8731517 [details] > MozReview Request: Bug 1254419 - Make always-set members of > AnimationProperty(Value)Details required > > https://reviewboard.mozilla.org/r/40675/#review37193 > > ::: dom/animation/KeyframeEffect.cpp:1843 > (Diff revision 1) > > } > > > > - propertyDetails.mValues.Construct(); > > + if (!propertyDetails.mValues.SetCapacity( > > - if (!propertyDetails.mValues.Value().SetCapacity( > > property.mSegments.Length(), mozilla::fallible)) { > > MOZ_CRASH("Out of memory allocating values array"); > > Why bother passing mozilla::fallible if you plan to MOZ_CRASH anyway? > Either don't pass it or handle the failure without crashing, I'd think... > (yes, I know, preexisting, probably fodder for a separate bug). I'm probably missing something but mValues here is a Sequence<> which inherits from FallibleTArray so I think we have to pass mozilla::fallible here. It doesn't seem worth making this method throw just to handle this more gracefully?

Oh, it's a Sequence, OK. If the list of things we're adding to it is not under page control, this is fine, but could use a comment. If it's under page control, we should probably handle this without crashing. Doesn't have to throw, could just pretend like the list is empty or something if that would be simpler....

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

Comment on attachment 8731548 [details] MozReview Request: Bug 1254419 - Throw if we fail to allocate memory for a values array in getProperties() https://reviewboard.mozilla.org/r/40699/#review37231 ::: dom/animation/KeyframeEffect.cpp:1844 (Diff revision 1) > propertyDetails.mWarning.Construct(localizedString); > } > > - if (!propertyDetails.mValues.SetCapacity( > - property.mSegments.Length(), mozilla::fallible)) { > - MOZ_CRASH("Out of memory allocating values array"); > + if (!propertyDetails.mValues.SetCapacity(property.mSegments.Length(), > + mozilla::fallible)) { > + aRv.Throw(NS_ERROR_FAILURE); NS_ERROR_OUT_OF_MEMORY, please. ::: dom/animation/KeyframeEffect.cpp:1845 (Diff revision 1) > } > > - if (!propertyDetails.mValues.SetCapacity( > - property.mSegments.Length(), mozilla::fallible)) { > - MOZ_CRASH("Out of memory allocating values array"); > + if (!propertyDetails.mValues.SetCapacity(property.mSegments.Length(), > + mozilla::fallible)) { > + aRv.Throw(NS_ERROR_FAILURE); > + aProperties.Clear(); No need to do that. It'll get ignored by the bindings anyway, and clear itself in its destructor. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/98445ee59e8b https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5d2092a6c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/631335668763 https://hg.mozilla.org/integration/mozilla-inbound/rev/f34fc7461cd1 https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf7ebb9b666 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba0c14ec7b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/d516fa6d001f https://hg.mozilla.org/integration/mozilla-inbound/rev/5822ac22539f https://hg.mozilla.org/integration/mozilla-inbound/rev/deabc24fe6ed

https://hg.mozilla.org/mozilla-central/rev/98445ee59e8b https://hg.mozilla.org/mozilla-central/rev/4f5d2092a6c3 https://hg.mozilla.org/mozilla-central/rev/631335668763 https://hg.mozilla.org/mozilla-central/rev/f34fc7461cd1 https://hg.mozilla.org/mozilla-central/rev/1bf7ebb9b666 https://hg.mozilla.org/mozilla-central/rev/6ba0c14ec7b6 https://hg.mozilla.org/mozilla-central/rev/d516fa6d001f https://hg.mozilla.org/mozilla-central/rev/5822ac22539f https://hg.mozilla.org/mozilla-central/rev/deabc24fe6ed