Closed Bug 1302038 Opened 5 years ago Closed 5 years ago

Update DocumentTimeline constructor to use DocumentTimelineOptions argument

Categories

(Core :: DOM: Animation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

The DocumentTimeline constructor now takes a DocumentTimelineOptions argument.[1]

We should update our implementation to match.

[1] https://w3c.github.io/web-animations/#dictdef-documenttimelineoptions

Spec changeset: https://github.com/w3c/web-animations/commit/61d65b7f77ad26536a7aa838ef2fc9a489446d9b
Comment on attachment 8794667 [details]
Bug 1302038 part 1 - Add DocumentTimeline constructor tests.

https://reviewboard.mozilla.org/r/80990/#review80180

r=me with the following changes

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:13
(Diff revision 1)
>  test(function(t) {
> -  var timeline = new DocumentTimeline(0);
> +  assert_throws({ name: 'TypeError'}, function() { new DocumentTimeline(0); });
> +}, 'We can\'t create the DocumentTimeline object with wrong type parameter.');

We don't need to test this since this is really just testing that the browser implements WebIDL correctly (and if we can't assume that, there are a *lot* of other things we need to test!)

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:21
(Diff revision 1)
>  
> +test(function(t) {
> +  var timeline = new DocumentTimeline();
> +
> +  assert_times_equal(timeline.currentTime, document.timeline.currentTime);
> +}, 'Use default origin time when it does not have the parameter.');

'An origin time of zero is used when none is supplied'

(Strictly speaking this is also testing the WebIDL bindings, but in this case it's probably ok.)

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:23
(Diff revision 1)
> +test(function(t) {
> +  var timeline = new DocumentTimeline({ time: 100 });
> +  assert_times_equal(timeline.currentTime, document.timeline.currentTime);
> +}, 'Use default origin time when it does not have correct dictionary member.');

Again, this is testing the WebIDL bindings so let's just drop this test.

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:31
(Diff revision 1)
> +}, 'Use default origin time when it does not have correct dictionary member.');
> +
> +test(function(t) {
> +  var timeline = new DocumentTimeline({ originTime: 0 });
>    assert_times_equal(timeline.currentTime, document.timeline.currentTime);
> -}, 'zero origin time');
> +}, 'It is same with default document timeline if origin time is zero.');

'A zero origin time produces a document timeline with a current time identical to the default document timeline'

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:38
(Diff revision 1)
>  test(function(t) {
> -  var timeline = new DocumentTimeline(10 * MS_PER_SEC);
> +  var timeline = new DocumentTimeline({ originTime: 10 * MS_PER_SEC });
>  
>    assert_times_equal(timeline.currentTime,
>                       (document.timeline.currentTime - 10 * MS_PER_SEC));
>  }, 'positive origin time');

While we're modifying this file, we could make this test description more helpful. e.g.

'A positive origin time makes the document timeline\'s current time lag behind the default document timeline'

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:45
(Diff revision 1)
>  test(function(t) {
> -  var timeline = new DocumentTimeline(-10 * MS_PER_SEC);
> +  var timeline = new DocumentTimeline({ originTime: -10 * MS_PER_SEC });
>  
>    assert_times_equal(timeline.currentTime,
>                  (document.timeline.currentTime + 10 * MS_PER_SEC));
>  }, 'negative origin time');

Likewise here, e.g.

'A negative origin time makes the document timeline\'s current time run ahead of the default document timeline'
Attachment #8794667 - Flags: review?(bbirtles) → review+
Comment on attachment 8794668 [details]
Bug 1302038 part 2 - Add DocumentTimelineOptions dictionary.

https://reviewboard.mozilla.org/r/80992/#review80186

r=me with the following change

::: dom/animation/DocumentTimeline.cpp:57
(Diff revision 1)
> +  TimeDuration originTime;
> +  if (aOptions.mOriginTime) {
> +    originTime = TimeDuration::FromMilliseconds(aOptions.mOriginTime);
> +  } else {
> +    originTime = TimeDuration(0);
> +  }

I don't think we need this branch right? Can't we just do:

  TimeDuration originTime = TimeDuration::FromMilliseconds(aOptions.mOriginTime);
Attachment #8794668 - Flags: review?(bbirtles) → review+
Attachment #8794668 - Flags: review?(bugs)
Thanks Brian,
I modified tests and parameter's check based on comment 6, comment 7.


Hi Olli,
I've changed the DocumentTimeline constructor's parameter based on Web Animations Spec[1].
[1] https://w3c.github.io/web-animations/#dictdef-documenttimelineoptions

Could you please review the change of this interface?
Comment on attachment 8794668 [details]
Bug 1302038 part 2 - Add DocumentTimelineOptions dictionary.

https://reviewboard.mozilla.org/r/80992/#review80250
Attachment #8794668 - Flags: review?(bugs) → review+
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d41cbb317f73
part 1 - Add DocumentTimeline constructor tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/eff98b9440e9
part 2 - Add DocumentTimelineOptions dictionary. r=birtles,smaug
https://hg.mozilla.org/mozilla-central/rev/d41cbb317f73
https://hg.mozilla.org/mozilla-central/rev/eff98b9440e9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.