Bug 210990

Created attachment 397499 [details]
Patch

Created attachment 397506 [details]
Patch

Created attachment 397542 [details]
Patch

Created attachment 397548 [details]
Patch

Created attachment 397552 [details]
Patch

Comment on attachment 397552 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397552&action=review

Patch looks good.

> Source/WebCore/dom/Document.cpp:7679
> +                    (*timestamp).milliseconds(),

Ok as is. No change needed. Optimal solution is to use -> because it is more idiomatic and aestically pleasing.

Created attachment 397634 [details]
Patch

Committed r260736: <https://trac.webkit.org/changeset/260736>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397634 [details].

<rdar://problem/62416809>

It looks like the changes in https://trac.webkit.org/changeset/260736/webkit

broke imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html

history:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fdocument-timelines.html

Diff:
--- /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines-actual.txt
@@ -1,4 +1,4 @@
 
-PASS Document timelines report current time relative to navigationStart 
+FAIL Document timelines report current time relative to navigationStart assert_equals: The current time matches requestAnimationFrame time expected 13.000000000000002 but got 13
 PASS Child frames do not report negative initial times 
 

I was able to reproduce this with 

run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html

We used to convert Performance::now() which is milliseconds to seconds in DOMWindow::nowTimestamp() and then we were converting these seconds back to milliseconds in serviceRequestAnimationFrameCallbacks(). So I thought we needed to round the timestamp to the nearest milliseconds.

When converting the timestamps from double to Seconds, I thought we do not need this rounding anymore. It turned out AnimationTimeline::bindingsCurrentTime() does similar rounding even with Seconds when it calls secondsToWebAnimationsAPITime().

Reopen to attach a new patch.

Created attachment 397873 [details]
Patch

Comment on attachment 397873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397873&action=review

> Source/WTF/wtf/Seconds.h:62
> +        auto milliseconds = std::round(this->microseconds()) / 1000;
> +        if (milliseconds == -0)
> +            return 0;
> +        return milliseconds;

Now that I see that this is really a Web Animations-specific behavior, I'm not sure it belongs here. I think a native roundedMilliseconds() that just returns std::round(milliseconds()) here is OK, but the -0 check is odd.

> Source/WebCore/animation/AnimationEffect.cpp:337
> -    computedTiming.delay = secondsToWebAnimationsAPITime(m_delay);
> -    computedTiming.endDelay = secondsToWebAnimationsAPITime(m_endDelay);
> +    computedTiming.delay = m_delay.roundedMilliseconds();
> +    computedTiming.endDelay = m_endDelay.roundedMilliseconds();

I think you should keep secondsToWebAnimationsAPITime() and have it call roundedMilliseconds()

> Source/WebCore/animation/AnimationTimeline.cpp:94
>  Optional<double> AnimationTimeline::bindingsCurrentTime()

Should these return DOMHighResTimestamp (in a later patch)?

> Source/WebCore/animation/DeclarativeAnimation.cpp:353
>      auto event = createEvent(eventType, time, pseudoId, timelineTime);

So event time is in seconds I guess. So confusing.

> Source/WebCore/animation/WebAnimation.cpp:364
>  Optional<double> WebAnimation::bindingsCurrentTime() const

DOMHighResTimestamp?

Created attachment 397912 [details]
Patch

Comment on attachment 397912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397912&action=review

> Source/WTF/wtf/Seconds.h:57
> +    double roundedMilliseconds() const { return std::round(microseconds()) / 1000; }

This is milliseconds rounded to the nearest microsecond.

Rounding to the nearest millisecond would be:

    return std::round(microseconds() * 0.001);

Also, I think we prefer "* 0.001" over "/ 1000".

So I think this needs a better name.

Comment on attachment 397873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397873&action=review

>> Source/WTF/wtf/Seconds.h:62
>> +        return milliseconds;
> 
> Now that I see that this is really a Web Animations-specific behavior, I'm not sure it belongs here. I think a native roundedMilliseconds() that just returns std::round(milliseconds()) here is OK, but the -0 check is odd.

I removed the -0 check, but

Why is it Web Animations-specific behavior? The specs https://drafts.csswg.org/web-animations-1/#precision-of-time-values does not say we have to do it for Web Animations. Timestamps should be positive but the result of a floating point operations can be -0 which is also equal to +0. So ensuring the timestamp is always +ve is a good thing and it does not affect the result. Besides the test imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html checks whether the timestamp of the rAF is equal to the currentTime of the DocumentTimeline. So how we guarantee the two times are equal if we round them differently?

>> Source/WebCore/animation/AnimationEffect.cpp:337
>> +    computedTiming.endDelay = m_endDelay.roundedMilliseconds();
> 
> I think you should keep secondsToWebAnimationsAPITime() and have it call roundedMilliseconds()

Done.

>> Source/WebCore/animation/AnimationTimeline.cpp:94
>>  Optional<double> AnimationTimeline::bindingsCurrentTime()
> 
> Should these return DOMHighResTimestamp (in a later patch)?

Done.

>> Source/WebCore/animation/WebAnimation.cpp:364
>>  Optional<double> WebAnimation::bindingsCurrentTime() const
> 
> DOMHighResTimestamp?

Done.

Comment on attachment 397912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397912&action=review

>> Source/WTF/wtf/Seconds.h:57
>> +    double roundedMilliseconds() const { return std::round(microseconds()) / 1000; }
> 
> This is milliseconds rounded to the nearest microsecond.
> 
> Rounding to the nearest millisecond would be:
> 
>     return std::round(microseconds() * 0.001);
> 
> Also, I think we prefer "* 0.001" over "/ 1000".
> 
> So I think this needs a better name.

You are right. This calculation is not right even for exposing the timestamps to JavaScript. The timestamps are reduced resolution times to the nearest milliseconds; see Performance::now(). So it does not make sense to round them later to the nearest microsecond.

I fixed the flakiness by putting std::round back in serviceRequestAnimationFrameCallbacks() since the Seconds::roundedMilliseconds() change is taking longer than I expected.

The flakiness is fixed in:

Committed r260870: <https://trac.webkit.org/changeset/260870>