Make sure MediaRecorder.requestData returns data with the new writer backend
Created attachment 389134 [details] Patch
*** Bug 212275 has been marked as a duplicate of this bug. ***
<rdar://problem/63601261>
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.