Implement DocumentTimeline Constructor

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(URL)

Attachments

(8 attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review commit: https://reviewboard.mozilla.org/r/55730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55730/
Attachment #8757147 - Flags: review?(bbirtles)
Attachment #8757148 - Flags: review?(bbirtles)
Attachment #8757149 - Flags: review?(bbirtles)
Attachment #8757150 - Flags: review?(bbirtles)
Attachment #8757151 - Flags: review?(bbirtles)
(Assignee)

Comment 4

2 years ago
Created attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review commit: https://reviewboard.mozilla.org/r/55732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55732/
(Assignee)

Comment 5

2 years ago
Created attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review commit: https://reviewboard.mozilla.org/r/55734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55734/
(Assignee)

Comment 6

2 years ago
Created attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review commit: https://reviewboard.mozilla.org/r/55736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55736/
(Assignee)

Comment 7

2 years ago
Created attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review commit: https://reviewboard.mozilla.org/r/55738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55738/
(Assignee)

Updated

2 years ago
Attachment #8757147 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8757148 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8757149 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8757150 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8757151 - Flags: review?(bbirtles)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/1-2/
Attachment #8757150 - Attachment description: MozReview Request: Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline. r?birtles → MozReview Request: Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline. r?smaug
Attachment #8757147 - Flags: review?(bbirtles)
Attachment #8757148 - Flags: review?(bbirtles)
Attachment #8757149 - Flags: review?(bbirtles)
Attachment #8757150 - Flags: review?(bugs)
Attachment #8757151 - Flags: review?(bbirtles)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/1-2/
(Assignee)

Comment 10

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/1-2/
(Assignee)

Comment 11

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/1-2/
(Assignee)

Comment 12

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/1-2/
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

https://reviewboard.mozilla.org/r/55730/#review53004

This is good. r=me with the changes mentioned below AND can we also move AnimationTimeline/idlharness.html to DocumentTimeline/idlhaness.html.

And I guess we should move AnimationTimeline/document-timeline.html to Document/timeline.html. That might need to be a separate patch since document-timeline.html needs to be tidied up (specifically the first two tests are probably fine, but the last test in that file should probably be moved to DocumentTimeline/currentTime.html, I suspect).

::: testing/web-platform/meta/web-animations/interfaces/AnimationTimeline/idlharness.html.ini
(Diff revision 2)
>  [idlharness.html]
>    type: testharness
> -  prefs: [dom.animations-api.core.enabled:true]

Why are we removing the pref annotation?

::: testing/web-platform/meta/web-animations/interfaces/AnimationTimeline/idlharness.html.ini:3
(Diff revision 2)
>    [AnimationTimeline must be primary interface of document.timeline]
>      expected: FAIL

I'm curious about why this is failing. We're passing the document.timeline as 'DocumentTimeline' so why does it think the primary interface should be AnimationTimeline?

Maybe we need to pass the AnimationTimeline definition using add_untested_idls?[1]

I know this was an existing bug, but we should probably fix the test while we're doing this.

[1] https://github.com/w3c/testharness.js/blob/master/docs/idlharness.md#add_untested_idlsidl_string

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:3
(Diff revision 2)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Animation constructor tests</title>

DocumentTimeline constructor tests

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:13
(Diff revision 2)
> +test(function(t) {
> +  assert_throws({name: 'TypeError'},
> +                function() { var timeline = new DocumentTimeline('string'); });
> +}, 'Invalid paramter of constructor test');
> +

I think idlharness.js will test this for us?

Can you please double-check that idlharness.js tests this and then remove this test? Thanks.

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:22
(Diff revision 2)
> +
> +test(function(t) {
> +  var timeline = new DocumentTimeline(0);
> +
> +  assert_times_equal(timeline.currentTime, document.timeline.currentTime);
> +}, 'DocumentTimeline has zero origin time');

Perhaps just 'zero origin time' would be enough?

Likewise for the other tests.
Attachment #8757147 - Flags: review?(bbirtles) → review+
https://reviewboard.mozilla.org/r/55730/#review53014

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:27
(Diff revision 2)
> +  assert_times_equal(timeline.currentTime,
> +                     (document.timeline.currentTime + 10 * MS_PER_SEC));

Oh, this is back-to-front:

If the origin time is 10s, then the timeline time should be 10 *behind* the default document time since it started 10s later.

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/constructor.html:34
(Diff revision 2)
> +  assert_times_equal(timeline.currentTime,
> +                (document.timeline.currentTime - 10 * MS_PER_SEC));

Likewise here.
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

https://reviewboard.mozilla.org/r/55732/#review53006

r=me with the following changes.

Note that we can pass a zero TimeDuration using TimeDuration(0) to make it easier to understand.

::: dom/animation/DocumentTimeline.h:31
(Diff revision 2)
>  class DocumentTimeline final
>    : public AnimationTimeline
>    , public nsARefreshObserver
>  {
>  public:
> -  explicit DocumentTimeline(nsIDocument* aDocument)
> +  explicit DocumentTimeline(nsIDocument* aDocument, double aOriginTime)

If a constructor has two arguments, it doesn't need 'explicit'.

Also, since this is an internal API, I think it should take a const TimeDuration&. Generally we use TimeDuration parameters for internal APIs and convert to/from doubles at the Web-facing layer.

::: dom/animation/DocumentTimeline.cpp:91
(Diff revision 2)
> -  result.SetValue(aTimeStamp - timing->GetNavigationStartTimeStamp());
> +  result.SetValue(aTimeStamp
> +                  - timing->GetNavigationStartTimeStamp()
> +                  + mOriginTime);

This should be - mOriginTime

::: dom/animation/DocumentTimeline.cpp:195
(Diff revision 2)
>    RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming();
>    if (MOZ_UNLIKELY(!timing)) {
>      return result;
>    }
>  
> -  result = timing->GetNavigationStartTimeStamp() + aTimeDuration;
> +  result = timing->GetNavigationStartTimeStamp() + aTimeDuration - mOriginTime;

This should be + mOriginTime
Attachment #8757148 - Flags: review?(bbirtles) → review+
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

https://reviewboard.mozilla.org/r/55734/#review53018

Looks good but I want to check the forever handling.

::: dom/animation/DocumentTimeline.cpp:44
(Diff revision 2)
> +/* static */ already_AddRefed<DocumentTimeline>
> +DocumentTimeline::Constructor(const GlobalObject& aGlobal,
> +                              const DOMHighResTimeStamp& aOriginTime,
> +                              ErrorResult& aRv)
> +{
> +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());

We need to check if doc is nullptr and throw if it is.

There's another bug where we'll try to work out what to do in contexts where GetCurrentRealmDocument returns nullptr.

::: dom/animation/DocumentTimeline.cpp:45
(Diff revision 2)
> +DocumentTimeline::Constructor(const GlobalObject& aGlobal,
> +                              const DOMHighResTimeStamp& aOriginTime,
> +                              ErrorResult& aRv)
> +{
> +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> +  RefPtr<DocumentTimeline> timeline = new DocumentTimeline(doc, aOriginTime);

We'll also need to convert to a TimeDuration here.

I think we should also check that the result of FromMilliseconds is not Forever or -Forever and throw if that is the case. Obviously we'll also need to test this which might be difficult since the value we'd need to pass to trigger this case is platform-dependent (and Gecko-specific).

Furthermore, we should probably add an assertion to the DocumentTimeline constructor in part 2 that the passed-in TimeDuration is not Forever or -Forever.
Attachment #8757149 - Flags: review?(bbirtles)
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

https://reviewboard.mozilla.org/r/55738/#review53020

r=me but I hope we can delete the primary interface exception too (as mentioned in part 1)

(And technically, this should probably be folded into part 5 when we land so that we can land these patches 1-by-1 without any tests failing.)
Attachment #8757151 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 20

2 years ago
Created attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review commit: https://reviewboard.mozilla.org/r/56484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56484/
Attachment #8758133 - Flags: review?(bbirtles)
Attachment #8758134 - Flags: review?(bbirtles)
Attachment #8758135 - Flags: review?(bbirtles)
Attachment #8757149 - Flags: review?(bbirtles)
(Assignee)

Comment 21

2 years ago
Created attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review commit: https://reviewboard.mozilla.org/r/56486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56486/
(Assignee)

Comment 22

2 years ago
Created attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review commit: https://reviewboard.mozilla.org/r/56488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56488/
(Assignee)

Comment 23

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/2-3/
(Assignee)

Comment 24

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/2-3/
(Assignee)

Comment 25

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/2-3/
(Assignee)

Comment 26

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/2-3/
(Assignee)

Comment 27

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/2-3/
(Assignee)

Comment 28

2 years ago
https://reviewboard.mozilla.org/r/55730/#review53004

Thanks Brian,
I moved AnimationTimeline/idharness.html at part8 patch.

> I'm curious about why this is failing. We're passing the document.timeline as 'DocumentTimeline' so why does it think the primary interface should be AnimationTimeline?
> 
> Maybe we need to pass the AnimationTimeline definition using add_untested_idls?[1]
> 
> I know this was an existing bug, but we should probably fix the test while we're doing this.
> 
> [1] https://github.com/w3c/testharness.js/blob/master/docs/idlharness.md#add_untested_idlsidl_string

I added the AnimationTimeline as untested idls.
Attachment #8757149 - Flags: review?(bbirtles)
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

https://reviewboard.mozilla.org/r/55734/#review53062

Looks good but I probably should check the added error message.

::: dom/animation/DocumentTimeline.cpp:52
(Diff revision 3)
> +    return nullptr;
> +  }
> +
> +  TimeDuration originTime = TimeDuration::FromMilliseconds(aOriginTime);
> +  if (originTime == TimeDuration::Forever() ||
> +      originTime == TimeDuration::FromMilliseconds(INT64_MIN)) {

Can't we use -TimeDuration::Forever() here?

::: dom/animation/DocumentTimeline.cpp:53
(Diff revision 3)
> +  }
> +
> +  TimeDuration originTime = TimeDuration::FromMilliseconds(aOriginTime);
> +  if (originTime == TimeDuration::Forever() ||
> +      originTime == TimeDuration::FromMilliseconds(INT64_MIN)) {
> +    aRv.Throw(NS_ERROR_FAILURE);

We should probably throw a TypeError here with a suitable message?

Something like MSG_TIME_OUT_OF_RANGE, "Time value '{0}' is out of range for {1}" where {1} here is "origin time"?
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

https://reviewboard.mozilla.org/r/56484/#review53492

r=me with changes made

::: dom/animation/test/mochitest.ini:40
(Diff revision 1)
>    css-transitions/file_keyframeeffect-getkeyframes.html
>    css-transitions/file_pseudoElement-get-animations.html
>    document-timeline/file_document-timeline.html
>    mozilla/file_deferred_start.html
>    mozilla/file_disabled_properties.html
> +  mozilla/file_document-timeline.html

Can we call this something more descriptive like document-timeline-origin-time-range.html ?

::: dom/animation/test/mozilla/file_document-timeline.html:8
(Diff revision 1)
> +// DocumentTimeline constructor has a time stamp parameter.
> +// If calculated value grater than Number.MAX or less than -Number.MAX,
> +// the behavior is platform-dependent.
> +// Gecko will assert in this case.

This isn't right -- what happens is that if when we calculate the number of ticks from milliseconds (and the result of *that* will be platform dependent) and the value happens to be greater than INT64_MAX or less than INT64_MIN, then we will throw.

We could make the comment something like:

// If the originTime parameter passed to the DocumentTimeline exceeds the range of the internal storage type (a signed 64-bit integer number of ticks--a platform-dependent unit) then we should throw. Infinity isn't allowed as an origin time value and clamping to just inside the allowed range will just mean we overflow elsewhere.

::: dom/animation/test/mozilla/file_document-timeline.html:14
(Diff revision 1)
> +// If calculated value grater than Number.MAX or less than -Number.MAX,
> +// the behavior is platform-dependent.
> +// Gecko will assert in this case.
> +
> +test(function(t) {
> +  assert_throws({name: 'NS_ERROR_FAILURE'},

(This will obviously need to be updated based on the changes to part 3.)

::: dom/animation/test/mozilla/file_document-timeline.html:16
(Diff revision 1)
> +// Gecko will assert in this case.
> +
> +test(function(t) {
> +  assert_throws({name: 'NS_ERROR_FAILURE'},
> +    function() { new DocumentTimeline(Number.MAX_SAFE_INTEGER); });
> +}, 'Calculated current time is positiove infinity');

Nit: positive
Nit: Calculated

::: dom/animation/test/mozilla/file_document-timeline.html:21
(Diff revision 1)
> +}, 'Calculated current time is positiove infinity');
> +
> +test(function(t) {
> +  assert_throws({name: 'NS_ERROR_FAILURE'},
> +    function() { new DocumentTimeline(-1 * Number.MAX_SAFE_INTEGER); });
> +}, 'Calclulated current time is negative infinity');

Calculated
Attachment #8758133 - Flags: review?(bbirtles) → review+
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

https://reviewboard.mozilla.org/r/56486/#review53496
Attachment #8758134 - Flags: review?(bbirtles) → review+
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

https://reviewboard.mozilla.org/r/56488/#review53064

According to MozReview, this patch doesn't actually change the file contents, it just renames the files.

Is MozReview not showing the change? Or is the patch not up-to-date? Or is the patch header wrong?

::: testing/web-platform/meta/MANIFEST.json:36059
(Diff revision 1)
>            {
>              "path": "web-animations/timing-model/animations/set-the-timeline-of-an-animation.html",
>              "url": "/web-animations/timing-model/animations/set-the-timeline-of-an-animation.html"
>            }
> +        ],
> +        "web-animations/timing-model/timelinse/default-document-timeline.html": [

This should be timeline (not timelinse)

(The renaming later in the patch has the same bug)
Attachment #8758135 - Flags: review?(bbirtles)
(Assignee)

Comment 34

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/3-4/
Attachment #8757149 - Flags: review?(bbirtles)
Attachment #8758135 - Flags: review?(bbirtles)
(Assignee)

Comment 35

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/3-4/
(Assignee)

Comment 36

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/3-4/
(Assignee)

Comment 37

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/3-4/
(Assignee)

Comment 38

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/3-4/
(Assignee)

Comment 39

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/1-2/
(Assignee)

Comment 40

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/1-2/
(Assignee)

Comment 41

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/1-2/
(Assignee)

Comment 42

2 years ago
https://reviewboard.mozilla.org/r/55734/#review53062

Thanks Brian.

> Can't we use -TimeDuration::Forever() here?

I think that -TimeDuration::Forever() is different from TimeDuration::FromMilliseconds(INT64_MIN).
TimeDuration::Forever() use INT64_MAX.

The difference of INT64_MIN and INT64_MAX is as follow.
http://melpon.org/wandbox/permlink/mrwa5SfICxnPhPro

If we use -TimeDuration::Forever(), we don't match this condition.

> We should probably throw a TypeError here with a suitable message?
> 
> Something like MSG_TIME_OUT_OF_RANGE, "Time value '{0}' is out of range for {1}" where {1} here is "origin time"?

I fixed as general OUT_OF_RANGE.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #42)
> https://reviewboard.mozilla.org/r/55734/#review53062
> 
> Thanks Brian.
> 
> > Can't we use -TimeDuration::Forever() here?
> 
> I think that -TimeDuration::Forever() is different from
> TimeDuration::FromMilliseconds(INT64_MIN).
> TimeDuration::Forever() use INT64_MAX.
> 
> The difference of INT64_MIN and INT64_MAX is as follow.
> http://melpon.org/wandbox/permlink/mrwa5SfICxnPhPro
> 
> If we use -TimeDuration::Forever(), we don't match this condition.

If we need these checks,  I'd prefer to add TimeDuration::IsFinite() or something like that.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #42)
> https://reviewboard.mozilla.org/r/55734/#review53062
> 
> Thanks Brian.
> 
> > Can't we use -TimeDuration::Forever() here?
> 
> I think that -TimeDuration::Forever() is different from
> TimeDuration::FromMilliseconds(INT64_MIN).
> TimeDuration::Forever() use INT64_MAX.

-TimeDuration::Forever() should have ticks == INT64_MIN. We have an unary operator- especially for that.[1] Is that not happening?

[1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/mozglue/misc/TimeStamp.h#162
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

https://reviewboard.mozilla.org/r/55734/#review53852

r=me with the change to use -TimeDuration::Forever()

::: dom/animation/DocumentTimeline.cpp:52
(Diff revision 4)
> +    return nullptr;
> +  }
> +
> +  TimeDuration originTime = TimeDuration::FromMilliseconds(aOriginTime);
> +  if (originTime == TimeDuration::Forever() ||
> +      originTime == TimeDuration::FromMilliseconds(INT64_MIN)) {

We should use -TimeDuration::Forever()

FromMilliseconds(INT64_MIN) is not right since the value we care about is -Infinity which is represented by a INT64_MIN number of *ticks* not milliseconds.

::: dom/animation/DocumentTimeline.cpp:53
(Diff revision 4)
> +    aRv.ThrowTypeError<dom::MSG_ENFORCE_RANGE_OUT_OF_RANGE>(
> +      NS_LITERAL_STRING("origin time"));

I wanted to avoid using ENFORCE_RANGE here so we can distinguish between errors that are due to failing to provide a valid value according to the WebIDL spec, versus errors that are implementation specific. However, I guess it doesn't really matter.
Attachment #8757149 - Flags: review?(bbirtles) → review+
https://reviewboard.mozilla.org/r/55732/#review53854

::: dom/animation/DocumentTimeline.h:31
(Diff revision 4)
> -  explicit DocumentTimeline(nsIDocument* aDocument)
> +  DocumentTimeline(nsIDocument* aDocument,
> +                            const TimeDuration& aOriginTime)

The indentation here is wrong.
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

https://reviewboard.mozilla.org/r/56488/#review53856

::: testing/web-platform/meta/MANIFEST.json:36061
(Diff revision 2)
> +            "path": "web-animations/timing-model/timelinse/default-document-timeline.html",
> +            "url": "/web-animations/timing-model/timelinse/default-document-timeline.html"

This is still wrong and the question from comment 33 is unanswered.
Attachment #8758135 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #45)
> ::: dom/animation/DocumentTimeline.cpp:53
> (Diff revision 4)
> > +    aRv.ThrowTypeError<dom::MSG_ENFORCE_RANGE_OUT_OF_RANGE>(
> > +      NS_LITERAL_STRING("origin time"));
> 
> I wanted to avoid using ENFORCE_RANGE here so we can distinguish between
> errors that are due to failing to provide a valid value according to the
> WebIDL spec, versus errors that are implementation specific. However, I
> guess it doesn't really matter.

We should define our implementation specific errors for this kind of restrictions.
Currently web developers get the following error message:

 "Value is out of range for origin time"

But the spec says the first argument is DOMHighResTimeStamp (== double),  if the developer uses a value which is greater than INT64_MAX / 1000, of course it's in the range of double,  the developers will be confused why the value is our of range.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> (In reply to Brian Birtles (:birtles) from comment #45)
> > ::: dom/animation/DocumentTimeline.cpp:53
> > (Diff revision 4)
> > > +    aRv.ThrowTypeError<dom::MSG_ENFORCE_RANGE_OUT_OF_RANGE>(
> > > +      NS_LITERAL_STRING("origin time"));
> > 
> > I wanted to avoid using ENFORCE_RANGE here so we can distinguish between
> > errors that are due to failing to provide a valid value according to the
> > WebIDL spec, versus errors that are implementation specific. However, I
> > guess it doesn't really matter.
> 
> We should define our implementation specific errors for this kind of
> restrictions.
> Currently web developers get the following error message:
> 
>  "Value is out of range for origin time"
> 
> But the spec says the first argument is DOMHighResTimeStamp (== double),  if
> the developer uses a value which is greater than INT64_MAX / 1000, of course

OOps, this is not 1000,  it depends on platform, it's 1000000.0 on Linux.
(Assignee)

Comment 50

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/4-5/
Attachment #8758135 - Attachment description: MozReview Request: Bug 1267510 part 8 - Separate document timeline test. r?birtles → MozReview Request: Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories. r?birtles
Attachment #8758135 - Flags: review?(bbirtles)
(Assignee)

Comment 51

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/4-5/
(Assignee)

Comment 52

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/4-5/
(Assignee)

Comment 53

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/4-5/
(Assignee)

Comment 54

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/2-3/
(Assignee)

Comment 55

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/2-3/
(Assignee)

Comment 56

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/2-3/
(Assignee)

Comment 57

2 years ago
https://reviewboard.mozilla.org/r/56488/#review53856

Thanks Brian,

> This is still wrong and the question from comment 33 is unanswered.

This patch is moving code only. So I modified the commit message.
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

https://reviewboard.mozilla.org/r/56488/#review53870

r=me with changes made.

::: testing/web-platform/meta/MANIFEST.json:36061
(Diff revision 3)
> +            "path": "web-animations/timing-model/timeline/default-document-timeline.html",
> +            "url": "/web-animations/timing-model/timeline/default-document-timeline.html"

This should be timelines (with an 's') to match the spec.
Attachment #8758135 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 59

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/4-5/
(Assignee)

Comment 60

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/5-6/
(Assignee)

Comment 61

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/5-6/
(Assignee)

Comment 62

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/5-6/
(Assignee)

Comment 63

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/5-6/
(Assignee)

Comment 64

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/3-4/
(Assignee)

Comment 65

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/3-4/
(Assignee)

Comment 66

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/3-4/
(Assignee)

Comment 67

2 years ago
Thanks hiro.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> But the spec says the first argument is DOMHighResTimeStamp (== double),  if
> the developer uses a value which is greater than INT64_MAX / 1000, of course
> it's in the range of double,  the developers will be confused why the value
> is our of range.
I've changed the error message.
I was thinking that we should display the range value. But range will change each platform, I think that developer will more confuse.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #67)
> Thanks hiro.
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> > But the spec says the first argument is DOMHighResTimeStamp (== double),  if
> > the developer uses a value which is greater than INT64_MAX / 1000, of course
> > it's in the range of double,  the developers will be confused why the value
> > is our of range.
> I've changed the error message.
> I was thinking that we should display the range value. But range will change
> each platform, I think that developer will more confuse.

Yes, I know, but I don't think proving no hint is a good idea.  If I am the web developer, I will try various numbers to avoid this error, then I will drop firefox with anger.  Can we define a safe limit value on all platforms?
(Assignee)

Comment 69

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #68)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #67)
> > Thanks hiro.
> > 
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> > > But the spec says the first argument is DOMHighResTimeStamp (== double),  if
> > > the developer uses a value which is greater than INT64_MAX / 1000, of course
> > > it's in the range of double,  the developers will be confused why the value
> > > is our of range.
> > I've changed the error message.
> > I was thinking that we should display the range value. But range will change
> > each platform, I think that developer will more confuse.
> 
> Yes, I know, but I don't think proving no hint is a good idea.  If I am the
> web developer, I will try various numbers to avoid this error, then I will
> drop firefox with anger.  Can we define a safe limit value on all platforms?
Umm, I understand your opinion. It is important to suggest message to fix web page's problem.

I think this range is depend on platform and user agent. If developer build the web app according to gecko's error message, it might not work on other user agent. 
And I think that this case is not occur frequently, because specifying out of range is same to setting many days to origin time. 
So I thought that it is enough as error message.

How do you feel about this reason?
Flags: needinfo?(hiikezoe)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #69)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #68)
> > (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #67)
> > > Thanks hiro.
> > > 
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> > > > But the spec says the first argument is DOMHighResTimeStamp (== double),  if
> > > > the developer uses a value which is greater than INT64_MAX / 1000, of course
> > > > it's in the range of double,  the developers will be confused why the value
> > > > is our of range.
> > > I've changed the error message.
> > > I was thinking that we should display the range value. But range will change
> > > each platform, I think that developer will more confuse.
> > 
> > Yes, I know, but I don't think proving no hint is a good idea.  If I am the
> > web developer, I will try various numbers to avoid this error, then I will
> > drop firefox with anger.  Can we define a safe limit value on all platforms?
> Umm, I understand your opinion. It is important to suggest message to fix
> web page's problem.
> 
> I think this range is depend on platform and user agent. If developer build
> the web app according to gecko's error message, it might not work on other
> user agent. 

I wonder why you chose it without any hint to web developers.  To be honest, it will be a some sort of web compact issue.

> And I think that this case is not occur frequently, because specifying out
> of range is same to setting many days to origin time. 
> So I thought that it is enough as error message.

I think a problem which happens infrequently but surely happens with specific values will hard to solve without any clues.  Also, I think this kind of values are usually set by calculated values in programs.  People often make mistakes in programs.
Flags: needinfo?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #70)
> I wonder why you chose it without any hint to web developers.  To be honest,
> it will be a some sort of web compact issue.

That was my suggestion. I think providing numbers here is not helpful. Suppose an author actually does hit this case (not likely for regular content), and we produce an error saying your value is outside the range -X to X. Do we really expect the author to make use of those values? Suppose she adjusts her content to use a value of X-1. The content may work correctly (or, at least, not throw) on her Mac, but then it fails on someone else's Windows machine because it uses a different tick frequency. That's not how we want people to author content.

> I think a problem which happens infrequently but surely happens with
> specific values will hard to solve without any clues.  Also, I think this
> kind of values are usually set by calculated values in programs.  People
> often make mistakes in programs.

In that case, simply saying value Y is out of range seems preferable. e.g. "Origin time 18446744080000 is outside the supported range for time values" should be enough to give the developer a hint that their calculation is wrong.

If I understand it, we're talking about a signed 64-bit integer, which has a range in the positive direction of 9,223,372,036,854,775,807.

On Linux we multiple milliseconds by 1000000.0 giving us a range of 9223372036854.775807ms = 292.26 years. So I don't think we're likely to hit this for regular content, but more likely due to a calculation bug, like you say. In that case I think returning the supplied value should be enough to alert the developer to their mistake?
(In reply to Brian Birtles (:birtles) from comment #71)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #70)
> > I wonder why you chose it without any hint to web developers.  To be honest,
> > it will be a some sort of web compact issue.
> 
> That was my suggestion. I think providing numbers here is not helpful.
> Suppose an author actually does hit this case (not likely for regular
> content), and we produce an error saying your value is outside the range -X
> to X. Do we really expect the author to make use of those values? Suppose
> she adjusts her content to use a value of X-1. The content may work
> correctly (or, at least, not throw) on her Mac, but then it fails on someone
> else's Windows machine because it uses a different tick frequency. That's
> not how we want people to author content.

That's why I suggest using a safe limit on all platforms.  She will be hard to notice the error if she has only a Mac.  She will notice the value is out of range eventually but it's wasting her time.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #72)
> That's why I suggest using a safe limit on all platforms.  She will be hard
> to notice the error if she has only a Mac.  She will notice the value is out
> of range eventually but it's wasting her time.

What would be the limit? I'm not even sure how we calculate it for Windows since we have two clock sources but suppose we decide it is +/-100 years, do we really want someone authoring content using a value of 200 years to adjust it to 90 years due to an error message? Is there a use case where that might be valid? Perhaps they just want to ensure that the animation never runs and in that case there are plenty of more suitable mechanisms (like pausing the animation, using a playback rate of 0, etc.).
(In reply to Brian Birtles (:birtles) from comment #73)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #72)
> > That's why I suggest using a safe limit on all platforms.  She will be hard
> > to notice the error if she has only a Mac.  She will notice the value is out
> > of range eventually but it's wasting her time.
> 
> What would be the limit? I'm not even sure how we calculate it for Windows
> since we have two clock sources but suppose we decide it is +/-100 years, do
> we really want someone authoring content using a value of 200 years to
> adjust it to 90 years due to an error message? Is there a use case where
> that might be valid? Perhaps they just want to ensure that the animation
> never runs and in that case there are plenty of more suitable mechanisms
> (like pausing the animation, using a playback rate of 0, etc.).

Yes, but you know, web developers sometimes try to do something in various unusual ways.  I am not convinced there is no such people in real world.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> Yes, but you know, web developers sometimes try to do something in various
> unusual ways.  I am not convinced there is no such people in real world.

Yes, I agree. However, I think giving them an actual number encourages them to do implementation-dependent things and is something we normally try to avoid. Suppose they see the error message and adjust their content to use 90 years instead, and then Edge brings out an implementation which only has a range of 70 years, their content will break.

Perhaps if we see people actually doing crazy things like this we can add guidance to the spec but at this stage I'm not sure how I would phrase it.
(In reply to Brian Birtles (:birtles) from comment #75)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> > Yes, but you know, web developers sometimes try to do something in various
> > unusual ways.  I am not convinced there is no such people in real world.
> 
> Yes, I agree. However, I think giving them an actual number encourages them
> to do implementation-dependent things and is something we normally try to
> avoid. Suppose they see the error message and adjust their content to use 90
> years instead, and then Edge brings out an implementation which only has a
> range of 70 years, their content will break.

Yes, this is exactly what I am worrying.  To be exact, in case of animation's delay, I am suspecting those people try to set Number.MAX_VALUE to stop starting animation *temporarily*. Yes, it works well on Chrome, but will be failed on Firefox.  I think it's a pity.
(Assignee)

Comment 77

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/5-6/
Attachment #8757147 - Attachment description: MozReview Request: Bug 1267510 part 1 - Add DocumentTimeline constructor tests. r?birtles → Bug 1267510 part 1 - Add DocumentTimeline constructor tests.
Attachment #8757148 - Attachment description: MozReview Request: Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline. r?birtles → Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.
Attachment #8757149 - Attachment description: MozReview Request: Bug 1267510 part 3 - Add DocumentTimeline constructor. r?birtles → Bug 1267510 part 3 - Add DocumentTimeline constructor.
Attachment #8757150 - Attachment description: MozReview Request: Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline. r?smaug → Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.
Attachment #8757151 - Attachment description: MozReview Request: Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline. r?birtles → Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.
Attachment #8758133 - Attachment description: MozReview Request: Bug 1267510 part 6 - Add animation test of gecko dependent. r?birtles → Bug 1267510 part 6 - Add animation test of gecko dependent.
Attachment #8758134 - Attachment description: MozReview Request: Bug 1267510 part 7 - Tidy up to document-timeline tests. r?birtles → Bug 1267510 part 7 - Tidy up to document-timeline tests.
Attachment #8758135 - Attachment description: MozReview Request: Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories. r?birtles → Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.
(Assignee)

Comment 78

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/6-7/
(Assignee)

Comment 79

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/6-7/
(Assignee)

Comment 80

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/6-7/
(Assignee)

Comment 81

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/6-7/
(Assignee)

Comment 82

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/4-5/
(Assignee)

Comment 83

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/4-5/
(Assignee)

Comment 84

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/4-5/
(Assignee)

Comment 85

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Thanks hiro, Brian.
I updated error message based on comment #71.

Brian,
Could you please reconfirm this patch?
Attachment #8757149 - Flags: review+ → review?(bbirtles)
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

https://reviewboard.mozilla.org/r/55734/#review54640

r=me with the change to the error string although I'd like to see if Cameron agrees with this usage of type errors.

::: dom/bindings/Errors.msg:101
(Diff revision 7)
>  MSG_DEF(MSG_INVALID_DURATION_ERROR, 1, JSEXN_TYPEERR, "Invalid duration '{0}'.")
>  MSG_DEF(MSG_INVALID_EASING_ERROR, 1, JSEXN_TYPEERR, "Invalid easing '{0}'.")
>  MSG_DEF(MSG_USELESS_SETTIMEOUT, 1, JSEXN_TYPEERR, "Useless {0} call (missing quotes around argument?)")
>  MSG_DEF(MSG_TOKENLIST_NO_SUPPORTED_TOKENS, 2, JSEXN_TYPEERR, "{0} attribute of <{1}> does not define any supported tokens")
>  MSG_DEF(MSG_CACHE_STREAM_CLOSED, 0, JSEXN_TYPEERR, "Response body is a cache file stream that has already been closed.")
> +MSG_DEF(MSG_TIME_VALUE_OUT_OF_RANGE, 2, JSEXN_TYPEERR, "{0} '{1}' is outside the supported range for time values.")

I was concerned that putting {0} first means we have to remember to make the first letter of that parameter a capital letter so I went and had a look at the other messages in Errors.msg and I think we don't actually need to specify which parameter is to blame (i.e. "origin time" in this case).

I think it would be enough to use:

"{0} is outside the supported range for time values."

For example, we have already have error messages like:

  {0} is not a date.
  {0} is not a valid URL.
  {0} is an invalid header value.
Attachment #8757149 - Flags: review?(bbirtles) → review+
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Hi Cameron,

This patch checks that the passed-in double can be represented as a TimeDuration and throws if it can't. It throws a TypeError that indicates that it's a limitation of the implementation as opposed to a spec violation. I made some suggestions about the wording of the message in comment 86 but I was wondering if you think a TypeError is appropriate here?
Attachment #8757149 - Flags: feedback?(cam)
TypeError sounds fine to me.
Attachment #8757149 - Flags: feedback?(cam) → feedback+
(Assignee)

Comment 89

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/6-7/
Attachment #8757149 - Flags: feedback+
(Assignee)

Comment 90

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/7-8/
(Assignee)

Comment 91

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/7-8/
(Assignee)

Comment 92

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/7-8/
(Assignee)

Comment 93

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/7-8/
(Assignee)

Comment 94

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/5-6/
(Assignee)

Comment 95

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/5-6/
(Assignee)

Comment 96

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/5-6/
(Assignee)

Comment 97

2 years ago
Thanks Cameron, Brian

I updated the messages and rebased patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=074a3a79a1af33a60c194eabbfae6b0ce842bb47
(Assignee)

Comment 98

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/7-8/
(Assignee)

Comment 99

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/8-9/
(Assignee)

Comment 100

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/8-9/
(Assignee)

Comment 101

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/8-9/
(Assignee)

Comment 102

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/8-9/
(Assignee)

Comment 103

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/6-7/
(Assignee)

Comment 104

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/6-7/
(Assignee)

Comment 105

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/6-7/
(Assignee)

Comment 106

2 years ago
Rebased patches again.
(Assignee)

Comment 107

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/8-9/
(Assignee)

Comment 108

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/9-10/
(Assignee)

Comment 109

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/9-10/
(Assignee)

Comment 110

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/9-10/
(Assignee)

Comment 111

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/9-10/
(Assignee)

Comment 112

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/7-8/
(Assignee)

Comment 113

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/7-8/
(Assignee)

Comment 114

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/7-8/
(Assignee)

Comment 115

2 years ago
Rebased patches again due to land failed.

Comment 116

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d18185f28d
part 1 - Add DocumentTimeline constructor tests. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/058ad3199edb
part 2 - Add origin time invariant to DocumentTimeline. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ace442f6123
part 3 - Add DocumentTimeline constructor. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/130a231c5acc
part 4 - Modify WebIDL of DocumentTimeline. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/884243ce4287
part 5 - Remove animation's w-p-f meta file associated DocumentTimeline. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89ec30077a0
part 6 - Add animation test of gecko dependent. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cdb7f5b6f7c
part 7 - Tidy up to document-timeline tests. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2c42608ff0
part 8 - Move animation's w-p-f tests to correct directories. r=birtles
Mantaroh and I discussed this and it looks like the following line is the problem:

  result = timing->GetNavigationStartTimeStamp() + aTimeDuration + mOriginTime;

In the test in question we have a transition with duration 1000s and we call finish() which adjusts the start time to some time far in the past. When we go to convert event times to timestamps in order to get the correct sequence between events (which may come from different documents with different navigation start times -- hence why we don't just use a TimeDuration) we end up with a situation where timing->GetNavigationStartTimeStamp() + aTimeDuration underflows.

We detect underflow in TimeStamp::operator-=(const TimeDuration& aOther)[1] and set mValue to 0 which produces a null timestamp.

That much has been true for a long time. However, this patch adds a further + operation which takes the temporary (null) result and attempts to add mOriginTime to it. It's at this point that we trip up on the assertion that we do not try to add to a null timestamp.

Given that we are using the timestamp for sorting events and the event sorting code is able to cope with null timestamps (and I can't think of a better way to sort events in this fairly extreme situation) it is probably sufficient to simply change this to:

  result = timing->GetNavigationStartTimeStamp() + (aTimeDuration + mOriginTime);

This will also have the effect that when we have a large minus time duration combined with a large positive origin time we'll be able to successfully represent the result as a TimeStamp.

The only other place this function is used that we might consider is in nsDisplayList[2] where we convert the start time to a timestamp before passing to the compositor.

There is probably a bug here (at very least we should double-check we handle null timestamps correctly here) but it would be an existing problem that we can fix in another bug, not here.

[1] http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/mozglue/misc/TimeStamp.h#521
[2] http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/layout/base/nsDisplayList.cpp#418
Flags: needinfo?(mantaroh)
(Assignee)

Comment 120

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/9-10/
(Assignee)

Comment 121

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/10-11/
(Assignee)

Comment 122

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/10-11/
(Assignee)

Comment 123

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/10-11/
(Assignee)

Comment 124

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/10-11/
(Assignee)

Comment 125

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/8-9/
(Assignee)

Comment 126

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/8-9/
(Assignee)

Comment 127

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/8-9/
(Assignee)

Comment 128

2 years ago
Thanks Brian.

(In reply to Brian Birtles (:birtles) from comment #119)
> There is probably a bug here (at very least we should double-check we handle
> null timestamps correctly here) but it would be an existing problem that we
> can fix in another bug, not here.
We should consider about situation of starting large duration animation immediately. I'll file this phenomenon as another bug.
(Assignee)

Comment 130

2 years ago
Comment on attachment 8757147 [details]
Bug 1267510 part 1 - Add DocumentTimeline constructor tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55730/diff/10-11/
(Assignee)

Comment 131

2 years ago
Comment on attachment 8757148 [details]
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55732/diff/11-12/
(Assignee)

Comment 132

2 years ago
Comment on attachment 8757149 [details]
Bug 1267510 part 3 - Add DocumentTimeline constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55734/diff/11-12/
(Assignee)

Comment 133

2 years ago
Comment on attachment 8757150 [details]
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55736/diff/11-12/
(Assignee)

Comment 134

2 years ago
Comment on attachment 8757151 [details]
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55738/diff/11-12/
(Assignee)

Comment 135

2 years ago
Comment on attachment 8758133 [details]
Bug 1267510 part 6 - Add animation test of gecko dependent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56484/diff/9-10/
(Assignee)

Comment 136

2 years ago
Comment on attachment 8758134 [details]
Bug 1267510 part 7 - Tidy up to document-timeline tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56486/diff/9-10/
(Assignee)

Comment 137

2 years ago
Comment on attachment 8758135 [details]
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56488/diff/9-10/
(Assignee)

Comment 138

2 years ago
The manifest file of web-platform-tests is conflicted.
So I rebased patches.

Comment 139

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a341db55fa17
part 1 - Add DocumentTimeline constructor tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/653d3d05adb6
part 2 - Add origin time invariant to DocumentTimeline. r=birtles
https://hg.mozilla.org/integration/autoland/rev/349919adb804
part 3 - Add DocumentTimeline constructor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b16a83e6f05b
part 4 - Modify WebIDL of DocumentTimeline. r=smaug
https://hg.mozilla.org/integration/autoland/rev/7c8720178842
part 5 - Remove animation's w-p-f meta file associated DocumentTimeline. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6ffdcafd2c0d
part 6 - Add animation test of gecko dependent. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8566f1b815b9
part 7 - Tidy up to document-timeline tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5af9bddf1fd3
part 8 - Move animation's w-p-f tests to correct directories. r=birtles
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1282342
Keywords: dev-doc-needed
Documented (we've already got the ctor documented; we just need to update the BCD and announce it): 

https://github.com/mdn/browser-compat-data/pull/1156
https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #142)
> Documented (we've already got the ctor documented; we just need to update
> the BCD and announce it): 
> 
> https://github.com/mdn/browser-compat-data/pull/1156
> https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM

Ooops, this was done in 50, not 60. Updating ;-)

https://developer.mozilla.org/en-US/Firefox/Releases/50#DOM
We're not shipping this on release channels yet (and probably not for a long time).
(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #144)
> We're not shipping this on release channels yet (and probably not for a long
> time).

Ah, ok, gotcha. I've removed mention of it from the release notes, and mentioned it in the experimental features page instead: https://developer.mozilla.org/en-US/Firefox/Experimental_features
You need to log in before you can comment on or make changes to this bug.