[webkit-reviews] review granted: [Bug 216856] MediaRecorder should support isTypeSupported : [Attachment 409884] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 216856: MediaRecorder should support isTypeSupported
https://bugs.webkit.org/show_bug.cgi?id=216856

Attachment 409884: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 409884
  --> https://bugs.webkit.org/attachment.cgi?id=409884
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53
> +    return page ? page->mediaRecorderProvider().isSupported(value) : false;

Clearer with &&?

> Source/WebCore/Modules/mediarecorder/MediaRecorderProvider.cpp:53
> +    auto mimeType = parseMIMEType(value);

If parseMIMEType takes a StringView then I suggest having this function and the
other function that calls it take a StringView and not a String.

> Source/WebCore/Modules/mediarecorder/MediaRecorderProvider.cpp:64
> +    if (!codecs.isEmpty()) {

Don’t need this check. The loop below will already correctly do nothing if the
string is empty.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:92
> +    m_writer->fetchData([completionHandler = WTFMove(completionHandler),
mimeType = this->mimeType()](auto&& buffer) mutable {

The old code didn’t need “this->”. Why do we need to add it now?

> Source/WebCore/platform/network/HTTPParsers.h:114
> +struct MimeType {

I would call this ParsedMIMEType.

Typically a MIMEType is just a String so I think that name is clearer.

> Source/WebCore/platform/network/HTTPParsers.h:121
> +Optional<MimeType> parseMIMEType(const String&);

Can this take a StringView instead of a String?

> Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:129
> +   
m_connection->sendWithAsyncReply(Messages::RemoteMediaRecorder::FetchData { },
[completionHandler = WTFMove(completionHandler), mimeType =
this->mimeType()](auto&& data) mutable {

I don’t think we need the “this->” here.