Bug 216856 – MediaRecorder should support isTypeSupported

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

MediaRecorder should support isTypeSupported.

[youenn fablet]

Created attachment 409883 [details]
Patch

[youenn fablet]

Created attachment 409884 [details]
Patch

[Darin Adler]

Comment on attachment 409884 [details]
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.

[Eric Carlson]

Comment on attachment 409884 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.cpp:999
> +// https://mimesniff.spec.whatwg.org/#parsing-a-mime-type
> +Optional<MimeType> parseMIMEType(const String& contentType)

The `ContentType` class used extensively in other media code also has a MIME type parser. We should figure out how to share code.

> Source/WebCore/platform/network/HTTPParsers.h:117
> +struct MimeType {
> +    String type;
> +    String subtype;
> +    HashMap<String, String> parameters;

Could you use ContentType?

[youenn fablet]

Created attachment 409999 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Radar WebKit Bug Importer]

<rdar://problem/69767695>

[EWS]

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

[youenn fablet]

Created attachment 410100 [details]
Patch for landing

[youenn fablet]

Created attachment 410101 [details]
Patch for landing

[youenn fablet]

> > Source/WebCore/platform/network/HTTPParsers.h:117
> > +struct MimeType {
> > +    String type;
> > +    String subtype;
> > +    HashMap<String, String> parameters;
> 
> Could you use ContentType?

I migrated to ContentType.

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

OK

> > 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.

OK

> > 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?

Removed

[EWS]

Downloading setuptools-44.1.1...
Installing setuptools-44.1.1...
Installed setuptools-44.1.1!
Downloading chardet-3.0.4...
Installing chardet-3.0.4...
Installed chardet-3.0.4!
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[youenn fablet]

Created attachment 410112 [details]
Patch for landing

[EWS]

Committed r267788: <https://trac.webkit.org/changeset/267788>

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

[Ryan Haddad]

Reverted r267788 for reason:

Broke internal builds.

Committed r267798: <https://trac.webkit.org/changeset/267798>

[youenn fablet]

Created attachment 410208 [details]
Patch for landing

[EWS]

Committed r267825: <https://trac.webkit.org/changeset/267825>

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

[Eric Carlson]

*** Bug 217280 has been marked as a duplicate of this bug. ***