Closed Bug 1198708 Opened 4 years ago Closed 4 years ago

Implement KeyframeEffect.getFrames()

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: birtles, Assigned: heycam)

References

Details

Attachments

(7 files)

No description provided.
To implement KeyframeEffectReadOnly.getFrames() in this bug, and the KeyframeEffectReadOnly constructor (in a soon to be filed bug that also blocks bug 1198705), we'll need to handle Keyframe dictionaries having this (partially) "open ended" behaviour, where in addition to a few pre-defined dictionary members, we can also have members corresponding to any animatable CSS property.

I'm wondering what the best way to implement this would be.  The spec currently has a definition of what to do for the JS -> IDL conversion (the IDL -> JS conversion will be added in https://github.com/w3c/web-animations/issues/104), which boils down to:

  * For each property name enumerated using [[Enumerate]] on the object:
      * If the property name matches one of the explicit dictionary members
        "offset", "easing" or "composite, then:
          * If "offset" and we have some flag set saying we don't accept
            offsets, then throw an exception.
          * Otherwise, do the usual IDL dictionary member [[Get]] and conversion.
      * If the property name matches the IDL name for a supported animatable CSS
        property, then:
          * Append the IDL name for the CSS property to a list.
  * Sort the list of accumulated IDL names for CSS properties encountered.
  * For each IDL name for a CSS property in the list:
      * Use [[Get]] to get its value, then convert it as if it were a dictionary
        member defined with type DOMString.  (Or with type
        (sequence<DOMString> or DOMString) in some cases.)

It's good that it gives a well defined order for enumerating/getting properties on the JS object, but I'm wondering if we should try to make it fit in better with the way normal dictionary types are converted to JS objects.

What about instead:

  * Do the usual dictionary JS -> IDL conversion, taking into account the
    explicit dictionary members.
  * Then enumerate the properties, recording the names of those that
    correspond to supported animatable CSS properties.
  * [[Get]] each of those and do the conversion of those JS values.

The only difference from the current spec is that (a) you'll [[Get]] the explicit dictionary members first, and in their order of declaration in the IDL, before the open ended values; and, (b) you'll do a little more work as you enumerate the properties for the explicit dictionary members after you've already previously done a [[Get]] for them.

I think this would make it more consistent with regular IDL dictionary conversion behaviour, and would make it easier to extend our bindings code to handle the open ended properties.  Does this sound reasonable, or are we stuck with the current algorithm?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
(In reply to Cameron McCormack (:heycam) from comment #1)
> It's good that it gives a well defined order for enumerating/getting
> properties on the JS object, but I'm wondering if we should try to make it
> fit in better with the way normal dictionary types are converted to JS
> objects.
> 
> What about instead:
> 
>   * Do the usual dictionary JS -> IDL conversion, taking into account the
>     explicit dictionary members.
>   * Then enumerate the properties, recording the names of those that
>     correspond to supported animatable CSS properties.
>   * [[Get]] each of those and do the conversion of those JS values.

What is the usual conversion? Is there some spec text we can link to for that?

I think I was under the impression that enumeration of properties wasn't consistent across browsers so if [[Get]] had side effects you would get different results in different implementations.

Maybe the steps above need a sort step where we sort the properties we're about to [[Get]]?

> The only difference from the current spec is that (a) you'll [[Get]] the
> explicit dictionary members first, and in their order of declaration in the
> IDL, before the open ended values; and, (b) you'll do a little more work as
> you enumerate the properties for the explicit dictionary members after
> you've already previously done a [[Get]] for them.

That sounds quite fine to me.

> I think this would make it more consistent with regular IDL dictionary
> conversion behaviour, and would make it easier to extend our bindings code
> to handle the open ended properties.  Does this sound reasonable, or are we
> stuck with the current algorithm?

I'm pretty sure we can change this. I'll run it by the Google guys once we've got the above details ironed out.
Flags: needinfo?(bbirtles)
(FTR the current algorithm is at http://w3c.github.io/web-animations/#processing-a-keyframe-object)

(In reply to Brian Birtles (:birtles) from comment #2)
> (In reply to Cameron McCormack (:heycam) from comment #1)
> > It's good that it gives a well defined order for enumerating/getting
> > properties on the JS object, but I'm wondering if we should try to make it
> > fit in better with the way normal dictionary types are converted to JS
> > objects.
> > 
> > What about instead:
> > 
> >   * Do the usual dictionary JS -> IDL conversion, taking into account the
> >     explicit dictionary members.
> >   * Then enumerate the properties, recording the names of those that
> >     correspond to supported animatable CSS properties.
> >   * [[Get]] each of those and do the conversion of those JS values.
> 
> What is the usual conversion? Is there some spec text we can link to for
> that?

The usual conversion is http://heycam.github.io/webidl/#dictionary-to-es which doesn't do any enumeration but does a [[Get]] for each dictionary member in order of declaration, with members on ancestors coming before those on descendants.

> I think I was under the impression that enumeration of properties wasn't
> consistent across browsers so if [[Get]] had side effects you would get
> different results in different implementations.
> 
> Maybe the steps above need a sort step where we sort the properties we're
> about to [[Get]]?

Yes, sorry, we should keep the sorting.

> I'm pretty sure we can change this. I'll run it by the Google guys once
> we've got the above details ironed out.

OK, great.

We should add some machinery to the IDL spec at some point to make this work without having to write big prose algorithms to interact with the JS object like this.

Boris, just want to check with you to see if there are any gotchas I've overlooked with the above plan?  Both from a spec and an implementation point of view, since I'll need to add bindings support for this.
Flags: needinfo?(bzbarsky)
Just to make sure we're on the same page... which plan?  I'm not sure I quite understand which proposal we're talking about at this point.  Are we doing the "big dictionary, then process it" thing or the "enumerate the object and then only do [[Get]] on the properties that are enumerated" thing?
Flags: needinfo?(bzbarsky)
Sorry, I'm asking whether you think the "process the dictionary as it appears in the IDL, then enumerate the JS object to gather the CSS property names, then [[Get]] each CSS property name" plan sounds reasonable.
Flags: needinfo?(bzbarsky)
Ah, so basically have a JS object, then init a dictionary from it to get some of the property values, then enumerate it to see which CSS property names it has on it and [[Get]] those?  So some of the properties you care about will be grabbed via unconditional [[Get]]s for the dictionary and some will come from the enumeration?

That's doable, but seems a bit odd in spec terms; why enumeration for some properties but not others?
Flags: needinfo?(bzbarsky)
Because (a) I think it's easier to extend the existing JS->IDL dictionary generated code, and (b) it would mean that the order that explicit dictionary members are got off the object doesn't change when you add this semi-open set of additional properties.

Whether that's a strong enough reason, I'm not sure.  Just going off the enumeration, including for the explicit dictionary members, is what the spec currently does.  I guess it doesn't matter too much either way but it'll be easier to implement enumerate-only-for-the-additional-properties.
(Ideally, since this is not a completely open ended set of additional dictionary members, we'd model Keyframe just like CSS2Properties, with a dictionary member for each CSS property, but calling [[Get]] for 400 properties does seem a bit wasteful when there are likely only going to be a few.)
> I think it's easier to extend the existing JS->IDL dictionary generated code, and

I'm not sure why we'd need any codegen changes here.  You'd declare your IDL as "object", then in the C++ init a dictionary form it and enumerate it.

> it would mean that the order that explicit dictionary members are got off the object
> doesn't change when you add this semi-open set of additional properties

Ah, that's a good reason.

> but calling [[Get]] for 400 properties does seem a bit wasteful 

Yes, that's what I was thinking a few comments back, which is why I didn't suggest we do that.
(In reply to Boris Zbarsky [:bz] from comment #9)
> I'm not sure why we'd need any codegen changes here.  You'd declare your IDL
> as "object", then in the C++ init a dictionary form it and enumerate it.

Oh, even better, thanks for that tip.
Depends on: 1206569
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8962653631f1

Tests still need to be written and the method needs to be made [ChromeOnly] for now.
Although it is not defined in the spec, I assume that the easing property on returned Keyframe objects should reflect the value of the animation-timing-function property (either on the element or an individual keyframe).  Currently on nsTimingFunction and ComputedTimingFunction we only distinguish between Function, StepStart and StepEnd timing functions.  So to be able to reflect the actual animation-timing-function value, we'll need to store the specific timing function type on nsTimingFunction and ComputedTimingFunction objects.

That is fine, but it leads me to ask whether ComputedTimingFunction::operator== should care about the difference between say:

  linear and cubic-bezier(0, 0, 1, 1)
  step-start and steps(1, start)
  steps(2, end) and steps(2)

(Note that the CSS Transitions/Animations specs define the computed value to be the same as the specified value for {animation,transition}-timing-function.)

We use the operator== method to determine whether to throw away a KeyframeEffectReadOnly object and create a new one, in response to changed style.  I cannot see where in http://w3c.github.io/web-animations/ that we define exactly how to produce KeyframeEffectReadOnly objects for CSS Animations and when to re-create these objects on style changes.  Is there somewhere else I should be looking?

That will determine whether operator== should return false for the above pairs of values, or if we need to have separate "logically equal" and "literally equal" methods.
Flags: needinfo?(bbirtles)
(In reply to Cameron McCormack (:heycam) from comment #12)
> Although it is not defined in the spec, I assume that the easing property on
> returned Keyframe objects should reflect the value of the
> animation-timing-function property (either on the element or an individual
> keyframe).

This will be defined in the CSS Animations 2 and CSS Transitions 2 specs that I'm working on.

> Currently on nsTimingFunction and ComputedTimingFunction we only
> distinguish between Function, StepStart and StepEnd timing functions.  So to
> be able to reflect the actual animation-timing-function value, we'll need to
> store the specific timing function type on nsTimingFunction and
> ComputedTimingFunction objects.
> 
> That is fine, but it leads me to ask whether
> ComputedTimingFunction::operator== should care about the difference between
> say:
> 
>   linear and cubic-bezier(0, 0, 1, 1)
>   step-start and steps(1, start)
>   steps(2, end) and steps(2)
> 
> (Note that the CSS Transitions/Animations specs define the computed value to
> be the same as the specified value for
> {animation,transition}-timing-function.)
> 
> We use the operator== method to determine whether to throw away a
> KeyframeEffectReadOnly object and create a new one, in response to changed
> style.  I cannot see where in http://w3c.github.io/web-animations/ that we
> define exactly how to produce KeyframeEffectReadOnly objects for CSS
> Animations and when to re-create these objects on style changes.  Is there
> somewhere else I should be looking?

Yes, this is the forthcoming CSS Animations 2 spec.

I think operator== should compare false for the above examples.

Are you referring to this code?

https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/layout/style/nsAnimationManager.cpp#505

I believe we should be keeping the same KeyframeEffectReadOnly object if the animation-timing-function changes. In BuildAnimations we should create a new object but then copy the properties and timing over to the old object. We use the operator== method to determine if we dispatch an animation changed mutation record. But perhaps you're referring to a different part of code?

In any case, if computed style or keyframe style changes from linear to cubic-bezier(0, 0, 1, 1) we should keep the original KeyframeEffectReadOnly object but update its properties/timing and dispatch an animationchanged mutation record.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #13)
> I think operator== should compare false for the above examples.

OK, that's good.

> Are you referring to this code?
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> abe43c30d78d7546ed7c622c5cf62d265709cdfd/layout/style/nsAnimationManager.
> cpp#505

Yes.

> I believe we should be keeping the same KeyframeEffectReadOnly object if the
> animation-timing-function changes. In BuildAnimations we should create a new
> object but then copy the properties and timing over to the old object. We
> use the operator== method to determine if we dispatch an animation changed
> mutation record. But perhaps you're referring to a different part of code?

I quickly scanned it and thought the comparison was relevant to replacing KeyframeEffectReadOnly objects but I see it's about the mutation observers (which I added, so I should remember that!).

> In any case, if computed style or keyframe style changes from linear to
> cubic-bezier(0, 0, 1, 1) we should keep the original KeyframeEffectReadOnly
> object but update its properties/timing and dispatch an animationchanged
> mutation record.

Great, that makes it clearer if I can keep operator== and operator< in sync.
Since Keyframe.easing should reflect the {transition,animation}-timing-
function value relevant to each keyframe, we'll need to store on
nsTimingFunction the specific timing function value that was used, and
copy it down into ComputedTimingFunction for
KeyframeEffectReadOnly.getFrames() to access.  This includes storing
whether the optional start/end keyword in a steps() function was
specified.
Attachment #8665352 - Flags: review?(bbirtles)
The CSS Transitions and Animations specs define the computed value of
a {transition,animation}-timing-function property as being the same as
the specified value, so we should expose the specific value we recorded
on nsTimingFunction in Part 1 through nsComputedDOMStyle.
Attachment #8665354 - Flags: review?(bbirtles)
When inspecting the specified value of {transition,animation}-timing-
function on a declaration, we should omit the start/end keyword in a
steps() value if it was omitted in the style sheet.
Attachment #8665355 - Flags: review?(bbirtles)
Since getFrames() must gather all properties set at a given keyframe
offset time for a given easing function, we need to provide a total
ordering for ComputedTimingFunction objects.  Until the spec defines how
to do this, we sort first by NS_STYLE_TRANSITION_TIMING_FUNCTION_*
value, then second by the four values in a cubic-bezier() function (in
order) or the integer and optional keyword in a steps() function.

Because we don't support automatic spacing of keyframes yet,
ComputedKeyFrame.computedOffset is always the same as Keyframe.offset.

Another assumption made is that the value of easing for a Keyframe
object at 100% should be the same as the value from the previous
Keyframe for the same property.  An alternative would be to leave off
easing from that Keyframe, which would need the default value for that
IDL dictionary member removed (otherwise it would always be set to
"linear").

r?bz for the dictionary/JS stuff in KeyframeEffectReadOnly::GetFrames and the IDL changes; r?birtles for the rest.
Attachment #8665357 - Flags: review?(bzbarsky)
Attachment #8665357 - Flags: review?(bbirtles)
Attached patch Part 7: Tests.Splinter Review
Attachment #8665358 - Flags: review?(bbirtles)
Comment on attachment 8665352 [details] [diff] [review]
Part 1: Store exact timing-function type on nsTimingFunction and ComputedTimingFunction.

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
>index 44e8713..08dd921 100644
>--- a/layout/style/nsStyleStruct.cpp
>+++ b/layout/style/nsStyleStruct.cpp
>@@ -2458,24 +2458,33 @@ nsStyleBackground::Layer::operator==(const Layer& aOther) const
> // --------------------
> // nsStyleDisplay
> //
> void nsTimingFunction::AssignFromKeyword(int32_t aTimingFunctionType)
> {
>   switch (aTimingFunctionType) {
>     case NS_STYLE_TRANSITION_TIMING_FUNCTION_STEP_START:
>       mType = StepStart;
>+      mStepSyntax = Keyword;
>       mSteps = 1;
>       return;
>+    default:
>+      MOZ_ASSERT_UNREACHABLE("aTimingFunctionType must be a keyword value");
>+      // fall through
>     case NS_STYLE_TRANSITION_TIMING_FUNCTION_STEP_END:
>       mType = StepEnd;
>+      mStepSyntax = Keyword;
>       mSteps = 1;
>       return;
>-    default:
>-      mType = Function;

What's the reasoning behind the fall-through here?

I can see we used to default to a function type but we're changing that to
step-end.

And I guess, even though we have a fatal assertion, we're playing it safe since
the parameter is an int32_t so we don't get the same degree of static checking
that we would if it were, say, an enum class parameter.

Is that right?


>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>index 29e4bb1..715cfdf 100644
>--- a/layout/style/nsStyleStruct.h
>+++ b/layout/style/nsStyleStruct.h
> struct nsTimingFunction {
>-  enum Type { Function, StepStart, StepEnd };
>+
>+  enum Type {
>+    Ease,         // ease
>+    Linear,       // linear
>+    EaseIn,       // ease-in
>+    EaseOut,      // ease-out
>+    EaseInOut,    // ease-in-out
>+    StepStart,    // step-start and steps(..., start)
>+    StepEnd,      // step-end, steps(..., end) and steps(...)
>+    CubicBezier,  // cubic-bezier()
>+  };

What do you think about making this an enum class? It would make bareword
references like 'CubicBezier' later on a little easier to parse I think.

>+  enum StepSyntax {
>+    Keyword,                     // step-start and step-end
>+    FunctionalWithoutKeyword,    // steps(...)
>+    FunctionalWithStartKeyword,  // steps(..., start)
>+    FunctionalWithEndKeyword,    // steps(..., end)
>+  };

Likewise here. Up to you though.

>-  nsTimingFunction(Type aType, uint32_t aSteps)
>+  nsTimingFunction(Type aType, uint32_t aSteps, bool aExplicitKeyword)
>     : mType(aType)
>   {

Could we do something other than a bool parameter? It makes call sites hard
to read.

If that's too much trouble, we could add a comment in nsRuleNode.cpp and
Layers.cpp saying what the parameter reflects.

>     MOZ_ASSERT(mType == StepStart || mType == StepEnd, "wrong type");

(Ideally, we could even remove the default ctor for this and add
 a StepTimingFunction subclass that takes a StepTimingFunctionType enum
 parameter so we can statically check this. And likewise
 a CubicBezierTimingFunction subclass. This is definitely fine for now, though.)



Otherwise, this is great. Thanks!

Could you file a follow-up to move ComputedTimingFunction to a separate file?

Also, as another follow-up, do you think it would make sense to rename
nsTimingFunction to something like TimingFunctionSpec/TimingFunctionParameters
and store that inside ComputedTimingFunction? It seems like that would remove
some redundancy like the operator== logic.
Attachment #8665352 - Flags: review?(bbirtles) → review+
Comment on attachment 8665353 [details] [diff] [review]
Part 2: Factor out computed nsTimingFunction serialization to public utility methods.

>diff --git a/layout/style/nsStyleUtil.h b/layout/style/nsStyleUtil.h
>index 3f32b43..98643ac 100644
>--- a/layout/style/nsStyleUtil.h
>+++ b/layout/style/nsStyleUtil.h
>@@ -67,16 +67,23 @@ public:
> 
>   static void AppendUnicodeRange(const nsCSSValue& aValue, nsAString& aResult);
> 
>   static void AppendCSSNumber(float aNumber, nsAString& aResult)
>   {
>     aResult.AppendFloat(aNumber);
>   }
> 
>+  static void AppendStepsTimingFunction(nsTimingFunction::Type aType,
>+                                       uint32_t aSteps,
>+                                       nsAString& aResult);

Nit: The hanging indent here seems to be off by one.
Attachment #8665353 - Flags: review?(bbirtles) → review+
Comment on attachment 8665354 [details] [diff] [review]
Part 3: Serialize computed {transition,animation}-timing-function using their specified values.

This is great. Thanks for fixing this. Hopefully we don't hit any compatibility
issues.

>diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
...
> /* static */ void
> nsStyleUtil::AppendStepsTimingFunction(nsTimingFunction::Type aType,
>                                        uint32_t aSteps,
>+                                       nsTimingFunction::StepSyntax aSyntax,
>                                        nsAString& aResult)
> {
...
>   aResult.AppendLiteral("steps(");
>   aResult.AppendInt(aSteps);
>-  if (aType == nsTimingFunction::StepStart) {
>-    aResult.AppendLiteral(", start)");
>-  } else {
>-    aResult.AppendLiteral(", end)");
>+  switch (aSyntax) {
>+    case nsTimingFunction::FunctionalWithStartKeyword:
>+      aResult.AppendLiteral(", start)");
>+      break;
>+    case nsTimingFunction::FunctionalWithEndKeyword:
>+      aResult.AppendLiteral(", end)");
>+      break;
>+    default:
>+      MOZ_ASSERT_UNREACHABLE("unexpected aSyntax value");
>+      // fall through

I wonder if this fall-through is necessary if aSyntax is an enum class
parameter. Shouldn't the compiler's static checks be enough? If we add to the
enum and don't update this we'll fail to compile on at least some platforms.

>@@ -589,16 +611,38 @@ nsStyleUtil::AppendCubicBezierTimingFunction(float aX1, float aY1,
>   aResult.AppendFloat(aY1);
>   aResult.AppendLiteral(", ");
>   aResult.AppendFloat(aX2);
>   aResult.AppendLiteral(", ");
>   aResult.AppendFloat(aY2);
>   aResult.Append(')');
> }
> 
>+/* static */ void
>+nsStyleUtil::AppendSplineKeywordTimingFunction(nsTimingFunction::Type aType,
>+                                               nsAString& aResult)
>+{

Any reason to call this AppendSpline...? It seems like AppendCubicBezier...
would be better?

It would be more consistent with AppendCubicBezierTimingFunction right?


Also, we should update nsStyleUtil::AppendCubicBezierTimingFunction, added in
part 2, to drop the comment:

   // (We could try to regenerate the keywords if we want.)


r=me with those comments addressed
Attachment #8665354 - Flags: review?(bbirtles) → review+
Attachment #8665355 - Flags: review?(bbirtles) → review+
Comment on attachment 8665356 [details] [diff] [review]
Part 5: Add method to serialize a ComputedTimingFunction.

Looks good.

>+void
>+ComputedTimingFunction::AppendToString(nsAString& aResult) const
>+{
>+  switch (mType) {
>+    case nsTimingFunction::CubicBezier:
>+      nsStyleUtil::AppendCubicBezierTimingFunction(mTimingFunction.X1(),
>+                                                   mTimingFunction.Y1(),
>+                                                   mTimingFunction.X2(),
>+                                                   mTimingFunction.Y2(),
>+                                                   aResult);
>+      break;
>+    case nsTimingFunction::StepStart:
>+    case nsTimingFunction::StepEnd:
>+      nsStyleUtil::AppendStepsTimingFunction(mType, mSteps, mStepSyntax,
>+                                             aResult);
>+      break;
>+    default:
>+      nsStyleUtil::AppendSplineKeywordTimingFunction(mType, aResult);
>+      break;
>+  }
>+}

Aside: As mentioned at the end of comment 23, it seems like if we store the
nsTimingFunction inside the ComputedTimingFunction we could share this code
with nsComputedDOMStyle::AppendTimingFunction.
Attachment #8665356 - Flags: review?(bbirtles) → review+
Comment on attachment 8665357 [details] [diff] [review]
Part 6: Implement KeyframeEffectReadOnly.getFrames().

So first, some meta-comments on the spec that should probably get turned into spec issues:

1)  The spec says "Use the internal [[Enumerate]] operation on keyframe input to iterate over its properties." but it needs to define what it actually means by that.  In particular, it needs to define whether one is supposed to perform all the substeps of step 7 there for each property _while_ stepping the iterator or whether they should be performed once the iterator has been snapshotted.  This matters in various situations involving non-idempotent getters on the three properties that call [[Get]] inside step 7.  Of course your code doesn't implement this algorithm anyway, so that might be worth a spec issue too.  ;)

2)  ES doesn't define the order in which the iterator returned from [[Enumerate]] enumerates properties, so this spec algorithm can produce different results in different browsers for the same object.  There's probably no good way around this, but worth explicitly calling out in the spec, probably.

3)  It's a bit weird to have dictionaries with null meant to indicate default values as opposed to just using missing values.  But OK, I haven't thought about this much, so maybe there's a good reason for it....

4)  The definition of getFrames is sorely lacking in terms of defining what ends up on the object and how (e.g. define vs set, enumerability, and so forth).

5)  Instead of a dictionary called Keyframe which isn't actually a dictionary, the spec needs to just use 'object' or perhaps 'object?' if it wants to allow null.

Now on to the patch:

>+      JSString* valueStr =
>+        JS_NewUCStringCopyN(aCx, entry.mValue.get(), entry.mValue.Length());
>+      JS::Rooted<JSString*> value(aCx, valueStr);

Please use ToJSValue on entry.mValue.  That will do various things like not copying the string if it can share it instead, etc.

Also, allocating a JSString can throw and return null.  Needs to be checked for (though if you just use ToJSValue you'd check that for false return value anyway).

The rest of the JS/IDL stuff looks good.
Attachment #8665357 - Flags: review?(bzbarsky) → review+
Comment on attachment 8665357 [details] [diff] [review]
Part 6: Implement KeyframeEffectReadOnly.getFrames().

>Bug 1198708 - Part 6: Implement KeyframeEffectReadOnly.getFrames(). r?birtles
...
>Another assumption made is that the value of easing for a Keyframe
>object at 100% should be the same as the value from the previous
>Keyframe for the same property.

Ideally we'd preserve the specified value but that's going to be really hard
in this case and especially when we do the more generalized keyframe handling
currently being discussed.[1]

I think this is fine, but I wonder if linear would make more sense?

[1] https://lists.w3.org/Archives/Public/www-style/2015Jul/0391.html

>diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp
>+int32_t
>+ComputedTimingFunction::Compare(const ComputedTimingFunction& aRhs) const
>+{
>+  if (mType != aRhs.mType) {
>+    return int32_t(mType) - int32_t(aRhs.mType);
>+  }
>+
>+  if (mType == nsTimingFunction::CubicBezier) {
>+    int32_t order = mTimingFunction.Compare(aRhs.mTimingFunction);
>+    if (order != 0) {
>+      return order;
>+    }

What is this test for? We will still end up returning 0 anyway right?

>+  } else if (mType == nsTimingFunction::StepStart ||
>+             mType == nsTimingFunction::StepEnd) {
>+    if (mSteps != aRhs.mSteps) {
>+      return int32_t(mSteps) - int32_t(aRhs.mSteps);
>+    }
>+    if (mStepSyntax != aRhs.mStepSyntax) {
>+      return int32_t(mStepSyntax) - int32_t(aRhs.mStepSyntax);
>+    }
>+  }
>+
>+  return 0;
>+}

>+void
>+KeyframeEffectReadOnly::GetFrames(JSContext*& aCx,
>+                                  nsTArray<JSObject*>& aResult,
>+                                  ErrorResult& aRv)
>+{
...
>+    for (size_t i = 0, n = property.mSegments.Length(); i < n; i++) {
>+      const AnimationPropertySegment& segment = property.mSegments[i];
>+      KeyframeValueEntry* entry = entries.AppendElement();
>+      entry->mOffset = segment.mFromKey;
>+      entry->mProperty = property.mProperty;
>+      entry->mTimingFunction = &segment.mTimingFunction;
>+      StyleAnimationValue::UncomputeValue(property.mProperty,
>+                                          segment.mFromValue,
>+                                          entry->mValue);
>+    }
>+    const AnimationPropertySegment& segment = property.mSegments.LastElement();
>+    KeyframeValueEntry* entry = entries.AppendElement();
>+    entry->mOffset = segment.mToKey;
>+    entry->mProperty = property.mProperty;
>+    entry->mTimingFunction = &segment.mTimingFunction;
>+    StyleAnimationValue::UncomputeValue(property.mProperty,
>+                                        segment.mToValue,
>+                                        entry->mValue);

I think this last part might deserve a comment mentioning what we do about
the timing function on the final frame.

>+  for (size_t i = 0, n = entries.Length(); i < n; ) {
>+    // Create a JS object with the explicit ComputedKeyframe dictionary members.
>+    ComputedKeyframe keyframeDict;
>+    keyframeDict.mOffset.SetValue(entries[i].mOffset);
>+    keyframeDict.mComputedOffset.Construct(entries[i].mOffset);
>+    keyframeDict.mEasing.Truncate();
>+    entries[i].mTimingFunction->AppendToString(keyframeDict.mEasing);
>+    keyframeDict.mComposite.SetValue(CompositeOperation::Replace);
>+
>+    JS::Rooted<JS::Value> keyframeValue(aCx);
>+    if (!ToJSValue(aCx, keyframeDict, &keyframeValue)) {
>+      aRv.Throw(NS_ERROR_FAILURE);
>+      return;
>+    }

Why would this fail? Should we assert if it does?

>+    JS::Rooted<JSObject*> keyframe(aCx, &keyframeValue.toObject());
>+
>+    // Set the property name/value pairs on the JS object.
>+    do {
>+      const KeyframeValueEntry& entry = entries[i];
>+      const char* name = nsCSSProps::PropertyIDLName(entry.mProperty);
>+      JSString* valueStr =
>+        JS_NewUCStringCopyN(aCx, entry.mValue.get(), entry.mValue.Length());
>+      JS::Rooted<JSString*> value(aCx, valueStr);
>+      if (!JS_DefineProperty(aCx, keyframe, name, value, JSPROP_ENUMERATE)) {
>+        aRv.Throw(NS_ERROR_FAILURE);
>+        return;
>+      }

Likewise here.

>diff --git a/dom/webidl/Keyframe.webidl b/dom/webidl/Keyframe.webidl
>new file mode 100644
>index 0000000..c822350
>--- /dev/null
>+++ b/dom/webidl/Keyframe.webidl
>@@ -0,0 +1,25 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * 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/.
>+ *
>+ * The origin of this IDL file is
>+ * http://w3c.github.io/web-animations/#the-compositeoperation-enumeration
>+ * http://w3c.github.io/web-animations/#the-keyframe-dictionary
>+ * http://w3c.github.io/web-animations/#the-computedkeyframe-dictionary

Nit: We should use the https version of these links

>diff --git a/dom/webidl/KeyframeEffect.webidl b/dom/webidl/KeyframeEffect.webidl
>index b3392b9..93dbfca 100644
>--- a/dom/webidl/KeyframeEffect.webidl
>+++ b/dom/webidl/KeyframeEffect.webidl
>@@ -14,10 +14,10 @@
>  Func="nsDocument::IsWebAnimationsEnabled"]
> interface KeyframeEffectReadOnly : AnimationEffectReadOnly {
>   readonly attribute Element?  target;
>   // Not yet implemented:
>   // readonly attribute IterationCompositeOperation iterationComposite;
>   // readonly attribute CompositeOperation          composite;
>   // readonly attribute DOMString                   spacing;
>   // KeyframeEffect             clone();
>-  // sequence<ComputedKeyframe> getFrames ();
>+  [Throws] sequence<object> getFrames();

We probably need a comment to say why we are using 'object' instead of
'ComputedKeyframe' here but perhaps its the spec that needs to be updated.

I'll follow-up Boris' comment shortly.
Attachment #8665357 - Flags: review?(bbirtles) → review+
> Why would this fail? 

The JS engine doesn't do infallible allocation.  Any JSAPI operation that might need to allocate can fail if it runs into the JS heap cap or any other sort of allocation failure.
Comment on attachment 8665358 [details] [diff] [review]
Part 7: Tests.

>diff --git a/dom/animation/test/css-animations/file_effect-getframes.html b/dom/animation/test/css-animations/file_effect-getframes.html

This should be file_keyframeeffect-getframes.html since getFrames is only
defined on KeyframeEffectReadOnly.

>+test(function(t) {
>+  var div = addDiv(t);
>+
>+  div.style.animation = 'anim-different-props-and-easing 100s';
>+  var frames = getFrames(div);
>+
>+  assert_equals(frames.length, 5, "number of frames");
>+
>+  var expected = [
>+    { offset: 0, computedOffset: 0, easing: "linear", composite: "replace",
>+      color: "rgb(0, 0, 0)", marginTop: "8px" },
>+    { offset: 0.25, computedOffset: 0.25, easing: "step-end", composite: "replace",
>+      color: "rgb(0, 0, 255)" },
>+    { offset: 0.75, computedOffset: 0.75, easing: "ease-in", composite: "replace",
>+      marginTop: "12px" },
>+    { offset: 1, computedOffset: 1, easing: "ease-in", composite: "replace",
>+      marginTop: "16px" },
>+    { offset: 1, computedOffset: 1, easing: "step-end", composite: "replace",
>+      color: "rgb(255, 255, 255)", },
>+  ];

Aside: These last two frames make we think we should use linear for the final
keyframe.
Attachment #8665358 - Flags: review?(bbirtles) → review+
(In reply to Boris Zbarsky [:bz] from comment #27)
> 1)  The spec says "Use the internal [[Enumerate]] operation on keyframe
> input to iterate over its properties." but it needs to define what it
> actually means by that.  In particular, it needs to define whether one is
> supposed to perform all the substeps of step 7 there for each property
> _while_ stepping the iterator or whether they should be performed once the
> iterator has been snapshotted.  This matters in various situations involving
> non-idempotent getters on the three properties that call [[Get]] inside step
> 7.  Of course your code doesn't implement this algorithm anyway, so that
> might be worth a spec issue too.  ;)

I think if we adopt the approach from comment 1, we'll be ok because we won't
be calling [[Get]] while enumerating.

I filed https://github.com/w3c/web-animations/issues/107 for this.

> 2)  ES doesn't define the order in which the iterator returned from
> [[Enumerate]] enumerates properties, so this spec algorithm can produce
> different results in different browsers for the same object.  There's
> probably no good way around this, but worth explicitly calling out in the
> spec, probably.

The intention is to enumerate and record just the property names, sort the
property names, then [[Get]] each property in that order. I think last time we
discussed this that was ok?

> 3)  It's a bit weird to have dictionaries with null meant to indicate
> default values as opposed to just using missing values.  But OK, I haven't
> thought about this much, so maybe there's a good reason for it....

I really can't remember why we did that and I haven't been able to turn up
anything in the change history. I think we just didn't understand WebIDL's
handling of absent members on dictionaries and just followed this pattern in
the spec:

  DOMString? strokePattern = null;

What's the most natural way of doing this? Just leave it nullable without
a default value?

> 4)  The definition of getFrames is sorely lacking in terms of defining what
> ends up on the object and how (e.g. define vs set, enumerability, and so
> forth).

This belongs in CSS Animations 2 and CSS Transitions 2 which I'm just starting
on. For the regular API, the result of getFrames is basically a sanitized
version of whatever gets passed to setFrames.

> 5)  Instead of a dictionary called Keyframe which isn't actually a
> dictionary, the spec needs to just use 'object' or perhaps 'object?' if it
> wants to allow null.

I was under that OpenEndedDictionary was coming and would cover this but
maybe that's not what it's for?
> we'll be ok because we won't be calling [[Get]] while enumerating

You'll still want to explicitly define exactly how the enumeration works.  There are several different ways of doing it, conceptually, which don't lead to identical results in some corner cases...  So you need to explicitly define what happens with the return value of the [[Enumerate]] call.

> I think last time we discussed this that was ok?

Ah, if you sort before doing the [[Get]]s then I think you're OK in terms of getting consistent behavior, at least in the simple evil cases I've thought about.

> What's the most natural way of doing this?

I think the other obvious option would be:

  DOMString strokePattern;

in which case the property not being present on the object would correspond to whatever null corresponds to right now in the spec.  Again, I haven't thought through the use cases to see which option makes more sense.

> For the regular API, the result of getFrames is basically a sanitized version of whatever
> gets passed to setFrames.

That doesn't address my concern.  The point is, you have to define exactly how properties end up on that object, because if it's done via [[Set]] that allows the page to run code as you set up the object, whereas if it's done via [[DefineOwnProperty]] it does not, and that difference is clearly observable by the web page.

> I was under that OpenEndedDictionary was coming

OpenEndedDictionary would kind of deal with this problem, yes, and would presumably define all the enumeration stuff for you to boot (albeit with different semantics from what you describe above).  But if that's what we want, why aren't we using our existing MozMap thing, which is basically what OpenEndedDictionary will look like, modulo the fact that MozMap is not order-preserving?
(In reply to Boris Zbarsky [:bz] from comment #32)
> > we'll be ok because we won't be calling [[Get]] while enumerating
> 
> You'll still want to explicitly define exactly how the enumeration works. 
> There are several different ways of doing it, conceptually, which don't lead
> to identical results in some corner cases...  So you need to explicitly
> define what happens with the return value of the [[Enumerate]] call.

Ok, that makes sense. I filed https://github.com/w3c/web-animations/issues/108 for that.

> > What's the most natural way of doing this?
> 
> I think the other obvious option would be:
> 
>   DOMString strokePattern;
> 
> in which case the property not being present on the object would correspond
> to whatever null corresponds to right now in the spec.  Again, I haven't
> thought through the use cases to see which option makes more sense.

I think I was concerned with authors having to deal with multiple representations
of the same thing. e.g. "member absent" or "member present but undefined" or
"member present but null" etc. So I think the intention was to make the
member always present but null in order to represent the default behavior (e.g.
auto-distribution of keyframes when the offset is not specified). I'm not really
up-to-date with how much WebIDL helps with normalizing this, however.

> > For the regular API, the result of getFrames is basically a sanitized version of whatever
> > gets passed to setFrames.
> 
> That doesn't address my concern.  The point is, you have to define exactly
> how properties end up on that object, because if it's done via [[Set]] that
> allows the page to run code as you set up the object, whereas if it's done
> via [[DefineOwnProperty]] it does not, and that difference is clearly
> observable by the web page.

That makes sense. Thanks!

I filed https://github.com/w3c/web-animations/issues/109 for this.

> > I was under that OpenEndedDictionary was coming
> 
> OpenEndedDictionary would kind of deal with this problem, yes, and would
> presumably define all the enumeration stuff for you to boot (albeit with
> different semantics from what you describe above).  But if that's what we
> want, why aren't we using our existing MozMap thing, which is basically what
> OpenEndedDictionary will look like, modulo the fact that MozMap is not
> order-preserving?

(Sorry, I messed up question there but I'm glad you made sense of it!)

I suppose Cameron can answer that. It seems like that's the better way forward.
> e.g. "member absent" or "member present but undefined" or "member present but null" etc.

Right.  "DOMString strokePattern;" would allow "member absent" or "member present, is a string", no other options.  "DOMString? strokePattern = null" would allow "member present, is null" and "member present, is a string", no other options.

Either one is supported fine by IDL; it's a question of which leads to a better API for web developers.  I'm not sure which is better, honestly.
I'm not really familiar with how MozMap works.

The main thing that makes me hesitant to use something like OpenEndedDictionary is that we have these other dictionary members on there, with types different from the open-ended ones.  Maybe in the future IDL can allow this kind of open-ended set of properties with a given type in addition to explicit dictionary members, e.g.:

  dictionary Keyframe {
    double?             offset = null;
    DOMString           easing = "linear";
    CompositeOperation? composite = null;

    other_enumerated_properties<DOMString>;
  };

or using whatever syntax we want.  I guess I just want to ensure that we maintain compatibility with existing dictionary member conversion order, and avoid having to move the explicit dictionary members into the open-ended bit, which is what it seems MozMap might require.
Another thing that comes to mind: since the set of properties we're interested in on a Keyframe is actually limited to the IDL names for the CSS properties we support animating, we can skip [[Get]]ing irrelevant properties.  If we had IDL syntax for overlaying an open-ended set of properties onto a dictionary, then that would probably want to call [[Get]] for all additional properties we enumerate.
Blocks: 1197100
Blocks: 1208940
(In reply to Brian Birtles (:birtles) from comment #23)
> Comment on attachment 8665352 [details] [diff] [review]
> Part 1: Store exact timing-function type on nsTimingFunction and
> ComputedTimingFunction.
...
> What's the reasoning behind the fall-through here?

I was following some other code that defaulted to step-end when encountering unexpected timing function types.

> I can see we used to default to a function type but we're changing that to
> step-end.
> 
> And I guess, even though we have a fatal assertion, we're playing it safe
> since
> the parameter is an int32_t so we don't get the same degree of static
> checking
> that we would if it were, say, an enum class parameter.
> 
> Is that right?

Pretty much.  It seemed safer to explicitly pick step-end as the fall back than to read outside the end of timingFunctionValues.

> >diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
> >index 29e4bb1..715cfdf 100644
> >--- a/layout/style/nsStyleStruct.h
> >+++ b/layout/style/nsStyleStruct.h
> > struct nsTimingFunction {
> >-  enum Type { Function, StepStart, StepEnd };
> >+
> >+  enum Type {
> >+    Ease,         // ease
> >+    Linear,       // linear
> >+    EaseIn,       // ease-in
> >+    EaseOut,      // ease-out
> >+    EaseInOut,    // ease-in-out
> >+    StepStart,    // step-start and steps(..., start)
> >+    StepEnd,      // step-end, steps(..., end) and steps(...)
> >+    CubicBezier,  // cubic-bezier()
> >+  };
> 
> What do you think about making this an enum class? It would make bareword
> references like 'CubicBezier' later on a little easier to parse I think.

I'll do that.

> >+  enum StepSyntax {
> >+    Keyword,                     // step-start and step-end
> >+    FunctionalWithoutKeyword,    // steps(...)
> >+    FunctionalWithStartKeyword,  // steps(..., start)
> >+    FunctionalWithEndKeyword,    // steps(..., end)
> >+  };
> 
> Likewise here. Up to you though.
> 
> >-  nsTimingFunction(Type aType, uint32_t aSteps)
> >+  nsTimingFunction(Type aType, uint32_t aSteps, bool aExplicitKeyword)
> >     : mType(aType)
> >   {
> 
> Could we do something other than a bool parameter? It makes call sites hard
> to read.

Sorry, I should know better.  I'll use an enum.

> Could you file a follow-up to move ComputedTimingFunction to a separate file?

Filed bug 1208940.

> Also, as another follow-up, do you think it would make sense to rename
> nsTimingFunction to something like
> TimingFunctionSpec/TimingFunctionParameters
> and store that inside ComputedTimingFunction? It seems like that would remove
> some redundancy like the operator== logic.

Yes, if you think it's fine to hold on to two copies of the spline coordinates.  Or would you want to split that out somehow?
(In reply to Brian Birtles (:birtles) from comment #25)
> Comment on attachment 8665354 [details] [diff] [review]
...
> I wonder if this fall-through is necessary if aSyntax is an enum class
> parameter. Shouldn't the compiler's static checks be enough? If we add to the
> enum and don't update this we'll fail to compile on at least some platforms.

Yeah, though I'll need to have a |case nsTimingFunction::StepSyntax::Keyword| that does nothing, since we handle that earlier in the function.

I feel like there's a tradeoff with switch statements on enums, between compilers catching cases that you miss, and having runtime assertions that let you know if an unexpected value has crept in to the function somehow.

...
> >+/* static */ void
> >+nsStyleUtil::AppendSplineKeywordTimingFunction(nsTimingFunction::Type aType,
> >+                                               nsAString& aResult)
> >+{
> 
> Any reason to call this AppendSpline...? It seems like AppendCubicBezier...
> would be better?
> 
> It would be more consistent with AppendCubicBezierTimingFunction right?

So call it AppendCubicBezierKeywordTimingFunction?  OK.  I just wanted a name that didn't look like it was going to append an actual cubic-bezier() value.
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #37)
> > Also, as another follow-up, do you think it would make sense to rename
> > nsTimingFunction to something like
> > TimingFunctionSpec/TimingFunctionParameters
> > and store that inside ComputedTimingFunction? It seems like that would remove
> > some redundancy like the operator== logic.
> 
> Yes, if you think it's fine to hold on to two copies of the spline
> coordinates.  Or would you want to split that out somehow?

Good point. I hadn't really though about that. That complicates it a bit.

I think the renaming is still worthwhile. Perhaps we could reuse nsTimingFunction::operator== by getting ComputedTimingFunction to generate temporary nsTimingFunction objects. I'm not sure.
(In reply to Brian Birtles (:birtles) from comment #28)
> Comment on attachment 8665357 [details] [diff] [review]
> Part 6: Implement KeyframeEffectReadOnly.getFrames().
...
> I think this is fine, but I wonder if linear would make more sense?

I'm in three minds about it.  linear makes sense if you think about it being unspecified, and so filled in by the dictionary, but I don't think it makes sense to the author looking at the object.  Taking the previous keyframe's value looked more "right" to me when I was playing around with it.  But if anything I think leaving the property off altogether makes more sense.  As I say that would require a spec change to make the dictionary member not optional.  If you decide to do that we can fix it up in a followup.

Given your later comment I'm going to change it to linear for now.

> [1] https://lists.w3.org/Archives/Public/www-style/2015Jul/0391.html
> 
> >diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp
> >+int32_t
> >+ComputedTimingFunction::Compare(const ComputedTimingFunction& aRhs) const
> >+{
> >+  if (mType != aRhs.mType) {
> >+    return int32_t(mType) - int32_t(aRhs.mType);
> >+  }
> >+
> >+  if (mType == nsTimingFunction::CubicBezier) {
> >+    int32_t order = mTimingFunction.Compare(aRhs.mTimingFunction);
> >+    if (order != 0) {
> >+      return order;
> >+    }
> 
> What is this test for? We will still end up returning 0 anyway right?

I'm following the pattern of checking for differences, returning if there is one, and continuing if there isn't.  It just makes the flow of the function clearer if we extend it later.
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #40)
> (In reply to Brian Birtles (:birtles) from comment #28)
> > Comment on attachment 8665357 [details] [diff] [review]
> > Part 6: Implement KeyframeEffectReadOnly.getFrames().
> ...
> > I think this is fine, but I wonder if linear would make more sense?
> 
> I'm in three minds about it.  linear makes sense if you think about it being
> unspecified, and so filled in by the dictionary, but I don't think it makes
> sense to the author looking at the object.  Taking the previous keyframe's
> value looked more "right" to me when I was playing around with it.  But if
> anything I think leaving the property off altogether makes more sense.  As I
> say that would require a spec change to make the dictionary member not
> optional.  If you decide to do that we can fix it up in a followup.
> 
> Given your later comment I'm going to change it to linear for now.

Feel free to leave it as is. I don't have a strong view about this. We'll have another chance to revisit this when we spec it and when we move this test into web-platform-tests and get feedback from other vendors.
(In reply to Brian Birtles (:birtles) from comment #41)
> > Given your later comment I'm going to change it to linear for now.
> 
> Feel free to leave it as is. I don't have a strong view about this. We'll
> have another chance to revisit this when we spec it and when we move this
> test into web-platform-tests and get feedback from other vendors.

OK.
Blocks: 998245
You need to log in before you can comment on or make changes to this bug.