Bug 236422 - [macOS] Support both versions of ScreenCaptureKit API
Summary: [macOS] Support both versions of ScreenCaptureKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-09 18:14 PST by Eric Carlson
Modified: 2022-02-10 10:17 PST (History)
9 users (show)

See Also:

Attachments
Patch (20.54 KB, patch)
2022-02-09 19:08 PST, Eric Carlson 		youennf: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (21.27 KB, patch)
2022-02-10 08:45 PST, Eric Carlson 		eric.carlson: commit-queue+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.