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
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-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.)