Created attachment 390794 [details] Patch

<rdar://problem/59480761>

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.

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.

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!