Summary: | Make sure MediaRecorder.requestData returns data with the new writer backend | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, eric.carlson, ews-watchlist, glenn, jer.noble, majo, philipj, sergio, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
youenn fablet
2020-01-29 07:15:36 PST
Created attachment 389134 [details]
Patch
*** Bug 212275 has been marked as a duplicate of this bug. *** Created attachment 401889 [details]
Adding deprecation macros
Comment on attachment 401889 [details] Adding deprecation macros View in context: https://bugs.webkit.org/attachment.cgi?id=401889&action=review > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:489 > + ALLOW_DEPRECATED_DECLARATIONS_BEGIN > + [m_writer flush]; > + ALLOW_DEPRECATED_DECLARATIONS_END Do we have to write code using a deprecated function? What’s the long term strategy for the deprecation? Why can’t we take advantage of it now? > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496 > + RefPtr<SharedBuffer> buffer; > + if (weakThis) > + buffer = WTFMove(m_data); > + > + completionHandler(WTFMove(buffer)); How about this version? if (!weakThis) return completionHandler({ }); completionHandler(WTFMove(m_data)); Seems easier to read than the one with the local variable and double WTFMove? And has the same early exit here as above. I probably would have written it with nullptr and without using a "void return", but I figured we could stay consistent. (In reply to Darin Adler from comment #5) > Comment on attachment 401889 [details] > Adding deprecation macros > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401889&action=review > > > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:489 > > + ALLOW_DEPRECATED_DECLARATIONS_BEGIN > > + [m_writer flush]; > > + ALLOW_DEPRECATED_DECLARATIONS_END > > Do we have to write code using a deprecated function? What’s the long term > strategy for the deprecation? Why can’t we take advantage of it now? The replacement method is not available yet everywhere. Plan is to switch at some point. > > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496 > > + RefPtr<SharedBuffer> buffer; > > + if (weakThis) > > + buffer = WTFMove(m_data); > > + > > + completionHandler(WTFMove(buffer)); > > How about this version? > > if (!weakThis) > return completionHandler({ }); > > completionHandler(WTFMove(m_data)); > > Seems easier to read than the one with the local variable and double > WTFMove? And has the same early exit here as above. I probably would have > written it with nullptr and without using a "void return", but I figured we > could stay consistent. I wrote the double move to ensure m_data gets cleared. I'll do the early return. Created attachment 401986 [details]
Patch for landing
Went with sd::exchange Committed r263084: <https://trac.webkit.org/changeset/263084> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401986 [details]. Comment on attachment 401889 [details] Adding deprecation macros View in context: https://bugs.webkit.org/attachment.cgi?id=401889&action=review >>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496 >>> + completionHandler(WTFMove(buffer)); >> >> How about this version? >> >> if (!weakThis) >> return completionHandler({ }); >> >> completionHandler(WTFMove(m_data)); >> >> Seems easier to read than the one with the local variable and double WTFMove? And has the same early exit here as above. I probably would have written it with nullptr and without using a "void return", but I figured we could stay consistent. > > I wrote the double move to ensure m_data gets cleared. > I'll do the early return. If you want to ensure m_data gets cleared, then we should write std::exchange(m_data, {}) instead of WTFMove(m_data). (In reply to Darin Adler from comment #10) > Comment on attachment 401889 [details] > Adding deprecation macros > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401889&action=review > > >>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496 > >>> + completionHandler(WTFMove(buffer)); > >> > >> How about this version? > >> > >> if (!weakThis) > >> return completionHandler({ }); > >> > >> completionHandler(WTFMove(m_data)); > >> > >> Seems easier to read than the one with the local variable and double WTFMove? And has the same early exit here as above. I probably would have written it with nullptr and without using a "void return", but I figured we could stay consistent. > > > > I wrote the double move to ensure m_data gets cleared. > > I'll do the early return. > > If you want to ensure m_data gets cleared, then we should write > std::exchange(m_data, {}) instead of WTFMove(m_data). Yes, that is what I ended up landing. |