Summary: | MediaRecorder should support isTypeSupported | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cdumez, clopez, darin, esprehn+autocc, ews-watchlist, glenn, hta, jer.noble, kondapallykalyan, philipj, sergio, tommyw, webkit-bug-importer, youennf, zach | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 85851, 215018 | ||||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2020-09-22 15:57:11 PDT
Created attachment 409883 [details]
Patch
Created attachment 409884 [details]
Patch
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. 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? Created attachment 409999 [details]
Patch
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 ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. Created attachment 410100 [details]
Patch for landing
Created attachment 410101 [details]
Patch for landing
> > 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 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!. Created attachment 410112 [details]
Patch for landing
Committed r267788: <https://trac.webkit.org/changeset/267788> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410112 [details]. Reverted r267788 for reason: Broke internal builds. Committed r267798: <https://trac.webkit.org/changeset/267798> Created attachment 410208 [details]
Patch for landing
Committed r267825: <https://trac.webkit.org/changeset/267825> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410208 [details]. *** Bug 217280 has been marked as a duplicate of this bug. *** |