Bug 236422

Summary: [macOS] Support both versions of ScreenCaptureKit API
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebRTCAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
youennf: review+, ews-feeder: commit-queue-
Patch for landing eric.carlson: commit-queue+

Description Eric Carlson 2022-02-09 18:14:30 PST 
Support the new and deprecated ScreenCaptureKit API simultaneously so not everyone has to update at the same time.
Comment 1 Radar WebKit Bug Importer 2022-02-09 18:15:02 PST 
<rdar://problem/88726849>
Comment 2 Eric Carlson 2022-02-09 19:08:24 PST 
Created attachment 451481 [details]
Patch
Comment 3 youenn fablet 2022-02-10 05:21:09 PST 
Comment on attachment 451481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451481&action=review

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:95
>      WeakPtr<ScreenCaptureKitCaptureSource> _callback;

Style question: Is it _callback or callback_ or m_callback that we are supposed to use?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:180
> +    m_useNewAPI = [PAL::getSCStreamClass() instancesRespondToSelector:@selector(stopCaptureWithCompletionHandler:)];

, m_useNewAPI(...)

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:211
>      if (m_contentStream) {

We could do if (!m_contentStream) return;

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:447
> +

Not needed
Comment 4 Eric Carlson 2022-02-10 08:45:05 PST 
Created attachment 451544 [details]
Patch for landing
Comment 5 Eric Carlson 2022-02-10 09:14:34 PST 
Comment on attachment 451481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451481&action=review

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:95
>>      WeakPtr<ScreenCaptureKitCaptureSource> _callback;
> 
> Style question: Is it _callback or callback_ or m_callback that we are supposed to use?

As far as I can tell, it is usually `_callback` in ObjC code.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:180
>> +    m_useNewAPI = [PAL::getSCStreamClass() instancesRespondToSelector:@selector(stopCaptureWithCompletionHandler:)];
> 
> , m_useNewAPI(...)

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:211
>>      if (m_contentStream) {
> 
> We could do if (!m_contentStream) return;

Oops, I missed this but will change in an upcoming patch.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:447
>> +
> 
> Not needed

Removed.
Comment 6 EWS 2022-02-10 09:48:34 PST 
Committed r289547 (247078@main): <https://commits.webkit.org/247078@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451544 [details].
Comment 7 Eric Carlson 2022-02-10 10:17:22 PST 
Comment on attachment 451544 [details]
Patch for landing

This code is macOS only, the Windows failure is unrelated.