Comment on attachment 8697367 [details] [diff] [review] Part 2: Tests for Animation Constructor v1 Review of attachment 8697367 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/web-animations/animation/constructor.html @@ +13,5 @@ > +<style> > +#target { > + border-style: solid; /* so border-*-width values don't compute to 0 */ > +} > +</style> This style element is unnecessary. @@ +17,5 @@ > +</style> > +<script> > +"use strict"; > + > +var target = document.getElementById("target"); I know this is just copying what keyframe-effect/constructor.html does, but I think we should probably name this "gTarget" since it's a global variable. (We should probably fix keyframe-effect/constructor.html too. I spoke to Cameron and he says there's no reason for calling it "target" other than making it match the ID.) @@ +19,5 @@ > +"use strict"; > + > +var target = document.getElementById("target"); > + > +var gOptionalArguments = [ This isn't necessary. In WebIDL, if you pass 'undefined' to a nullable argument, it will be converted to null so we only need to test null. @@ +49,5 @@ > + "Animation has no effect"); > + assert_equals(animation.timeline, null, > + "Animation has no timeline"); > + }, "Animation can be constructed without any options"); > +}); I think we should just write three tests: * null effect, non-null timeline * non-null effect, null timeline * both null @@ +55,5 @@ > +var gInvalidArguments = [ > + {}, > + [], > + 1.0 > +]; We don't need to test any of these. WebIDL does this for us (and I think idlharness.js does some tests that the implementation's interface matches the spec). @@ +77,5 @@ > + var effect = new KeyframeEffectReadOnly(target, { opacity: [0, 1] }); > + var animation = new Animation(effect, document.timeline); > + assert_equals(animation.timeline, document.timeline); > + assert_equals(animation.effect, effect); > +}, "Animation can be constructed"); This is good. We should also check that playState of the Animation is initially 'idle'.

This patch is not actually related to Animation Constructor but without this fix animation which has no timeline created by the constructor leads to crash when animation.play() is called.

Addressed all comments in comment#3. I think we need automation tests for all methods of Animation interface, play(), cancel(), etc., for the animations created by the constructor. Brian, Should I write these tests in this bug? Or in a followup bug?

Comment on attachment 8708175 [details] [diff] [review] Part 1: Implement Animation Constructor v2 Review of attachment 8708175 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/Animation.cpp @@ +96,5 @@ > + animation->SetEffect(aEffect); > + } > + if (aTimeline) { > + animation->SetTimeline(aTimeline); > + } The spec sets the timeline then the effect.[1] It shouldn't really matter, but unless there's a reason to do it this way, perhaps we should match the spec. [1] http://w3c.github.io/web-animations/#dom-animation-animation

Comment on attachment 8708175 [details] [diff] [review] Part 1: Implement Animation Constructor v2 Review of attachment 8708175 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/Animation.cpp @@ +94,5 @@ > + > + if (aEffect) { > + animation->SetEffect(aEffect); > + } > + if (aTimeline) { Actually, can throw if aTimeline is not null with a comment that we don't support null timelines yet (bug 1096776). We don't really support null effects either (we'll handle that in bug 1049975). For now we should probably throw if effect is null.

Comment on attachment 8708177 [details] [diff] [review] Part 3: Tests for Animation Constructor v2 Review of attachment 8708177 [details] [diff] [review]: ----------------------------------------------------------------- Once we add the exceptions in part 1, we will need to add test annotations for the failures here (like in attachment 8676625 [details] [diff] [review]).

https://hg.mozilla.org/integration/mozilla-inbound/rev/d058052ef8a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a192535066e https://hg.mozilla.org/integration/mozilla-inbound/rev/44befa1b31f8

https://hg.mozilla.org/mozilla-central/rev/d058052ef8a1 https://hg.mozilla.org/mozilla-central/rev/7a192535066e https://hg.mozilla.org/mozilla-central/rev/44befa1b31f8