[webkit-reviews] review granted: [Bug 234029] [macOS] Add new screen and window capture backend : [Attachment 446623] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 9 16:08:25 PST 2021


Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 234029: [macOS] Add new screen and window capture backend
https://bugs.webkit.org/show_bug.cgi?id=234029

Attachment 446623: Patch

https://bugs.webkit.org/attachment.cgi?id=446623&action=review




--- Comment #10 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 446623
  --> https://bugs.webkit.org/attachment.cgi?id=446623
Patch

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

r=me with a nit and a couple of questions.

> Source/WebCore/platform/mediastream/MediaConstraints.h:576
> -	   if (!m_exact.isEmpty())
> +	   if (m_exact.isEmpty())

!!

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:170
> +#if HAVE(SCREEN_CAPTURE_KIT)
> +inline bool RealtimeMediaSourceCenter::useScreenCaptureKit() const
> +{
> +    return m_useScreenCaptureKit;
> +}
> +
> +inline void RealtimeMediaSourceCenter::setUseScreenCaptureKit(bool
useScreenCaptureKit)
> +{
> +    m_useScreenCaptureKit = useScreenCaptureKit;
> +}
> +#endif
> +

Minor nit: it would be slightly less screen real estate to just put these
definitions inline in the class definition.

> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.h:64
> +class CapturerObserver : public CanMakeWeakPtr<CapturerObserver> {
> +public:
> +    virtual ~CapturerObserver() = default;
> +
> +    virtual void capturerIsRunningChanged(bool) { }
> +    virtual void capturerFailed() { };
> +};
> +
> +class DisplayCaptureSourceCocoa final
> +    : public RealtimeMediaSource
> +    , public CapturerObserver {

I'm having a hard time understanding why the CapturerObserver class is
necessary. It seems to only be accessed within DisplayCaptureSourceCocoa. Can't
the Capturer just reference DisplayCaptureSourceCocoa directly?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:91
> +    static void forEachNSWindow(const Function<bool(NSDictionary *,
unsigned, const String&)>&);

Nit: static privates can just be free functions in the implementation file.

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:543
> +    RetainPtr<NSArray> windowList = adoptNS((__bridge NSArray
*)CGWindowListCopyWindowInfo(kCGWindowListOptionOnScreenOnly |
kCGWindowListExcludeDesktopElements, kCGNullWindowID));

How expensive is this call? Should we consider caching the answer? I guess
invalidation would be a big problem.


More information about the webkit-reviews mailing list