[webkit-reviews] review granted: [Bug 205802] Implement MediaRecorder backend in GPUProcess : [Attachment 386961] 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 205802: Implement MediaRecorder backend in GPUProcess
https://bugs.webkit.org/show_bug.cgi?id=205802

Attachment 386961: Patch

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




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

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

> Source/WebCore/testing/Internals.cpp:555
> +    page.mediaRecorderProvider().setUseGPUProcess(true);

Is this the correct default?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:153
> +#if PLATFORM(COCOA) && USE(LIBWEBRTC)

Should this be "#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM"?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:181
> +    if (decoder.messageReceiverName() ==
Messages::RemoteMediaRecorders::messageReceiverName()) {
> +	   mediaRecorders().didReceiveMessageFromWebProcess(connection,
decoder);
> +	   return;
> +    }
> +    if (decoder.messageReceiverName() ==
Messages::RemoteMediaRecorder::messageReceiverName()) {

"RemoteMediaRecorders" is a bit subtle and easy to confuse with
"RemoteMediaRecorder". Maybe something like "RemoteMediaRecorderManager"
instead?

> Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:52
> +    // FIXME: We would better to throw an exception to JavaScript if writer
creation fails.

s/We would better to throw an exception/We should throw an exception/