It has been confusing to deal with DOMHighResTimeStamp since the units can be not deduced easily. Some places use it as seconds other places use it as milliseconds. We will change DOMWindow::nowTimestamp() to return Seconds and change the callers accordingly.
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>