Summary: | [CoordinatedGraphics] serviceScriptedAnimations expects time in seconds | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rafael Brandao <rafael.lobo> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Rafael Brandao <rafael.lobo> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dglazkov, jamesr, kbalazs, luis.gabriel, luiz, noam, webkit.review.bot, zeno | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Rafael Brandao
2013-03-18 09:20:17 PDT
Created attachment 193580 [details]
Patch
Comment on attachment 193580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193580&action=review Is EFL using a different LayerTreeHost? They don't seem to be affected by this. Also, you could add a test here... > Source/WebKit2/ChangeLog:9 > + which takes the time delta and multiply for 1000 to get the time in ms. "multiply by (...)" ?! Created attachment 193805 [details]
Patch
(In reply to comment #2) > (From update of attachment 193580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193580&action=review > > Is EFL using a different LayerTreeHost? They don't seem to be affected by this. Also, you could add a test here... They're using the same layer tree host, but EFL use a timer to simulate rAF with USE(REQUEST_ANIMATION_FRAME_TIMER). The code change here is on the code path that doesn't use it. I've added a layout test. > > > Source/WebKit2/ChangeLog:9 > > + which takes the time delta and multiply for 1000 to get the time in ms. > > "multiply by (...)" ?! Thanks! Created attachment 193826 [details]
Patch
Modified test thanks to kbalazs suggestions. :-) Comment on attachment 193826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193826&action=review Need to fix changelog. Fix looks ok to me, please ask a WK2 owner for r. > Source/WebKit/qt/ChangeLog:9 > + The tip came from ScriptedAnimationController::serviceScriptedAnimations > + which takes the time delta and multiply by 1000 to get the time in ms. You don't need this in the Changelog. > Source/WebKit/qt/ChangeLog:11 > + We've been exposing different time unit for requestAnimationFrame because > + of this wrong conversion and web apps could get unexpected results. ... fixed to use seconds instead of milliseconds. Created attachment 193829 [details]
Patch
Comment on attachment 193829 [details] Patch Attachment 193829 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17230180 New failing tests: fast/animation/request-animation-frame-time-unit.html Created attachment 193853 [details]
Patch
I was too otimistic to set the tolerance to only 5ms, I'll try again with 200ms and see what bots think.
Comment on attachment 193853 [details] Patch Attachment 193853 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17190618 New failing tests: fast/animation/request-animation-frame-time-unit.html Created attachment 194124 [details]
Patch
(In reply to comment #12) > Created an attachment (id=194124) [details] > Patch Thanks to jamesr for the tip, there was a missing testRunner.display() call after the second requestAnimationFrame which avoided it to be executed. Is there any WK2 owner available for review? Comment on attachment 194124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194124&action=review No problem with the change, but the test needs work. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:3 > +var toleranceInMs = 50; Why does this need to be global? 50 seems huge. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:7 > +var callbackTimeRef = 0; > +var timeRefInMs = 0; Ditto. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:18 > + if (window.testRunner) > + testRunner.display(); I am surprised this is inside the callback and not in the test setup. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:26 > +setTimeout(function() { > + shouldBeTrue("isTimeUnitInMs"); > +}, 250); Nope, this should be done from the second callback of window.requestAnimationFrame. With this, you would introduce a mandatory 250 ms delay for this test, while it could be ~34ms. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:29 > +if (window.testRunner) > + testRunner.waitUntilDone(); js-tests have a variable for this, you should not do it manually. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:35 > +setTimeout(function() { > + isSuccessfullyParsed(); > + if (window.testRunner) > + testRunner.notifyDone(); > +}, 300); Wait what? (In reply to comment #14) > (From update of attachment 194124 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194124&action=review > > No problem with the change, but the test needs work. > > > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:3 > > +var toleranceInMs = 50; > > Why does this need to be global? > > 50 seems huge. I'll use 5 instead. > > > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:7 > > +var callbackTimeRef = 0; > > +var timeRefInMs = 0; > > Ditto. Ok. > > > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:18 > > + if (window.testRunner) > > + testRunner.display(); > > I am surprised this is inside the callback and not in the test setup. I was surprised too, but it seems that "display()" is what should move requestAnimationFrame calls. A browser would continuously call display() and then this would not be needed. This is what solved the trick to chromium-linux pass the test. > > > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:26 > > +setTimeout(function() { > > + shouldBeTrue("isTimeUnitInMs"); > > +}, 250); > > Nope, this should be done from the second callback of window.requestAnimationFrame. > With this, you would introduce a mandatory 250 ms delay for this test, while it could be ~34ms. Indeed, excellent idea! > > > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:29 > > +if (window.testRunner) > > + testRunner.waitUntilDone(); > > js-tests have a variable for this, you should not do it manually. Ok, I'll look into this. > > > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:35 > > +setTimeout(function() { > > + isSuccessfullyParsed(); > > + if (window.testRunner) > > + testRunner.notifyDone(); > > +}, 300); > > Wait what? Got it. I've suddenly figured out that nested testRunner.display() with rAF does not work, or it is a problem with notifyDone(). See how it looks like now: var isTimeUnitInMs = false; window.requestAnimationFrame(function(time) { var callbackTimeRef = time; var timeRefInMs = Date.now(); window.requestAnimationFrame(function(time) { var delta = time - callbackTimeRef; var deltaTimeInMs = Date.now() - timeRefInMs; var toleranceInMs = 5; isTimeUnitInMs = Math.abs(delta - deltaTimeInMs) <= toleranceInMs; shouldBeTrue("isTimeUnitInMs"); isSuccessfullyParsed(); if (window.testRunner) testRunner.notifyDone(); }); if (window.testRunner) testRunner.display(); }); if (window.testRunner) testRunner.display(); if (window.testRunner) testRunner.waitUntilDone(); When I leave the notifyDone() inside the nested rAF, even chromium stops working and I only get test timeouts. If I move notifyDone to a timeout check like it was before, then it works. Do you know if there's anything obvious that I'm not following in this workflow? Here I'm following up the logic of "testRunner.display()" after each rAF to simulate a paint on the browser and then consequently triggering rAF callback afterwards. Also ccing jamesr since as he knows a lot about rAF. :-) In my understanding rAF should make the browser schedule a repaint, so you should not need to use testRunner.display() at all. If that's not the case for chrome or other ports it might be a bug, so you should probably add an expected fail expectation for it. I wonder what do others think. In a browser, yes. In DumpRenderTree, no. The repaint tests depend on the test having full control over when frames are produced so we can't schedule frames to happen at other indeterminate times. (In reply to comment #17) > In a browser, yes. In DumpRenderTree, no. The repaint tests depend on the test having full control over when frames are produced so we can't schedule frames to happen at other indeterminate times. I've reported the trouble with display-driven test to bug 112946, I invite you all to take a look. Since apparently the solution that would work for everyone is to add a timeout to call notifyDone, I'm willing to do this here for the time being. Benjamin, would you mind with this? Created attachment 194348 [details]
Patch
This is the best I could do to make it work everywhere I've tested. It seems that setTimeout drives things through time inside a layout test, but there's certainly something odd on all of this. I've separated in another bug for more investigation. Comment on attachment 194348 [details]
Patch
LGTM, can one of the WK2 owners sign off?
Comment on attachment 194348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194348&action=review signed off by me, but I am sad about the test. > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:9 > + // FIXME: We should not need to use any timeout here (bug 112946). Please remove this. Realistically, most of those tests needs this fixme. Created attachment 196463 [details]
Patch
Not sure if I could land this right away or if I need to get an official r+ first, so I decided to upload it again. Could someone approve it?
Committed r148022: <http://trac.webkit.org/changeset/148022> |