Refactoring: Remove AudioSourceObserverObjC and AudioCaptureSourceProviderObjC
Created attachment 301937 [details] Patch
Created attachment 301940 [details] Patch
Created attachment 301952 [details] Patch
Created attachment 301956 [details] Patch
Comment on attachment 301956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301956&action=review > Source/WebCore/ChangeLog:17 > + a lot of unnecessary conversion code and observer duplication. Very nice! > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:63 > : m_captureSource(&source) Are we nullifying m_captureSource or can we make it a reference? > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225 > + if (!m_ringBuffer || !is<WebAudioBufferList>(data)) Can data be something else than a WebAudioBufferList? If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if statement? > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:230 > + m_ringBuffer->store(bufferList.list(), frameCount, m_writeCount); IIRC, bufferList.list() is this ugly audio structure. Maybe we should try to limit its usage in the API or have a sanitising wrapper for it.
(In reply to comment #5) > Comment on attachment 301956 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301956&action=review > > > Source/WebCore/ChangeLog:17 > > + a lot of unnecessary conversion code and observer duplication. > > Very nice! > > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:63 > > : m_captureSource(&source) > > Are we nullifying m_captureSource or can we make it a reference? We are not, but we probably should be in unprepare(). > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225 > > + if (!m_ringBuffer || !is<WebAudioBufferList>(data)) > > Can data be something else than a WebAudioBufferList? > If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if > statement? Right now, it can be nothing other than a WebAudioBufferList. So we could. > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:230 > > + m_ringBuffer->store(bufferList.list(), frameCount, m_writeCount); > > IIRC, bufferList.list() is this ugly audio structure. > Maybe we should try to limit its usage in the API or have a sanitising > wrapper for it. We can't, because this API touches the non-mac/cocoa code, ad PlatformAudioData is the platform-independent way of passing these around.
(In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 301956 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=301956&action=review > > > > > Source/WebCore/ChangeLog:17 > > > + a lot of unnecessary conversion code and observer duplication. > > > > Very nice! > > > > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:63 > > > : m_captureSource(&source) > > > > Are we nullifying m_captureSource or can we make it a reference? > > We are not, but we probably should be in unprepare(). > > > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225 > > > + if (!m_ringBuffer || !is<WebAudioBufferList>(data)) > > > > Can data be something else than a WebAudioBufferList? > > If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if > > statement? > > Right now, it can be nothing other than a WebAudioBufferList. So we could. > > > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:230 > > > + m_ringBuffer->store(bufferList.list(), frameCount, m_writeCount); > > > > IIRC, bufferList.list() is this ugly audio structure. > > Maybe we should try to limit its usage in the API or have a sanitising > > wrapper for it. > > We can't, because this API touches the non-mac/cocoa code, ad > PlatformAudioData is the platform-independent way of passing these around. I'll take care of these ASSERT additions and adding things to unprepared() in an upcoming patch.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > Comment on attachment 301956 [details] > > > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225 > > > > + if (!m_ringBuffer || !is<WebAudioBufferList>(data)) > > > > > > Can data be something else than a WebAudioBufferList? > > > If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if > > > statement? > > > > Right now, it can be nothing other than a WebAudioBufferList. So we could. It turns out that downcast<> already has an ASSERT_WITH_SECURITY_IMPLICATION() so I think all that's left to do here is remove the " || !is<...>" part.
Comment on attachment 301956 [details] Patch Clearing flags on attachment: 301956 Committed r212572: <http://trac.webkit.org/changeset/212572>
All reviewed patches have been landed. Closing bug.