Closed Bug 1244591 Opened 8 years ago Closed 8 years ago

implement KeyframeEffect.setFrames

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: motozawa, Assigned: r_kato)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files)

      No description provided.
Depends on: 1211783
Summary: implement setFrames → implement KeyframeEffect.setFrames
This should be really easy once bug 1245748 is done -- basically just involves writing tests.
Depends on: 1245748
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!
Flags: needinfo?(ryo_kato)
OK, I will try this! ;)
Flags: needinfo?(ryo_kato)
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
Assignee: nobody → ryo_kato
(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 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.
Attachment #8739646 - Flags: review?(hiikezoe)
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/
Attachment #8739646 - Attachment description: MozReview Request: Bug 1244591 - (WIP) Part 1: Implement KeyframeEffect.setFrames r?hiro → MozReview Request: Bug 1244591 - (WIP) Part 1: Implement KeyframeEffect.setFrames r?
Attachment #8739646 - Flags: review?(hiikezoe)
My previous comment was somewhat misleading.
I *think* setFrames should throw exceptions.  Is that correct? Brian?
Flags: needinfo?(bbirtles)
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/
Attachment #8739646 - Attachment description: MozReview Request: Bug 1244591 - (WIP) Part 1: Implement KeyframeEffect.setFrames r? → MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r?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?
Attachment #8739646 - Flags: review?(hiikezoe)
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 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/
Attachment #8739646 - Attachment description: MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r?hiro → MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r?birtles
Attachment #8739646 - Flags: review?(bbirtles)
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.
(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.
Flags: needinfo?(bbirtles)
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
Attachment #8739646 - Flags: review?(bbirtles)
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...?
(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.
(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
(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 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/
Attachment #8739646 - Flags: review?(bbirtles)
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.
Attachment #8739646 - Flags: review?(bbirtles) → review+
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/
Attachment #8739646 - Attachment description: MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r?birtles → MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r?smaug
Attachment #8739646 - Flags: review?(bugs)
smaug, I'd like you to review the change of KeyframeEffect.webidl (comment #22).
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?
Attachment #8739646 - Flags: review?(bugs) → review+
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/
Attachment #8739646 - Attachment description: MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r?smaug → MozReview Request: Bug 1244591 - Part 1: Implement KeyframeEffect.setFrames r=birtles r=smaug
Attachment #8742072 - Flags: review?(bbirtles)
Attachment #8742073 - Flags: review?(bbirtles)
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 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 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:cam@mcc.id.au">

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:cam@mcc.id.au">
>  <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
Attachment #8742072 - Flags: review?(bbirtles) → review+
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:ryo_kato@hashedhyphen.com">

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 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/
Attachment #8742072 - Attachment description: MozReview Request: Bug 1244591 - Part 2: Extract useful keyframes tests to a new file r?birtles → MozReview Request: Bug 1244591 - Part 2: Extract useful keyframes tests to a new file r=birtles
Attachment #8742073 - Flags: review?(bbirtles)
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/
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...?
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 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
Attachment #8742073 - Flags: review?(bbirtles) → review+
(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 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/
Attachment #8742073 - Attachment description: MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r?birtles → MozReview Request: Bug 1244591 - Part 3: Add web-platform tests for KeyframeEffect.setFrames r=birtles
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>();`
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?
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 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/
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...)
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
Kato-san, is this ready to land?
Flags: needinfo?(ryo_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?
Flags: needinfo?(ryo_kato)
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
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 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/
You need to log in before you can comment on or make changes to this bug.