[webkit-reviews] review granted: [Bug 221608] Split RemoteRealtimeMediaSource in two audio-specific and video-specific classes : [Attachment 419725] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 221608: Split RemoteRealtimeMediaSource in two audio-specific and
video-specific classes
https://bugs.webkit.org/show_bug.cgi?id=221608

Attachment 419725: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 419725
  --> https://bugs.webkit.org/attachment.cgi?id=419725
Patch

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

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeAudioSource.cpp:131
> +void RemoteRealtimeAudioSource::whenReady(CompletionHandler<void(String)>&&
callback)
> +{
> +    if (m_isReady)
> +	   return callback(WTFMove(m_errorMessage));
> +    m_callback = WTFMove(callback);
> +}
> +
> +void RemoteRealtimeAudioSource::didFail(String&& errorMessage)
> +{
> +    m_isReady = true;
> +    m_errorMessage = WTFMove(errorMessage);
> +    if (m_callback)
> +	   m_callback(m_errorMessage);
> +}
> +
> +void RemoteRealtimeAudioSource::setAsReady()
> +{
> +    m_isReady = true;
> +    if (m_callback)
> +	   m_callback({ });
> +}
> +
> +void
RemoteRealtimeAudioSource::setCapabilities(RealtimeMediaSourceCapabilities&&
capabilities)
> +{
> +    m_capabilities = WTFMove(capabilities);
> +}
> +
> +void RemoteRealtimeAudioSource::setSettings(RealtimeMediaSourceSettings&&
settings)
> +{
> +    auto changed = m_settings.difference(settings);
> +    m_settings = WTFMove(settings);
> +    notifySettingsDidChangeObservers(changed);
> +}
> +

These methods are identical in the Audio and Video classes, can they be moved
into a base class?

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeAudioSource.cpp:182
> +IPC::Connection* RemoteRealtimeAudioSource::connection()
> +{
> +    ASSERT(isMainThread());
> +#if ENABLE(GPU_PROCESS)
> +    if (m_shouldCaptureInGPUProcess)
> +	   return
&WebProcess::singleton().ensureGPUProcessConnection().connection();
> +#endif
> +    return WebProcess::singleton().parentProcessConnection();
> +}
> +
> +void RemoteRealtimeAudioSource::startProducingData()
> +{
> +   
connection()->send(Messages::UserMediaCaptureManagerProxy::StartProducingData {
m_identifier }, 0);
> +}
> +
> +void RemoteRealtimeAudioSource::stopProducingData()
> +{
> +   
connection()->send(Messages::UserMediaCaptureManagerProxy::StopProducingData {
m_identifier }, 0);
> +}
> +
> +const WebCore::RealtimeMediaSourceCapabilities&
RemoteRealtimeAudioSource::capabilities()
> +{
> +    return m_capabilities;
> +}
> +
> +void RemoteRealtimeAudioSource::applyConstraints(const MediaConstraints&
constraints, ApplyConstraintsHandler&& completionHandler)
> +{
> +    m_pendingApplyConstraintsCallbacks.append(WTFMove(completionHandler));
> +    // FIXME: Use sendAsyncWithReply.
> +   
connection()->send(Messages::UserMediaCaptureManagerProxy::ApplyConstraints {
m_identifier, constraints }, 0);
> +}
> +
> +void
RemoteRealtimeAudioSource::applyConstraintsSucceeded(RealtimeMediaSourceSetting
s&& settings)
> +{
> +    setSettings(WTFMove(settings));
> +
> +    auto callback = m_pendingApplyConstraintsCallbacks.takeFirst();
> +    callback({ });
> +}
> +
> +void RemoteRealtimeAudioSource::applyConstraintsFailed(String&&
failedConstraint, String&& errorMessage)
> +{
> +    auto callback = m_pendingApplyConstraintsCallbacks.takeFirst();
> +    callback(ApplyConstraintsError { WTFMove(failedConstraint),
WTFMove(errorMessage) });
> +}

Ditto

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeAudioSource.cpp:215
> +void RemoteRealtimeAudioSource::captureStopped()
> +{
> +    stop();
> +    hasEnded();
> +}
> +
> +void RemoteRealtimeAudioSource::captureFailed()
> +{
> +    RealtimeMediaSource::captureFailed();
> +    hasEnded();
> +}
> +
> +#if ENABLE(GPU_PROCESS)
> +void
RemoteRealtimeAudioSource::gpuProcessConnectionDidClose(GPUProcessConnection&)
> +{
> +    ASSERT(m_shouldCaptureInGPUProcess);
> +    if (isEnded())
> +	   return;
> +
> +    m_manager.remoteCaptureSampleManager().didUpdateSourceConnection(*this);
> +    createRemoteMediaSource();
> +    // FIXME: We should update the track according current settings.
> +    if (isProducingData())
> +	   startProducingData();
> +}

Ditto.

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:118
> +    if (auto source = m_audioSources.get(id))
>	   source->captureStopped();
> +    else if (auto source = m_videoSources.get(id))
> +	   source->captureStopped();

This pattern is used in enough places this it would be good to have a method
that returned the source given an identifier.