Closed Bug 1267510 Opened 8 years ago Closed 8 years ago

Implement DocumentTimeline Constructor

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(8 files)

Bug 1267510 part 1 - Add DocumentTimeline constructor tests.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1267510 part 2 - Add origin time invariant to DocumentTimeline.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1267510 part 3 - Add DocumentTimeline constructor.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1267510 part 4 - Modify WebIDL of DocumentTimeline.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1267510 part 5 - Remove animation's w-p-f meta file associated DocumentTimeline.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1267510 part 6 - Add animation test of gecko dependent.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1267510 part 7 - Tidy up to document-timeline tests.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1267510 part 8 - Move animation's w-p-f tests to correct directories.
58 bytes, text/x-review-board-request
birtles
: review+
Details 
      No description provided.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
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)
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)
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)
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/
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/
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/
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/
Attachment #8757150 - Flags: review?(bugs) → review+
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+
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)
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/
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/
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/
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/
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/
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.
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)
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)
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/
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/
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/
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/
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/
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/
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/
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.
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)
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/
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/
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/
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/
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/
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/
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+
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/
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/
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/
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/
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/
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/
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/
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/
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?
(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.
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.
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/
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/
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/
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/
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/
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/
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/
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+
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+
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/
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/
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/
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/
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/
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/
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/
Thanks Cameron, Brian

I updated the messages and rebased patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=074a3a79a1af33a60c194eabbfae6b0ce842bb47
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/
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/
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/
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/
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/
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/
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/
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/
Rebased patches again.
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/
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/
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/
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/
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/
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/
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/
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/
Rebased patches again due to land failed.
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)
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/
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/
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/
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/
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/
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/
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/
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/
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.
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/
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/
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/
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/
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/
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/
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/
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/
The manifest file of web-platform-tests is conflicted.
So I rebased patches.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: