Closed Bug 1096774 Opened 6 years ago Closed 5 years ago

Implement Animation constructor

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: birtles, Assigned: hiro)

References

()

Details

Attachments

(3 files, 4 obsolete files)

No description provided.
Depends on: 1096776
Component: DOM → DOM: Animation
Summary: Implement AnimationPlayer constructor → Implement Animation constructor
Depends on: 1151731
Depends on: 1198705
Depends on: 1179627
Assignee: nobody → hiikezoe
Attachment #8697367 - Flags: review?(bbirtles)
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'.
Attachment #8697367 - Flags: review?(bbirtles)
Attachment #8697365 - Attachment is obsolete: true
Attachment #8708175 - Flags: review?(bugs)
Attachment #8708175 - Flags: review?(bbirtles)
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.
Attachment #8708176 - Flags: review?(bbirtles)
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?
Attachment #8697367 - Attachment is obsolete: true
Attachment #8708177 - Flags: review?(bbirtles)
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
Attachment #8708175 - Flags: review?(bbirtles) → review+
Attachment #8708176 - Flags: review?(bbirtles) → review+
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]).
Attachment #8708177 - Flags: review?(bbirtles) → review+
Attachment #8708175 - Flags: review?(bugs) → review+
Attachment #8708175 - Attachment is obsolete: true
Attachment #8708747 - Flags: review+
Attachment #8708177 - Attachment is obsolete: true
Attachment #8708748 - Flags: review+
You need to log in before you can comment on or make changes to this bug.