Summary: | requestFrameAnimation() callback timestamp should be very close to Performance.now() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Component: | Animations | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, dino, simon.fraser, thorton, zalan | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=207153 | ||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2016-06-22 14:23:34 PDT
Created attachment 281869 [details]
Testcase
Created attachment 281875 [details]
Patch
Comment on attachment 281875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281875&action=review > Source/WebCore/ChangeLog:16 > + * platform/graphics/mac/DisplayRefreshMonitorMac.cpp: > + (WebCore::displayLinkCallback): > + (WebCore::DisplayRefreshMonitorMac::displayLinkFired): You need to fix DisplayRefreshMonitorIOS too. > Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp:95 > + setMonotonicAnimationStartTime(monotonicallyIncreasingTime()); I don't think this is the right level at which to set the time; it only fixes the Mac DisplayRefreshMonitor. I think you could remove passing the timestamps all the way down to ScriptedAnimationController::serviceScriptedAnimations(), and grab the current time there. Also, you should grab it via Performance::now(), not just monotonicallyIncreasingTime(), since it should be quantized in the same way. I would also like to see a test that checks that multiple rAF callbacks fired at the same time get the same timestamp (if we don't have one already). Created attachment 281921 [details]
Patch
Created attachment 281927 [details]
Patch
Comment on attachment 281927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281927&action=review > Source/WebCore/dom/ScriptedAnimationController.cpp:152 > + double legacyHighResNowMs = 1000 * m_document->loader()->timing().windowTimeToPseudoWallTime(timestamp); I don't know if it's worth preserving this legacy behavior. If it is, I would rename windowTimeToPseudoWallTime() to something like timeForLegacyRequestAnimationFrame() (or move the computation to here). > Source/WebCore/page/DOMWindow.cpp:757 > + if (!timestamp && document()) { > + DocumentLoader* loader = document()->loader(); > + timestamp = loader ? loader->timing().monotonicTimeToZeroBasedDocumentTime(monotonicallyIncreasingTime()) : 0; > + } Is timestamp only 0 when ENABLE(WEB_TIMING) is off? If so, I'd just #ifdef the entire block. > Source/WebCore/page/FrameView.cpp:2832 > +void FrameView::serviceScriptedAnimations(double timestamp) Are you passing the timestamp through here just for coordinated graphics? I'm not sure why we'd leave their rAF callbacks with a different behavior. > LayoutTests/animations/animation-callback-timestamp-expected.txt:3 > +PASS successfullyParsed is true > + > +TEST COMPLETE Would be nice if the test result said something like 'PASS: requestAnimationFrame callback time was within blah blah" > LayoutTests/animations/animation-multiple-callbacks-timestamp-expected.txt:3 > +PASS successfullyParsed is true > + > +TEST COMPLETE Ditto. Created attachment 281933 [details]
Patch
Comment on attachment 281927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281927&action=review >> Source/WebCore/dom/ScriptedAnimationController.cpp:152 >> + double legacyHighResNowMs = 1000 * m_document->loader()->timing().windowTimeToPseudoWallTime(timestamp); > > I don't know if it's worth preserving this legacy behavior. If it is, I would rename windowTimeToPseudoWallTime() to something like timeForLegacyRequestAnimationFrame() (or move the computation to here). The function DocumentLoadTiming::referenceWallTime() was added and the calculation was moved to here. >> Source/WebCore/page/DOMWindow.cpp:757 >> + } > > Is timestamp only 0 when ENABLE(WEB_TIMING) is off? If so, I'd just #ifdef the entire block. The entire function is now surrounded by #if #endif directives which is similar to HTMLMediaElement.cpp had before. >> Source/WebCore/page/FrameView.cpp:2832 >> +void FrameView::serviceScriptedAnimations(double timestamp) > > Are you passing the timestamp through here just for coordinated graphics? I'm not sure why we'd leave their rAF callbacks with a different behavior. The timestamp is not passed to FrameView::serviceScriptedAnimations() anymore. >> LayoutTests/animations/animation-callback-timestamp-expected.txt:3 >> +TEST COMPLETE > > Would be nice if the test result said something like 'PASS: requestAnimationFrame callback time was within blah blah" Fixed. >> LayoutTests/animations/animation-multiple-callbacks-timestamp-expected.txt:3 >> +TEST COMPLETE > > Ditto. Fixed. *** Bug 115333 has been marked as a duplicate of this bug. *** Comment on attachment 281933 [details] Patch Clearing flags on attachment: 281933 Committed r202399: <http://trac.webkit.org/changeset/202399> All reviewed patches have been landed. Closing bug. |