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

Although we know that the animation properties will always be filled in for a transition in the cases where we need to query them (going forward we will have a situation where an animation may only have frames, not properties, but that will only happen when the animation isn't attached to an element or the element is not attached to a document, but we don't run animations in that case and cancel existing ones when we enter that state so although they *can* enter that state, we'll never run these methods on them when they do), we still want to move towards making frames the primary unit for interacting with animation values since frames always exist and represent the public interface. Ultimately it would be good to make the properties array on a KeyframeEffect(ReadOnly) an encapsulated detail so that we can freely change their structure (e.g. segments might not be the best setup, it might be better to just have arrays of free-standing values to avoid the duplication of values when segments are continuous). This patch removes or encapsulates a few references to properties and simplifies the code at the same time. Review commit: https://reviewboard.mozilla.org/r/43639/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43639/

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

Comment on attachment 8737004 [details] MozReview Request: Bug 1260976 - Make nsTransitionManager use Keyframe objects to set up transitions; r?heycam https://reviewboard.mozilla.org/r/43637/#review41923 ::: layout/style/nsTransitionManager.cpp:748 (Diff revision 1) > + Keyframe& fromFrame = *keyframes.AppendElement(); > + fromFrame.mOffset.emplace(0.0); > + PropertyValuePair& pv = *fromFrame.mPropertyValues.AppendElement(); > + pv.mProperty = aProperty; > + DebugOnly<bool> uncomputeResult = > + StyleAnimationValue::UncomputeValue(aProperty, Move(aStartValue), > + pv.mValue); > + MOZ_ASSERT(uncomputeResult, > + "Unable to get specified value from computed from value"); Consider factoring out this keyframe creation since it's basically duplicated just below (apart from the timing function setting)?

Comment on attachment 8737005 [details] MozReview Request: Bug 1260976 - Remove some references to properties within nsTransitionManager; r?heycam https://reviewboard.mozilla.org/r/43639/#review41925

Comment on attachment 8737006 [details] MozReview Request: Bug 1260976 - Remove the old AnimationProperty-based GetFrames; r?heycam https://reviewboard.mozilla.org/r/43641/#review41927 I like seeing code removal, especially when it's code I wrote that's a bit complex. :-)

(In reply to Cameron McCormack (:heycam) from comment #4) > ::: layout/style/nsTransitionManager.cpp:748 > (Diff revision 1) > > + Keyframe& fromFrame = *keyframes.AppendElement(); > > + fromFrame.mOffset.emplace(0.0); > > + PropertyValuePair& pv = *fromFrame.mPropertyValues.AppendElement(); > > + pv.mProperty = aProperty; > > + DebugOnly<bool> uncomputeResult = > > + StyleAnimationValue::UncomputeValue(aProperty, Move(aStartValue), > > + pv.mValue); > > + MOZ_ASSERT(uncomputeResult, > > + "Unable to get specified value from computed from value"); > > Consider factoring out this keyframe creation since it's basically > duplicated just below (apart from the timing function setting)? Good idea. Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82bd962907de https://hg.mozilla.org/integration/mozilla-inbound/rev/3791a7ec5d6f https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf416db49d5

https://hg.mozilla.org/mozilla-central/rev/82bd962907de https://hg.mozilla.org/mozilla-central/rev/3791a7ec5d6f https://hg.mozilla.org/mozilla-central/rev/2cf416db49d5