Bug 236531 - [macOS] Use system window and screen picker when available
Summary: [macOS] Use system window and screen picker when available
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-11 15:24 PST by Eric Carlson
Modified: 2022-02-14 09:59 PST (History)
13 users (show)

See Also:


Attachments
Patch (49.19 KB, patch)
2022-02-11 16:24 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (1.51 KB, patch)
2022-02-12 13:28 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2022-02-11 15:24:30 PST
Use system window and screen picker when available
Comment 1 Eric Carlson 2022-02-11 15:28:56 PST
rdar://87111816
Comment 2 Eric Carlson 2022-02-11 16:24:24 PST
Created attachment 451757 [details]
Patch
Comment 3 Jer Noble 2022-02-11 16:33:17 PST
Comment on attachment 451757 [details]
Patch

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

r=me with nit

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:151
> +    RunLoop::main().dispatch([self, strongSelf = RetainPtr { self }, session = RetainPtr { session }]() mutable {

We shouldn't use RunLoop::main().dispatch() from WebCore code, because it will dispatch to the wrong thread on iOS/WK1. Just use runOnMainThread(). It's a known problem that this pattern is super confusing; apologies!

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:159
> +    RunLoop::main().dispatch([self, strongSelf = RetainPtr { self }, session = RetainPtr { session }]() mutable {

Ditto.
Comment 4 EWS 2022-02-12 08:10:04 PST
Committed r289696 (247181@main): <https://commits.webkit.org/247181@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451757 [details].
Comment 5 Eric Carlson 2022-02-12 13:28:05 PST
Reopening to attach new patch.
Comment 6 Eric Carlson 2022-02-12 13:28:07 PST
Created attachment 451793 [details]
Patch
Comment 7 EWS 2022-02-12 14:59:00 PST
Committed r289701 (247186@main): <https://commits.webkit.org/247186@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451793 [details].
Comment 8 Geoffrey Garen 2022-02-14 09:59:58 PST
> > Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:151
> > +    RunLoop::main().dispatch([self, strongSelf = RetainPtr { self }, session = RetainPtr { session }]() mutable {
> 
> We shouldn't use RunLoop::main().dispatch() from WebCore code, because it
> will dispatch to the wrong thread on iOS/WK1. Just use runOnMainThread().
> It's a known problem that this pattern is super confusing; apologies!

Would this review comment be easier to understand if we renamed callOnMainThread to callOnWebCoreThread (or similar)? (I have been low-key wanting to rename that function for a few years, but not sure if the proposed new name would help most people or not.)