I have a problem about the grammar of paced({ident}). Should the format of ident be 'marinLeft' or 'margin-left'? We use 'marginLeft' in the keyframe list, so should we use the same format in paced() function?

(In reply to Boris Chiou [:boris] from comment #1) > I have a problem about the grammar of paced({ident}). Should the format of > ident be 'marinLeft' or 'margin-left'? We use 'marginLeft' in the keyframe > list, so should we use the same format in paced() function? In my current implementation, I use 'margin-left' as the expected value, so I can use nsCSSProps::LookupProperty() to get the property id easily.

(In reply to Boris Chiou [:boris] from comment #1) > I have a problem about the grammar of paced({ident}). Should the format of > ident be 'marinLeft' or 'margin-left'? We use 'marginLeft' in the keyframe > list, so should we use the same format in paced() function? I *think* 'margin-left' is probably better here (since it's in a string, and we only use the camelCase when it's an JS identifier; and because we'll likely expose this syntax to CSS some day).

Hi Brian, Let's check Issue 24 [1], applying spacing to Keyframes if context element is null. I use StyleAnimationValue::ComputeDistance() to calculate the cumulative distance between each pair of paceable Keyframes, so if the target element (context element) is null (and no style context), I have to handle these three cases properly: 1. When creating null-target effect whose spacing mode is "paced": My thought - Apply distribute spacing to all Keyframes directly. 2. When setting a target to null: My thought - Do nothing. We still use the original computed offsets. (Or should we apply distribute spacing if original spacing mode is paced?) 3. When setting a new valid target: My thought - Just use this new target as the context element and re-calculate the computed offsets. How do you think about these? Thanks. [1] https://w3c.github.io/web-animations/#issue-bff293c9

(In reply to Boris Chiou [:boris] from comment #4) > Let's check Issue 24 [1], applying spacing to Keyframes if context element > is null. > I use StyleAnimationValue::ComputeDistance() to calculate the cumulative > distance between each pair of paceable Keyframes, so if the target element > (context element) is null (and no style context), I have to handle these > three cases properly: > > 1. When creating null-target effect whose spacing mode is "paced": > My thought - Apply distribute spacing to all Keyframes directly. Yes, I think falling back to distribute spacing is the best thing to do. We already use 'distribute' spacing unconditionally when merging frames in [1] and we fall back to 'distribute' when the paced property is not available so I think it's the correct thing to do here too. [1] https://w3c.github.io/web-animations/#processing-a-frames-argument > 2. When setting a target to null: > My thought - Do nothing. We still use the original computed offsets. (Or > should we apply distribute spacing if original spacing mode is paced?) I think we should apply distribute spacing in this case. This makes the API stateless in this regard (i.e. it shouldn't matter than you previously attached this effect to an element with a certain font-size). > 3. When setting a new valid target: > My thought - Just use this new target as the context element and > re-calculate the computed offsets. Yes. That sounds correct. > How do you think about these? Thanks. I will work on the spec changes (I have a long list of spec changes I'm working on now!) but please add web-platform-tests for these. Thanks!

Some CSS properties are not supported neither for StyleAnimationValue::ComputeDistance() nor paced spacing yet, e.g. transform [1]. In these cases, I think we should also fall back to distribute spacing. Do we have a bug to trace them? I guess I have to file a bug for them. [1] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/layout/style/StyleAnimationValue.cpp#864-872

Hi Brian, If we have duplicated Keyframes or the distance of a pair CSS propertry values are 0.0. e.g. var anim = div.animate([ { marginLeft: '0px' }, { marginLeft: '0px' }, { marginLeft: '100px' , offset: 0.5}, { marginLeft: '50px'} ], { duration: 1000, spacing: 'paced(margin-left)' }); anim.effect.frame[0].computedOffset = 0.0; anim.effect.frame[1].computedOffset = 0.0; // 0.0 or 0.25 ? anim.effect.frame[2].computedOffset = 0.5; anim.effect.frame[3].computedOffset = 1.0; What is the expected computed offset of the duplicated Keyframe in paced spacing mode? Thanks.

(In reply to Boris Chiou [:boris] from comment #7) > var anim = div.animate([ { marginLeft: '0px' }, > { marginLeft: '0px' }, > { marginLeft: '100px' , offset: 0.5}, > { marginLeft: '50px'} ], > { duration: 1000, spacing: 'paced(margin-left)' }); Another similar case is var anim = div.animate([ { marginLeft: '0px' }, { marginLeft: '0px' }, { marginLeft: '100px' }, { marginLeft: '50px' } ], { duration: 1000, spacing: 'paced(margin-left)' }); anim.effect.frame[0].computedOffset = 0.0 anim.effect.frame[1].computedOffset = 0.0 // This is 0.0, right? anim.effect.frame[2].computedOffset = 0.667 anim.effect.frame[3].computedOffset = 1.0

(In reply to Boris Chiou [:boris] from comment #7) > Hi Brian, > > If we have duplicated Keyframes or the distance of a pair CSS propertry > values are 0.0. > e.g. > > var anim = div.animate([ { marginLeft: '0px' }, > { marginLeft: '0px' }, > { marginLeft: '100px' , offset: 0.5}, > { marginLeft: '50px'} ], > { duration: 1000, spacing: 'paced(margin-left)' }); > > anim.effect.frame[0].computedOffset = 0.0; > anim.effect.frame[1].computedOffset = 0.0; // 0.0 or 0.25 ? > anim.effect.frame[2].computedOffset = 0.5; > anim.effect.frame[3].computedOffset = 1.0; > > What is the expected computed offset of the duplicated Keyframe in paced > spacing mode? Why wouldn't it be 0.0?

(In reply to Brian Birtles (:birtles) from comment #9) > Why wouldn't it be 0.0? I think the spec there says it should be: offset|paced A| + (offset|paced B| − offset|paced A|) × dist|k| / dist|paced B| = 0.0 + (0.5 - 0.0) * 0 / 100 = 0.0 + 0.5 * 0 = 0.0

(In reply to Boris Chiou [:boris] from comment #8) > Another similar case is > > var anim = div.animate([ { marginLeft: '0px' }, > { marginLeft: '0px' }, > { marginLeft: '100px' }, > { marginLeft: '50px' } ], > { duration: 1000, spacing: 'paced(margin-left)' }); > > anim.effect.frame[0].computedOffset = 0.0 > anim.effect.frame[1].computedOffset = 0.0 // This is 0.0, right? > anim.effect.frame[2].computedOffset = 0.667 > anim.effect.frame[3].computedOffset = 1.0 So, according to the spec we have: pacedA = frame[0] pacedB = frame[3] dist|paced B| = 150 For frame[1] computedOffset = offset|paced A| + (offset|paced B| − offset|paced A|) × dist|k| / dist|paced B| = 0.0 + (1.0 - 0.0) * 0.0 / 150 = 0.0 + 1 * 0.0 = 0.0 For frame[2] computedOffset = offset|paced A| + (offset|paced B| − offset|paced A|) × dist|k| / dist|paced B| = 0.0 + (1.0 - 0.0) * 100 / 150 = 0.0 + 1 * 0.667 = 0.667

OK. Thanks. BTW, there is still a special case: var anim = div.animate([ { marginLeft: '0px' }, { marginLeft: '0px' }, { marginLeft: '0px' }, { marginLeft: '0px' } ], { duration: 1000, spacing: 'paced(margin-left)' }); The cumulative distance of |Paced B| would be 0, so I think we have to fall it back to distribute spacing.

(In reply to Boris Chiou [:boris] from comment #12) > OK. Thanks. > > BTW, there is still a special case: > > var anim = div.animate([ { marginLeft: '0px' }, > { marginLeft: '0px' }, > { marginLeft: '0px' }, > { marginLeft: '0px' } ], > { duration: 1000, spacing: 'paced(margin-left)' }); > > The cumulative distance of |Paced B| would be 0, so I think we have to fall > it back to distribute spacing. Yeah, dist|paced B| will be zero so the division is undefined. I guess distribute spacing makes the most sense here. I've updated the spec to include this.[1] [1] https://github.com/w3c/web-animations/commit/ea84078dbc9fd2ccb759c3ef5096a24ec3a1f604

Hi, Brian, I haven't implemented writable spacing. Part 1 ~ Part 10 are for read-only spacing mode. I think we could review them first. Thanks.

https://reviewboard.mozilla.org/r/51809/#review49369 ::: dom/animation/KeyframeUtils.cpp:495 (Diff revision 1) > + // d) Fill non-assigned computed offsets in (Paced A, Paced B). > + for (size_t k = pA + 1; k < pB; ++k) { > + if (aKeyframes[k].mComputedOffset) { > + continue; > + } > + > + size_t startIdx = k - 1; > + size_t endIdx = k + 1; > + while (!aKeyframes[endIdx].mComputedOffset && endIdx < pB) { > + ++endIdx; > + } > + ApplyDistributingOffset(aKeyframes, startIdx, endIdx); > + k = endIdx + 1; > + } I'm thinking do we have any way to skip this step. Maybe we can use a counter, e.g. uint32_t assignedKeyframes = aKeyframes.length, and then minus one if we assign a mComputedOffset each time. If assignedKeyframes == 0 at this step, we could skip this loop, or early return if all the mComputedOffsets are assigned in the loop.

Comment on attachment 8752104 [details] Bug 1244590 - Part 1: Introduce KeyframeEffectParams. https://reviewboard.mozilla.org/r/51507/#review49588 Thanks for looking into this. I'd like to look at this again with the changes made since I think it will change quite a lot. ::: dom/animation/KeyframeEffect.h:24 (Diff revision 1) > #include "mozilla/ComputedTiming.h" > #include "mozilla/ComputedTimingFunction.h" > #include "mozilla/EffectCompositor.h" > #include "mozilla/LayerAnimationInfo.h" // LayerAnimations::kRecords > #include "mozilla/Maybe.h" > #include "mozilla/OwningNonNull.h" // OwningNonNull<...> Nit: While you're touching this file, can you remove this no-longer-needed include? Thanks. ::: dom/animation/KeyframeEffect.h:197 (Diff revision 1) > { > public: > KeyframeEffectReadOnly(nsIDocument* aDocument, > const Maybe<OwningAnimationTarget>& aTarget, > - const TimingParams& aTiming); > + const TimingParams& aTiming, > + const Maybe<KeyframeEffectOptionsData>& aOptions); Based on the changes to KeyframeEffectOptionsData mentioned below, I think this object will be sufficiently lightweight that we can just always pass a const reference to one (i.e. no need for Maybe). ::: dom/animation/KeyframeEffect.h:239 (Diff revision 1) > > IterationCompositeOperation IterationComposite() const; > CompositeOperation Composite() const; > - void GetSpacing(nsString& aRetVal) const { > - aRetVal.AssignLiteral("distribute"); > + void GetSpacing(nsString& aRetVal) const > + { > + aRetVal = mEffectOptions.mSpacing; Later on I'm suggesting we store an enum and nsCSSProperty pair to represent the spacing mode. Let's add a method to serialize those parameters to KeyframeEffectOptionsData and call that here. ::: dom/animation/KeyframeEffectOptionsData.h:19 (Diff revision 1) > +namespace dom { > + > +enum class IterationCompositeOperation : uint32_t; > +enum class CompositeOperation : uint32_t; > + > +struct KeyframeEffectOptionsData I wonder if we can call this KeyframeEffectParams? To make up with TimingParams? ::: dom/animation/KeyframeEffectOptionsData.h:21 (Diff revision 1) > +enum class IterationCompositeOperation : uint32_t; > +enum class CompositeOperation : uint32_t; > + > +struct KeyframeEffectOptionsData > +{ > + KeyframeEffectOptionsData(); As mentioned below, if we use member initializers, I think we can drop this. ::: dom/animation/KeyframeEffectOptionsData.h:23 (Diff revision 1) > + explicit KeyframeEffectOptionsData( > + const Maybe<KeyframeEffectOptionsData>& aOptions); If we make the changes suggested earlier, I think we can drop this. (In general, I think it's a bit odd to have a class with a constructor that take a Maybe<> version of itself. The probably suggests we should be doing something differently elsewhere.) ::: dom/animation/KeyframeEffectOptionsData.h:26 (Diff revision 1) > + IterationCompositeOperation mIterationComposite; > + CompositeOperation mComposite; It might be better to leave these out until we need them and just add a // FIXME comment for this? ::: dom/animation/KeyframeEffectOptionsData.h:28 (Diff revision 1) > + explicit KeyframeEffectOptionsData( > + const Maybe<KeyframeEffectOptionsData>& aOptions); > + > + IterationCompositeOperation mIterationComposite; > + CompositeOperation mComposite; > + nsString mSpacing = NS_LITERAL_STRING("distribute"); Let's just make this an enum and an nsCSSProperty? And initialize them here to 'distribute' and eCSSProperty_UNKNOWN. Then we don't need a constructor at all.

Comment on attachment 8752105 [details] Bug 1244590 - Part 2: Retrieve KeyframeEffectOptions from constructor. https://reviewboard.mozilla.org/r/51509/#review49590 This is mostly fine but I think it will change a lot based on the changes to part 1 so I'd like to look at it again. ::: dom/animation/KeyframeEffect.cpp:701 (Diff revision 1) > +static const KeyframeEffectOptions& > +GetAsEffectOptions(const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions) > +{ > + MOZ_ASSERT(aOptions.IsKeyframeEffectOptions()); > + return aOptions.GetAsKeyframeEffectOptions(); > +} What is the reasoning behind putting these methods here? I think it might make more sense to declare them just before we use them? ::: dom/animation/KeyframeEffect.cpp:702 (Diff revision 1) > KeyframeEffectReadOnly::~KeyframeEffectReadOnly() > { > } > > +static const KeyframeEffectOptions& > +GetAsEffectOptions(const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions) This naming is a bit awkward: GetAsEffectOptions vs GetEffectOptions further down. How about: * KeyframeEffectOptionsFromUnion() * KeyframeEffectParamsFromUnion() ::: dom/animation/KeyframeEffect.cpp:719 (Diff revision 1) > + Maybe<KeyframeEffectOptionsData> result; > + if (!aOptions.IsUnrestrictedDouble()) { > + const KeyframeEffectOptions& options = GetAsEffectOptions(aOptions); > + result.emplace(options); > + } > + return result; Based on my feedback to part 1, I think this will change quite a bit. We'll probably want to do the parsing here and return a KeyframeEffectParams object. We'll also like need to pass in an ErrorResult object so we can report parse errors.

Comment on attachment 8752106 [details] Bug 1244590 - Part 3: Parse spacing. https://reviewboard.mozilla.org/r/51511/#review49592 ::: dom/animation/KeyframeEffectOptionsData.h:31 (Diff revision 1) > + bool ParseSpacing(nsIDocument* aDoc, ErrorResult& aRv); > + Let's make this a static method like we do with TimingParams. It can return the spacing mode and property and then we can call this to check for errors before setting up the object (similar to what we do in TimingParamsFromOptionsUnion). (I think the pattern of having a partially-initialized object where mSpacing is filled-in but the other parameters are not, and then calling ParseSpacing isn't ideal. It means the mSpacing can be out-of-sync with the other members.) ::: dom/animation/KeyframeEffectOptionsData.h:36 (Diff revision 1) > + // We use SpacingMode and nsCSSProperty pair to know which property should > + // be paced. mSpacing is the original string. > + enum class SpacingMode > + { > + distribute, > + paced > + }; > + SpacingMode mSpacingMode = SpacingMode::distribute; > + nsCSSProperty mPacedProperty = eCSSProperty_UNKNOWN; Yes, this is more like what I had in mind in part 1. I don't think we need to store the original string, however. We can serialize from the parsed properties as needed. ::: dom/animation/KeyframeEffectOptionsData.cpp:50 (Diff revision 1) > +KeyframeEffectOptionsData::ParseSpacing(nsIDocument* aDoc, > + ErrorResult& aRv) > +{ > + // 1. distribute > + if (mSpacing.EqualsLiteral("distribute")) { > + return true; Probably we should just make the return type void here and require that the caller checks aRv.Failed(). That's better than having two different means of reporting failure (because then we avoid the situation where, e.g. we return false, but !aRv.Failed()). ::: dom/animation/KeyframeEffectOptionsData.cpp:53 (Diff revision 1) > + // 2. paced(css-property). > + // e.g. "paced(margin-left)" is accepted, > + // "paced(marginLeft)" is not accepted. > + // "paced()" is not accepted. > + // "paced( margin-left)" is not accepted. > + nsString regex(NS_LITERAL_STRING("paced\\([a-z-]+\\)")); > + if (nsContentUtils::IsPatternMatching(mSpacing, regex, aDoc)) { I don't think we want to use regexs here. Their performance is typically not good, and it's really easy to fall off a performance cliff any time you tweak it. I was thinking we should probably follow CSS parsing rules here (e.g. recognize and ignore comments etc.) but it doesn't look like that's particularly easy to do. We can't reuse nsCSSScanner for example. So we'd probably need to modify nsCSSParser to support this. So, for now, I think we can just say this attribute doesn't support CSS parsing rules. Instead, we can just do some manual parsing like we do in dom/smil/nsSMILParserUtils.cpp. (I think we try to use iterators and aStr.BeginReading etc. but I notice we're also using ranged pointers there a bit.) If we later introduce animation-spacing etc. then we can add it to the CSS parser then and support CSS comments etc. from the API.

https://reviewboard.mozilla.org/r/51507/#review49598 ::: dom/animation/KeyframeEffectOptionsData.h:13 (Diff revision 1) > +namespace mozilla { > +namespace dom { Oh, one more note, I don't think this needs to be in the dom namespace. I think our approach so far is to only put objects exposed through the API there.

Comment on attachment 8752107 [details] Bug 1244590 - Part 4: Rewrite ApplyDistributeSpacing. https://reviewboard.mozilla.org/r/51549/#review49600 ::: dom/animation/KeyframeEffect.cpp:470 (Diff revision 1) > if (KeyframesEqualIgnoringComputedOffsets(aFrames, mFrames)) { > return; > } > > mFrames = Move(aFrames); > - KeyframeUtils::ApplyDistributeSpacing(mFrames); > + KeyframeUtils::ApplySpacing(mFrames, mEffectOptions.mSpacingMode); When I added this line, it was only intended to be temporary. I wonder if we need to do spacing in SetFrames? We only *need* the computed offsets to be filled-in when: a) We compose style b) We get a call to GetKeyframes However, we need to update computed offsets whenever the style context changes in which case UpdateProperties will be called so we could just apply spacing there. However, we might get a call to GetKeyframes when we don't have a style context so we *could* do something like: * In SetFrames just use distribute spacing (it's fairly cheap and always works). * In UpdateProperties apply the correct spacing. Does that work? This here is fine for now, but the above might be slightly simpler/cheaper, I'm not sure. ::: dom/animation/KeyframeUtils.h:55 (Diff revision 1) > GetKeyframesFromObject(JSContext* aCx, > JS::Handle<JSObject*> aFrames, > ErrorResult& aRv); > > /** > - * Fills in the mComputedOffset member of each keyframe in the given array > + * Fills in the mComputedOffset member of each keyframe in the given array. ... 'using the specified spacing mode.' You also need to document the additional aSpacingMode parameter. ::: dom/animation/KeyframeUtils.h:57 (Diff revision 1) > > /** > - * Fills in the mComputedOffset member of each keyframe in the given array > + * Fills in the mComputedOffset member of each keyframe in the given array. > - * using the "distribute" spacing algorithm. > * > * http://w3c.github.io/web-animations/#distribute-keyframe-spacing-mode This is no longer correct. ::: dom/animation/KeyframeUtils.cpp:1142 (Diff revision 1) > +static void > +ApplyDistributingOffset(nsTArray<Keyframe>& aKeyframes, > + size_t aStartIdx, size_t aEndIdx) How about we call this DistributeRange and pass a mozilla::Range ?

Comment on attachment 8752108 [details] Bug 1244590 - Part 5: Make the default value of computed offsets be -1. https://reviewboard.mozilla.org/r/51505/#review49606 I wonder if it would be simpler/cheaper to just set mComputedOffset to -1 to represent this (we can define a constant like UNRESOLVED_OFFSET to this)? Maybe it doesn't matter. I'll have another look after we decide what to do with the next patch. What do you think? At very least, I'd want to make this patch assert that mComputedOffset is filled-in when we get a call to GetKeyframes (not just use valueOr(0.0)).

Comment on attachment 8752109 [details] Bug 1244590 - Part 7: Calculate paced spacing. https://reviewboard.mozilla.org/r/51809/#review49602 We need to work out what should happen for shorthands. Is it reasonably to want to pace 'margin'? Unfortunately, I think it might be since often properties get "promoted" from longhands to shorthands so if we say that the paced property must be a longhand, content will stop working. That, unfortunately complicates this a lot and probably means the spec needs to be changed. In any case, I think we should try to use KeyframeEffectReadOnly::mProperties rather than recalculating the StyleAnimationValues using ComputeValues. The reasons are: * We should probably use the values in mProperties since they will be up to date and reflect what we'll actually animate * If mProperties is empty we shouldn't do pacing anyway * It saves recalculating values which we already have available * mProperties is arranged by property so we don't need to cache the paced property. * Whenever we update mProperties we need to re-apply paced spacing so there's a natural connection there I haven't looked through the rest of the algorithms yet since I think this patch will change quite a bit based on what we do here. I think what we might end up doing, however, is, for the case of a shorthand, looking up the component properties and doing some sort of squared distance calculation for all the longhand components (like we do for color here: https://www.w3.org/TR/SVG11/animate.html#complexDistances). The tricky bit is how to define whether or not a keyframe has the paced property. We could say any keyframe having any of the longhand components counts but then what value would we use for the missing components? I think we might define it as any keyframe specifying *all* the longhand components. I need to think about it a bit more. What do you think? ::: dom/animation/KeyframeEffect.h:118 (Diff revision 1) > + mPacedPropertyValue.emplace(pair); > + return; > + } > + } > + } > + bool IsPaceable() const { return static_cast<bool>(mPacedPropertyValue); } Is the static_cast necessary? Doesn't operator bool()[1] does that for us? [1] https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/mfbt/Maybe.h#154 ::: dom/animation/KeyframeEffect.cpp:471 (Diff revision 1) > + if (mEffectOptions.mSpacingMode == SpacingMode::paced && aStyleContext) { > + // Cache paced property at each frame. > + for (Keyframe& frame : mFrames) { > + frame.UpdatePacedPropertyValue(mEffectOptions.mPacedProperty); > + } > + // If we have aStyleContext, mTarget must be non-null. > + KeyframeUtils::ApplySpacing(mFrames, SpacingMode::paced, > + mTarget->mElement, aStyleContext); > + } else { > + // Fall back to distribute spacing. > + KeyframeUtils::ApplySpacing(mFrames, SpacingMode::distribute); > + } As mentioned in part 4, we need to re-apply spacing when the style context changes, i.e. UpdateProperties not in SetFrames. ::: dom/animation/KeyframeUtils.h:56 (Diff revision 1) > JS::Handle<JSObject*> aFrames, > ErrorResult& aRv); > > /** > * Fills in the mComputedOffset member of each keyframe in the given array. > + * We need dom::Element and nsStyleContext only if the spacing mode is paced. How about, "If dom::Element or nsStyleContext are null, paced spacing mode falls back to distribute spacing made (as per spec)." ::: dom/animation/KeyframeUtils.h:62 (Diff revision 1) > * > * http://w3c.github.io/web-animations/#distribute-keyframe-spacing-mode > * > - * @param keyframes The set of keyframes to adjust. > + * @param aKeyframes The set of keyframes to adjust. > + * @param aSpacingMode The applied spacing mode to aKeyframes. > + * @param aElement the context element. The context element for resolving paced property values against. ::: dom/animation/KeyframeUtils.cpp:1221 (Diff revision 1) > +ApplyPacedOffset(nsTArray<Keyframe>& aKeyframes, > + size_t aPacedA, size_t aPacedB, Can we call this PaceRange and pass a mozilla::Range? ::: dom/animation/KeyframeUtils.cpp:1244 (Diff revision 1) > + nsTArray<PropertyStyleAnimationValuePair> values; > + if (!StyleAnimationValue::ComputeValues(property, > + CSSEnabledState::eForAllContent, aElement, aStyleContext, > + aKeyframes[aPacedA].mPacedPropertyValue->mValue, false, values)) { This isn't right. ComputeValues() returns an array of values but we're only ever looking at the first one. We need to decide how shorthands should work.

https://reviewboard.mozilla.org/r/51507/#review49588 > Based on the changes to KeyframeEffectOptionsData mentioned below, I think this object will be sufficiently lightweight that we can just always pass a const reference to one (i.e. no need for Maybe). OK. I will use const ref in the constructors. > I wonder if we can call this KeyframeEffectParams? To make up with TimingParams? OK. I will rename it. > If we make the changes suggested earlier, I think we can drop this. (In general, I think it's a bit odd to have a class with a constructor that take a Maybe<> version of itself. The probably suggests we should be doing something differently elsewhere.) OK. It makes sense to drop them. > It might be better to leave these out until we need them and just add a // FIXME comment for this? OK. I will add a // FIXME and drop these definitions. > Let's just make this an enum and an nsCSSProperty? > > And initialize them here to 'distribute' and eCSSProperty_UNKNOWN. Then we don't need a constructor at all. OK. I will use enum and nsCSSProperty directly and add a method to serialize those parameters. Thanks.

Comment on attachment 8752110 [details] Bug 1244590 - Part 8: Rewrite GetStyleContext code. https://reviewboard.mozilla.org/r/51895/#review49626 ::: dom/animation/KeyframeEffect.h:395 (Diff revision 1) > void RequestRestyle(EffectCompositor::RestyleType aRestyleType); > > + // We need to be careful to *not* call this when we are updating the style > + // context. That's because calling GetStyleContextForElement when we are in > + // the process of building a style context may trigger various forms of > + // infinite recursion. We should probably mention what this does, e.g. "Looks up the style context associated with the target element, if any." We should also mention that if aDoc is not specified, we will use the owner doc of the target element. Normally we try to avoid default parameters, but maybe it's ok here. ::: dom/animation/KeyframeEffect.cpp:699 (Diff revision 1) > +already_AddRefed<nsStyleContext> > +KeyframeEffectReadOnly::GetTargetStyleContext(nsIDocument* aDoc) > +{ The order in the .cpp file doesn't seem to match the order in the .h file. ::: dom/animation/KeyframeEffect.cpp:713 (Diff revision 1) > + nsIAtom* pseudo = mTarget->mPseudoType < CSSPseudoElementType::Count ? > + nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType) : > + nullptr; While we're touching this, we should move the ? and : to the next line. I'm pretty sure Moz coding-style is: nsIAtom* pseudo = mTarget->mPseudoType < CSSPseudoElementType::Count ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType) : nullptr;

Comment on attachment 8752111 [details] Bug 1244590 - Part 9: Update spacing in SetTarget. https://reviewboard.mozilla.org/r/52137/#review49610 ::: dom/animation/KeyframeEffect.cpp:1480 (Diff revision 1) > + if (mEffectOptions.mSpacingMode == SpacingMode::paced && styleContext) { > + KeyframeUtils::ApplySpacing(mFrames, SpacingMode::paced, > + mTarget->mElement, styleContext); > + } else { > + // No style context, so fall back to distribute spacing. > + KeyframeUtils::ApplySpacing(mFrames, SpacingMode::distribute); > + } > + > if (styleContext) { > UpdateProperties(styleContext); > } As discussed in previous patches I think we should make UpdateProperties update spacing. In the case where styleContext is null, however, we need to apply distribute spacing. ::: dom/animation/KeyframeUtils.cpp:453 (Diff revision 1) > (aSpacingMode == dom::SpacingMode::paced && aElement && > aStyleContext), > "Paced spacing mode should have valid context element and " > "style context"); > > + // We should clean up the computed offset first if using paced spacing. // Reset the computed offsets if using paced spacing.

Comment on attachment 8752112 [details] Bug 1244590 - Part 10: Test for creating animations with a specific spacing mode. https://reviewboard.mozilla.org/r/52139/#review49604 ::: testing/web-platform/meta/MANIFEST.json:35408 (Diff revision 1) > + "path": "web-animations/keyframe-effect/spacingMode.html", > + "url": "/web-animations/keyframe-effect/spacingMode.html" The attribute is just called 'spacing'. Also, we should probably put these tests in: animation-model/keyframe-effects/spacing-keyframes.html As suggested in [1] [1] https://github.com/w3c/web-platform-tests/blob/master/web-animations/README.md ::: testing/web-platform/tests/web-animations/keyframe-effect/spacingMode.html:119 (Diff revision 1) > + var slots = frames.length - 1; > + assert_equals(frames[1].computedOffset, 1.0 / slots, '2nd frame offset'); > + assert_equals(frames[2].computedOffset, 2.0 / slots, '3rd frame offset'); > +}, 'Test falling back to distribute spacing mode if all paced property value ' + > + 'are not changed'); > + I think there are a few more tests needed including: * Specifying a paced property that is not used anywhere. * Perhaps even one for a property that is not supported / doesn't exist. * Various parsing tests -- e.g. that adding extra spaces triggers an error (this is actually really important because it's a common source of interop issues). * Specifying a paced property that appears on only some keyframes. * Various combinations of this with fixed and missing offsets. There are some tricky cases here I think. * Specifying a shorthand property as a paced property (e.g. 'margin') Also, maybe this belongs in the next patch, but we need to test when the style context changes things like font-size and the dimension against which percentages are resolved. e.g. when you have values for margin-left of 3em, 50%, 4em etc.

Comment on attachment 8752113 [details] Bug 1244590 - Part 11: Test for different targets. https://reviewboard.mozilla.org/r/52151/#review49628 Looks good to me although please re-request review if you decide to add the tests mentioned in the review for part 9 to this. Also, we will need to: * Move the tests from spacingMode.html to the test file mentioned in part 9 * Do a big s/getFrames/getKeyframes/g and s/frames/keyframes/g

Oh, we also need to test that distance works as expected when mixing % values and px values for margin-left. (It should work, since the common unit will become 'calc' and we're able to calculate distances for calc values.)

https://reviewboard.mozilla.org/r/51509/#review49590 > What is the reasoning behind putting these methods here? > > I think it might make more sense to declare them just before we use them? ConvertTarget is also used by KeyframeEffectReadOnly::ConstructKeyframeEffect(), so I put these functions before ConvertTarget() according to the order of usage. > Based on my feedback to part 1, I think this will change quite a bit. We'll probably want to do the parsing here and return a KeyframeEffectParams object. > > We'll also like need to pass in an ErrorResult object so we can report parse errors. I agree. I will pass in an ErrorResult object and leave a comment which says "do parsing here in part 3".

(In reply to Brian Birtles (:birtles) from comment #38) > Oh, we also need to test that distance works as expected when mixing % > values and px values for margin-left. (It should work, since the common unit > will become 'calc' and we're able to calculate distances for calc values.) Sure. I will add them into part 9.

https://reviewboard.mozilla.org/r/51511/#review49592 > I don't think we want to use regexs here. Their performance is typically not good, and it's really easy to fall off a performance cliff any time you tweak it. > > I was thinking we should probably follow CSS parsing rules here (e.g. recognize and ignore comments etc.) but it doesn't look like that's particularly easy to do. We can't reuse nsCSSScanner for example. So we'd probably need to modify nsCSSParser to support this. > > So, for now, I think we can just say this attribute doesn't support CSS parsing rules. > > Instead, we can just do some manual parsing like we do in dom/smil/nsSMILParserUtils.cpp. (I think we try to use iterators and aStr.BeginReading etc. but I notice we're also using ranged pointers there a bit.) > > If we later introduce animation-spacing etc. then we can add it to the CSS parser then and support CSS comments etc. from the API. OK. Let me check nsSMILParserUtils.cpp first and try to add some manual parsing into KeyframeEffectParams.h

https://reviewboard.mozilla.org/r/51549/#review49600 > When I added this line, it was only intended to be temporary. I wonder if we need to do spacing in SetFrames? > > We only *need* the computed offsets to be filled-in when: > > a) We compose style > b) We get a call to GetKeyframes > > However, we need to update computed offsets whenever the style context changes in which case UpdateProperties will be called so we could just apply spacing there. > > However, we might get a call to GetKeyframes when we don't have a style context so we *could* do something like: > > * In SetFrames just use distribute spacing (it's fairly cheap and always works). > * In UpdateProperties apply the correct spacing. > > Does that work? > > This here is fine for now, but the above might be slightly simpler/cheaper, I'm not sure. OK. I will try to do this. > How about we call this DistributeRange and pass a mozilla::Range ? Cool. I can try it.

https://reviewboard.mozilla.org/r/51505/#review49606 I agree. I just need a special value to check if a computed offset is _null_ or assigned to 0 intentionally.

https://reviewboard.mozilla.org/r/51809/#review49602 I. If we want to reuse the StyleAnimationValues in mProperties, we have to build segments first, and then apply paced spacing, so the order is: 1. SetKeyframes a. Apply distribute spacing forcely 2. UpdateProperties a. Build segments b. Apply paced spacing by the current segments (in mProperties) because we can reuse the StyleAnimationValues. My question is: The mFromKey and mToKey of each segment are assigned from Keyframe::mComputedOffsets, so if we apply paced spacing in the last step, the mFromKey and ToKey of each segement are not correct until we call UpdateProperties() next time. Is this acceptable? II. For the shorthand, it is really a big problem. As you mentioned, the missing components are the problem. If there is any missing component in a keyframe, we cannot calculate the diff between this keyframe and others easily. Maybe we could add some restrictions. e.g. 1) [ { marginLeft: '0px' }, { marginRight: '-20px' }, { marginTop: '100px' } ], { duration: 1000, spacing: 'paced(margin)' } Fall back to distribute spacing because there is no common longhand component of magin. 2) [ { marginLeft: '0px' }, { marginLeft: '10px', marginRight: '-20px' }, { marginLeft: '20px', marginTop: '100px' } ], { duration: 1000, spacing: 'paced(margin)' } We can calculate the distance by margin-left. 3) [ { marginLeft: '0px', marginRight: '0px' }, { marginLeft: '10px', marginRight: '-20px' }, { marginLeft: '20px', marginRight: '100px', marginTop: '100px' } ], { duration: 1000, spacing: 'paced(margin)' } We can calculate the distance by sqrt(diff(margin-left) * diff(margin-left) + diff(margin-right) * diff(margin-right)). Does it make sense? > Is the static_cast necessary? Doesn't operator bool()[1] does that for us? > > [1] https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/mfbt/Maybe.h#154 If we return mPacedPropertyValue directly, I got this error: error: no viable conversion from returned value of type 'const Maybe<mozilla::PropertyValuePair>' to function return type 'bool' Therefore, I add the static_cast<bool>.

https://reviewboard.mozilla.org/r/51809/#review49602 Sorry, I forgot to consider partially paced. I should think about it more.

(In reply to Boris Chiou [:boris] from comment #44) > https://reviewboard.mozilla.org/r/51809/#review49602 > > I. > If we want to reuse the StyleAnimationValues in mProperties, we have to > build segments first, and then apply paced spacing, so the order is: > 1. SetKeyframes > a. Apply distribute spacing forcely > 2. UpdateProperties > a. Build segments > b. Apply paced spacing by the current segments (in mProperties) because we > can reuse the StyleAnimationValues. > > My question is: The mFromKey and mToKey of each segment are assigned from > Keyframe::mComputedOffsets, so if we apply paced spacing in the last step, > the mFromKey and ToKey of each segement are not correct until we call > UpdateProperties() next time. Is this acceptable? Oh, good point. Hmm, I'm not sure how that should work then. I guess we need to do something like: 1. SetKeyframes a. Apply distribute spacing 2. UpdateProperties a. Apply actual spacing mode If spacing mode is paced, calculate actual computed values at this point. This includes taking care to ignore invalid values, expanding shorthands etc. b. KeyframeUtils::GetAnimationPropertiesFromKeyframes So there will be some redundancy between (a) and (b) but it's probably ok. We can factor out common code in KeyframeUtils (I imagine (a) will happen in KeyframeUtils), and this will only happen when we use paced timing (rare) and only when we update properties (hopefully fairly rare). > II. > For the shorthand, it is really a big problem. As you mentioned, the missing > components are the problem. If there is any missing component in a keyframe, > we cannot calculate the diff between this keyframe and others easily. Maybe > we could add some restrictions. > > e.g. > 1) > [ { marginLeft: '0px' }, > { marginRight: '-20px' }, > { marginTop: '100px' } ], > { duration: 1000, spacing: 'paced(margin)' } > > Fall back to distribute spacing because there is no common longhand > component of magin. Yes, I think so. > 2) > [ { marginLeft: '0px' }, > { marginLeft: '10px', marginRight: '-20px' }, > { marginLeft: '20px', marginTop: '100px' } ], > { duration: 1000, spacing: 'paced(margin)' } > > We can calculate the distance by margin-left. I don't know if this is right. I think we will need to require all components. > 3) > [ { marginLeft: '0px', marginRight: '0px' }, > { marginLeft: '10px', marginRight: '-20px' }, > { marginLeft: '20px', marginRight: '100px', marginTop: '100px' } ], > { duration: 1000, spacing: 'paced(margin)' } > > We can calculate the distance by sqrt(diff(margin-left) * diff(margin-left) > + diff(margin-right) * diff(margin-right)). > > Does it make sense? Yes, but I don't think that will give the correct result when some of the components are missing.

(In reply to Brian Birtles (:birtles) from comment #46) > 2. UpdateProperties > a. Apply actual spacing mode > If spacing mode is paced, calculate actual computed values at this point. > This includes taking care to ignore invalid values, expanding > shorthands etc. > b. KeyframeUtils::GetAnimationPropertiesFromKeyframes > > So there will be some redundancy between (a) and (b) but it's probably ok. > We can factor out common code in KeyframeUtils (I imagine (a) will happen in > KeyframeUtils), and this will only happen when we use paced timing (rare) > and only when we update properties (hopefully fairly rare). OK. I will try to factor out the calculation of StyleAnimationValues in ApplySpacing() and GetAnimationPropertiesFromKeyframes(). After finishing the calculation of StyleAnimationValue, pass it (an array, I guess), to both functions, so we can reuse it. Maybe more parts could be factored out, so I should go through them again. About the shorthand case, in this phase we only support paced if users define _all_ components of the specific shorthand property (or use shorthand in the keyframes directly). e.g. All the paceable keyframes should have all components; otherwise, we fall it back to distribute spacing. So, > [ { marginLeft: '0px' }, > { marginLeft: '10px', marginRight: '-20px' }, > { marginLeft: '20px', marginTop: '100px' } ], > { duration: 1000, spacing: 'paced(margin)' } will fall back to distribute, and > [ { marginLeft: '0px', marginRight: '0px' }, > { marginLeft: '10px', marginRight: '-20px' }, > { marginLeft: '20px', marginRight: '100px', marginTop: '100px' } ], > { duration: 1000, spacing: 'paced(margin)' } also falls back to distribute. Only [ { marginLeft: '0px', marginRight: '0px', marginTop: 'xxx', marginBottom: 'xxx' }, { marginLeft: '10px', marginRight: '-20px', marginTop: 'xxx', marginBottom: 'xxx' }, { marginLeft: '20px', marginRight: '100px', marginTop: '100px', marginBottom: 'xxx' } ], { duration: 1000, spacing: 'paced(margin)' } uses paced spacing.

(In reply to Boris Chiou [:boris] from comment #47) > (In reply to Brian Birtles (:birtles) from comment #46) > > 2. UpdateProperties > > a. Apply actual spacing mode > > If spacing mode is paced, calculate actual computed values at this point. > > This includes taking care to ignore invalid values, expanding > > shorthands etc. > > b. KeyframeUtils::GetAnimationPropertiesFromKeyframes > > > > So there will be some redundancy between (a) and (b) but it's probably ok. > > We can factor out common code in KeyframeUtils (I imagine (a) will happen in > > KeyframeUtils), and this will only happen when we use paced timing (rare) > > and only when we update properties (hopefully fairly rare). > > OK. I will try to factor out the calculation of StyleAnimationValues in > ApplySpacing() and GetAnimationPropertiesFromKeyframes(). After finishing > the calculation of StyleAnimationValue, pass it (an array, I guess), to both > functions, so we can reuse it. Maybe more parts could be factored out, so I > should go through them again. Thanks. Don't worry too much about getting the right factoring of methods first time around. I can give some suggestions during review. Firstly, let's just see if we can get this to do something sensible. > About the shorthand case, in this phase we only support paced if users > define _all_ components of the specific shorthand property (or use shorthand > in the keyframes directly). > e.g. All the paceable keyframes should have all components; otherwise, we > fall it back to distribute spacing. > So, > > [ { marginLeft: '0px' }, > > { marginLeft: '10px', marginRight: '-20px' }, > > { marginLeft: '20px', marginTop: '100px' } ], > > { duration: 1000, spacing: 'paced(margin)' } > will fall back to distribute, and > > [ { marginLeft: '0px', marginRight: '0px' }, > > { marginLeft: '10px', marginRight: '-20px' }, > > { marginLeft: '20px', marginRight: '100px', marginTop: '100px' } ], > > { duration: 1000, spacing: 'paced(margin)' } > also falls back to distribute. > > Only > [ { marginLeft: '0px', marginRight: '0px', marginTop: 'xxx', marginBottom: > 'xxx' }, > { marginLeft: '10px', marginRight: '-20px', marginTop: 'xxx', > marginBottom: 'xxx' }, > { marginLeft: '20px', marginRight: '100px', marginTop: '100px', > marginBottom: 'xxx' } ], > { duration: 1000, spacing: 'paced(margin)' } > uses paced spacing. Right. I think that's what we want. I think it's reasonable to be able to pace: [ { margin: '10px' }, { margin: '0px' }, { margin: '20px' } ] And, like I mentioned, we have a habit of promoting longhands to shorthands so it's good if we can support shorthands. However, if we have: [ { margin: '10px' }, { marginLeft: '0px' }, { marginTop: '5px' }, { margin: '10px' }, { margin: '20px' } ] What is the expected result? I'm not really sure.

(In reply to Brian Birtles (:birtles) from comment #48) > However, if we have: > > [ { margin: '10px' }, > { marginLeft: '0px' }, > { marginTop: '5px' }, > { margin: '10px' }, > { margin: '20px' } ] > > What is the expected result? I'm not really sure. All five keyframes are paceable, but we cannot calculate the distances of "1st & 2nd", "2nd & 3rd", and "3rd & 4th". Therefore, I prefer to fall it back to distribute spacing. (Ha, so many restrictions for shorthand, but I found falling back to distribute makes implementation easier.)

(In reply to Boris Chiou [:boris] from comment #49) > (In reply to Brian Birtles (:birtles) from comment #48) > > However, if we have: > > > > [ { margin: '10px' }, > > { marginLeft: '0px' }, > > { marginTop: '5px' }, > > { margin: '10px' }, > > { margin: '20px' } ] > > > > What is the expected result? I'm not really sure. > > All five keyframes are paceable, but we cannot calculate the distances of > "1st & 2nd", "2nd & 3rd", and "3rd & 4th". Therefore, I prefer to fall it > back to distribute spacing. (Ha, so many restrictions for shorthand, but I > found falling back to distribute makes implementation easier.) Or as you mentioned in comment 32, we should treat those keyframes which don't specify *all* the longhand components as not paceable, so only apply paced spacing to the 4th keyframe (with Paced A is the 1st keyframe and Paced B is the 5th keyframe). Therefore, we need a definition of _paceable_ for shorthand property. What do other editors think?

(In reply to Boris Chiou [:boris] from comment #50) > (In reply to Boris Chiou [:boris] from comment #49) > > (In reply to Brian Birtles (:birtles) from comment #48) > > > However, if we have: > > > > > > [ { margin: '10px' }, > > > { marginLeft: '0px' }, > > > { marginTop: '5px' }, > > > { margin: '10px' }, > > > { margin: '20px' } ] > > > > > > What is the expected result? I'm not really sure. > > > > All five keyframes are paceable, but we cannot calculate the distances of > > "1st & 2nd", "2nd & 3rd", and "3rd & 4th". Therefore, I prefer to fall it > > back to distribute spacing. (Ha, so many restrictions for shorthand, but I > > found falling back to distribute makes implementation easier.) > > Or as you mentioned in comment 32, we should treat those keyframes which > don't specify *all* the longhand components as not paceable, so only apply > paced spacing to the 4th keyframe (with Paced A is the 1st keyframe and > Paced B is the 5th keyframe). Therefore, we need a definition of _paceable_ > for shorthand property. What do other editors think? Yeah, that's my thinking. Paceable keyframe = "The keyframe specifies the paced property. If the paced property is a shorthand property, then the keyframe specifies all the shorthand components for which there is a non-trivial definition of distance with a value for which we can calculate a meaningful distance" ? So, if we have marginLeft: 'auto' I wonder if we should treat that keyframe as paceable for 'margin'? I suspect we shouldn't?

(In reply to Brian Birtles (:birtles) from comment #51) > Yeah, that's my thinking. Paceable keyframe = "The keyframe specifies the > paced property. If the paced property is a shorthand property, then the > keyframe specifies all the shorthand components for which there is a > non-trivial definition of distance with a value for which we can calculate a > meaningful distance" ? Yeah, I think I try this way. Only keyframes specifying all components (sub-properties) are paceable if the paced property is shorthand. The meaningful distance of a shorthand property should be calculated by all components. > > So, if we have marginLeft: 'auto' I wonder if we should treat that keyframe > as paceable for 'margin'? I suspect we shouldn't? Ha, I also think we shouldn't. Thanks.

(In reply to Brian Birtles (:birtles) from comment #38) > Oh, we also need to test that distance works as expected when mixing % > values and px values for margin-left. (It should work, since the common unit > will become 'calc' and we're able to calculate distances for calc values.) When I was writing this test case, I got this problem: e.g. div.style.width = '100px'; var anim = div.animate([ { marginLeft: '0px' }, { marginLeft: '-20%' }, // 100px * -20% = -20px { marginLeft: '100px' }, { marginLeft: '50px' } ], { duration: 100 * MS_PER_SEC, spacing: 'paced(margin-left)' }); The cumulative distances are [0, 20, 140, 190]. (We use app unit during calculation, but I list by css pixel unit.) I think users expect the computed offsets are: [0, 20/190, 140/190, 1] = [0, 0.105, 0.737, 1]. But according to the current implementation of StyleAnimationValue::ComputedDistance(), dist of 1st & 2nd (eUnit_Calc [1]): sqrt(0 * 0 + 0.2 * 0.2) = 0.2. dist of 2nd & 3rd (eUnit_Calc) : sqrt(100 * 100 + 0.2 * 0.2) = 100.0002. (we use css pixel unit if unit is eUnit_Calc. [2]) dist of 3rd & 4th (eUnit_Coord) : 3000 (app unit). so the cumulative distances are [0, 0.2, 100.2002, 3100.2002]. And the actual computed offsets are: [0, 0.2/3100.2002, 100.2002/3100.2002, 1] = [0, 0.00006451, 0.03232, 1]. They are not correct. If we mix % values and pixel values, the summation of distances may have problems. (i.e. Looks like we cannot add them by |+| directly.) StyleAnimationValue::ComputeDistance() returns the distance of type |double|, instead of type |StyleAnimationValue|, so we lost the information of original unit. Therefore, I think we cannot mix the |double| distances which calculated from different common units because the proportion is not correct. Do you have any suggestion for this? For example, do I have to go through all the StyleAnimationValues of each keyframe and convert them into eUnit_Calc if necessary first? However, even if I change them into eUnit_Calc, the answer is still not the same as the expected one because we don't convert the % values into pixel values while calculating the distance of two eUnit_Calc values. [1] We use the square-root of sum of squares as the final distance of eUnit_Calc. https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/layout/style/StyleAnimationValue.cpp#592 [2] When we calculate the distance of eUnit_Calc, we extract the coord value, and then convert it from app unit to css unit. https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/layout/style/StyleAnimationValue.cpp#332

(In reply to Boris Chiou [:boris] from comment #53) > (In reply to Brian Birtles (:birtles) from comment #38) > > Oh, we also need to test that distance works as expected when mixing % > > values and px values for margin-left. (It should work, since the common unit > > will become 'calc' and we're able to calculate distances for calc values.) > > When I was writing this test case, I got this problem: > > e.g. > div.style.width = '100px'; > var anim = div.animate([ { marginLeft: '0px' }, > { marginLeft: '-20%' }, // 100px * -20% = -20px > { marginLeft: '100px' }, > { marginLeft: '50px' } ], > { duration: 100 * MS_PER_SEC, > spacing: 'paced(margin-left)' }); > > The cumulative distances are [0, 20, 140, 190]. (We use app unit during > calculation, but I list by css pixel unit.) > I think users expect the computed offsets are: > [0, 20/190, 140/190, 1] = [0, 0.105, 0.737, 1]. > > But according to the current implementation of > StyleAnimationValue::ComputedDistance(), > dist of 1st & 2nd (eUnit_Calc [1]): sqrt(0 * 0 + 0.2 * 0.2) = 0.2. > dist of 2nd & 3rd (eUnit_Calc) : sqrt(100 * 100 + 0.2 * 0.2) = 100.0002. > (we use css pixel unit if unit is eUnit_Calc. [2]) > dist of 3rd & 4th (eUnit_Coord) : 3000 (app unit). Yeah, we only have computed values, not used values at this point. I wonder if we have any code depending on this particular implementation. I think only SMIL uses ComputeDistance at the moment? And most SVG attributes don't support calc (yet) so I wonder if we can change this behavior to resolve used values in this case.

(In reply to Brian Birtles (:birtles) from comment #54) > Yeah, we only have computed values, not used values at this point. > > I wonder if we have any code depending on this particular implementation. I > think only SMIL uses ComputeDistance at the moment? And most SVG attributes > don't support calc (yet) so I wonder if we can change this behavior to > resolve used values in this case. I checked dxr and looks like both SMIL and nsDOMWindowUtils::ComputeAnimationDistance use ComputeDistance. And test_transitions_per_property also use it [2]! If we want to revise ComputeDistance for eUnit_Calc, we have to make sure these test cases [3] would be passed. [1] https://dxr.mozilla.org/mozilla-central/rev/1806d405c8715949b39fa3a4fc142d14a60df590/dom/base/nsDOMWindowUtils.cpp#2539 [2] https://dxr.mozilla.org/mozilla-central/rev/1806d405c8715949b39fa3a4fc142d14a60df590/layout/style/test/test_transitions_per_property.html#1049 [3] https://dxr.mozilla.org/mozilla-central/rev/1806d405c8715949b39fa3a4fc142d14a60df590/layout/style/test/test_transitions_per_property.html#1263 My idea is: We have to convert the calc value into length (coord) value, so we could calculate the cumulative distances correctly. Therefore, we need a way to resolve the % values in ComputeDistance for different CSS properties.

I believe nsDOMWindowUtils::ComputeAnimationDistance is only provided so we can write unit tests for that function. I don't think it's used by add-ons / browser-chrome etc. If that's the case it shouldn't prevent us from changing the behavior.

(In reply to Brian Birtles (:birtles) from comment #56) > I believe nsDOMWindowUtils::ComputeAnimationDistance is only provided so we > can write unit tests for that function. I don't think it's used by add-ons / > browser-chrome etc. If that's the case it shouldn't prevent us from changing > the behavior. OK. I will add an extra patch to change ComputeDistance for eUnit_Calc (convert computed values to used values).

(In reply to Brian Birtles (:birtles) from comment #54) > I wonder if we have any code depending on this particular implementation. I > think only SMIL uses ComputeDistance at the moment? And most SVG attributes > don't support calc (yet) so I wonder if we can change this behavior to > resolve used values in this case. Resolving used values here may not be easy. There are so many different css properties supporting both length and percentage. e.g. fontSize, margin, padding, etc. If we have to convert a specific calc value into used value, we have to get the reference value first (e.g. parent's width for margin-left, parent's font-size for fontSize, or parent's specific css value for others). It looks like we have to write a huge function to calculate the calc values for different css properties. I wonder if we have another easy way to calculate the computed offsets if we use mix units.

(In reply to Boris Chiou [:boris] from comment #59) > (In reply to Brian Birtles (:birtles) from comment #54) > > I wonder if we have any code depending on this particular implementation. I > > think only SMIL uses ComputeDistance at the moment? And most SVG attributes > > don't support calc (yet) so I wonder if we can change this behavior to > > resolve used values in this case. > > Resolving used values here may not be easy. There are so many different css > properties supporting both length and percentage. e.g. fontSize, margin, > padding, etc. If we have to convert a specific calc value into used value, > we have to get the reference value first (e.g. parent's width for > margin-left, parent's font-size for fontSize, or parent's specific css value > for others). It looks like we have to write a huge function to calculate the > calc values for different css properties. I wonder if we have another easy > way to calculate the computed offsets if we use mix units. Oh. I found we convert font-size into pixel values if we use % values. Looks like only position-related data are needed to be calculated from calc/% value into pixel value. e.g. case eCSSProperty_left: case eCSSProperty_right: case eCSSProperty_top: case eCSSProperty_bottom: case eCSSProperty_margin_left: case eCSSProperty_margin_right: case eCSSProperty_margin_top: case eCSSProperty_margin_bottom: case eCSSProperty_padding_left: case eCSSProperty_padding_right: case eCSSProperty_padding_top: case eCSSProperty_padding_bottom: For the xxx_left, xxx_right values, we need to get the width of its parent element, and for the xxx_top, xxx_bottom values, we need to get the height of its parent element. For other css properties, we just use the corresponding value of its parent element's css property as the reference value. However, how do we calculate if the reference value is still a calc/percent value?

Yeah, you're right. Depending on layout is probably a bad idea. I think we'd be creating a cyclic dependency since the positions of the keyframe offsets could also affect layout. Maybe we just have to stick with the awkward behavior in comment 53.

(In reply to Brian Birtles (:birtles) from comment #61) > Yeah, you're right. Depending on layout is probably a bad idea. I think we'd > be creating a cyclic dependency since the positions of the keyframe offsets > could also affect layout. Maybe we just have to stick with the awkward > behavior in comment 53. OK, thanks. I will stick with the behavior in comment 53. For other cases, I will fall back to the original distance calculation for eUnit_Calc (i.e. sqrt(|length diff| * |length diff| + |percent diff| * |percent diff|)).

Comment on attachment 8752104 [details] Bug 1244590 - Part 1: Introduce KeyframeEffectParams. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51507/diff/1-2/

Comment on attachment 8752105 [details] Bug 1244590 - Part 2: Retrieve KeyframeEffectOptions from constructor. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51509/diff/1-2/

Comment on attachment 8752110 [details] Bug 1244590 - Part 8: Rewrite GetStyleContext code. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51895/diff/1-2/

Comment on attachment 8752111 [details] Bug 1244590 - Part 9: Update spacing in SetTarget. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52137/diff/1-2/

Comment on attachment 8752113 [details] Bug 1244590 - Part 11: Test for different targets. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52151/diff/1-2/

Comment on attachment 8755340 [details] MozReview Request: Bug 1244590 - Part 12: Revise ComputeDistance for eUnit_Calc. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54416/diff/1-2/

Comment on attachment 8755341 [details] MozReview Request: Bug 1244590 - Part 13: Test for different units. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54418/diff/1-2/

Comment on attachment 8752104 [details] Bug 1244590 - Part 1: Introduce KeyframeEffectParams. https://reviewboard.mozilla.org/r/51507/#review51312 ::: dom/animation/KeyframeEffect.h:25 (Diff revision 2) > #include "mozilla/ComputedTimingFunction.h" > #include "mozilla/EffectCompositor.h" > +#include "mozilla/KeyframeEffectParams.h" > #include "mozilla/LayerAnimationInfo.h" // LayerAnimations::kRecords > #include "mozilla/Maybe.h" > -#include "mozilla/OwningNonNull.h" // OwningNonNull<...> > +#include "mozilla/OwningNonNull.h" I thought we were going to remove this include? (The OwningNonNull one) It's not referenced in this header file right? ::: dom/animation/KeyframeEffectParams.h:23 (Diff revision 2) > + void GetSpacing(nsAString& aSpacing) const > + { Do you think GetSpacingAsString would be more clear?

Comment on attachment 8752105 [details] Bug 1244590 - Part 2: Retrieve KeyframeEffectOptions from constructor. https://reviewboard.mozilla.org/r/51509/#review51380

https://reviewboard.mozilla.org/r/51507/#review51312 > I thought we were going to remove this include? (The OwningNonNull one) It's not referenced in this header file right? Yes, you're right. Looks like I forgot to remove this while changing the type of mTiming. > Do you think GetSpacingAsString would be more clear? OK. Looks better.

Comment on attachment 8752106 [details] Bug 1244590 - Part 3: Parse spacing. https://reviewboard.mozilla.org/r/51511/#review51390 This is heading in the right direction but I think it can be made simpler. More importantly, however, we need to work out how to handle unrecognized properties. Should they really throw? The spec doesn't say they should. If we should keep them then we'll need to store the unrecognized property string so we can serialize it again. I'll have a think about it. Please let me know if you have any suggestions. ::: dom/animation/KeyframeEffectParams.cpp:9 (Diff revision 2) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/KeyframeEffectParams.h" > + > +#include "mozilla/RangedPtr.h" Don't we need to include ReadableUtils.h for StringBeginsWith etc.? ::: dom/animation/KeyframeEffectParams.cpp:13 (Diff revision 2) > + > +#include "mozilla/RangedPtr.h" > + > +namespace mozilla { > + > +#define PACED_PREFIX NS_LITERAL_STRING("paced") Can we just make this a static local variable in ParseSpacing? Like: static const nsLiteralString kPacedPrefix = NS_LITERAL_STRING("paced("); ::: dom/animation/KeyframeEffectParams.cpp:15 (Diff revision 2) > +static const nsDependentSubstring > +TrimWhitespace(const nsAString& aString) > +{ > + RangedPtr<const char16_t> start(aString.Data(), aString.Length()); > + RangedPtr<const char16_t> end(aString.Data() + aString.Length(), > + aString.Data(), aString.Length()); > + > + // Skip whitespace characters at the beginning > + while (start != end && *start == ' ') { > + ++start; > + } > + > + // Skip whitespace characters at the end. > + while (end != start) { > + --end; > + if (*end != ' ') { > + // Step back to the last non-whitespace character. > + ++end; > + break; > + } > + } > + > + return Substring(start.get(), end.get()); > +} I think we can probably do without this. For a start, I don't think ' ' is enough. If we're going to do this, we should probably use a whitespace list like: https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/base/nsContentUtils.h#403 However, that should really be spec'ed. But currently the spec doesn't say anything about ignoring leading and trailing whitespace. We should just reject ' distribute' and add web-platform-tests to make sure all UAs reject leading/trailing whitespace. ::: dom/animation/KeyframeEffectParams.cpp:40 (Diff revision 2) > +/* static */ void > +KeyframeEffectParams::ParseSpacing(const nsAString& aSpacing, > + SpacingMode& aSpacingMode, > + nsCSSProperty& aPacedProperty, > + ErrorResult& aRv) Considering that we always fail with the same error (especially after we drop the trimming behavior), it seems like it would be simpler to just make this return a bool and have the call site fill in an ErrorResult object as needed. However, see my comments below. We need to decide what to do about unrecognized properties. If we treat them differently, then we probably need to pass an ErrorResult object after all. ::: dom/animation/KeyframeEffectParams.cpp:53 (Diff revision 2) > + // Throw the original string to let the user know white spaces or > + // empty string is not acceptable. > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(aSpacing); > + } > + > + // 1. distribute spacing. We should quote the grammar that we're trying to parse: > distribute | paced({ident}) > https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-spacing ::: dom/animation/KeyframeEffectParams.cpp:59 (Diff revision 2) > + // 2. paced spacing, paced(css-property). > + // e.g. "paced(margin-left)" is accepted, > + // "paced(marginLeft)" is not accepted. > + // "paced()" is not accepted. > + // "paced( margin-left)" is not accepted. Nit: Not sure we need this comment if we quote the grammar above. ::: dom/animation/KeyframeEffectParams.cpp:71 (Diff revision 2) > + RangedPtr<const char16_t> end(spacing.Data() + spacing.Length(), > + spacing.Data(), spacing.Length()); Can't we just make this: const char16_t* const end = spacing.EndReading(); ::: dom/animation/KeyframeEffectParams.cpp:74 (Diff revision 2) > + if (*iter++ != '(') { > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(spacing); > + return; > + } Why not just check for StringBeginsWidth(spacing, NS_LITERAL_STRING("paced(")) above, then we can skip this check? ::: dom/animation/KeyframeEffectParams.cpp:79 (Diff revision 2) > + RangedPtr<const char16_t> tokenEnd(iter); > + while (tokenEnd != end && *tokenEnd != ')') { > + ++tokenEnd; > + } > + > + if (tokenEnd == end) { > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(spacing); > + return; > + } > + > + const nsAString& token = Substring(iter.get(), tokenEnd.get()); I'm not sure this is quite right. We should be consuming an ident here. Basically something along the lines of: https://drafts.csswg.org/css-syntax-3/#consume-an-ident-like-token Which, in this case, probably just means correctly handling escapes. Unfortunately, I guess that means we need to build up another string buffer to write the unescaped value into. ::: dom/animation/KeyframeEffectParams.cpp:96 (Diff revision 2) > + nsCSSProps::LookupProperty(token, CSSEnabledState::eForAllContent); > + if (aPacedProperty == eCSSProperty_UNKNOWN) { > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(spacing); > + return; > + } > + I notice that LookupProperty can also return eCSSPropertyExtra_variable. I guess we should just reject that. This ties back to the open issue in the spec, "Need to define what happens above when {ident} is not recognized."[1] In this implementation we will throw, but I wonder if we should make spec this to: * throw if the supplied value does not match the grammar * fall back to distribute if the property is not recognized (preferably with a console warning). This would include variables. Note that if we're going to do that, however, we'll probably need to store the variable token so that we can serialize it again. What do you think we should do here? [1] https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-spacing ::: dom/animation/KeyframeEffectParams.cpp:99 (Diff revision 2) > + return; > + } > + > + iter += token.Length(); > + > + // Skip check ')' because we did already. Nit: If possible, I think this would be more clear if we just consume the ident, then check that the next character is ), then check that we are at the end. (Also, depending on what we do about unrecognized properties, we might need to be careful to check that the supplied string matches the expected grammar *before* checking that the supplied property is recognized.)

Comment on attachment 8752107 [details] Bug 1244590 - Part 4: Rewrite ApplyDistributeSpacing. https://reviewboard.mozilla.org/r/51549/#review51402 This is mostly fine but I'd like to see what you think of some of the proposed simplifications. ::: dom/animation/KeyframeEffect.cpp:470 (Diff revision 2) > if (KeyframesEqualIgnoringComputedOffsets(aKeyframes, mKeyframes)) { > return; > } > > mKeyframes = Move(aKeyframes); > - KeyframeUtils::ApplyDistributeSpacing(mKeyframes); > + // Apply distribute spacing directly in SetFrames(). I think this comment is supposed to say something like: // Apply distribute spacing irrespective of the spacing mode. We will apply // the specified spacing mode when we generate computed animation property // values from the keyframes since both operations require a style context // and need to be performed whenever the style context changes. ::: dom/animation/KeyframeUtils.h:58 (Diff revision 2) > > /** > * Fills in the mComputedOffset member of each keyframe in the given array > - * using the "distribute" spacing algorithm. > + * using the specified spacing mode. > * > - * http://w3c.github.io/web-animations/#distribute-keyframe-spacing-mode > + * http://w3c.github.io/web-animations/#spacing-keyframes Nit: Can we update the URL to use https at the same time? Thanks. ::: dom/animation/KeyframeUtils.h:61 (Diff revision 2) > - * using the "distribute" spacing algorithm. > + * using the specified spacing mode. > * > - * http://w3c.github.io/web-animations/#distribute-keyframe-spacing-mode > + * http://w3c.github.io/web-animations/#spacing-keyframes > * > - * @param keyframes The set of keyframes to adjust. > + * @param aKeyframes The set of keyframes to adjust. > + * @param aSpacingMode The applied spacing mode to aKeyframes. Nit: 'The spacing mode to apply' How does this work? Surely we need to provide the paced property too? ::: dom/animation/KeyframeUtils.cpp:389 (Diff revision 2) > +static void > +ApplyDistributingOffset(const Range<Keyframe>& aDistributeRange); Can we call this function DistributeRange? (The argument can still be called aKeyframes) ::: dom/animation/KeyframeUtils.cpp:467 (Diff revision 2) > Keyframe& lastElement = aKeyframes.LastElement(); > lastElement.mComputedOffset = lastElement.mOffset.valueOr(1.0); > if (aKeyframes.Length() > 1) { > Keyframe& firstElement = aKeyframes[0]; > firstElement.mComputedOffset = firstElement.mOffset.valueOr(0.0); > } This is a very minor point, but would this work as: if (aKeyframes.Length() > 1) { Keyframe& firstElement = aKeyframes[0]; firstElement.mComputedOffset = firstElement.mOffset.valueOr(0.0); // We will fill in the last keyframe's offset below } else { Keyframe& lastElement = aKeyframes.LastElement(); lastElement.mComputedOffset = lastElement.mOffset.valueOr(1.0); } The reason, is that otherwise it's a bit confusing as to why we use valueOr(1.0) twice. What do you think? ::: dom/animation/KeyframeUtils.cpp:475 (Diff revision 2) > size_t i = 0; > while (i < aKeyframes.Length() - 1) { > - double start = aKeyframes[i].mComputedOffset; > + // Find the frame A (index i) and frame B (index j). > size_t j = i + 1; > while (aKeyframes[j].mOffset.isNothing() && j < aKeyframes.Length() - 1) { > ++j; > } > - double end = aKeyframes[j].mOffset.valueOr(1.0); > - size_t n = j - i; > - for (size_t k = 1; k < n; ++k) { > - double offset = start + double(k) / n * (end - start); > - aKeyframes[i + k].mComputedOffset = offset; > + aKeyframes[j].mComputedOffset = aKeyframes[j].mOffset.valueOr(1.0); > + > + // Fill computed offsets in (A, B) > + if (aSpacingMode == SpacingMode::distribute) { > + ApplyDistributingOffset(Range<Keyframe>(&aKeyframes[i], j - i + 1)); > + } else { > + // TODO > + MOZ_ASSERT(false, "not implement yet"); > } > i = j; > - aKeyframes[j].mComputedOffset = end; > } > } I'm curious if this would be more simple using iterators, e.g. something like: const Keyframe* const last = aKeyframes.cend() - 1; const Keyframe* keyframeA = aKeyframes.begin(); while (keyframeA != last) { // Find keyframe A and keyframe B to apply spacing *between*. const Keyframe* keyframeB = a + 1; while (keyframeB->mOffset.isNothing() && ++keyframeB != last); keyframeB->mComputedOffset = keyframeB->mOffset.valueOr(1.0); if (aSpacingMode == SpacingMode::distribute) { DistributeRange(Range<Keyframe>(keyframeA, keyframeB - keyframeA + 1); } else { // TODO MOZ_ASSERT(false, "not implement yet"); } keyframeA = keyframeB; } I guess it depends on what you do with this function in later patches, but assuming the above is right, it seems like it avoids some math and double-naming (e.g. frame A at index i) and is closer to what the spec does. (That said, using a loop without a body is probably not allowed according to our coding style so you'll need to tweak at least that.) ::: dom/animation/KeyframeUtils.cpp:477 (Diff revision 2) > } > > // Fill in remaining missing offsets. > size_t i = 0; > while (i < aKeyframes.Length() - 1) { > - double start = aKeyframes[i].mComputedOffset; > + // Find the frame A (index i) and frame B (index j). // Find frame A (index i) and frame B (index j) *between* which we will apply spacing ::: dom/animation/KeyframeUtils.cpp:1193 (Diff revision 2) > +/** > + * Apply evenly distributing computed offsets in (A, B). We should pass the > + * range keyframes in [A, B] and use A, B to calculate computed offsets in > + * (A, B). > + * > + * @param aDistributeRange The set of keyframes. "The sequence of keyframes between whose endpoints we should apply distribute spacing"? I think in future we'll need to make this take *two* ranges: the range to use for spacing and the range of keyframes to adjust. But this is fine for now. ::: dom/animation/KeyframeUtils.cpp:1199 (Diff revision 2) > + */ > +static void > +ApplyDistributingOffset(const Range<Keyframe>& aDistributeRange) > +{ > + const size_t n = aDistributeRange.length() - 1; > + const double start = aDistributeRange[0].mComputedOffset; Can we call this startOffset?

Comment on attachment 8752108 [details] Bug 1244590 - Part 5: Make the default value of computed offsets be -1. https://reviewboard.mozilla.org/r/51505/#review51414 This looks good but I want to check about the kComputedOffsetNotSet value. ::: dom/animation/KeyframeEffect.h:110 (Diff revision 2) > mPropertyValues = Move(aOther.mPropertyValues); > return *this; > } > > Maybe<double> mOffset; > - double mComputedOffset = 0.0; > + double mComputedOffset = -1.0; Could we add 'static const double kComputedOffsetNotSet = -1.0' to Keyframe and compare against that instead of -1.0? ::: dom/animation/KeyframeEffect.cpp:1031 (Diff revision 2) > - keyframeDict.mComputedOffset.Construct(keyframe.mComputedOffset); > + MOZ_ASSERT(keyframe.mComputedOffset != -1.0, "Invalid computed offset"); > + keyframeDict.mComputedOffset.Construct( > + keyframe.mComputedOffset == -1.0 ? 0.0 : keyframe.mComputedOffset); We've just asserted that mComputedOffset is not -1.0 so we don't need to check it again.

https://reviewboard.mozilla.org/r/51511/#review51390 > Can we just make this a static local variable in ParseSpacing? Like: > > static const nsLiteralString kPacedPrefix = NS_LITERAL_STRING("paced("); Sure. > I think we can probably do without this. > > For a start, I don't think ' ' is enough. If we're going to do this, we should probably use a whitespace list like: https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/base/nsContentUtils.h#403 > > However, that should really be spec'ed. But currently the spec doesn't say anything about ignoring leading and trailing whitespace. We should just reject ' distribute' and add web-platform-tests to make sure all UAs reject leading/trailing whitespace. OK. I will remove this part, so we don't accept leading/trailing whitespace. (And add some test cases, of course.) > We should quote the grammar that we're trying to parse: > > > distribute | paced({ident}) > > https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-spacing OK. I will add them. > Nit: Not sure we need this comment if we quote the grammar above. After quoting the grammer, I will remove this because developers can refer to the spec. > Can't we just make this: > > const char16_t* const end = spacing.EndReading(); We can use EndReading(). Actually, either RangePtr or StartBeginWith/EndReading works in this case. We mix the usage of RangePtr and StartBeginWith/EndReading in SMIL parser, so I just chose one of them. The reason I chose RangePtr is I think it may be safer. However, the readibility is better if using StartBeginWith/EndReading. I will try this. > Why not just check for StringBeginsWidth(spacing, NS_LITERAL_STRING("paced(")) above, then we can skip this check? Ha, yes. we should check "paced(" directly. > I'm not sure this is quite right. We should be consuming an ident here. Basically something along the lines of: > > https://drafts.csswg.org/css-syntax-3/#consume-an-ident-like-token > > Which, in this case, probably just means correctly handling escapes. Unfortunately, I guess that means we need to build up another string buffer to write the unescaped value into. OK. I will try to consume an ident token here. (Maybe adding a ConsumeIdentToken() function which also handles escapes for this case.) > I notice that LookupProperty can also return eCSSPropertyExtra_variable. I guess we should just reject that. > > This ties back to the open issue in the spec, "Need to define what happens above when {ident} is not recognized."[1] > > In this implementation we will throw, but I wonder if we should make spec this to: > > * throw if the supplied value does not match the grammar > * fall back to distribute if the property is not recognized (preferably with a console warning). This would include variables. > > Note that if we're going to do that, however, we'll probably need to store the variable token so that we can serialize it again. > > What do you think we should do here? > > [1] https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-spacing I agree. If the property is not reconized, we should print a warning and fall back to distribue. e.g. "paced(cabbage)" // cabbage is not a css property, so we should pring a warning and use distribute spacing. (I should also fix the test cases for this change in part 10.) However, as you mentioned, we should store the unreconized property string, so we have to add one more data member in KeyframeEffectParams.h. If mPacedProperty is eCSSProperty_UNKNOWN, we should use the unreconized string, in GetSpacingAsSteing(). If mPacedProperty is a valid property, the data member would be an empty string.

(In reply to Boris Chiou [:boris] from comment #84) > > Can't we just make this: > > > > const char16_t* const end = spacing.EndReading(); > > We can use EndReading(). Actually, either RangePtr or > StartBeginWith/EndReading works in this case. We mix the usage of RangePtr > and StartBeginWith/EndReading in SMIL parser, so I just chose one of them. > The reason I chose RangePtr is I think it may be safer. However, the > readibility is better if using StartBeginWith/EndReading. I will try this. I'm suggesting you can use both. I think RangePtr can be compared with a raw pointer. For the end pointer, if we use "const char16_t* const end" it can't be updated so we don't need to range check it. We can still use a RangePtr for the iterator. > I agree. If the property is not reconized, we should print a warning and > fall back to distribue. > e.g. "paced(cabbage)" // cabbage is not a css property, so we should pring a > warning and use distribute spacing. (I should also fix the test cases for > this change in part 10.) Ok great, let's do that. I filed https://github.com/w3c/web-animations/issues/152 to get this added to the spec. > However, as you mentioned, we should store the unreconized property string, > so we have to add one more data member in KeyframeEffectParams.h. If > mPacedProperty is eCSSProperty_UNKNOWN, we should use the unreconized > string, in GetSpacingAsSteing(). If mPacedProperty is a valid property, the > data member would be an empty string. Sounds good. Of course, we only need to store the actual property name (not the "paced()" part).

(In reply to Brian Birtles (:birtles) from comment #82) > I'm curious if this would be more simple using iterators, e.g. something > like: > > const Keyframe* const last = aKeyframes.cend() - 1; > const Keyframe* keyframeA = aKeyframes.begin(); > > while (keyframeA != last) { > // Find keyframe A and keyframe B to apply spacing *between*. > const Keyframe* keyframeB = a + 1; > while (keyframeB->mOffset.isNothing() && ++keyframeB != last); > > keyframeB->mComputedOffset = keyframeB->mOffset.valueOr(1.0); > > if (aSpacingMode == SpacingMode::distribute) { > DistributeRange(Range<Keyframe>(keyframeA, keyframeB - keyframeA + 1); > } else { > // TODO > MOZ_ASSERT(false, "not implement yet"); > } > keyframeA = keyframeB; > } > > I guess it depends on what you do with this function in later patches, but > assuming the above is right, it seems like it avoids some math and > double-naming (e.g. frame A at index i) and is closer to what the spec does. Looking into part 7, I think this is probably worth trying. However, we should use RangedPtr for keyframeA and keyframeB. All these indices get pretty complicated in part 7 so it would be good to have some additional checks that we're not pointing to out-of-range memory. I also think it might make the code simpler.

(In reply to Brian Birtles (:birtles) from comment #85) > Ok great, let's do that. I filed > https://github.com/w3c/web-animations/issues/152 to get this added to the > spec. Hi, do you know how to produce a js console warning? Looks like aRv.Throw() will throw an exception and then aRv.Failed() makes me return nullptr, so I shouldn't use Throw(). Or use SuppressException() to suppress it for this case? Thanks.

https://reviewboard.mozilla.org/r/51549/#review51402 > Nit: 'The spacing mode to apply' > > How does this work? Surely we need to provide the paced property too? Yes. I add paced property in the next patch. > Can we call this function DistributeRange? (The argument can still be called aKeyframes) Oops. Ha, looks like I misunderstand your meaning in the previous review. OK. I will revise this. > This is a very minor point, but would this work as: > > if (aKeyframes.Length() > 1) { > Keyframe& firstElement = aKeyframes[0]; > firstElement.mComputedOffset = firstElement.mOffset.valueOr(0.0); > // We will fill in the last keyframe's offset below > } else { > Keyframe& lastElement = aKeyframes.LastElement(); > lastElement.mComputedOffset = lastElement.mOffset.valueOr(1.0); > } > > The reason, is that otherwise it's a bit confusing as to why we use valueOr(1.0) twice. > > What do you think? OK. This is better. Thanks. > Can we call this startOffset? Yes, of course.

(In reply to Brian Birtles (:birtles) from comment #86) > Looking into part 7, I think this is probably worth trying. However, we > should use RangedPtr for keyframeA and keyframeB. All these indices get > pretty complicated in part 7 so it would be good to have some additional > checks that we're not pointing to out-of-range memory. I also think it might > make the code simpler. OK. I will try RangedPtr<Keyframe> in both part 4 and part 7.

https://reviewboard.mozilla.org/r/51505/#review51414 > Could we add 'static const double kComputedOffsetNotSet = -1.0' to Keyframe and compare against that instead of -1.0? Sounds great. Thanks. > We've just asserted that mComputedOffset is not -1.0 so we don't need to check it again. OK. I see.

(In reply to Boris Chiou [:boris] from comment #89) > (In reply to Brian Birtles (:birtles) from comment #86) > > Looking into part 7, I think this is probably worth trying. However, we > > should use RangedPtr for keyframeA and keyframeB. All these indices get > > pretty complicated in part 7 so it would be good to have some additional > > checks that we're not pointing to out-of-range memory. I also think it might > > make the code simpler. > > OK. I will try RangedPtr<Keyframe> in both part 4 and part 7. BTW. RangedPtr doesn't support operator->(), so if I want to access the member of RangedPtr<T>::mPtr, I have to use operator*(), e.g. (*keyframeA).mOffset, instead of keyframeA->mOffset. We may file a bug for this feature after landing this.

I'm going through parts 6 and 7 and my two biggest concerns are: * Storing the paceable properties on Keyframes feels awkward. It's really only needed while calculating pacing and it's clobbered every time we do that so there doesn't seem to be any value in keeping it around. It also introduces an awkward dependency between KeyframeEffect and KeyframeUtils, i.e. KeyframeEffect has to make sure to update these members before calling ApplySpacing. Instead, the information about which keyframes are paceable etc. should just be some temporary state stored while doing spacing. * Passing back an array of arrays from CalculateAnimationValues seems a bit odd somehow. This is a pretty minor point, though. Currently what I'm thinking is that so long as we're using parallel arrays for the set of Keyframes and the set of "animation values" we should just create another parallel array inside ApplySpacing to store the paced keyframe data. The tricky part, however, is that we only need to provide those "animation values" to ApplySpacing when doing paced spacing mode. The same is true for the paced property too. I wonder if we should just have: void ApplySpacing(nsTArray<Keyframe>& aKeyframes, SpacingMode aSpacingMode, nsCSSProperty aPacedProperty, const ComputedKeyframeValuesArray& aComputedValues); void ApplyDistributeSpacing(nsTArray<Keyframe>& aKeyframes) { static ComputedKeyframeValuesArray emptyArray; ApplySpacing(aKeyframes, SpacingMode::distribute, eCSSProperty_UNKNOWN, emptyArray); } I'll follow up later with specific suggestions in the reviews.

(In reply to Boris Chiou [:boris] from comment #87) > (In reply to Brian Birtles (:birtles) from comment #85) > > Ok great, let's do that. I filed > > https://github.com/w3c/web-animations/issues/152 to get this added to the > > spec. > > Hi, do you know how to produce a js console warning? Looks like aRv.Throw() > will throw an exception and then aRv.Failed() makes me return nullptr, so I > shouldn't use Throw(). Or use SuppressException() to suppress it for this > case? Thanks. Perhaps nsContentUtils::ReportToConsole? Perhaps you can avoid touching the ErrorResult and, at the call site, if the paced property is unknown / variable and spacing mode is paced, report the error to the console there. Of course, if there are multiple call sites it might be better to report the console error inside ParseSpacing (we'll need to pass a document in, I guess).

(In reply to Boris Chiou [:boris] from comment #91) > (In reply to Boris Chiou [:boris] from comment #89) > > (In reply to Brian Birtles (:birtles) from comment #86) > > > Looking into part 7, I think this is probably worth trying. However, we > > > should use RangedPtr for keyframeA and keyframeB. All these indices get > > > pretty complicated in part 7 so it would be good to have some additional > > > checks that we're not pointing to out-of-range memory. I also think it might > > > make the code simpler. > > > > OK. I will try RangedPtr<Keyframe> in both part 4 and part 7. > > BTW. RangedPtr doesn't support operator->(), so if I want to access the > member of RangedPtr<T>::mPtr, I have to use operator*(), e.g. > (*keyframeA).mOffset, instead of keyframeA->mOffset. We may file a bug for > this feature after landing this. You can use keyframeA.get()->mOffset I think, but that's a bit awkward. Jeff, is there any reason RangedPtr doesn't provide operator-> ?

I just noticed Tab's comment[1], "unrecognized values should be rejected and use the default value". I wonder if that means that for unsupported properties, we don't need to store the property name as a string--i.e. whether it would be ok to just store distribute as the spacing mode. [1] https://github.com/w3c/web-animations/issues/152#issuecomment-221363108

(In reply to Brian Birtles (:birtles) from comment #92) > I'm going through parts 6 and 7 and my two biggest concerns are: > > * Storing the paceable properties on Keyframes feels awkward. It's really > only needed while calculating pacing and it's clobbered every time we do > that so there doesn't seem to be any value in keeping it around. It also > introduces an awkward dependency between KeyframeEffect and KeyframeUtils, > i.e. KeyframeEffect has to make sure to update these members before calling > ApplySpacing. Instead, the information about which keyframes are paceable > etc. should just be some temporary state stored while doing spacing. > > * Passing back an array of arrays from CalculateAnimationValues seems a bit > odd somehow. This is a pretty minor point, though. > > Currently what I'm thinking is that so long as we're using parallel arrays > for the set of Keyframes and the set of "animation values" we should just > create another parallel array inside ApplySpacing to store the paced > keyframe data. > > The tricky part, however, is that we only need to provide those "animation > values" to ApplySpacing when doing paced spacing mode. The same is true for > the paced property too. I wonder if we should just have: > > void > ApplySpacing(nsTArray<Keyframe>& aKeyframes, > SpacingMode aSpacingMode, > nsCSSProperty aPacedProperty, > const ComputedKeyframeValuesArray& aComputedValues); > > void ApplyDistributeSpacing(nsTArray<Keyframe>& aKeyframes) > { > static ComputedKeyframeValuesArray emptyArray; > ApplySpacing(aKeyframes, SpacingMode::distribute, > eCSSProperty_UNKNOWN, emptyArray); > } > > I'll follow up later with specific suggestions in the reviews. Yes, I agree. Putting these data into Keyframe is weird. Your suggestion is better. Using another parallel arrays is OK for now, but if we use RangedPtr<Keyframe>, we lost the indexes for mapping between Keyframes and ComputedKeyframeValuesArray, so I think we might still use indexes, i.e. i (frame A), j (frame B), and use the indexes to create two Range<>s, i.e. Range<Keyframes> and Range<ComputedKeyframeValueArray>, for calculation in PaceRange().

(In reply to Brian Birtles (:birtles) from comment #93) > Perhaps nsContentUtils::ReportToConsole? Perhaps you can avoid touching the > ErrorResult and, at the call site, if the paced property is unknown / > variable and spacing mode is paced, report the error to the console there. > Of course, if there are multiple call sites it might be better to report the > console error inside ParseSpacing (we'll need to pass a document in, I > guess). Yes, good. Looks like we only one call site (i.e. in ConstructKeyframeEffet) now, so I will check the condition and call nsContentUtils::ReportToConsole() in ConstructKeyframeEffect(). Thanks.

(In reply to Brian Birtles (:birtles) from comment #95) > I just noticed Tab's comment[1], "unrecognized values should be rejected and > use the default value". I wonder if that means that for unsupported > properties, we don't need to store the property name as a string--i.e. > whether it would be ok to just store distribute as the spacing mode. > > [1] https://github.com/w3c/web-animations/issues/152#issuecomment-221363108 I'm not sure, but if so, we don't need to store the unrecognized property string, and the code looks better. :)

(In reply to Boris Chiou [:boris] from comment #98) > (In reply to Brian Birtles (:birtles) from comment #95) > > I just noticed Tab's comment[1], "unrecognized values should be rejected and > > use the default value". I wonder if that means that for unsupported > > properties, we don't need to store the property name as a string--i.e. > > whether it would be ok to just store distribute as the spacing mode. > > > > [1] https://github.com/w3c/web-animations/issues/152#issuecomment-221363108 > > I'm not sure, but if so, we don't need to store the unrecognized property > string, and the code looks better. :) I think it also means it's possible to feature-detect if a browser supports the property (otherwise the only indication is a console warning). Perhaps it might be even better still to check if the property has an animation type != none and store "distribute" if *either* it's not a recognized property or if it has an animation type of none (and, for shorthands, I guess "has an animation type of none" means "all of the component shorthands have an animation type of none" -- fortunately we already has IsAnimatableProperty for that: https://hg.mozilla.org/mozilla-central/rev/8d85de1a5c24#l1.55)

(In reply to Brian Birtles (:birtles) from comment #99) > I think it also means it's possible to feature-detect if a browser supports > the property (otherwise the only indication is a console warning). Perhaps > it might be even better still to check if the property has an animation type > != none and store "distribute" if *either* it's not a recognized property or > if it has an animation type of none (and, for shorthands, I guess "has an > animation type of none" means "all of the component shorthands have an > animation type of none" -- fortunately we already has IsAnimatableProperty > for that: https://hg.mozilla.org/mozilla-central/rev/8d85de1a5c24#l1.55) OK. Lets try this. I will also use KeyframeUtils::IsAnimatableProperty() to check it. If the paced property is not recognized or IsAnimatableProperty() returns false, we fall it back to distribute, so GetSpacingAsString() return "distribute", not the unrecognized paced property string.

Comment on attachment 8755339 [details] Bug 1244590 - Part 6: Refactor the calculation of StyleAnimationValue. https://reviewboard.mozilla.org/r/54414/#review51678 ::: dom/animation/KeyframeUtils.h:30 (Diff revision 1) > +using KeyframeAnimValues = nsTArray<nsTArray<PropertyStyleAnimationValuePair>>; > + Can we call this ComputedKeyframeValuesArray and give it a comment describing what it is? e.g. "Represents the set of property-value pairs on an array of Keyframes when converted to computed values". Or perhaps it would be simpler to just typedef the inner-type? e.g. // Represents the set of property-value pairs on a Keyframe converted to // computed values. using ComputedKeyframeValues = nsTArray<PropertyStyleAnimationValuePair>; Then we can pass nsTArray<ComputedKeyframeValues> around. Also, we need to forward-declare PropertyStyleAnimationValuePair. (And we really should include StyleAnimationValue.h in KeyframeUtils.cpp. It seems like we don't yet.) ::: dom/animation/KeyframeUtils.h:61 (Diff revision 1) > /** > + * Calculate the StyleAnimationValues of properties of each keyframe. > + * This involves expanding shorthand properties into longhand properties, > + * removing the duplicated properties for each keyframe, and creating an > + * array of |property:computed value| pairs for each keyframe. > + * We should expand this comment to say *why* we do this. e.g. These computed values are used *both* when computing the final set of per-property animation values (see GetAnimationPropertiesFromKeyframes) as well when applying paced spacing. By returning these values here, we allow the result to be re-used in both operations. ::: dom/animation/KeyframeUtils.h:65 (Diff revision 1) > + * @return The set of nsTArray<PropertyStyleAnimationValuePair>. The length > + * should be the same as aFrames. This comment will need to be updated. Also "will be the same" rather than "should be the same". ::: dom/animation/KeyframeUtils.h:69 (Diff revision 1) > + * @param aStyleContext The style context to use when computing values. > + * @return The set of nsTArray<PropertyStyleAnimationValuePair>. The length > + * should be the same as aFrames. > + */ > + static KeyframeAnimValues > + CalculateAnimationValues(const nsTArray<Keyframe>& aFrames, Can we call this GetComputedKeyframeValues? ::: dom/animation/KeyframeUtils.h:82 (Diff revision 1) > - static void ApplySpacing(nsTArray<Keyframe>& aKeyframes, > + static void > + ApplySpacing(nsTArray<Keyframe>& aKeyframes, > - SpacingMode aSpacingMode); > + SpacingMode aSpacingMode); Sorry, I've probably made this mistake too many times, but according to our coding style the type should go on the same line as the function name (unless it doesn't fit). https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes ::: dom/animation/KeyframeUtils.h:92 (Diff revision 1) > * @param aFrames The input keyframes. > + * @param aValues The calculated StyleAnimatioValues of properties of each We are now referring to keyframes as "keyframes" (not "frames") wherever possible (to avoid confusion with animation frames, as in requestAnimationFrame). Can we name these parameters: * aKeyframes * aComputedValues ? ::: dom/animation/KeyframeUtils.h:93 (Diff revision 1) > + * @param aValues The calculated StyleAnimatioValues of properties of each > + * keyframe. This comment has a typo. Also, perhaps we could say something like: @param aComputedValues The computed keyframe values (as returned by GetComputedKeyframeValues) used to fill in the individual AnimationPropertySegment objects. Although these values could be calculated from |aKeyframes|, passing them in as a separate parameter allows the result of GetComputedKeyframeValues to be re-used both here and in ApplySpacing. ::: dom/animation/KeyframeUtils.cpp:503 (Diff revision 1) > - const nsTArray<Keyframe>& aFrames) > { > MOZ_ASSERT(aStyleContext); > MOZ_ASSERT(aElement); > > - nsTArray<KeyframeValueEntry> entries; > + // The length should be the same as aFrames. (Not sure we need this comment?) ::: dom/animation/KeyframeUtils.cpp:504 (Diff revision 1) > + const size_t len = aFrames.Length(); > + KeyframeAnimValues result; > + result.SetLength(len); > > - for (const Keyframe& frame : aFrames) { > + for (size_t i = 0; i < len; ++i) { Can we just use SetCapacity and then keep the range-based for loop? ::: dom/animation/KeyframeUtils.cpp:551 (Diff revision 1) > + result[i].AppendElement(value); > + propertiesOnThisKeyframe.AddProperty(value.mProperty); > + } > + } > + } > + return result; Can we add an assertion here that result.Length() == aKeyframes.Length() ::: dom/animation/KeyframeUtils.cpp:559 (Diff revision 1) > +/* static */ nsTArray<AnimationProperty> > +KeyframeUtils::GetAnimationPropertiesFromKeyframes( > + const nsTArray<Keyframe>& aFrames, > + const KeyframeAnimValues& aValues) > +{ > + MOZ_ASSERT(aFrames.Length() == aValues.Length(), "Array length mismatch"); I wonder if it's even worth adding an #ifdef DEBUG block where we call GetComputedKeyframeValues and check that its contents equal aComputedValues? ::: dom/animation/KeyframeUtils.cpp:561 (Diff revision 1) > + const nsTArray<Keyframe>& aFrames, > + const KeyframeAnimValues& aValues) > +{ > + MOZ_ASSERT(aFrames.Length() == aValues.Length(), "Array length mismatch"); > + > + nsTArray<KeyframeValueEntry> entries; We could call SetCapacity on entries too to avoid unnecessary allocations? ::: dom/animation/KeyframeUtils.cpp (Diff revision 1) > - MOZ_ASSERT(frame.mComputedOffset != -1.0, "Invalid computed offset"); > - entry->mOffset = frame.mComputedOffset; > + entry->mOffset = frame.mComputedOffset; I think this assertion is still valid? We should have computed the computed offsets by now, right?

Comment on attachment 8752109 [details] Bug 1244590 - Part 7: Calculate paced spacing. https://reviewboard.mozilla.org/r/51809/#review51680 As mentioned in comment 92, I think we should try to avoid storing mPacedPropertyAnimValue on Keyframe and make up a parallel array inside ApplySpacing (or some function called from ApplySpacing). A few other minor comments below although I'll wait to see the next revision before looking at the algorithms in detail. ::: dom/animation/KeyframeEffect.cpp:518 (Diff revision 2) > + const nsCSSProperty longhandList[] = {aProperty, eCSSProperty_UNKNOWN}; > + const nsCSSProperty* propList = &longhandList[0]; > + if (nsCSSProps::IsShorthand(aProperty)) { > + propList = nsCSSProps::SubpropertyEntryFor(aProperty); > + } Just a thought, would it be possible to build up an nsCSSPropertySet containing the longhand components? If we do that, then I think we could change the code below from: for each keyframe { for each longhand property { for each property { // check it matches } } } to: for each keyframe { for each property { check it is in the nsCSSPropertySet } } ::: dom/animation/KeyframeEffect.cpp:561 (Diff revision 2) > + // Cache the paced property and its StyleAnimationValues, so we don't have > + // to search them each time. > + UpdateKeyframePacedPropertyAnimValues(mKeyframes, keyframeAnimValues, > + mEffectOptions.mPacedProperty); > + KeyframeUtils::ApplySpacing(mKeyframes, SpacingMode::paced, > + mEffectOptions.mPacedProperty); As mentioned in comment 92, I think we should probably be able to merge together into ApplySpacing. ::: dom/animation/KeyframeUtils.cpp:390 (Diff revision 2) > -ApplyDistributingOffset(const Range<Keyframe>& aDistributeRange); > +ApplyDistributingOffset(const Range<Keyframe>& aDistributeRange, > + size_t aPacedA = 0, size_t aPacedB = 0); I wonder if we can pass two ranges here? The spacing range and range to adjust? ::: dom/animation/KeyframeUtils.cpp:394 (Diff revision 2) > +ApplyPacedOffset(const Range<Keyframe>& aPaceRange, > + nsCSSProperty aProperty); I think we can call this PaceRange (and call the parameter aKeyframes) ::: dom/animation/KeyframeUtils.cpp:506 (Diff revision 2) > + size_t pA = i; > + while (pA <= j && !aKeyframes[pA].IsPaceable()) { > + ++pA; > + } > + size_t pB = j; > + while (pB >= i && !aKeyframes[pB].IsPaceable()) { > + if (pB == 0) { > + // pB is unsigned, so avoid overflow. > + break; > + } > + --pB; > + } > + // If no Paced A (which also means no Paced B), let both refer to B > + if (pA == j + 1) { > + pA = pB = j; > + } I'd like to see how far we can get with using iterators instead of indices although we will probably need one or two indices. (When finding corresponding elements in parallel arrays we might be able to use pointer arithmetic to, e.g. find an index such as iter - aKeyframes.begin() / iter - range.start().) ::: dom/animation/KeyframeUtils.cpp:1280 (Diff revision 2) > + // Skip (Paced A, Paced B). > + if (aPacedA < i && i < aPacedB) { > + continue; > + } If possible it would be neater to express the parameters to this in terms of the spacing range and the range to adjust since that matches the spec better (and since this method is not really related to paced timing). ::: dom/animation/KeyframeUtils.cpp:1314 (Diff revision 2) > + > + // a) Calculate cumulative dist in [Paced A, Paced B]. > + // Cumulative distance array stores the cumulative distances in > + // [Paced A, Paced B]. If the Keyframe is not paceable, just copy the > + // cumulative distance from the previous one. > + std::vector<double> cumulativeDist(len, 0.0); Any reason we are using vector here?

Comment on attachment 8752111 [details] Bug 1244590 - Part 9: Update spacing in SetTarget. https://reviewboard.mozilla.org/r/52137/#review51682

Comment on attachment 8752112 [details] Bug 1244590 - Part 10: Test for creating animations with a specific spacing mode. https://reviewboard.mozilla.org/r/52139/#review51684 I think the tests for TypeError and default handling should go under: interfaces/KeyframeEffectReadOnly/spacing.html While the tests for the offsets should go in this file. We should also add tests for: * The default value of spacing * Whitespace handling: " paced(margin-left)", "paced(margin-left) ", "paced( margin-left)", "paced(margin-left )" * CSS variables specified as paced properties (behaves like an unrecognized property) * Non-animatable properties (e.g. animation-duration) * Non-animatable shorthand properties (e.g. animation) ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:14 (Diff revision 2) > + assert_throws(new TypeError, function() { > + var anim = createDiv(t).animate([ { marginLeft: '0px' }, > + { marginLeft: '-20px' }, > + { marginLeft: '100px' }, > + { marginLeft: '50px' } ], > + { duration: 100 * MS_PER_SEC, > + spacing: 'dist' }); > + }); Could we write this as just: assert_throws(new TypeError, function() { createDiv(t).animate(null, { spacing: 'dist' }); }); Likewise for following tests. ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:31 (Diff revision 2) > + var anim = createDiv(t).animate([ { marginLeft: '0px' }, > + { marginLeft: '-20px' }, > + { marginLeft: '100px' }, > + { marginLeft: '50px' } ], > + { duration: 100 * MS_PER_SEC, > + spacing: 'pace(margin-left)' }); I'm not sure if this adds anything of the 'dist' test? ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:42 (Diff revision 2) > + var anim = createDiv(t).animate([ { marginLeft: '0px' }, > + { marginLeft: '-20px' }, > + { marginLeft: '100px' }, > + { marginLeft: '50px' } ], > + { duration: 100 * MS_PER_SEC, > + spacing: 'paced(cabbage)' }); As discussed, we'll need to make this simply fall back to using 'distribute' ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:68 (Diff revision 2) > +test(function(t) { > + var anim = createDiv(t).animate([ { marginLeft: '0px' }, > + { marginLeft: '-20px' }, > + { marginLeft: '100px' }, > + { marginLeft: '50px' } ], > + { duration: 100 * MS_PER_SEC, > + spacing: 'distribute' }); > + > + assert_equals(anim.effect.spacing, 'distribute', 'spacing mode'); > + > + var frames = anim.effect.getKeyframes(); > + var slots = frames.length - 1; > + assert_equals(frames[0].computedOffset, 0.0, '1st frame offset'); > + assert_equals(frames[1].computedOffset, 1.0 / slots, '2nd frame offset'); > + assert_equals(frames[2].computedOffset, 2.0 / slots, '3rd frame offset'); > + assert_equals(frames[3].computedOffset, 1.0, 'last frame offset'); > +}, 'Test distribute spacing without specific offsets'); In terms of making each test as simple as possible, could we split this into two tests? One that checks that anim.effect.spacing is set correctly, and one that checks that the offsets are set as expected? ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:92 (Diff revision 2) > + var anim = createDiv(t).animate([ { marginLeft: '0px' }, > + { marginLeft: '-20px' }, > + { marginLeft: '100px', offset: 0.5 }, > + { marginLeft: '50px' } ], > + { duration: 100 * MS_PER_SEC, > + spacing: 'distribute' }); Nit: Once we have a test that the default spacing mode is set correctly, we can drop this line ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:172 (Diff revision 2) > + var frames = anim.effect.getKeyframes(); > + var slots = frames.length - 1; > + assert_equals(frames[1].computedOffset, 1.0 / slots, '2nd frame offset'); > + assert_equals(frames[2].computedOffset, 2.0 / slots, '3rd frame offset'); > +}, 'Test falling back to distribute spacing if all paced property value ' + > + 'are not changed'); s/are not changed/are equal/ ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:207 (Diff revision 2) > +}, 'Test paced spacing if a paced property that appears on only some ' + > + 'keyframes with a specific offset'); I think we need a few more variations of this: * Where there are at least two keyframes without offsets and without the paced property before the first paceable keyframe and after the last paceable keyframe to check that we actually do distribute spacing for those keyframes. * Likewise but between paceable keyframes. e.g. paceable, not paceable, not paceable, paceable, not paceable, paceable I suspect there may be others. ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:283 (Diff revision 2) > + '2nd frame offset using distribute spacing'); > + assert_equals(frames[2].computedOffset, frames[3].computedOffset * 2 / 3, > + '3rd frame offset using distribute spacing'); > + assert_equals(frames[3].computedOffset, 100 / 150, > + '4th frame offset using paced spacing'); > +}, 'Test paced spacing only for keyframes specifying all some components, ' + all longhand components?

Comment on attachment 8755340 [details] MozReview Request: Bug 1244590 - Part 12: Revise ComputeDistance for eUnit_Calc. https://reviewboard.mozilla.org/r/54416/#review51686 I didn't think we were going to do this but you seemed to work it out. And I think the cyclic dependency I was concerned about it probably not an issue after all. Still I think heycam or dbaron would be a better reviewer for this so perhaps you could request review from them after rebasing on top of the changes on the underlying patches?

Comment on attachment 8755341 [details] MozReview Request: Bug 1244590 - Part 13: Test for different units. https://reviewboard.mozilla.org/r/54418/#review51688 This looks fine to me, but it probably makes sense to get whoever you ask to review part 12 to review this so they can see the result you are trying to achieve and suggest any additional test cases that need to be covered.

(In reply to Brian Birtles (:birtles) from comment #105) > I didn't think we were going to do this but you seemed to work it out. And I > think the cyclic dependency I was concerned about it probably not an issue > after all. Still I think heycam or dbaron would be a better reviewer for > this so perhaps you could request review from them after rebasing on top of > the changes on the underlying patches? OK. After getting r+ on part 1 ~ part 11, I will ask them about this issue. Thanks.

Updated the spec to cover unrecognized/non-animatable properties: https://github.com/w3c/web-animations/commit/bc38f6e02b35678f2861bd716dfb73f4d2a7bf50

https://reviewboard.mozilla.org/r/54414/#review51678 > Can we call this ComputedKeyframeValuesArray and give it a comment describing what it is? > > e.g. "Represents the set of property-value pairs on an array of Keyframes when converted to computed values". > > Or perhaps it would be simpler to just typedef the inner-type? > > e.g. > > // Represents the set of property-value pairs on a Keyframe converted to > // computed values. > using ComputedKeyframeValues = nsTArray<PropertyStyleAnimationValuePair>; > > Then we can pass nsTArray<ComputedKeyframeValues> around. > > Also, we need to forward-declare PropertyStyleAnimationValuePair. (And we really should include StyleAnimationValue.h in KeyframeUtils.cpp. It seems like we don't yet.) OK. I will declare this: using ComputedKeyframeValues = nsTArray<PropertyStyleAnimationValuePair>; > Can we just use SetCapacity and then keep the range-based for loop? OK. I can use range-based for loop here and pass |len| as the argument to the nsTArray constructor which treats it as the initial capacity length. > I wonder if it's even worth adding an #ifdef DEBUG block where we call GetComputedKeyframeValues and check that its contents equal aComputedValues? If we keep this assertion and add an assertion as above, I think it might be enough for now. > We could call SetCapacity on entries too to avoid unnecessary allocations? Yes. The actual capacity may larger than aKeyframe.Length() because we may have one or more properties for each keyframe. I can set capacity to aKeyframe.Length() to reduce some uncessary allocations. > I think this assertion is still valid? We should have computed the computed offsets by now, right? Yes. At lease we applied distribute spacing already.

https://reviewboard.mozilla.org/r/51809/#review51680 > Just a thought, would it be possible to build up an nsCSSPropertySet containing the longhand components? > > If we do that, then I think we could change the code below from: > > for each keyframe { > for each longhand property { > for each property { > // check it matches > } > } > } > > to: > > for each keyframe { > for each property { > check it is in the nsCSSPropertySet > } > } OK. I will try to use nsCSSPropertySet. > I wonder if we can pass two ranges here? The spacing range and range to adjust? OK. I would like to pass two Range<>s, one for spacing range, and one for _Not adjusted_ range because we apply distribute spacing to (A, Paced A], [Paced B, B).

(In reply to Boris Chiou [:boris] from comment #110) > > I wonder if we can pass two ranges here? The spacing range and range to adjust? > > OK. I would like to pass two Range<>s, one for spacing range, and one for > _Not adjusted_ range because we apply distribute spacing to (A, Paced A], > [Paced B, B). Why don't we just call it twice? It seems that would be a more simple API and closer to the spec?

(In reply to Brian Birtles (:birtles) from comment #111) > Why don't we just call it twice? It seems that would be a more simple API > and closer to the spec? OK. I will try it. The second argument is the range to adjust and we call DistribueRange() twice for (A, PacedA] and [Paced B, B). Thanks.

(In reply to Brian Birtles (:birtles) from comment #104) > Comment on attachment 8752112 [details] > MozReview Request: Bug 1244590 - Part 10: Test for creating animations with > a specific spacing mode. > > https://reviewboard.mozilla.org/r/52139/#review51684 > > I think the tests for TypeError and default handling should go under: > > interfaces/KeyframeEffectReadOnly/spacing.html > > While the tests for the offsets should go in this file. > > We should also add tests for: > > * The default value of spacing > * Whitespace handling: " paced(margin-left)", "paced(margin-left) ", "paced( > margin-left)", "paced(margin-left )" > * CSS variables specified as paced properties (behaves like an unrecognized > property) > * Non-animatable properties (e.g. animation-duration) > * Non-animatable shorthand properties (e.g. animation) OK, so I should put the tests into 1. interfaces/KeyframeEffectReadOnly/spacing.html a) Basic handling for distribute and paced (including different combinations of shorthand and longhand properties) b) Parser related tests (including using CSS variables, and non-animatable properties. 2. animation-model/keyframe-effects/spacing-keyframes.html a) all tests with specific offsets. BTW, all the tests related to SetTarget(), I will put them into "interfaces/KeyframeEffect/setTarget.html" (part 11). Thanks.

Comment on attachment 8752108 [details] Bug 1244590 - Part 5: Make the default value of computed offsets be -1. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51505/diff/3-4/

Comment on attachment 8752106 [details] Bug 1244590 - Part 3: Parse spacing. https://reviewboard.mozilla.org/r/51511/#review52378 ::: dom/animation/KeyframeEffect.cpp:730 (Diff revision 4) > + nsAString& aUnrecognizedProperty, > + bool& aHasUnrecognizedProperty, Can we call this aInvalidPacedProperty and just check if IsEmpty() is true rather than passing a bool as well? ::: dom/animation/KeyframeEffectParams.h:40 (Diff revision 4) > + * @param [out] aUnrecognizedProperty The unrecognized property string > + * if we can't recognized the css property > + * in aSpacing. > + * @param [out] aReportUnrecognized The flag for checking if we should report > + * the unrecognized property which could be > + * an empty string. As mentioned earlier, let's drop aReportUnrecognized and just pass back aUnrecognizedProperty (after renaming it to aInvalidPacedProperty). If we get 'paced()' then '' is not an <ident> so we should throw a type error (i.e. set aRv). We can truncate the string at the start of the function or simply assert that is empty. For the parameter description, something like "A string that, if we parsed a string of the form 'paced(<ident>)' where <ident> is not a recognized animatable property, will be set to <ident>." ::: dom/animation/KeyframeEffectParams.cpp:15 (Diff revision 4) > +// This is a simplified version of consuming ident token, which means we don't > +// decode special escapes, and we use the last paired ')' as the end of this > +// token if it exists. > +static void > +ConsumeIdentToken(RangedPtr<const char16_t>& aStart, > + const char16_t* const aEnd, > + nsAString& aResult) > +{ > + RangedPtr<const char16_t> iter = aStart; > + size_t skipRightParenCount = 0; > + bool escape = false; > + aResult.Truncate(); > + > + while (iter != aEnd) { > + if (*iter == '(') { > + ++skipRightParenCount; > + } else if (*iter == ')') { > + if (skipRightParenCount > 0) { > + --skipRightParenCount; > + } else { > + break; > + } > + } > + > + if (!escape && *iter == '\\') { > + escape = true; > + } else { > + aResult.Append(*iter); > + escape = false; > + } > + > + ++iter; > + } > + > + aStart = iter; > +} Why do we do the paren-counting? Idents don't allow parens, right? As I understand it, all we really need to do here is: 1. Check the first 1~3 code points are allowed: https://drafts.csswg.org/css-syntax-3/#check-if-three-code-points-would-start-an-identifier 2. Walk through the string and: a. If the character is not a name character (https://drafts.csswg.org/css-syntax-3/#name-code-point) return b. If it's '\': If the next character doesn't exist or is a newline (https://drafts.csswg.org/css-syntax-3/#newline but doing the equivalent preprocessing here) return. Otherwise, append the next character and continue c. Append the next character Does that sound right? ::: dom/animation/KeyframeEffectParams.cpp:19 (Diff revision 4) > + > +// This is a simplified version of consuming ident token, which means we don't > +// decode special escapes, and we use the last paired ')' as the end of this > +// token if it exists. > +static void > +ConsumeIdentToken(RangedPtr<const char16_t>& aStart, Nit: Call this aIter since it won't represent the start once we return? ::: dom/animation/KeyframeEffectParams.cpp:62 (Diff revision 4) > + if (aSpacing.IsEmpty()) { > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(aSpacing); > + return; > + } > + I don't think we need this, right? ::: dom/animation/KeyframeEffectParams.cpp:100 (Diff revision 4) > + if (*iter++ != ')') { > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(aSpacing); > + return; > + } > + > + if (iter != end) { > + aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(aSpacing); > + return; > + } Is this first check safe? Don't we first need to check that iter != end? (ConsumeIdent should be written to return with iter pointing to the first character that is *not* part of the ident token.) Perhaps we can combine the checks: if (end - iter != 1 || *iter != ')') { ... } ::: dom/animation/KeyframeUtils.h:86 (Diff revision 4) > + > + /** > + * Check if the property or, for shorthands, one or more of > + * its subproperties, is animatable. > + * > + * @param aProperty The property we check. Nit: "The property to check" ::: dom/locales/en-US/chrome/dom/dom.properties:230 (Diff revision 4) > # LOCALIZATION NOTE: Do not translate "ServiceWorker". %1$S is the ServiceWorker scope URL. %2$S is an error string. > PushMessageDecryptionFailure=The ServiceWorker for scope ‘%1$S’ encountered an error decrypting a push message: ‘%2$S’. For help with encryption, please see https://developer.mozilla.org/en-US/docs/Web/API/Push_API/Using_the_Push_API#Encryption > # LOCALIZATION NOTE: %1$S is the type of a DOM event. 'passive' is a literal parameter from the DOM spec. > PreventDefaultFromPassiveListenerWarning=Ignoring ‘preventDefault()’ call on event of type ‘%1$S’ from a listener registered as ‘passive’. > +# LOCALIZATION NOTE: %1$S is the unrecongnized property string. > +UnrecognizedPacedProperty=The paced property '%1$S' is unrecognized. I think this should be UnanimatablePacedProperty and the string should be: Paced property ‘%1S’ is not an animatable property. (Note: We seem to use smart quotes ‘’ in this file.)

https://reviewboard.mozilla.org/r/51511/#review52378 > As mentioned earlier, let's drop aReportUnrecognized and just pass back aUnrecognizedProperty (after renaming it to aInvalidPacedProperty). > > If we get 'paced()' then '' is not an <ident> so we should throw a type error (i.e. set aRv). > > We can truncate the string at the start of the function or simply assert that is empty. > > For the parameter description, something like "A string that, if we parsed a string of the form 'paced(<ident>)' where <ident> is not a recognized animatable property, will be set to <ident>." OK. 'paced()' is what my question here. Thanks for your clarifiation. If it is a type error, then we can drop the flag. Thanks. > Why do we do the paren-counting? Idents don't allow parens, right? > > As I understand it, all we really need to do here is: > > 1. Check the first 1~3 code points are allowed: https://drafts.csswg.org/css-syntax-3/#check-if-three-code-points-would-start-an-identifier > > 2. Walk through the string and: > a. If the character is not a name character (https://drafts.csswg.org/css-syntax-3/#name-code-point) return > b. If it's '\': > If the next character doesn't exist or is a newline (https://drafts.csswg.org/css-syntax-3/#newline but doing the equivalent preprocessing here) return. Otherwise, append the next character and continue > c. Append the next character > > Does that sound right? Sorry, I though we should handle this case "paced(abd())", which reports a warning like this: "Paced property 'acd()' is not an animatable property". I will remove the parens part and refine the handling of escapes. Thanks. > Is this first check safe? Don't we first need to check that iter != end? > > (ConsumeIdent should be written to return with iter pointing to the first character that is *not* part of the ident token.) > > Perhaps we can combine the checks: > > if (end - iter != 1 || *iter != ')') { > ... > } Yes. I should check |end| first. Thanks.

(In reply to Boris Chiou [:boris] from comment #141) > Sorry, I though we should handle this case "paced(abd())", which reports a > warning like this: "Paced property 'acd()' is not an animatable property". > I will remove the parens part and refine the handling of escapes. Thanks. Yeah, "paced(abd())" doesn't conform to the grammar so it should throw.

Comment on attachment 8752107 [details] Bug 1244590 - Part 4: Rewrite ApplyDistributeSpacing. https://reviewboard.mozilla.org/r/51549/#review52390 ::: dom/animation/KeyframeUtils.cpp:478 (Diff revision 4) > - size_t i = 0; > - while (i < aKeyframes.Length() - 1) { > - double start = aKeyframes[i].mComputedOffset; > - size_t j = i + 1; > - while (aKeyframes[j].mOffset.isNothing() && j < aKeyframes.Length() - 1) { > - ++j; > + const Keyframe* const last = aKeyframes.cend() - 1; > + RangedPtr<Keyframe> keyframeA(aKeyframes.begin(), aKeyframes.Length()); > + while (keyframeA != last) { > + // Find frame A and frame B *between* which we will apply spacing. > + RangedPtr<Keyframe> keyframeB = keyframeA + 1; > + while ((*keyframeB).mOffset.isNothing() && keyframeB != last) { I wonder if keyframeB.get()->mOffset would be more clear? (Then, if we implement operator->() we can just do s/.get()//) ::: dom/animation/KeyframeUtils.cpp:1185 (Diff revision 4) > return !propertiesWithFromValue.Equals(properties) || > !propertiesWithToValue.Equals(properties); > } > > +/** > + * Apply evenly distributing computed offsets in (A, B). We should pass the Nit: Evenly distribute the computed offsets...

Comment on attachment 8752108 [details] Bug 1244590 - Part 5: Make the default value of computed offsets be -1. https://reviewboard.mozilla.org/r/51505/#review52392

Comment on attachment 8752107 [details] Bug 1244590 - Part 4: Rewrite ApplyDistributeSpacing. https://reviewboard.mozilla.org/r/51549/#review52394 ::: dom/animation/KeyframeUtils.cpp:476 (Diff revision 4) > > // Fill in remaining missing offsets. > - size_t i = 0; > - while (i < aKeyframes.Length() - 1) { > - double start = aKeyframes[i].mComputedOffset; > - size_t j = i + 1; > + const Keyframe* const last = aKeyframes.cend() - 1; > + RangedPtr<Keyframe> keyframeA(aKeyframes.begin(), aKeyframes.Length()); > + while (keyframeA != last) { > + // Find frame A and frame B *between* which we will apply spacing. Nit: s/ frame/ keyframe/g Also a few lines down.

Remove part 12 and part 13 because I decide to move them into a new bug.

(In reply to Boris Chiou [:boris] from comment #157) > Remove part 12 and part 13 because I decide to move them into a new bug. File Bug 1276193.

(In reply to Brian Birtles (:birtles, high review load) from comment #92) > void ApplyDistributeSpacing(nsTArray<Keyframe>& aKeyframes) > { > static ComputedKeyframeValuesArray emptyArray; > ApplySpacing(aKeyframes, SpacingMode::distribute, > eCSSProperty_UNKNOWN, emptyArray); > } An interesting thing: using 'static nsTArray<...> emptyArray' causes 8 bytes leaked on nsTArray_base, so I will remove |static| from this statement.

Comment on attachment 8755339 [details] Bug 1244590 - Part 6: Refactor the calculation of StyleAnimationValue. https://reviewboard.mozilla.org/r/54414/#review52768 r=me with comments addressed ::: dom/animation/KeyframeUtils.cpp (Diff revision 4) > - MOZ_ASSERT(frame.mComputedOffset != Keyframe::kComputedOffsetNotSet, > - "Invalid computed offset"); I thought your comment in comment 109 meant we could keep this assertion?

https://reviewboard.mozilla.org/r/51809/#review52772 I think there's a bit too much +1, -1, .get() etc. in the code quick makes it hard to read and, in some cases, error prone. I think there's a few things we can do. Firstly, we should add operator->() to RangedPtr. We haven't gotten a response from Jeff yet so I think the best thing is just to file the bug, make the patch and request review. Secondly, a lot of the complexity seems to come about because we use mozilla::Range and that takes a raw pointer and a length and we are mostly dealing with RangedPtrs. Using Range is preferable to a pair of RangedPtrs because it ensures they are in the correct order and have the same range. I think we could probably do one of two things: a) Stop using mozilla::Range and just pass RangedPtrs around. Doing that, however, would mean we'd lose the guarantee that the two pointers are in order and have the same range. If the second pointer is always just a const pointer that we never update then we could just assert the iterator we intend to move is < the end iterator. We couldn't easily assert that the end iterator is in range of the first iterator, however, without adding extra API to RangedPtr. b) Make mozilla::Range easier to use so that it accepts two RangedPtr arguments and does the necessary order and range checking. Again, we'd probably need to add API to RangedPtr so that we could assert that the end iterator is within the range of the first iterator. I'm slightly leaning towards (b) I think. I think if we did that, this patch could become simpler and easier to read. As a related but somewhat separate point, it would be nice if we can make DistributeRange not require the second argument rather than having to pass an empty range when it is not needed. Perhaps we could overload the function with a one-argument version that fills in the second range object using the same logic we currently do to set up filterStart / filterEnd. Also, see my comments about returning an array of cumulative distances instead of StyleAnimationValues. ::: dom/animation/KeyframeUtils.h:89 (Diff revision 7) > * > * https://w3c.github.io/web-animations/#spacing-keyframes > * > * @param aKeyframes The set of keyframes to adjust. > * @param aSpacingMode The spacing mode to apply. > + * @param aProperty The paced property. "The paced property. Only used when |aSpacingMode| is SpacingMode::paced. In all other cases it is ignored and hence may be any value, e.g. eCSSProperty_UNKNOWN" ? ::: dom/animation/KeyframeUtils.h:90 (Diff revision 7) > + * @param aComputedValues The set of computed keyframe values got by > + * GetComputedKeyframeValues. "The set of computed keyframe values as returned by GetComputedKeyframeValues. Only used when |aSpacingMode| is SpacingMode::paced. In all other cases this parameter is unused and may be any value including an empty array." ::: dom/animation/KeyframeUtils.h:99 (Diff revision 7) > + * Fills in the mComputedOffset member of each keyframe in the given array > + * using distribute spacing mode. "Wrapper for ApplySpacing to simplify using distribute spacing." ? ::: dom/animation/KeyframeUtils.cpp:388 (Diff revision 7) > -DistributeRange(const Range<Keyframe>& aKeyframes); > +DistributeRange(const Range<Keyframe>& aKeyframes, > + const Range<Keyframe>& aFilteredKeyframes); How about something like: * |aSpacingRange| * |aRangeToAdjust| ? ::: dom/animation/KeyframeUtils.cpp:477 (Diff revision 7) > } > > + nsTArray<ComputedKeyframeValues> pacedValues; > + if (aSpacingMode == SpacingMode::paced) { > + MOZ_ASSERT(IsAnimatableProperty(aProperty), > + "Only support animatable property for paced spacing"); "Paced property should be animatable" ::: dom/animation/KeyframeUtils.cpp:519 (Diff revision 7) > + while (pacedA <= keyframeB && pacedValues[pacedA - begin].IsEmpty()) { > + ++pacedA; > + } Should this be pacedA < keyframeB ? ::: dom/animation/KeyframeUtils.cpp:523 (Diff revision 7) > + while (pacedB >= keyframeA && pacedValues[pacedB - begin].IsEmpty()) { > + if (pacedB == keyframeA) { > + break; > + } Should this be pacedB > keyframeA? Then we wouldn't need the check for pacedB == keyframeA, right? ::: dom/animation/KeyframeUtils.cpp:540 (Diff revision 7) > + // b) Apply evenly distributing offsets in (keyframe A, Paced A] and > + // [Paced B, keyframe B). > + DistributeRange(Range<Keyframe>(keyframeA.get(), rangeLen), > + Range<Keyframe>(keyframeA.get(), pacedA - keyframeA + 1)); > + DistributeRange(Range<Keyframe>(keyframeA.get(), rangeLen), > + Range<Keyframe>(pacedB.get(), keyframeB - pacedB + 1)); Is the +1 here correct? If not, we should try to write a test that will fail if we leave the + 1 here. ::: dom/animation/KeyframeUtils.cpp:542 (Diff revision 7) > + DistributeRange(Range<Keyframe>(keyframeA.get(), rangeLen), > + Range<Keyframe>(keyframeA.get(), pacedA - keyframeA + 1)); > + DistributeRange(Range<Keyframe>(keyframeA.get(), rangeLen), > + Range<Keyframe>(pacedB.get(), keyframeB - pacedB + 1)); > + // c) Apply paced offsets in (Paced A, Paced B). > + const size_t idx = pacedA - begin; Maybe call it pacedAIdx? (Or just do the calculation when setting up the range, unless it makes the line-wrapping horrible.) ::: dom/animation/KeyframeUtils.cpp:547 (Diff revision 7) > + const size_t idx = pacedA - begin; > + const size_t pacedLen = pacedB - pacedA + 1; > + PaceRange(Range<Keyframe>(pacedA.get(), pacedLen), > + Range<ComputedKeyframeValues>(&pacedValues[idx], pacedLen), > + aProperty); > + // d) Fill null computed offsets in (Paced A, Paced B). "Fill in any computed offsets in (Paced A, Paced B) that are still not set (e.g. because the keyframe was not paceable, or because the cumulative distance between paceable properties was zero)" ::: dom/animation/KeyframeUtils.cpp:549 (Diff revision 7) > + PaceRange(Range<Keyframe>(pacedA.get(), pacedLen), > + Range<ComputedKeyframeValues>(&pacedValues[idx], pacedLen), > + aProperty); > + // d) Fill null computed offsets in (Paced A, Paced B). > + for (RangedPtr<Keyframe> frame = pacedA + 1; frame < pacedB; ++frame) { > + if ((*frame).mComputedOffset != Keyframe::kComputedOffsetNotSet) { Nit: frame.get()->mComputedOffset might be better? ::: dom/animation/KeyframeUtils.cpp:1294 (Diff revision 7) > static void > -DistributeRange(const Range<Keyframe>& aKeyframes) > +DistributeRange(const Range<Keyframe>& aKeyframes, > + const Range<Keyframe>& aFilteredKeyframes) > { > + // aFilteredKeyframes could be an empty range. If it is, we should apply > + // distribute spacing to all the keyframes (excluding A and B). > + const RangedPtr<Keyframe> filterStart = aFilteredKeyframes.start() > + ? aFilteredKeyframes.start() > + : aKeyframes.start() + 1; > + const RangedPtr<Keyframe> filterEnd = aFilteredKeyframes.end() > + ? aFilteredKeyframes.end() > + : aKeyframes.end() - 1; See my earlier comment: I think we should just make an overload of DistributeRange that takes a single range, fills in the range to adjust, then calls this function. ::: dom/animation/KeyframeUtils.cpp:1306 (Diff revision 7) > + ? aFilteredKeyframes.start() > + : aKeyframes.start() + 1; > + const RangedPtr<Keyframe> filterEnd = aFilteredKeyframes.end() > + ? aFilteredKeyframes.end() > + : aKeyframes.end() - 1; > + We should add an assertion that the range to adjust is within the distribute range. ::: dom/animation/KeyframeUtils.cpp:1321 (Diff revision 7) > + * @param aKeyframes The sequence of keyframes between whose endpoints we should > + * apply paced distribute, [Paced A, Paced B], and both Paced A & Paced B > + * should be paceable. "The range of keyframes between whose endpoints we should apply paced spacing. Both endpoints should be paceable, i.e. the corresponding elements in |aPacedValues| should specify all the necessary values for |aPacedProperty|. Within this function, we refer to the start and end points of this range as Paced A and Paced B respectively in keeping with the notation used in the spec." ::: dom/animation/KeyframeUtils.cpp:1324 (Diff revision 7) > + * @param aPacedValues The sequence of computed values of the paced property. > + * We get this by GetPacedPropertyKeyframeValues(). "... as returned by GetPacedPropertyKeyframeValues(). This acts as a parallel range to |aKeyframes|." ::: dom/animation/KeyframeUtils.cpp:1331 (Diff revision 7) > + * @param aProperty The paced property. > + */ > +static void > +PaceRange(const Range<Keyframe>& aKeyframes, > + const Range<ComputedKeyframeValues>& aPacedValues, > + nsCSSProperty aProperty) Can we call this |aPacedProperty| ? ::: dom/animation/KeyframeUtils.cpp:1332 (Diff revision 7) > + */ > +static void > +PaceRange(const Range<Keyframe>& aKeyframes, > + const Range<ComputedKeyframeValues>& aPacedValues, > + nsCSSProperty aProperty) > +{ Add an assertion that the length of aKeyframes and aPacedValues is equal. ::: dom/animation/KeyframeUtils.cpp:1333 (Diff revision 7) > + const size_t len = aKeyframes.length(); > + if (len < 3) { > + return; > + } This needs a comment, e.g. "If there is nothing between the end points, there is nothing to space." ::: dom/animation/KeyframeUtils.cpp:1344 (Diff revision 7) > + // a) Calculate cumulative dist in [Paced A, Paced B]. > + // Cumulative distance array stores the cumulative distances in > + // [Paced A, Paced B]. If the Keyframe is not paceable, just copy the > + // cumulative distance from the previous one. At first I thought we should factor this out into a separate method but actually I wonder if we can actually calculate all the distances in GetPacedPropertyKeyframeValues? If we do that, I don't think we even need to return the StyleAnimationValues? All we really need is to know all the computed distances and which keyframes are paceable, right? For example, could we simply return a parallel array of doubles representing the cumulative distances across the whole set of keyframes? For non-paceable keyframes we could set the cumulative distance to kNotPaceable = -1. Then we'd still pass a range within that cumulative distances array to PaceRange and PaceRange would need to get the cumulative distance of the first element in that range and subtract that from other cumulative distances in order to get a relative distance. I think if we do that, this becomes a lot simpler? ::: dom/animation/KeyframeUtils.cpp:1348 (Diff revision 7) > + > + // a) Calculate cumulative dist in [Paced A, Paced B]. > + // Cumulative distance array stores the cumulative distances in > + // [Paced A, Paced B]. If the Keyframe is not paceable, just copy the > + // cumulative distance from the previous one. > + nsTArray<double> cumulativeDist(len); Nit: Can we use AutoTArray here with an initial storage of, say, 8? That will save heap allocations when len <= 8 (which is probably most of the time?) ::: dom/animation/KeyframeUtils.cpp:1355 (Diff revision 7) > + const nsCSSProperty* subp = nsCSSProps::SubpropertyEntryFor(aProperty); > + while (*subp != eCSSProperty_UNKNOWN) { I think we normally use CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES for this. ::: dom/animation/KeyframeUtils.cpp:1363 (Diff revision 7) > + if (aPacedValues[i].IsEmpty()) { > + cumulativeDist[i] = cumulativeDist[i - 1]; > + continue; > + } We should move the comment above about non-paceable keyframes down here. ::: dom/animation/KeyframeUtils.cpp:1374 (Diff revision 7) > + if (pacedIsShorthand) { > + ComputedKeyframeValues& prePairs = aPacedValues[preIdx]; > + ComputedKeyframeValues& curPairs = aPacedValues[i]; > + > + // Calculate distance. > + // mPacedPropertyAnimValue is sorted, so the components should be aligned. I don't think this comment makes sense any more? ::: dom/animation/KeyframeUtils.cpp:1437 (Diff revision 7) > +static nsTArray<ComputedKeyframeValues> > +GetPacedPropertyKeyframeValues(const nsTArray<ComputedKeyframeValues>& aValues, > + nsCSSProperty aProperty) > +{ See my comments above. I wonder if we can replace this with a function that returns a cumulative lengths array. ::: dom/animation/KeyframeUtils.cpp:1441 (Diff revision 7) > + */ > +static nsTArray<ComputedKeyframeValues> > +GetPacedPropertyKeyframeValues(const nsTArray<ComputedKeyframeValues>& aValues, > + nsCSSProperty aProperty) > +{ > + nsTArray<ComputedKeyframeValues> result; Can we initialize this to aValues.Length()? ::: dom/animation/KeyframeUtils.cpp:1448 (Diff revision 7) > + const nsCSSProperty* p = nsCSSProps::SubpropertyEntryFor(aProperty); > + for (; *p != eCSSProperty_UNKNOWN; ++p) { I think we normally use CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES for this. ::: dom/animation/KeyframeUtils.cpp:1460 (Diff revision 7) > + propertyCount = 1; > + } > + > + // b) Search each component (shorthand) or the longhand property, > + for (const ComputedKeyframeValues& computedValues : aValues) { > + ComputedKeyframeValues* pacedValues = result.AppendElement(); Add a comment that we add an element even if this frame does not contain the paced property since |result| is a parallel array to |aValues|. ::: dom/animation/KeyframeUtils.cpp:1473 (Diff revision 7) > + // properties, it should be non-paceable. For longhand, the number of > + // components is always 1. > + pacedValues->Clear(); > + } > + } > + return result; Assert that result.Length() == aValues.Length()

https://reviewboard.mozilla.org/r/54414/#review52768 > I thought your comment in comment 109 meant we could keep this assertion? Oh, Yes. Looks like I drop this line by accident.

Comment on attachment 8752106 [details] Bug 1244590 - Part 3: Parse spacing. https://reviewboard.mozilla.org/r/51511/#review52786 r=me with comments addressed. I hope we have *lots* of tests for this! (Including all the weird edge cases like \ before all three types of newline etc.) ::: dom/animation/KeyframeEffectParams.cpp:19 (Diff revisions 4 - 5) > namespace mozilla { > > -// This is a simplified version of consuming ident token, which means we don't > -// decode special escapes, and we use the last paired ')' as the end of this > -// token if it exists. > +static inline bool > +IsAlpha(char16_t aCh) > +{ > + return std::isalpha(static_cast<unsigned char>(aCh)) != 0; So isalpha is locale dependent.[1] Maybe it doesn't matter in this case but would it be easier to call this IsLetter and just check if |aCh| is between 0x41 <= aCh <= 0x5A and 0x61 <= aCh <= 0x7A ? [1] http://en.cppreference.com/w/cpp/string/byte/isalpha ::: dom/animation/KeyframeEffectParams.cpp:25 (Diff revisions 4 - 5) > +} > + > +static inline bool > +IsDigit(char16_t aCh) > +{ > + return std::isdigit(static_cast<unsigned char>(aCh)) != 0; Similarly here? Between 0x30 and 0x39 (inclusive)? Then we don't need to include cctype. ::: dom/animation/KeyframeEffectParams.cpp:31 (Diff revisions 4 - 5) > +} > + > +static inline bool > +IsNameStartCode(char16_t aCh) > +{ > + // 0x80 is control code. Not sure we need this comment ::: dom/animation/KeyframeEffectParams.cpp:52 (Diff revisions 4 - 5) > +} > + > +static inline bool > +IsValidEscape(char16_t aFirst, char16_t aSecond) > +{ > + return (aFirst == '\\') && !IsNewLine(aSecond); Nit: Do we need the () around aFirst == '\\' ? Likewise elsewhere in this file. ::: dom/animation/KeyframeEffectParams.cpp:55 (Diff revisions 4 - 5) > +static bool > +StartIdent(RangedPtr<const char16_t> aIter, > + const char16_t* const aEnd) Perhaps call this IsIdentStart? ::: dom/locales/en-US/chrome/dom/dom.properties:229 (Diff revisions 4 - 5) > RewriteYoutubeEmbedInvalidQuery=Rewriting old-style Youtube Flash embed (%S) to iframe embed (%S). Query was invalid and removed from URL. Please update page to use iframe instead of embed/object, if possible. > # LOCALIZATION NOTE: Do not translate "ServiceWorker". %1$S is the ServiceWorker scope URL. %2$S is an error string. > PushMessageDecryptionFailure=The ServiceWorker for scope ‘%1$S’ encountered an error decrypting a push message: ‘%2$S’. For help with encryption, please see https://developer.mozilla.org/en-US/docs/Web/API/Push_API/Using_the_Push_API#Encryption > # LOCALIZATION NOTE: %1$S is the type of a DOM event. 'passive' is a literal parameter from the DOM spec. > PreventDefaultFromPassiveListenerWarning=Ignoring ‘preventDefault()’ call on event of type ‘%1$S’ from a listener registered as ‘passive’. > -# LOCALIZATION NOTE: %1$S is the unrecongnized property string. > +# LOCALIZATION NOTE: %1S is the unanimatable paced propety. s/propety/property/ ::: dom/locales/en-US/chrome/dom/dom.properties:229 (Diff revisions 4 - 5) > -# LOCALIZATION NOTE: %1$S is the unrecongnized property string. > -UnrecognizedPacedProperty=The paced property '%1$S' is unrecognized. > +# LOCALIZATION NOTE: %1S is the unanimatable paced propety. > +UnanimatablePacedProperty=Paced property ‘%1S’ is not an animatable property. Should %1S be %1$S ?

I've given r+ to part 3 and part 6 but there was no corresponding review request in MozReview so I think if you want to be able to use Autoland, you'll have to request review through MozReview for those (otherwise it will probably not let you because it thinks the patches haven't been reviewed).

https://reviewboard.mozilla.org/r/51511/#review52786 OK. I will add more tests for the parser in part 10. > So isalpha is locale dependent.[1] Maybe it doesn't matter in this case but would it be easier to call this IsLetter and just check if |aCh| is between 0x41 <= aCh <= 0x5A and 0x61 <= aCh <= 0x7A ? > > [1] http://en.cppreference.com/w/cpp/string/byte/isalpha OK. We can us IsLetter() and check the code point range directly. > Nit: Do we need the () around aFirst == '\\' ? Likewise elsewhere in this file. Actually, no. I can remove '()'. > Perhaps call this IsIdentStart? Sure > Should %1S be %1$S ? I will update this to use %1$S.

(In reply to Brian Birtles (:birtles, high review load) from comment #94) > You can use keyframeA.get()->mOffset I think, but that's a bit awkward. > > Jeff, is there any reason RangedPtr doesn't provide operator-> ? Hi, Jeff I file bug 1276550 and cc you.

(In reply to Brian Birtles (:birtles, high review load) from comment #174) > I've given r+ to part 3 and part 6 but there was no corresponding review > request in MozReview so I think if you want to be able to use Autoland, > you'll have to request review through MozReview for those (otherwise it will > probably not let you because it thinks the patches haven't been reviewed). Thanks, Brian. I resend the review requests for p3 and p6.

Comment on attachment 8752107 [details] Bug 1244590 - Part 4: Rewrite ApplyDistributeSpacing. https://reviewboard.mozilla.org/r/51549/#review52794 ::: dom/animation/KeyframeUtils.cpp:461 (Diff revision 5) > { > if (aKeyframes.IsEmpty()) { > return; > } > > - // If the first or last keyframes have an unspecified offset, > + // If the first keyframes have an unspecified offset, fill it in with 0%. Nit: s/have/has/

https://reviewboard.mozilla.org/r/52139/#review52800 I guess this isn't ready for review since the review flag hasn't been set, but at a glance: * I think we need more tests for parsing behavior (different invalid escape sequences, leading space, etc.) * We need to test a non-animatable shorthand (e.g. 'animation') * Most of the tests that cover the actual spacing operation should go in the animation-model test, not in the interfaces test. That said I didn't look at those tests too closely so I might be missing something.

(In reply to Brian Birtles (:birtles, high review load) from comment #171) > b) Make mozilla::Range easier to use so that it accepts two RangedPtr > arguments and does the necessary order and range checking. Again, we'd > probably need to add API to RangedPtr so that we could assert that the end > iterator is within the range of the first iterator. > > I'm slightly leaning towards (b) I think. I think if we did that, this patch > could become simpler and easier to read. OK. Let's try (b). I will file a bug to add a new constructor which accepts two RangedPtr<T> arguments.

https://reviewboard.mozilla.org/r/51809/#review52772 > Is the +1 here correct? If not, we should try to write a test that will fail if we leave the + 1 here. I think we have to '+1'. Range<T>::mEnd is excluded, so 'keyframeB - pacedB + 1' makes Range<T>::mEnd be the element after keyframeB. Therefore, if we add a new constructor for Range<T>, this line may be changed to "Range<Keyframe>(pacedB.get(), (keyframeB + 1).get());". We still have to add '+1' in this case. > At first I thought we should factor this out into a separate method but actually I wonder if we can actually calculate all the distances in GetPacedPropertyKeyframeValues? > > If we do that, I don't think we even need to return the StyleAnimationValues? All we really need is to know all the computed distances and which keyframes are paceable, right? > > For example, could we simply return a parallel array of doubles representing the cumulative distances across the whole set of keyframes? For non-paceable keyframes we could set the cumulative distance to kNotPaceable = -1. Then we'd still pass a range within that cumulative distances array to PaceRange and PaceRange would need to get the cumulative distance of the first element in that range and subtract that from other cumulative distances in order to get a relative distance. > > I think if we do that, this becomes a lot simpler? Yes, all we really need is to know all the computed distances and which keyframes are paceable. Let me try this way. Thanks.

https://reviewboard.mozilla.org/r/51809/#review52772 > I think we have to '+1'. Range<T>::mEnd is excluded, so 'keyframeB - pacedB + 1' makes Range<T>::mEnd be the element after keyframeB. Therefore, if we add a new constructor for Range<T>, this line may be changed to "Range<Keyframe>(pacedB.get(), (keyframeB + 1).get());". We still have to add '+1' in this case. > 'keyframeB - pacedB + 1' makes Range<T>::mEnd be the element after keyframeB But I thought we *don't* want to include keyframeB in the range to update, i.e. keyframeB *is* the end iterator. The comment says we want to adjust the range "[Paced B, keyframe B)" i.e. Paced B <= x < keyframe B If we add a new constructor for Range<T> it should take a RangedPtr so that we write "Range<Keyframe>(pacedB, keyframeB)" (or Range<Keyframe>(pacedB, keyframeB + 1) if we *do* want to include keyframeB in the range).

https://reviewboard.mozilla.org/r/51809/#review52772 > > 'keyframeB - pacedB + 1' makes Range<T>::mEnd be the element after keyframeB > > But I thought we *don't* want to include keyframeB in the range to update, i.e. keyframeB *is* the end iterator. The comment says we want to adjust the range "[Paced B, keyframe B)" i.e. Paced B <= x < keyframe B > > If we add a new constructor for Range<T> it should take a RangedPtr so that we write "Range<Keyframe>(pacedB, keyframeB)" (or Range<Keyframe>(pacedB, keyframeB + 1) if we *do* want to include keyframeB in the range). Oh yes. You're right. We don't have to apply spacing to keyframeB here. :)

(In reply to Brian Birtles (:birtles) from comment #179) > * Most of the tests that cover the actual spacing operation should go in the > animation-model test, not in the interfaces test. That said I didn't look at > those tests too closely so I might be missing something. Does this mean that any combination of paceable/nonpaceable keyframes (with shorthand or longhand paced property) _without_ specific offsets should be put into interfaces test? And other combinations _with_ specific offsets are in animation-model. If yes, I will follow this rule. Thanks.

Comment on attachment 8752106 [details] Bug 1244590 - Part 3: Parse spacing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51511/diff/5-6/

Comment on attachment 8752107 [details] Bug 1244590 - Part 4: Rewrite ApplyDistributeSpacing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51549/diff/5-6/

Comment on attachment 8755339 [details] Bug 1244590 - Part 6: Refactor the calculation of StyleAnimationValue. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54414/diff/4-5/

(In reply to Boris Chiou [:boris] (away 6/6~6/10) from comment #184) > (In reply to Brian Birtles (:birtles) from comment #179) > > * Most of the tests that cover the actual spacing operation should go in the > > animation-model test, not in the interfaces test. That said I didn't look at > > those tests too closely so I might be missing something. > > Does this mean that any combination of paceable/nonpaceable keyframes (with > shorthand or longhand paced property) _without_ specific offsets should be > put into interfaces test? And other combinations _with_ specific offsets are > in animation-model. > > If yes, I will follow this rule. Thanks. The rule is just to try to match the spec. So, any behavior defined by the "Applying spacing to keyframes" procedure in the spec[1], should go in a test that roughly matches the spec's hierarchy, e.g. animation-model/keyframe-effects/spacing-keyframes.html. If you're testing things like parse errors, then that's defined alongside the spacing member of the KeyframeEffect interface[2] so it should go in interfaces/KeyframeEffect/spacing.html or something like that. Sometimes it's not entirely clear where to put things but mostly we should try to follow the spec's hierarchy but without adding too many levels of nesting (see [3]). [1] https://w3c.github.io/web-animations/#applying-spacing-to-keyframes [2] https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-spacing [3] https://github.com/w3c/web-platform-tests/blob/master/web-animations/README.md

Comment on attachment 8752109 [details] Bug 1244590 - Part 7: Calculate paced spacing. https://reviewboard.mozilla.org/r/51809/#review53814 This is good but I'd like to check once more after some of the suggested simplifications have been made (and with the question about ComputeDistance failing for shorthands answered). ::: dom/animation/KeyframeUtils.cpp:404 (Diff revision 9) > +static void > +PaceRange(const Range<Keyframe>& aKeyframes, > + const Range<double>& aCumulativeDist); > + > +static nsTArray<double> > +GetPacedCumulativeDistance(const nsTArray<ComputedKeyframeValues>& aValues, nit: This should probably be "Distances" (plural) since it is returning an array. Perhaps GetCumulativeDistances would be enough? ::: dom/animation/KeyframeUtils.cpp:533 (Diff revision 9) > + while (pacedB > keyframeA && > + cumulativeDist[pacedB - begin] == kNotPaceable) { > + --pacedB; > + } > + // As spec says, if there is no paceable keyframe > + // in [keyframe A, keyframe B], we let Paced A and Paced refer to Nit: Paced A and Paced B ::: dom/animation/KeyframeUtils.cpp:538 (Diff revision 9) > + // in [keyframe A, keyframe B], we let Paced A and Paced refer to > + // keyframe B. > + if (pacedA > pacedB) { > + pacedA = pacedB = keyframeB; > + } > + // b) Apply evenly distributing offsets in (keyframe A, Paced A] and Nit: b) Apply distribute spacing in ... ::: dom/animation/KeyframeUtils.cpp:544 (Diff revision 9) > + // [Paced B, keyframe B). > + DistributeRange(Range<Keyframe>(keyframeA, keyframeB + 1), > + Range<Keyframe>(keyframeA + 1, pacedA + 1)); > + DistributeRange(Range<Keyframe>(keyframeA, keyframeB + 1), > + Range<Keyframe>(pacedB, keyframeB)); > + // c) Apply paced offsets in (Paced A, Paced B). We should probably add a note explaining why we pass the range [Paced A, Paced B] when the comment says (Paced A, Paced B). e.g. Apply paced offsets to each paceable keyframe in (Paced A, Paced B). We pass the range [Paced A, Paced B] since PaceRange needs the end points of the range in order to calculate the correct offset. ::: dom/animation/KeyframeUtils.cpp:546 (Diff revision 9) > + Range<double>(&cumulativeDist[pacedA - begin], > + pacedB - pacedA + 1)); Nit: Indentation here. I think "pacedB - paced A + 1" should sit under &cumulativeDist... ::: dom/animation/KeyframeUtils.cpp:1336 (Diff revision 9) > - * Evenly distribute the computed offsets in (A, B). We should pass the > - * range keyframes in [A, B] and use A, B to calculate computed offsets in > - * (A, B). > + * Evenly distribute the computed offsets in (A, B). > + * We pass the range keyframes in [A, B] and use A, B to calculate distributing > + * computed offsets in (A, B). I'm not sure this comment is correct any longer. It should mention the two different ranges we pass in. ::: dom/animation/KeyframeUtils.cpp:1348 (Diff revision 9) > + MOZ_ASSERT(aRangeToAdjust.start() >= aSpacingRange.start() && > + aRangeToAdjust.end() <= aSpacingRange.end(), > + "Out of range"); Is it ok for aRangeToAdjust.start() == aSpacingRange.start()? ::: dom/animation/KeyframeUtils.cpp:1354 (Diff revision 9) > + const size_t startIdx = aRangeToAdjust.start() - aSpacingRange.start(); > + const size_t endIdx = aRangeToAdjust.end() - aSpacingRange.start(); > + for (size_t i = startIdx; i < endIdx; ++i) { > + aSpacingRange[i].mComputedOffset = startOffset + double(i) / n * diffOffset; > + } Would something like the following be simpler: for (auto iter = aRangeToAdjust.start(); iter != aRangeToAdjust.end(); ++iter) { size_t index = iter - aSpacingRange.start(); iter->mComputedOffset = startOffset + double(index) / n * diffOffset; } ::: dom/animation/KeyframeUtils.cpp:1362 (Diff revision 9) > + * Overloading for DistributeRange if we want to apply distribute spacing > + * to all keyframes. Overload of DistributeRange to apply distribute spacing to all keyframes in between the endpoints of the given range. ::: dom/animation/KeyframeUtils.cpp:1378 (Diff revision 9) > + Range<Keyframe>(aSpacingRange.start() + 1, > + aSpacingRange.end() - 1)); > +} > + > +/** > + * Apply paced computed offsets in (Paced A, Paced B). Nit: Apply paced spacing to all paceable keyframes in between the endpoints of the given range. ::: dom/animation/KeyframeUtils.cpp:1394 (Diff revision 9) > - const size_t n = aKeyframes.length() - 1; > - const double startOffset = aKeyframes[0].mComputedOffset; > + MOZ_ASSERT(aKeyframes.length() == aCumulativeDist.length(), > + "Range length mismatch"); I think we also need to assert that aKeyframes.start()->mComputedOffset != Keyframe::kComputedOffsetNotSet? And likewise for end() - 1? ::: dom/animation/KeyframeUtils.cpp:1403 (Diff revision 9) > + const size_t pacedA = 0; > + const size_t pacedB = len - 1; > + MOZ_ASSERT(aCumulativeDist[pacedA] != kNotPaceable && > + aCumulativeDist[pacedB] != kNotPaceable, > + "Both Paced A and Paced B should be paceable."); > + > + // If total distance is 0.0, we should fall it back to distribute spacing, so > + // just return. > + if (aCumulativeDist[pacedA] == aCumulativeDist[pacedB]) { > + return; > + } > + > + // Apply computed offset. > + const double offsetA = aKeyframes[pacedA].mComputedOffset; > + const double diffOffset = aKeyframes[pacedB].mComputedOffset - offsetA; > + const double previousCumulativeDist = aCumulativeDist[pacedA]; > + const double totalDist = aCumulativeDist[pacedB] - previousCumulativeDist; > + for (size_t i = pacedA + 1; i < pacedB; ++i) { > + if (aCumulativeDist[i] == kNotPaceable) { > + continue; > + } > + double currDist = aCumulativeDist[i] - previousCumulativeDist; > + aKeyframes[i].mComputedOffset = offsetA + diffOffset * currDist / totalDist; > + } I think this could be more simple if we used iterators. Something like: const double distA = *(aCumulativeDist.start()); const double distB = *(aCumulativeDist.end() - 1); MOZ_ASSERT(*distA != kNotPaceable && distB != kNotPaceable, "Both Paced A and Paced B should be paceable"); // If the total distance is zero, we should fall back to distribute spacing. // The caller will fill-in any keyframes without a computed offset using // distribute spacing so we can just return here. if (distA == distB) { return; } const RangedPtr<Keyframe> pacedA = aKeyframes.start(); const RangedPtr<Keyframe> pacedB = aKeyframes.end() - 1; const double offsetA = pacedA->mComputedOffset; const double diffOffset = pacedB->mComputedOffset - offsetA; const double initialDist = distA; const double totalDist = distB - initialDist; for (auto iter = pacedA + 1; iter != pacedB; ++iter) { size_t k = iter - aKeyframes.start(); if (aCumulativeDist[k] == kNotPaceable) { continue; } double dist = aCumulativeDist[k] - initialDist; iter->mComputedOffset = offsetA + diffOffset * dist / totalDist; } ::: dom/animation/KeyframeUtils.cpp:1432 (Diff revision 9) > +} > + > +/** > + * Get cumulative distances for the paced property. > + * > + * @param aValues The computed values got by GetComputedKeyframeValues. s/got by/returned by/ ::: dom/animation/KeyframeUtils.cpp:1443 (Diff revision 9) > + size_t propertyCount = 0; > + nsCSSPropertySet propSet; > + bool pacedIsShorthand = nsCSSProps::IsShorthand(aPacedProperty); I think this would be a lot easier to follow if we changed the naming. For example, propertyCount -> pacedPropertyCount propSet -> pacedPropertySet pacedIsShorthand -> pacedPropertyIsShorthand (or even just 'isShorthand' might be enough) ::: dom/animation/KeyframeUtils.cpp:1460 (Diff revision 9) > + } > + > + // b) Search each component (shorthand) or the longhand property, and > + // calculate the cumulative distances of paceable keyframe pairs. > + const size_t len = aValues.Length(); > + nsTArray<double> cumulativeDist(len); Nit: Can we call this cumulativeDistances? (In general it's nice to avoid abbreviations unless they're common or the word is really long.) ::: dom/animation/KeyframeUtils.cpp:1467 (Diff revision 9) > + // the length of |aValues|. > + cumulativeDist.SetLength(len); > + ComputedKeyframeValues prevPacedValues; > + size_t preIdx = 0; > + for (size_t i = 0; i < len; ++i) { > + // 1. Find computed values of the paced property. Maybe we don't need the numbering here? We only have two numbers? ::: dom/animation/KeyframeUtils.cpp:1468 (Diff revision 9) > + cumulativeDist.SetLength(len); > + ComputedKeyframeValues prevPacedValues; > + size_t preIdx = 0; > + for (size_t i = 0; i < len; ++i) { > + // 1. Find computed values of the paced property. > + ComputedKeyframeValues currPacedValues; Can we call this currentPacedValues (or just pacedValues which should contrast enough to prevPacedValues?) ::: dom/animation/KeyframeUtils.cpp:1475 (Diff revision 9) > + if (propSet.HasProperty(pair.mProperty)) { > + currPacedValues.AppendElement(pair); > + } > + } > + > + // 2. Calculate cumulative distance. This comment doesn't describe the code immediately following it so it's a bit hard to read. Perhaps just change the comment to "Check we have values for all the paceable longhand components" ? ::: dom/animation/KeyframeUtils.cpp:1483 (Diff revision 9) > + cumulativeDist[i] = kNotPaceable; > + continue; > + } > + > + if (prevPacedValues.IsEmpty()) { > + // This is the first paceable keyframe and its cumulative distance is 0.0. Nit: s/and/so/ ::: dom/animation/KeyframeUtils.cpp:1488 (Diff revision 9) > + for (size_t subIdx = 0; subIdx < propertyCount; ++subIdx) { > + nsCSSProperty subProperty = prevPacedValues[subIdx].mProperty; I'm not sure if the 'subIdx' / 'subProp' naming is very clear? Perhaps 'propIdx' and 'prop' would be ok? Also, we should add a comment to explain what we're doing. Something about applying the square distance formula (or whatever the correct term is) to each of the longhand components. ::: dom/animation/KeyframeUtils.cpp:1493 (Diff revision 9) > + for (size_t subIdx = 0; subIdx < propertyCount; ++subIdx) { > + nsCSSProperty subProperty = prevPacedValues[subIdx].mProperty; > + MOZ_ASSERT(currPacedValues[subIdx].mProperty == subProperty, > + "subProperty mismatch"); > + > + // If ComputeDistance() is failed, componentDist is still 0.0, so Nit: If ComputeDistance() fails, componentDist will still be zero... ::: dom/animation/KeyframeUtils.cpp:1494 (Diff revision 9) > + // there is distance for this component. I believe if one component > + // has no distance, neither do others, so the final dist is still 0.0. Why do you believe this? I'm not sure I understand. Does it matter if one component fails? ::: dom/animation/KeyframeUtils.cpp:1501 (Diff revision 9) > + double componentDist = 0.0; > + StyleAnimationValue::ComputeDistance(subProperty, > + prevPacedValues[subIdx].mValue, > + currPacedValues[subIdx].mValue, > + componentDist); > + // FIXME: Any way to avoid overflow? I don't think we need to worry about this. Do you have any reason to expect we will reach this situation often? ::: dom/animation/KeyframeUtils.cpp:1507 (Diff revision 9) > + // If ComputeDistance() is failed, dist is still 0.0, so there is no > + // distance between this keyframe pair. Nit: If ComputeDistance() fails, |dist| will remain zero so there will be no distance between the previous paced value and this value.

https://reviewboard.mozilla.org/r/51809/#review53814 > Is it ok for aRangeToAdjust.start() == aSpacingRange.start()? If aRangeToAdjust.start() == aSpacingRange.start(), the index of the first keyframe is 0, so we just copy its mComputedOffset to itself according to the formula[1]. I think it is ok. However, I can remove the equality from the assertion if we want to avoid this redundant calculation and copy. BTW, now we only have one case for this: if Keyframe A is the only paceable keyframe, i.e. Paced A == Paced B == Keyframe A, aRangeToAdjust.start() will be equal to aSpacingRange.start(). [1] aSpacingRange[0].mComputedOffset = startOffset + 0 / n * diffOffset; // startOffset is equal to aSpacingRange[0].mComputedOffset. > I think we also need to assert that aKeyframes.start()->mComputedOffset != Keyframe::kComputedOffsetNotSet? And likewise for end() - 1? Oh Yes, I should also add this assertion. > I think this could be more simple if we used iterators. Something like: > > const double distA = *(aCumulativeDist.start()); > const double distB = *(aCumulativeDist.end() - 1); > MOZ_ASSERT(*distA != kNotPaceable && distB != kNotPaceable, > "Both Paced A and Paced B should be paceable"); > > // If the total distance is zero, we should fall back to distribute spacing. > // The caller will fill-in any keyframes without a computed offset using > // distribute spacing so we can just return here. > if (distA == distB) { > return; > } > > const RangedPtr<Keyframe> pacedA = aKeyframes.start(); > const RangedPtr<Keyframe> pacedB = aKeyframes.end() - 1; > > const double offsetA = pacedA->mComputedOffset; > const double diffOffset = pacedB->mComputedOffset - offsetA; > const double initialDist = distA; > const double totalDist = distB - initialDist; > > for (auto iter = pacedA + 1; iter != pacedB; ++iter) { > size_t k = iter - aKeyframes.start(); > if (aCumulativeDist[k] == kNotPaceable) { > continue; > } > > double dist = aCumulativeDist[k] - initialDist; > iter->mComputedOffset = offsetA + diffOffset * dist / totalDist; > } Thanks for your suggestion, Brian. I will try to use iterators, just as DistributeRange does. > I think this would be a lot easier to follow if we changed the naming. > > For example, > > propertyCount -> pacedPropertyCount > propSet -> pacedPropertySet > pacedIsShorthand -> pacedPropertyIsShorthand (or even just 'isShorthand' might be enough) Thanks, Brian. They look great. > Maybe we don't need the numbering here? We only have two numbers? OK. I will remove them. > Why do you believe this? I'm not sure I understand. > > Does it matter if one component fails? ComputeDistance() returns false if 1. We haven't implement for this case. 2. Ther is no common unit. 3. Other reasons So I though we can treat them as 0.0, and continue to calculate other parts to get a distance. However, after thinking more, I think I'm wroing. This may cause problems when this distance is added to other distances (whose components are all calculated correctly). Maybe we can set this keyframe as not paceable, so it will be falled back to distribute spacing? > I don't think we need to worry about this. Do you have any reason to expect we will reach this situation often? No often, I think. Only if the user wants to do something unusual. I will remove this comment.

https://reviewboard.mozilla.org/r/51809/#review53814 > Thanks for your suggestion, Brian. I will try to use iterators, just as DistributeRange does. BTW, I found RangedPtr<T>::operator+() is not a const method, so we cannot add a const RangedPtr<T> with an integer, so I will remove the constness from pacedA. > ComputeDistance() returns false if > 1. We haven't implement for this case. > 2. Ther is no common unit. > 3. Other reasons > > So I though we can treat them as 0.0, and continue to calculate other parts to get a distance. However, after thinking more, I think I'm wroing. This may cause problems when this distance is added to other distances (whose components are all calculated correctly). Maybe we can set this keyframe as not paceable, so it will be falled back to distribute spacing? Or just treat the square root distance as zero.

Comment on attachment 8752109 [details] Bug 1244590 - Part 7: Calculate paced spacing. https://reviewboard.mozilla.org/r/51809/#review54208 r=me with the following comments addressed. Let me know what you think we should do when there is a component where we can't apply spacing. My hunch is we should just ignore it, but let me know what you think. Thanks for all your work on this! ::: dom/animation/KeyframeUtils.cpp:1432 (Diff revisions 8 - 10) > // Apply computed offset. > - const double offsetA = aKeyframes[pacedA].mComputedOffset; > - const double diffOffset = aKeyframes[pacedB].mComputedOffset - offsetA; > - const double previousCumulativeDist = aCumulativeDist[pacedA]; > - const double totalDist = aCumulativeDist[pacedB] - previousCumulativeDist; > - for (size_t i = pacedA + 1; i < pacedB; ++i) { > + const double offsetA = pacedA->mComputedOffset; > + const double diffOffset = pacedB->mComputedOffset - offsetA; > + const double initialDist = distA; > + const double totalDist = distB - initialDist; > + for (auto iter = RangedPtr<Keyframe>(pacedA) + 1; iter != pacedB; ++iter) { > BTW, I found RangedPtr<T>::operator+() is not a const method, so we cannot > add a const RangedPtr<T> with an integer, so I will remove the constness > from pacedA. Can you please file a bug for this? I'm pretty sure that should be const. Thanks. And then can we add a comment here referring to that bug number to explain why we're wrapping the pointer like this. ::: dom/animation/KeyframeUtils.cpp:1515 (Diff revision 10) > + // If we can not calculate one or more components, it may cause > + // problems when adding it to other cumulative distances, so set > + // |dist| to be zero directly. > + dist = 0.0; > + break; Can you think of an example where this would be a problem? For example, if we have: some-shorthand-prop: 2px 3px outside some-shorthand-prop: 5px 10px inside And we discover we can't compute a distance between 'outside' and 'inside', it seems like it might still be ok to just use the distances we can calculate? What do you think? (If you agree, we could just use 'continue' here I guess. Or, better still just change the check to "if (StyleAnimationValue::ComputeDistance) ... { dist += componentDistance * componentDistance; }")

https://reviewboard.mozilla.org/r/51809/#review54208 > > BTW, I found RangedPtr<T>::operator+() is not a const method, so we cannot > > add a const RangedPtr<T> with an integer, so I will remove the constness > > from pacedA. > > Can you please file a bug for this? I'm pretty sure that should be const. Thanks. > > And then can we add a comment here referring to that bug number to explain why we're wrapping the pointer like this. OK. I file Bug 1277740, I will add a comment for this. Thanks. > Can you think of an example where this would be a problem? > > For example, if we have: > > some-shorthand-prop: 2px 3px outside > some-shorthand-prop: 5px 10px inside > > And we discover we can't compute a distance between 'outside' and 'inside', it seems like it might still be ok to just use the distances we can calculate? > > What do you think? > > (If you agree, we could just use 'continue' here I guess. Or, better still just change the check to "if (StyleAnimationValue::ComputeDistance) ... { dist += componentDistance * componentDistance; }") Yes, I agree. This is definitely a good case to continue the loop. The problem I mentioned is not easy to happen, so let's continue the loop if we can't compute a distance. I will update this soon. Thanks for your review. :)

Comment on attachment 8752109 [details] Bug 1244590 - Part 7: Calculate paced spacing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51809/diff/10-11/

Comment on attachment 8752112 [details] Bug 1244590 - Part 10: Test for creating animations with a specific spacing mode. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52139/diff/10-11/

https://reviewboard.mozilla.org/r/51809/#review54208 > Yes, I agree. This is definitely a good case to continue the loop. The problem I mentioned is not easy to happen, so let's continue the loop if we can't compute a distance. I will update this soon. Thanks for your review. :) The problem I metioned is just like: [1] some-shorthand-prop: 2px 3px calc(2px + 3%) // or other values which we accept but ComputeDistance cannot handle. [2] some-shorthand-prop: 5px 10px 2px [3] some-shorthand-prop: 2px 3px 4px If the 3rd component cannot be calculated by ComptueDistance() for the keyframe pair [1] and [2], the total cumulative distance is not correct I think. However, this should be fixed in ComputeDistance(), not in the algorithm of applying paced spacing. Therefore, I agree your thought. (Or maybe I'm wrong because it is impossible to happen).

Comment on attachment 8752112 [details] Bug 1244590 - Part 10: Test for creating animations with a specific spacing mode. https://reviewboard.mozilla.org/r/52139/#review54668 This is excellent work. r=me with comments addressed. The only other test I'm not sure if we added was when specifying a paced property that is not provided on *any* of the keyframes. Do we have that? ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:97 (Diff revision 11) > + var cumDist = [0, 0, 100, 150]; > + assert_equals(frames[1].computedOffset, cumDist[1] / cumDist[3], > + '2nd frame offset'); > + assert_equals(frames[2].computedOffset, cumDist[2] / cumDist[3], > + '3rd frame offset'); > +}, 'Test paced spacing if some paced property value are not changed'); Perhaps s/value are not changes/values are equal/ would be easier to understand? ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:259 (Diff revision 11) > + dist.reduce(function(prev, curr, i) { return cumDist[i] = prev + curr; }, 0); > + assert_approx_equals(frames[1].computedOffset, cumDist[1] / cumDist[3], > + 0.0001, '2nd frame offset'); > + assert_approx_equals(frames[2].computedOffset, cumDist[2] / cumDist[3], > + 0.0001, '3rd frame offset'); > +}, 'Test paced spacing for using shorthand property'); Test paced spacing using shorthand property where only the longhand components are specified ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/spacing-keyframes.html:314 (Diff revision 11) > + 'first paceable keyframe from a non-null offset keyframe'); > +}, 'Test paced spacing only for keyframes specifying all some components, ' + > + 'and falling back to distribute spacing for the reset with some specific ' + > + 'offsets'); > + > +// FIXME: Test for mixing percent and pixel values. I think this got split off into a separate bug. Perhaps we should put the bug number here?

https://reviewboard.mozilla.org/r/52139/#review54668 Thanks, Brian. Your review is very helpful and you addressed many things I didn't notice. > The only other test I'm not sure if we added was when specifying a paced property that is not provided on *any* of the keyframes. I think the anwser is no. I will add one more test in spacing-keyframes.html: e.g. test(function(t) { var anim = createDiv(t).animate(null, { duration: 100 * MS_PER_SEC, spacing: 'paced(margin-left)' }); var frames = anim.effect.getKeyframes(); assert_equals(frames.length, 0, "empty keyframe list"); }, 'Test paced spacing without any keyframe'); > I think this got split off into a separate bug. Perhaps we should put the bug number here? Sure. I will add Bug 1276193 in the comment.

I would like to use other constructors of Range<> (i.e. Range(T* aPtr, size_t aLength)) in part 7 and add Bug 1276573 in the comment, so we can merge this bug ASAP. After landing Bug 1276573, I will file another bug to use the new constructor. Thanks.

Comment on attachment 8752109 [details] Bug 1244590 - Part 7: Calculate paced spacing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51809/diff/11-12/

Comment on attachment 8752112 [details] Bug 1244590 - Part 10: Test for creating animations with a specific spacing mode. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52139/diff/12-13/

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7526bb22d3a5 Part 1: Introduce KeyframeEffectParams. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/eedb4ecfeb3b Part 2: Retrieve KeyframeEffectOptions from constructor. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6aab557fc8 Part 3: Parse spacing. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/02de18e88fc7 Part 4: Rewrite ApplyDistributeSpacing. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcc3a1a3cad Part 5: Make the default value of computed offsets be -1. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/8755ef376a4c Part 6: Refactor the calculation of StyleAnimationValue. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/4609f300a921 Part 7: Calculate paced spacing. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd91e7fed52 Part 8: Rewrite GetStyleContext code. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/04b5585125ad Part 9: Update spacing in SetTarget. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/166e184b73cd Part 10: Test for creating animations with a specific spacing mode. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/981f75fa50d2 Part 11: Test for different targets. r=birtles

https://hg.mozilla.org/mozilla-central/rev/7526bb22d3a5 https://hg.mozilla.org/mozilla-central/rev/eedb4ecfeb3b https://hg.mozilla.org/mozilla-central/rev/3d6aab557fc8 https://hg.mozilla.org/mozilla-central/rev/02de18e88fc7 https://hg.mozilla.org/mozilla-central/rev/cfcc3a1a3cad https://hg.mozilla.org/mozilla-central/rev/8755ef376a4c https://hg.mozilla.org/mozilla-central/rev/4609f300a921 https://hg.mozilla.org/mozilla-central/rev/6bd91e7fed52 https://hg.mozilla.org/mozilla-central/rev/04b5585125ad https://hg.mozilla.org/mozilla-central/rev/166e184b73cd https://hg.mozilla.org/mozilla-central/rev/981f75fa50d2

Looks like this has been dropped from the latest spec, so I'll hold off on adding anything to the docs. Is it being replaced by something else instead?

No, it's been dropped outright. Google didn't want to implement it and it's less necessary given the way the CSS Motion path module has been specified.