Refactor: Allow WebKit2 to override the creation of RealtimeMediaSources
Created attachment 303574 [details] Patch
Comment on attachment 303574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303574&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:77 > + if (m_audioFactory == &factory) > + m_audioFactory = nullptr; Nit: is it ever legal for factory to not be m_audioFactory? If not, ASSERT(m_audioFactory == &factory) would be helpful > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:88 > + if (m_videoFactory == &factory) > + m_videoFactory = nullptr; Ditto. > Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:84 > + if (device && captureDevice.type() == CaptureDevice::DeviceType::Audio) It should not be possible to get a non-null device of the wrong type, so ASSERT(!device || (device && captureDevice.type() == CaptureDevice::DeviceType::Audio)) > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:112 > + AVCaptureDeviceTypedef *device = [getAVCaptureDeviceClass() deviceWithUniqueID:captureDevice.persistentId()]; > + if (device && captureDevice.type() == CaptureDevice::DeviceType::Video) Ditto.
Comment on attachment 303574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303574&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:86 > + class Factory { This does not apply to all realtime sources but only to capture. How about renaming it to CaptureFactory? > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:69 > + virtual RealtimeMediaSource::Factory& defaultVideoFactory() = 0; Is it needed to have two factories, one for audio and another for video? Can't we have a single capture factory that would handle audio and video? The factory class could have two methods instead of one. It may also contain the default implementation and WebKit2 would override it if needed. > Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:86 > + return nullptr; If you are touching this again, I would try to "return nullptr" first.
(In reply to comment #2) > Comment on attachment 303574 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303574&action=review > > > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:77 > > + if (m_audioFactory == &factory) > > + m_audioFactory = nullptr; > > Nit: is it ever legal for factory to not be m_audioFactory? If not, > ASSERT(m_audioFactory == &factory) would be helpful Imagine a scenario where one client sets a factory to Foo, and another client sets it to Bar. Then the first client tries to unset the factory (which they believe is Foo, but is really Bar). We should probably only unset the factory if it hasn't changed in the meanwhile, which is the intention of this patch. > > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:88 > > + if (m_videoFactory == &factory) > > + m_videoFactory = nullptr; > > Ditto. > > > Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:84 > > + if (device && captureDevice.type() == CaptureDevice::DeviceType::Audio) > > It should not be possible to get a non-null device of the wrong type, so > ASSERT(!device || (device && captureDevice.type() == > CaptureDevice::DeviceType::Audio)) Ok. > > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:112 > > + AVCaptureDeviceTypedef *device = [getAVCaptureDeviceClass() deviceWithUniqueID:captureDevice.persistentId()]; > > + if (device && captureDevice.type() == CaptureDevice::DeviceType::Video) > > Ditto.
(In reply to comment #3) > Comment on attachment 303574 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303574&action=review > > > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:86 > > + class Factory { > > This does not apply to all realtime sources but only to capture. > How about renaming it to CaptureFactory? > > > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:69 > > + virtual RealtimeMediaSource::Factory& defaultVideoFactory() = 0; > > Is it needed to have two factories, one for audio and another for video? > Can't we have a single capture factory that would handle audio and video? > The factory class could have two methods instead of one. Then overriding only creating of audio or video streams (which is the primary use case) becomes much more complicated. > It may also contain the default implementation and WebKit2 would override it > if needed. The default behavior won't never be in the base class, since it's platform dependent. > > Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:86 > > + return nullptr; > > If you are touching this again, I would try to "return nullptr" first.
Created attachment 304358 [details] Patch for landing One more revision to get the GTK EWS bot happy.
Created attachment 304393 [details] Patch for landing
Created attachment 304398 [details] Patch for landing
Created attachment 304402 [details] Patch for landing
Created attachment 304406 [details] Patch for landing
Comment on attachment 304406 [details] Patch for landing Attachment 304406 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3323337 New failing tests: imported/w3c/web-platform-tests/cors/status.htm
Created attachment 304416 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Committed r213941: <http://trac.webkit.org/changeset/213941>