Bug 207782 - Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after()
Summary: Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceA...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-14 12:06 PST by Peng Liu
Modified: 2020-03-15 12:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.24 KB, patch)
2020-02-14 12:08 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-02-14 12:06:05 PST
Replacing performSelector:withObject:afterDelay: in VideoFullscreenInterfaceAVKit with dispatch_after()
Comment 1 Peng Liu 2020-02-14 12:08:14 PST
Created attachment 390794 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-02-14 18:23:16 PST
<rdar://problem/59480761>
Comment 3 Darin Adler 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.
Comment 4 Peng Liu 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.
Comment 5 Darin Adler 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!