1244591 - implement KeyframeEffect.setFrames

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Brian Birtles (:birtles)]

This should be really easy once bug 1245748 is done -- basically just involves writing tests.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

r_kato, could you please take this?  As Brian noted in comment#1 this bug needs bug 1245748 and its dependency,  but all of dependent bugs has patches respectively.  So you can work this locally.

The WebIDL file you need to modify:
https://dxr.mozilla.org/mozilla-central/rev/bccb11375f2af838cda714d42fd8cef78f5c7bf1/dom/webidl/KeyframeEffect.webidl#81

And dom/animation/KeyframeEffect.{cpp|h} are files you need to implement setFrames.

The tests will be 
testing/web-platform/tests/web-animations/keyframe-effect/setframes.html

dom/animation/test/css-animations/file_keyframeeffect-getframes.html would be a useful reference for tests.

Thanks!

Comment 3

[Ryo Kato [:r_kato]]

OK, I will try this! ;)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thanks!

I think what you should do here are in KeyframeEffect::SetFrames():

1) Call KeyframeUtils::GetKeyframesFromObject and call KeyframeUtils::ApplyDistributeSpacing and get nsStyleContext just like KeyframeEffectReadOnly::ConstructKeyframeEffec does.
https://dxr.mozilla.org/mozilla-central/rev/538d248fa252a4100082fd9bc3fdc08d322cda22/dom/animation/KeyframeEffect.cpp#710
2) Call KeyframeEffectReadonly::SetFrames which will be introduced in https://reviewboard.mozilla.org/r/43161/diff/1#index_header

The below test file has also other interesting test cases as useful references.

testing/web-platform/tests/web-animations/keyframe-effect/constructor.html

Comment 5

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro, offline; no animaions 3-7 April) from comment #4)
> The below test file has also other interesting test cases as useful
> references.
> 
> testing/web-platform/tests/web-animations/keyframe-effect/constructor.html

Yes, we should probably create a .js file somewhere which includes a common set of "good keyframe values" and the expected result from getFrames(), and "bad keyframe values". Then we can use the same set of inputs for testing:

* KeyframeEffect(ReadOnly) constructor
* Element.animate()
* setFrames

Comment 6

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

https://reviewboard.mozilla.org/r/45335/#review41857

This is great!  One thing I am concerned about the spec is that setFrames should throw exceptions.

::: dom/animation/KeyframeEffect.cpp:1474
(Diff revision 1)
>                         mAnimation->CascadeLevel());
>      }
>    }
>  }
>  
> +void KeyframeEffect::SetFrames(JSContext* aContext,

I'd prefer that variable names are the same in header file.

::: dom/animation/KeyframeEffect.cpp:1479
(Diff revision 1)
> +void KeyframeEffect::SetFrames(JSContext* aContext,
> +                               JS::Handle<JSObject*> aFrames)
> +{
> +  ErrorResult rv;
> +
> +  CSSPseudoElementType pseudoType = CSSPseudoElementType::NotPseudo;

KeyframeEffect has CSSPseudoElementType as a member named mPseudoType.  We can use it here.

::: dom/animation/KeyframeEffect.cpp:1489
(Diff revision 1)
> +  nsIPresShell* shell = doc->GetShell();
> +
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContextForElement(mTarget, pseudo, shell);

Both of nsIPresShell and mTarget could be nullptr here.  We should call GetStyleContextForElement if both of them are not nullptr.

Comment 8

[Ryo Kato [:r_kato]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45335/diff/1-2/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

My previous comment was somewhat misleading.
I *think* setFrames should throw exceptions.  Is that correct? Brian?

Comment 10

[Ryo Kato [:r_kato]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45335/diff/2-3/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

https://reviewboard.mozilla.org/r/45335/#review42521

::: dom/animation/KeyframeEffect.cpp:1409
(Diff revisions 2 - 3)
>    if (!mTarget) {
>      return;
>    }

I think we should call setFrames even if the KeyframeEffect has no target.

::: dom/animation/KeyframeEffect.cpp:1423
(Diff revisions 2 - 3)
>    nsIPresShell* shell = doc->GetShell();
>    if (!shell) {
>      return;
>    }
>  
>    RefPtr<nsStyleContext> styleContext =
>      nsComputedDOMStyle::GetStyleContextForElement(mTarget, pseudo, shell);
>  
>    nsTArray<Keyframe> keyframes =
> -    KeyframeUtils::GetKeyframesFromObject(aContext, aFrames, rv);
> -  if (rv.Failed()) {
> +    KeyframeUtils::GetKeyframesFromObject(aContext, aFrames, aRv);
> +  if (aRv.Failed()) {
>      return;
>    }
>  

As well as mTarget case, we should call setFrames even if we can't obtain a valid nsStyleContext.

Could you please change these process as well as what KeyframeEffectReadOnly::ConstructKeyframeEffect does? Like this:

1. call KeyframeUtils::GetKeyframesFromObject
2. get nsIPresShell
3. get nsStyleContext if both of nsIPresShell and mTarget are valid.
4. call SetFrames

Or can we factor out these in a utility function and reuse it in ConstructKeyframeEffect and SetFramesFromJS?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I think attachment 8739646 [details] is basically OK but I am not convinced that throwing exceptions is right thing to do here.  Please request Brian to review next round of reviewing.
Thanks.

Comment 13

[Ryo Kato [:r_kato]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45335/diff/3-4/

Comment 14

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/45335/#review42521

> As well as mTarget case, we should call setFrames even if we can't obtain a valid nsStyleContext.
> 
> Could you please change these process as well as what KeyframeEffectReadOnly::ConstructKeyframeEffect does? Like this:
> 
> 1. call KeyframeUtils::GetKeyframesFromObject
> 2. get nsIPresShell
> 3. get nsStyleContext if both of nsIPresShell and mTarget are valid.
> 4. call SetFrames
> 
> Or can we factor out these in a utility function and reuse it in ConstructKeyframeEffect and SetFramesFromJS?

Thank you for reviewing! I will make it pending whether or not to refactor that.

Comment 15

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> My previous comment was somewhat misleading.
> I *think* setFrames should throw exceptions.  Is that correct? Brian?

Yes, it should throw exceptions.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

https://reviewboard.mozilla.org/r/45335/#review42801

Looks good but I'd like to see the simplifications to ConstructKeyframeEffect and understand what the naming conflict is.

::: dom/animation/KeyframeEffect.h:440
(Diff revision 4)
>                const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
>                ErrorResult& aRv);
>  
>    void NotifySpecifiedTimingUpdated();
>  
> +  void SetFramesFromJS(JSContext* aContext,

What is the name conflict you were concerned about? I think this should be ok to name SetFrames since the type an number of arguments differs from the existing SetFrames, right? We only use *FromJS when we have to.

::: dom/animation/KeyframeEffect.cpp:1405
(Diff revision 4)
> +KeyframeEffect::SetFramesFromJS(JSContext* aContext,
> +                                JS::Handle<JSObject*> aFrames,
> +                                ErrorResult& aRv)

I think we could call this from KeyframeEffectReadOnly::ConstructKeyframeEffect and simplify that method.

::: dom/animation/KeyframeEffect.cpp:1415
(Diff revision 4)
> +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aContext);
> +  if (!doc) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return;
> +  }

I think we should move this to the start of the method. We don't expect this to fail, but if it does, we should fail right away rather than trying to parse the keyframes then failing.

::: dom/webidl/KeyframeEffect.webidl:80
(Diff revision 4)
>    // inherit attribute IterationCompositeOperation iterationComposite;
>    // Bug 1216844 - implement additive animation
>    // inherit attribute CompositeOperation          composite;
>    // Bug 1244590 - implement spacing modes
>    // inherit attribute DOMString                   spacing;
> -  // Bug 1244591 - implement setFrames
> +  [Throws, BinaryName="setFramesFromJS"]

No need for BinaryName annotation

Comment 17

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/45335/#review42801

> What is the name conflict you were concerned about? I think this should be ok to name SetFrames since the type an number of arguments differs from the existing SetFrames, right? We only use *FromJS when we have to.

Thank you for reviewing! I suspect that the name conflict occurs in `effect->SetFrames()`.
https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.cpp#787

Actually, compile error says:
`dom/animation/KeyframeEffect.cpp(787): error C2660: 'mozilla::dom::KeyframeEffect::SetFrames': function does not take 2 arguments`

What would be the best thing to do...?

Comment 18

[Brian Birtles (:birtles)]

(In reply to Ryo Kato [:r_kato] from comment #17)
> https://reviewboard.mozilla.org/r/45335/#review42801
> 
> > What is the name conflict you were concerned about? I think this should be ok to name SetFrames since the type an number of arguments differs from the existing SetFrames, right? We only use *FromJS when we have to.
> 
> Thank you for reviewing! I suspect that the name conflict occurs in
> `effect->SetFrames()`.
> https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.
> cpp#787
> 
> Actually, compile error says:
> `dom/animation/KeyframeEffect.cpp(787): error C2660:
> 'mozilla::dom::KeyframeEffect::SetFrames': function does not take 2
> arguments`
> 
> What would be the best thing to do...?

You'll need to put both versions of SetFrames on KeyframeEffectReadOnly since you want to call the API-facing version on a KeyframeEffectReadOnly when constructing one.

I don't understand. There should be two versions of SetFrames and C++'s overload resolution should pick the correct one based on the number of arguments.

Comment 19

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #18)
> You'll need to put both versions of SetFrames on KeyframeEffectReadOnly
> since you want to call the API-facing version on a KeyframeEffectReadOnly
> when constructing one.
> 
> I don't understand. There should be two versions of SetFrames and C++'s
> overload resolution should pick the correct one based on the number of
> arguments.

Oops, ignore the second sentence here. What I had in mind was something like this: https://pastebin.mozilla.org/8867657

Comment 20

[Ryo Kato [:r_kato]]

(In reply to Brian Birtles (:birtles) from comment #19)

I see. I tried taking advantage of such overload, then no error occurs now! I will soon push an updated patch.

Comment 21

[Ryo Kato [:r_kato]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45335/diff/4-5/

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

https://reviewboard.mozilla.org/r/45335/#review43211

Looks great! Thank you!

Please also request review from a DOM peer (e.g. smaug or bz) for the WebIDL changes.

Also, we shouldn't land this until we have tests for it.

Comment 23

[Ryo Kato [:r_kato]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45335/diff/5-6/

Comment 24

[Ryo Kato [:r_kato]]

smaug, I'd like you to review the change of KeyframeEffect.webidl (comment #22).

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

https://reviewboard.mozilla.org/r/45335/#review43337

Has any browser shipped setFrames yet? if not, might make sense to file a spec bug to investigate better handling for 'frames', hopefully something which can be expressed in webidl.
But anyhow, r+ for this, given that it follows the spec.

::: dom/webidl/KeyframeEffect.webidl:81
(Diff revision 6)
>    // Bug 1216844 - implement additive animation
>    // inherit attribute CompositeOperation          composite;
>    // Bug 1244590 - implement spacing modes
>    // inherit attribute DOMString                   spacing;
> -  // Bug 1244591 - implement setFrames
> -  // void setFrames (object? frames);
> +  [Throws]
> +  void setFrames (object? frames);

Huh, frames param processing looks horrible in the spec. Very unique in the platform.
Have the spec editors thought of less weird param handling?

Comment 26

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1244591 - Part 2: Extract useful keyframes tests to a new file r=birtles

Extract a lot of tests from keyframe-effect/constructor.html and add some
documentation so that we can reuse them in other tests. This helper will
be used when we want to test these API:

* KeyframeEffect(ReadOnly) Constructor
* KeyframeEffect.setFrames()
* SharedKeyframeList Constructor
* Animatable.animate()

and so on.

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

Comment 27

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

Some tests are expected to fail until Bug 1216843 and 1216844 are resolved, so that
we specify that on the meta file `setFrames.html.ini`.

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

Comment 28

[Ryo Kato [:r_kato]]

Comment on attachment 8739646 [details]
MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45335/diff/6-7/

Comment 29

[Brian Birtles (:birtles)]

Comment on attachment 8742072 [details]
MozReview Request: Bug 1244591 - Part 2: Extract useful keyframes tests to a new file r=birtles

https://reviewboard.mozilla.org/r/46915/#review43539

Thanks for this! I filed bug 1265274 for using this same data to test Animatable.animate() and the KeyframeEffect constructor.

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:4
(Diff revision 1)
>  <link rel="help" href="http://w3c.github.io/web-animations/#the-keyframeeffect-interfaces">
>  <link rel="author" title="Cameron McCormack" href="mailto:[e-mail]">

Nit: While we're touching this file, can you please:
* Make this a https URL
* Drop the "author" line (we decided to stop including these)

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:9
(Diff revision 1)
>  <link rel="help" href="http://w3c.github.io/web-animations/#the-keyframeeffect-interfaces">
>  <link rel="author" title="Cameron McCormack" href="mailto:[e-mail]">
>  <script src="/resources/testharness.js"></script>
>  <script src="/resources/testharnessreport.js"></script>
>  <script src="../testcommon.js"></script>
> +<script src="../resources/keyframe-helper.js"></script>

I think we could call this keyframe-utils.js ?

'helper' sounds like it's only functions, but it's also data.

::: testing/web-platform/tests/web-animations/resources/keyframe-helper.js:3
(Diff revision 1)
> +// This file provides helper functions for tests and common keyframes.
> +// Originally, this file was extracts from ../keyframe-effect/constructor.html
> +// intended to be shared for various tests.

I think we could just make this comment:

// Utility functions and common keyframe test data.

::: testing/web-platform/tests/web-animations/resources/keyframe-helper.js:13
(Diff revision 1)
> + * @param {Array.<ComputedKeyframe>} a - actual computed keyframes
> + * @param {Array.<ComputedKeyframe>} b - expected computed keyframes

I'm just curious, where does the {Array.<ComputedKeyframe>} syntax come from?

::: testing/web-platform/tests/web-animations/resources/keyframe-helper.js:23
(Diff revision 1)
> +  for (var i = 0; i < Math.min(a.length, b.length); i++) {
> +    assert_frames_equal(a[i], b[i], "ComputedKeyframe #" + i);
> +  }
> +}
> +
> +/** helper for assert_frame_lists_equal */

Nit: Helper

::: testing/web-platform/tests/web-animations/resources/keyframe-helper.js:34
(Diff revision 1)
> +    assert_equals(a[p], b[p], "value for '" + p + "' on " + name);
> +  }
> +}
> +
> +// ------------------------------
> +//  Easing

Nit: Easing values

::: testing/web-platform/tests/web-animations/resources/keyframe-helper.js:61
(Diff revision 1)
> +  { desc:   "a multi-value easing",
> +    input:  [{ easing: "ease-in-out, ease-out" }] }
> +];
> +
> +// ------------------------------
> +//  Composite

Nit: Composite values

Comment 30

[Brian Birtles (:birtles)]

Comment on attachment 8742073 [details]
MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

https://reviewboard.mozilla.org/r/46917/#review43543

This looks good but I'd like to see the test for exceptions too.

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:5
(Diff revision 1)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>KeyframeEffect setFrames() tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#dom-keyframeeffect-setframes">
> +<link rel="author" title="Ryo Kato" href="mailto:[e-mail]">

We decided not to add these author lines any more (and we will gradually remove them from the existing files.)

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:20
(Diff revision 1)
> +
> +var target = document.getElementById('target');
> +
> +gPropertyIndexedKeyframesTests.forEach(function(subtest) {
> +  test(function(t) {
> +    var effect = new KeyframeEffect(target, { opacity: [ 0, 1 ] });

Seeing as we're not testing for empty keyframes, I wonder if we need the "{ opacity: [ 0, 1 ] }" part. I suppose "{ }" is enough.

Likewise below.

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:23
(Diff revision 1)
> +gPropertyIndexedKeyframesTests.forEach(function(subtest) {
> +  test(function(t) {
> +    var effect = new KeyframeEffect(target, { opacity: [ 0, 1 ] });
> +    effect.setFrames(subtest.input);
> +    assert_frame_lists_equal(effect.getFrames(), subtest.output);
> +  }, 'replace keyframes with ' + subtest.desc);

Nit: 'Keyframes can be replaced with '

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:31
(Diff revision 1)
> +gKeyframeSequenceTests.forEach(function(subtest) {
> +  test(function(t) {
> +    var effect = new KeyframeEffect(target, { opacity: [ 0, 1 ] });
> +    effect.setFrames(subtest.input);
> +    assert_frame_lists_equal(effect.getFrames(), subtest.output);
> +  }, 'replace keyframes with ' + subtest.desc);

Nit: 'Keyframes can be replaced with '

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:33
(Diff revision 1)
> +    var effect = new KeyframeEffect(target, { opacity: [ 0, 1 ] });
> +    effect.setFrames(subtest.input);
> +    assert_frame_lists_equal(effect.getFrames(), subtest.output);
> +  }, 'replace keyframes with ' + subtest.desc);
> +});
> +

We should add tests for empty keyframe lists (using gEmptyKeyframeListTests from part 2).

Also, we need a test that this throws as expected. That will happen, for example if:

* the set of keyframes is not loosely sorted, or
* there are keyframes with offsets < 0 or > 1.

We should have tests for that somewhere but if not, we will need to add set to keyframe-utils.js (probably with the input keyframes and expected exception name).

Comment 31

[Ryo Kato [:r_kato]]

Comment on attachment 8742072 [details]
MozReview Request: Bug 1244591 - Part 2: Extract useful keyframes tests to a new file r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46915/diff/1-2/

Comment 32

[Ryo Kato [:r_kato]]

Comment on attachment 8742073 [details]
MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46917/diff/1-2/

Comment 33

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/46915/#review43539

> I'm just curious, where does the {Array.<ComputedKeyframe>} syntax come from?

Thank you for reviewing! I got this idea from from JSDoc. But, to be exact, `@typedef {Object} ComputedKeyframe>` needs specified. Is this documentation verbose...?

Comment 34

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/46917/#review43543

> We should add tests for empty keyframe lists (using gEmptyKeyframeListTests from part 2).
> 
> Also, we need a test that this throws as expected. That will happen, for example if:
> 
> * the set of keyframes is not loosely sorted, or
> * there are keyframes with offsets < 0 or > 1.
> 
> We should have tests for that somewhere but if not, we will need to add set to keyframe-utils.js (probably with the input keyframes and expected exception name).

I've written some tests for that case. However, there are no error thrown when I test invalid keyframes... Is there dependency bug related to offset, easing, and composite?

Comment 35

[Brian Birtles (:birtles)]

Comment on attachment 8742073 [details]
MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

https://reviewboard.mozilla.org/r/46917/#review44079

This is excellent work. Thank you!

r=me with comments addressed.

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:44
(Diff revision 2)
> +});
> +
> +gInvalidKeyframesTests.forEach(function(subtest) {
> +  test(function(t) {
> +    assert_throws(subtest.expected, function() {
> +      var effect = new KeyframeEffect(target, {});

Nit: Can we put the 'var effect = new KeyframeFfect(...)' part *outside* the assert_throws? That we we're testing precisely that it is *setFrames()* that throws, not the constructor.

::: testing/web-platform/tests/web-animations/keyframe-effect/setFrames.html:50
(Diff revision 2)
> +      effect.setFrames(subtest);
> +    });
> +  }, "Invalid KeyframeEffect option by " + subtest.desc);
> +});
> +
> +done();

Is this needed?

::: testing/web-platform/tests/web-animations/resources/keyframe-utils.js:407
(Diff revision 2)
> +var gInvalidKeyframesTests = [
> +  { desc:     "keyframes with an out-of-bounded positive offset",
> +    input:    [ { opacity: 0 },
> +                { opacity: 0.5, offset: 2 },
> +                { opacity: 1 } ],
> +    expected: { name: "TypeError" } },

Nit: I think assert_throws can take just a string[1] so I think just:

  expected: "TypeError"

would work here?

[1] https://github.com/w3c/testharness.js/blob/master/docs/api.md#assert_throwscode-func-description

Comment 36

[Brian Birtles (:birtles)]

(In reply to Ryo Kato [:r_kato] from comment #34)
> https://reviewboard.mozilla.org/r/46917/#review43543
> 
> > We should add tests for empty keyframe lists (using gEmptyKeyframeListTests from part 2).
> > 
> > Also, we need a test that this throws as expected. That will happen, for example if:
> > 
> > * the set of keyframes is not loosely sorted, or
> > * there are keyframes with offsets < 0 or > 1.
> > 
> > We should have tests for that somewhere but if not, we will need to add set to keyframe-utils.js (probably with the input keyframes and expected exception name).
> 
> I've written some tests for that case. However, there are no error thrown
> when I test invalid keyframes... Is there dependency bug related to offset,
> easing, and composite?

We should throw for the offset not being sorted, right? There is code do that part:

  https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/dom/animation/KeyframeUtils.cpp#587

I'm not aware of any bugs filed for that so please file them if we don't throw correctly.

Comment 37

[Ryo Kato [:r_kato]]

Comment on attachment 8742073 [details]
MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46917/diff/2-3/

Comment 38

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/45335/#review44093

::: dom/animation/KeyframeEffect.cpp:482
(Diff revision 7)
> +  }
> +
> +  nsTArray<Keyframe> keyframes =
> +    KeyframeUtils::GetKeyframesFromObject(aContext, aFrames, aRv);
> +  if (aRv.Failed()) {
> +    return;

(In reply to commment #36)

Perhaps, should we throw a TypeError here?
e.g.
`aRv.ThrowTypeError<dom::MSG_INVALID_KEYFRAME_OFFSETS>();`

Comment 39

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/45335/#review44093

> (In reply to commment #36)
> 
> Perhaps, should we throw a TypeError here?
> e.g.
> `aRv.ThrowTypeError<dom::MSG_INVALID_KEYFRAME_OFFSETS>();`

If aRv.Failed() is true, the the appropriate exception should have already been set and the bindings should convert that into an exception. Is aRv.Failed() true but we are not throwing an exception?

Comment 40

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/45335/#review44093

> If aRv.Failed() is true, the the appropriate exception should have already been set and the bindings should convert that into an exception. Is aRv.Failed() true but we are not throwing an exception?

I'm sorry. It is my test code that is invalid. The TypeError is thrown after I fixed typo...

Comment 41

[Ryo Kato [:r_kato]]

Comment on attachment 8742073 [details]
MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46917/diff/3-4/

Comment 42

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/46917/#review44079

> Nit: I think assert_throws can take just a string[1] so I think just:
> 
>   expected: "TypeError"
> 
> would work here?
> 
> [1] https://github.com/w3c/testharness.js/blob/master/docs/api.md#assert_throwscode-func-description

"TypeError" isn't included in DOMException currently.
https://heycam.github.io/webidl/#idl-DOMException-error-names

When we pass a string to 1st arg of assert_throws(), testharness takes the string as DOMException's name, therefore tests fail because of invalid string.
https://github.com/w3c/testharness.js/blob/master/testharness.js#L1164

That is why we should specify `expected: { name: "TypeError" }`.

(But I'm not sure why TypeError isn't DOMException...)

Comment 43

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/46917/#review43543

> I've written some tests for that case. However, there are no error thrown when I test invalid keyframes... Is there dependency bug related to offset, easing, and composite?

I fixed typo.
https://reviewboard.mozilla.org/r/46917/diff/3-4#0

Comment 44

[Brian Birtles (:birtles)]

Kato-san, is this ready to land?

Comment 45

[Ryo Kato [:r_kato]]

Yes, there are no issues as far as I can see ;) I haven't had the permission, so could you push these patch to try server?

Comment 46

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e8f85c2f77c

Comment 47

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/46917/#review44803

::: testing/web-platform/meta/web-animations/keyframe-effect/setFrames.html.ini:3
(Diff revision 4)
> +[setFrames.html]
> +  type: testharness
> +  [replace keyframes with a one property one value property-indexed keyframes specification]

s/replace keyframes with/Keyframes can be replaced with/g
?

See https://treeherder.mozilla.org/logviewer.html#?job_id=19783056&repo=try

Comment 48

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/46917/#review44803

> s/replace keyframes with/Keyframes can be replaced with/g
> ?
> 
> See https://treeherder.mozilla.org/logviewer.html#?job_id=19783056&repo=try

I'm sorry, I had forgotten to fix that...

Comment 49

[Ryo Kato [:r_kato]]

Comment on attachment 8742073 [details]
MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46917/diff/4-5/

Comment 50

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06bb127f8382

Comment 51

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e956fb34b9d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2cae288211
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dc79c8d77c

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e956fb34b9d8
https://hg.mozilla.org/mozilla-central/rev/4f2cae288211
https://hg.mozilla.org/mozilla-central/rev/d2dc79c8d77c