NEW207782
Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after()
https://bugs.webkit.org/show_bug.cgi?id=207782
Summary Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceA...
Peng Liu
Reported 2020-02-14 12:06:05 PST
Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after()
Attachments
Patch (3.24 KB, patch)
2020-02-14 12:08 PST, Peng Liu
no flags
Peng Liu
Comment 1 2020-02-14 12:08:14 PST
Radar WebKit Bug Importer
Comment 2 2020-02-14 18:23:16 PST
Darin Adler
Comment 3 2020-02-16 22:25:21 PST
Comment on attachment 390794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390794&action=review > Source/WebCore/ChangeLog:3 > + Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after() Why? > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:302 > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC) , dispatch_get_main_queue(), _pendingResolveBoundsBlock); This looks wrong. Since animationDuration is in seconds, but we don’t convert it to nanoseconds. The math is done only on the 0.1. Since it’s wrong I suspect we have no tests for this.
Peng Liu
Comment 4 2020-03-15 11:18:03 PDT
Comment on attachment 390794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390794&action=review >> Source/WebCore/ChangeLog:3 >> + Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after() > > Why? This patch was an attempt to align the implementation of WebAVPlayerLayer with WKVideoLayerRemote, which was added for "media in GPU Process". @thorton suggested we replace performSelector with other mechanism. >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:302 >> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC) , dispatch_get_main_queue(), _pendingResolveBoundsBlock); > > This looks wrong. Since animationDuration is in seconds, but we don’t convert it to nanoseconds. The math is done only on the 0.1. > > Since it’s wrong I suspect we have no tests for this. You are right. Sorry for the mistake. There is no test for this animation code, unfortunately.
Darin Adler
Comment 5 2020-03-15 12:39:20 PDT
Comment on attachment 390794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390794&action=review >>> Source/WebCore/ChangeLog:3 >>> + Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after() >> >> Why? > > This patch was an attempt to align the implementation of WebAVPlayerLayer with WKVideoLayerRemote, which was added for "media in GPU Process". @thorton suggested we replace performSelector with other mechanism. OK, thanks. That’s the kind of thing a change log needs to mention. We can use source code management tools to find out *what* changed. It’s our job to tell each other *why* it’s changing. That’s the whole point of change logs. I’d go even further and ask Tim Horton why, and write that in the change log too. >>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:302 >>> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC) , dispatch_get_main_queue(), _pendingResolveBoundsBlock); >> >> This looks wrong. Since animationDuration is in seconds, but we don’t convert it to nanoseconds. The math is done only on the 0.1. >> >> Since it’s wrong I suspect we have no tests for this. > > You are right. Sorry for the mistake. > > There is no test for this animation code, unfortunately. Why is there no test? Can we make a test? As long as there’s no test it’s likely we’ll break it with refactoring just like we did here!
Note You need to log in before you can comment on or make changes to this bug.