65282 2011-07-27 13:59:54 -0700 SMILTimeContainer should use monotonicallyIncreasingTime() rather than currentTime() for animations 2026-04-01 08:03:03 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 scottmg scottmg dino kabakov.as krit simon.fraser webkit.review.bot zimmermann oldest_to_newest 443137 0 scottmg 2011-07-27 13:59:54 -0700 It appears it should be using monotonicallyIncreasingTime to handle changing system clocks while animating. 443142 1 102181 scottmg 2011-07-27 14:06:39 -0700 Created attachment 102181 use monotonicallyIncreasingTime rather than currentTime to update animations 443145 2 webkit.review.bot 2011-07-27 14:09:54 -0700 Attachment 102181 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 443146 3 102183 scottmg 2011-07-27 14:12:08 -0700 Created attachment 102183 use monotonicallyIncreasingTime rather than currentTime to update animations 443371 4 102183 krit 2011-07-27 23:46:13 -0700 Comment on attachment 102183 use monotonicallyIncreasingTime rather than currentTime to update animations View in context: https://bugs.webkit.org/attachment.cgi?id=102183&action=review The timing code is similar to the code in AnimationControler and AnimationBase. But these classes use currentTime() as well. Shouldn't they be affected as well some how? r- because of the changelog. > Source/WebCore/ChangeLog:9 > + SMILTimeContainer using currentTime for animations > + https://bugs.webkit.org/show_bug.cgi?id=65282 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests, existing tests should play correctly when system clock > + changed. This needs a detailed description. What did you change? Why did you change it? And it is not only important that do not break thinks: It is important how it affects the behavior. You need tests to verify the new behavior, especially what works now but didn't work before. Does your patch affect bug 31256 as well? > Source/WebCore/ChangeLog:11 > + * svg/animation/SMILTimeContainer.cpp: Is SMILTimeContainer the only file in WebCore/svg that uses currentTime()? 443373 5 krit 2011-07-27 23:49:38 -0700 Adding Dean and Simon to the bug report. I'd like to hear their opinion to the changes. Like I wrote before, the SMILTimeContainer code is similar to the animation stuff in WebCore/page/animation. But the AnimationBase and AnimationController use currentTime() as well. 443566 6 scottmg 2011-07-28 11:20:04 -0700 (In reply to comment #4) > (From update of attachment 102183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102183&action=review > > The timing code is similar to the code in AnimationControler and AnimationBase. But these classes use currentTime() as well. Shouldn't they be affected as well some how? I suspect they should be, but maybe someone else who knows better can comment. From my understanding, currentTime() is correct when you want to know what time the user sees on his clock. currentTime() can jump forward or backwards (suspend, change system time), so for calculating a delta between two events monotonicallyIncreasingTime() would be preferred. > r- because of the changelog. My apologies for forgetting this, that was dumb. > > > Source/WebCore/ChangeLog:9 > > + SMILTimeContainer using currentTime for animations > > + https://bugs.webkit.org/show_bug.cgi?id=65282 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + No new tests, existing tests should play correctly when system clock > > + changed. > > This needs a detailed description. What did you change? Why did you change it? And it is not only important that do not break thinks: It is important how it affects the behavior. You need tests to verify the new behavior, especially what works now but didn't work before. Does your patch affect bug 31256 as well? I wasn't aware of that bug, but no, it does not appear to be sufficient to fix that bug. This bug should probably be merged with that one. This change is probably not worth pursuing and I'll investigate the root cause of 31256 instead. I don't have a good idea on how to write an automated test for this behaviour. All of the currentTime() implementations appear to go directly to the system clock. I could try to add support for custom control of currentTime to DRT perhaps? But it seems a bit tortured as the test code will be an implementation of monotonicallyIncreasingTime. > > > Source/WebCore/ChangeLog:11 > > + * svg/animation/SMILTimeContainer.cpp: > > Is SMILTimeContainer the only file in WebCore/svg that uses currentTime()? Yes, I believe so. 443659 7 scottmg 2011-07-28 14:42:20 -0700 (In reply to comment #6) > > Does your patch affect bug 31256 as well? > I wasn't aware of that bug, but no, it does not appear to be sufficient to fix that bug. Scratch that, it *does* appear to fix bug 31256 as tested with http://upload.wikimedia.org/wikipedia/commons/4/4f/Soccer_ball_animated.svg I believe I confused my build system when changing the clock when I first tried. When using currentTime() when the clock is set back a little bit (~30s) normally the animation will pause until the previous (farther in the future time) is reached. Sometimes though, it will just "glitch" and restart. I'm not sure of the cause of that difference yet. But, when using monotonicallyIncreasingTime() I do not see either of these artifacts when setting the clock back a bit. If you agree Dirk, I will mark this bug as duplicate of 31256 and upload an improved patch there. And if you have suggestions on how to test I would be grateful. We should perhaps continue the discussion on AnimationController and AnimationBase in a new bug. 443746 8 dino 2011-07-28 17:52:09 -0700 (In reply to comment #7) > We should perhaps continue the discussion on AnimationController and AnimationBase in a new bug. Yeah. AnimationController and Base should move to the monotonic time as well, but that doesn't need to happen here. 443822 9 krit 2011-07-28 22:32:42 -0700 Ok, can you upload a new patch with better change log and a test case? 555375 10 zimmermann 2012-02-13 10:45:40 -0800 Any updates here Scott? 555409 11 scottmg 2012-02-13 11:22:12 -0800 (In reply to comment #10) > Any updates here Scott? No, sorry. I guess to write a test that shows the current code is broken, it would first be necessary to virtualize all the port's currentTime() implementations so that a test could then set it arbitrarily like a user can set system time. 1567169 12 zimmermann 2019-09-02 02:35:04 -0700 SMILTimeContainer and the CSS animation/transition code path uses MonoticTime::now(), which superseded monotonicallyIncreasingTime() since 27.08.2013 (r154706). 2195912 13 karlcow 2026-04-01 08:03:03 -0700 *** Bug 31256 has been marked as a duplicate of this bug. *** 102181 2011-07-27 14:06:39 -0700 2011-07-27 14:12:03 -0700 use monotonicallyIncreasingTime rather than currentTime to update animations bug-65282-20110727140638.patch text/plain 2335 scottmg U3VidmVyc2lvbiBSZXZpc2lvbjogOTE4NjQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxOTYyNGY3N2U0NmY3MDI5 YjI0YmJjNDRhZTA4OGIyMGUxZWVhNTQ5Li5mY2I3Nzg1MGUyYzRjZDllNjU2Yzg4MmQyNjljNzBi NmY4YmJmMGFjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMDctMjcgIFNjb3R0 IEdyYWhhbSAgPHNjb3R0bWdAZ29vZ2xlLmNvbT4KKworICAgICAgICBTTUlMVGltZUNvbnRhaW5l ciB1c2luZyBjdXJyZW50VGltZSBmb3IgYW5pbWF0aW9ucworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjUyODIKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGV4aXN0aW5nIHRlc3RzIHNo b3VsZCBwbGF5IGNvcnJlY3RseSB3aGVuIHN5c3RlbSBjbG9jaworCQljaGFuZ2VkLgorCisgICAg ICAgICogc3ZnL2FuaW1hdGlvbi9TTUlMVGltZUNvbnRhaW5lci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpTTUlMVGltZUNvbnRhaW5lcjo6ZWxhcHNlZCk6CisgICAgICAgIChXZWJDb3JlOjpTTUlM VGltZUNvbnRhaW5lcjo6YmVnaW4pOgorICAgICAgICAoV2ViQ29yZTo6U01JTFRpbWVDb250YWlu ZXI6OnBhdXNlKToKKyAgICAgICAgKFdlYkNvcmU6OlNNSUxUaW1lQ29udGFpbmVyOjpyZXN1bWUp OgorCiAyMDExLTA3LTI1ICBNaWhhaSBQYXJwYXJpdGEgIDxtaWhhaXBAY2hyb21pdW0ub3JnPgog CiAgICAgICAgIFtDaHJvbWl1bV0gQWRkIGJldHRlciBXZWJLaXQgQVBJIGZvciBjaHJvbWUudGFi cy5pbnNlcnRDU1MgZXh0ZW5zaW9uIEFQSQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvc3Zn L2FuaW1hdGlvbi9TTUlMVGltZUNvbnRhaW5lci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9zdmcvYW5p bWF0aW9uL1NNSUxUaW1lQ29udGFpbmVyLmNwcAppbmRleCBlOWJiMzhhZWIxMDRlMjQwNjRmYzFh ZDg4Mjg4M2VhNmY3ZTE0OTA5Li44MWRkYzgzMWYwODNjZDMyNTU3NTE1YTBiN2QzOTRiNWEwYzQw ZjdjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9zdmcvYW5pbWF0aW9uL1NNSUxUaW1lQ29u dGFpbmVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9zdmcvYW5pbWF0aW9uL1NNSUxUaW1lQ29u dGFpbmVyLmNwcApAQCAtODIsNyArODIsNyBAQCBTTUlMVGltZSBTTUlMVGltZUNvbnRhaW5lcjo6 ZWxhcHNlZCgpIGNvbnN0CiB7CiAgICAgaWYgKCFtX2JlZ2luVGltZSkKICAgICAgICAgcmV0dXJu IDA7Ci0gICAgcmV0dXJuIGN1cnJlbnRUaW1lKCkgLSBtX2JlZ2luVGltZSAtIG1fYWNjdW11bGF0 ZWRQYXVzZVRpbWU7CisgICAgcmV0dXJuIG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpIC0g bV9iZWdpblRpbWUgLSBtX2FjY3VtdWxhdGVkUGF1c2VUaW1lOwogfQogICAgIAogYm9vbCBTTUlM VGltZUNvbnRhaW5lcjo6aXNBY3RpdmUoKSBjb25zdApAQCAtOTgsNyArOTgsNyBAQCBib29sIFNN SUxUaW1lQ29udGFpbmVyOjppc1BhdXNlZCgpIGNvbnN0CiB2b2lkIFNNSUxUaW1lQ29udGFpbmVy OjpiZWdpbigpCiB7CiAgICAgQVNTRVJUKCFtX2JlZ2luVGltZSk7Ci0gICAgbV9iZWdpblRpbWUg PSBjdXJyZW50VGltZSgpOworICAgIG1fYmVnaW5UaW1lID0gbW9ub3RvbmljYWxseUluY3JlYXNp bmdUaW1lKCk7CiAgICAgdXBkYXRlQW5pbWF0aW9ucygwKTsKIH0KIApAQCAtMTA3LDcgKzEwNyw3 IEBAIHZvaWQgU01JTFRpbWVDb250YWluZXI6OnBhdXNlKCkKICAgICBpZiAoIW1fYmVnaW5UaW1l KQogICAgICAgICByZXR1cm47CiAgICAgQVNTRVJUKCFpc1BhdXNlZCgpKTsKLSAgICBtX3BhdXNl VGltZSA9IGN1cnJlbnRUaW1lKCk7CisgICAgbV9wYXVzZVRpbWUgPSBtb25vdG9uaWNhbGx5SW5j cmVhc2luZ1RpbWUoKTsKICAgICBtX3RpbWVyLnN0b3AoKTsKIH0KIApAQCAtMTE2LDcgKzExNiw3 IEBAIHZvaWQgU01JTFRpbWVDb250YWluZXI6OnJlc3VtZSgpCiAgICAgaWYgKCFtX2JlZ2luVGlt ZSkKICAgICAgICAgcmV0dXJuOwogICAgIEFTU0VSVChpc1BhdXNlZCgpKTsKLSAgICBtX2FjY3Vt dWxhdGVkUGF1c2VUaW1lICs9IGN1cnJlbnRUaW1lKCkgLSBtX3BhdXNlVGltZTsKKyAgICBtX2Fj Y3VtdWxhdGVkUGF1c2VUaW1lICs9IG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpIC0gbV9w YXVzZVRpbWU7CiAgICAgbV9wYXVzZVRpbWUgPSAwOwogICAgIHN0YXJ0VGltZXIoMCk7CiB9Cg== 102183 2011-07-27 14:12:08 -0700 2011-07-27 23:46:13 -0700 use monotonicallyIncreasingTime rather than currentTime to update animations bug-65282-20110727141207.patch text/plain 2341 scottmg U3VidmVyc2lvbiBSZXZpc2lvbjogOTE4NjQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxOTYyNGY3N2U0NmY3MDI5 YjI0YmJjNDRhZTA4OGIyMGUxZWVhNTQ5Li5lMmQ0NjI3NDI2OWExZjgxMzZjZjYxZjRiMjc3Mjk4 OThlZjQ5NWJhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMDctMjcgIFNjb3R0 IEdyYWhhbSAgPHNjb3R0bWdAZ29vZ2xlLmNvbT4KKworICAgICAgICBTTUlMVGltZUNvbnRhaW5l ciB1c2luZyBjdXJyZW50VGltZSBmb3IgYW5pbWF0aW9ucworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjUyODIKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGV4aXN0aW5nIHRlc3RzIHNo b3VsZCBwbGF5IGNvcnJlY3RseSB3aGVuIHN5c3RlbSBjbG9jaworICAgICAgICBjaGFuZ2VkLgor CisgICAgICAgICogc3ZnL2FuaW1hdGlvbi9TTUlMVGltZUNvbnRhaW5lci5jcHA6CisgICAgICAg IChXZWJDb3JlOjpTTUlMVGltZUNvbnRhaW5lcjo6ZWxhcHNlZCk6CisgICAgICAgIChXZWJDb3Jl OjpTTUlMVGltZUNvbnRhaW5lcjo6YmVnaW4pOgorICAgICAgICAoV2ViQ29yZTo6U01JTFRpbWVD b250YWluZXI6OnBhdXNlKToKKyAgICAgICAgKFdlYkNvcmU6OlNNSUxUaW1lQ29udGFpbmVyOjpy ZXN1bWUpOgorCiAyMDExLTA3LTI1ICBNaWhhaSBQYXJwYXJpdGEgIDxtaWhhaXBAY2hyb21pdW0u b3JnPgogCiAgICAgICAgIFtDaHJvbWl1bV0gQWRkIGJldHRlciBXZWJLaXQgQVBJIGZvciBjaHJv bWUudGFicy5pbnNlcnRDU1MgZXh0ZW5zaW9uIEFQSQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvc3ZnL2FuaW1hdGlvbi9TTUlMVGltZUNvbnRhaW5lci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9z dmcvYW5pbWF0aW9uL1NNSUxUaW1lQ29udGFpbmVyLmNwcAppbmRleCBlOWJiMzhhZWIxMDRlMjQw NjRmYzFhZDg4Mjg4M2VhNmY3ZTE0OTA5Li44MWRkYzgzMWYwODNjZDMyNTU3NTE1YTBiN2QzOTRi NWEwYzQwZjdjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9zdmcvYW5pbWF0aW9uL1NNSUxU aW1lQ29udGFpbmVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9zdmcvYW5pbWF0aW9uL1NNSUxU aW1lQ29udGFpbmVyLmNwcApAQCAtODIsNyArODIsNyBAQCBTTUlMVGltZSBTTUlMVGltZUNvbnRh aW5lcjo6ZWxhcHNlZCgpIGNvbnN0CiB7CiAgICAgaWYgKCFtX2JlZ2luVGltZSkKICAgICAgICAg cmV0dXJuIDA7Ci0gICAgcmV0dXJuIGN1cnJlbnRUaW1lKCkgLSBtX2JlZ2luVGltZSAtIG1fYWNj dW11bGF0ZWRQYXVzZVRpbWU7CisgICAgcmV0dXJuIG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGlt ZSgpIC0gbV9iZWdpblRpbWUgLSBtX2FjY3VtdWxhdGVkUGF1c2VUaW1lOwogfQogICAgIAogYm9v bCBTTUlMVGltZUNvbnRhaW5lcjo6aXNBY3RpdmUoKSBjb25zdApAQCAtOTgsNyArOTgsNyBAQCBi b29sIFNNSUxUaW1lQ29udGFpbmVyOjppc1BhdXNlZCgpIGNvbnN0CiB2b2lkIFNNSUxUaW1lQ29u dGFpbmVyOjpiZWdpbigpCiB7CiAgICAgQVNTRVJUKCFtX2JlZ2luVGltZSk7Ci0gICAgbV9iZWdp blRpbWUgPSBjdXJyZW50VGltZSgpOworICAgIG1fYmVnaW5UaW1lID0gbW9ub3RvbmljYWxseUlu Y3JlYXNpbmdUaW1lKCk7CiAgICAgdXBkYXRlQW5pbWF0aW9ucygwKTsKIH0KIApAQCAtMTA3LDcg KzEwNyw3IEBAIHZvaWQgU01JTFRpbWVDb250YWluZXI6OnBhdXNlKCkKICAgICBpZiAoIW1fYmVn aW5UaW1lKQogICAgICAgICByZXR1cm47CiAgICAgQVNTRVJUKCFpc1BhdXNlZCgpKTsKLSAgICBt X3BhdXNlVGltZSA9IGN1cnJlbnRUaW1lKCk7CisgICAgbV9wYXVzZVRpbWUgPSBtb25vdG9uaWNh bGx5SW5jcmVhc2luZ1RpbWUoKTsKICAgICBtX3RpbWVyLnN0b3AoKTsKIH0KIApAQCAtMTE2LDcg KzExNiw3IEBAIHZvaWQgU01JTFRpbWVDb250YWluZXI6OnJlc3VtZSgpCiAgICAgaWYgKCFtX2Jl Z2luVGltZSkKICAgICAgICAgcmV0dXJuOwogICAgIEFTU0VSVChpc1BhdXNlZCgpKTsKLSAgICBt X2FjY3VtdWxhdGVkUGF1c2VUaW1lICs9IGN1cnJlbnRUaW1lKCkgLSBtX3BhdXNlVGltZTsKKyAg ICBtX2FjY3VtdWxhdGVkUGF1c2VUaW1lICs9IG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgp IC0gbV9wYXVzZVRpbWU7CiAgICAgbV9wYXVzZVRpbWUgPSAwOwogICAgIHN0YXJ0VGltZXIoMCk7 CiB9Cg==