Bug 211583

Created attachment 398761 [details]
Patch

Comment on attachment 398761 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Move UserMediaCaptureManager::Source in its own file and rename it to RemoveRealtimeMediaSource.

s/RemoveRealtimeMediaSource/RemoteRealtimeMediaSource/

> Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:63
> +    dispatchToThread([this, source = WTFMove(source)]() mutable {

Should we assert that this is called on the main thread?

> Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:74
> +    dispatchToThread([this, identifier] {

Ditto

> Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:127
> +    WebAudioBufferList audioData(source.description, numberOfFrames);

It would be more efficient to to have this in an instance variable, which gets reset when necessary

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSource.cpp:29
> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)

We're going to have to split this into a base class and a Cocoa-specific derived class at some point.

Created attachment 398805 [details]
Patch

Comment on attachment 398805 [details]
Patch

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

r=me once the bots are happy.

> Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.h:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2020

> Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.messages.in:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

2020

Created attachment 398844 [details]
Patch for landing

Need to land https://bugs.webkit.org/show_bug.cgi?id=211509 to fix iOS compilation errors first

Thanks for the review.

(In reply to Eric Carlson from comment #2)
> Comment on attachment 398761 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398761&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        Move UserMediaCaptureManager::Source in its own file and rename it to RemoveRealtimeMediaSource.
> 
> s/RemoveRealtimeMediaSource/RemoteRealtimeMediaSource/

Done

> > Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:63
> > +    dispatchToThread([this, source = WTFMove(source)]() mutable {
> 
> Should we assert that this is called on the main thread?

This is not strictly needed but does not hurt since that is the spirit of the model.
Added.

> > Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:74
> > +    dispatchToThread([this, identifier] {
> 
> Ditto

Ditto.

> > Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:127

I renamed AudioCaptureSampleManager to RemoteCaptureSampleManager since we will want to do the same thing for video content.

> > +    WebAudioBufferList audioData(source.description, numberOfFrames);
> 
> It would be more efficient to to have this in an instance variable, which
> gets reset when necessary

Right! Added a FIXME.

> > Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSource.cpp:29
> > +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)
> 
> We're going to have to split this into a base class and a Cocoa-specific
> derived class at some point.

Right, I think splitting the class helps, UserMediaCaptureManager should no longer be cocoa specific.

As of RemoteRealtimeMediaSource, we might be able to keep it platform agnostic but make its RemoteAudio subclass platform specific in the future.

Committed r261375: <https://trac.webkit.org/changeset/261375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398844 [details].

<rdar://problem/63018424>