Merge MediaDevicesRequest and MediaDevicesEnumerationRequest to tighten up code and object lifetime
Created attachment 313248 [details] Patch
Instead of isActive I could store the DOMPromise in a std::optional and set it to nullopt after we are done with it or when we cancel. I guess that would be a bit more elegant.
Created attachment 313249 [details] Patch
Comment on attachment 313249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313249&action=review > Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:63 > + auto* controller = UserMediaController::from(document.page()); > + if (!controller) { > + // FIXME: Should we resolve or reject the promise here instead of leaving the website waiting? > + return; > + } The fact that UserMediaController::from can fail seems so opaque. What it is really checking is if there is page. It makes me think we should make UserMediaController::from (and therefore Supplement::from) take a Page& and return a Foo& make the caller check that page is not null instead of the foo. > Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:113 > +#if !PLATFORM(COCOA) > + UNUSED_PARAM(devices); > +#else This doesn't feel like a COCOA not COCOA divide, but rather a ENABLE(ATYPICAL_DEVICE_FILTERING) thing. Actually, it would be better if that were a setting, all things considered, then it could be tested with different values. Thoughts for the future. > Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:139 > + Vector<RefPtr<MediaDeviceInfo>> devices; I would say this should be a Vector<> of Ref<MediaDeviceInfo>, but I would only be impugning myself. I really need to make Ref<> work with the bindings! > Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:150 > + if (id.isEmpty()) > + continue; Seems like id could be computed first and this check could be done directly after. > Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152 > + devices.append(MediaDeviceInfo::create(&document, label, id, groupId, deviceType)); Quite silly that MediaDeviceInfo::create takes the document by pointer.
Comment on attachment 313249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313249&action=review >> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152 >> + devices.append(MediaDeviceInfo::create(&document, label, id, groupId, deviceType)); > > Quite silly that MediaDeviceInfo::create takes the document by pointer. Turns out it doesn’t even need a document pointer. Can this be a structure instead of an object? I don’t remember when we have that kind of choice in IDL.
Committed r218530: <http://trac.webkit.org/changeset/218530>
Comment on attachment 313249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313249&action=review >> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:63 >> + } > > The fact that UserMediaController::from can fail seems so opaque. What it is really checking is if there is page. It makes me think we should make UserMediaController::from (and therefore Supplement::from) take a Page& and return a Foo& make the caller check that page is not null instead of the foo. Done. >> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:113 >> +#else > > This doesn't feel like a COCOA not COCOA divide, but rather a ENABLE(ATYPICAL_DEVICE_FILTERING) thing. Actually, it would be better if that were a setting, all things considered, then it could be tested with different values. Thoughts for the future. Used a #define in the file for clarity. >> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:150 >> + continue; > > Seems like id could be computed first and this check could be done directly after. Done. >>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152 >>> + devices.append(MediaDeviceInfo::create(&document, label, id, groupId, deviceType)); >> >> Quite silly that MediaDeviceInfo::create takes the document by pointer. > > Turns out it doesn’t even need a document pointer. Can this be a structure instead of an object? I don’t remember when we have that kind of choice in IDL. Removed the document argument.
After this patch several mediastream tests have started to crash on Debug builds with Assertion failures: https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r218588%20(1650)/results.html stdio: ASSERTION FAILED: !m_promise /Volumes/Data/slave/sierra-debug/build/Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp(137) : void WebCore::MediaDevicesEnumerationRequest::setDeviceInfo(const Vector<WebCore::CaptureDevice> &, const WTF::String &, bool) 1 0x114c442fd WTFCrash 2 0x10a87ca40 WebCore::MediaDevicesEnumerationRequest::setDeviceInfo(WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool) 3 0x1059aaf57 WebKit::UserMediaPermissionRequestManager::didCompleteMediaDeviceEnumeration(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool) 4 0x105c2c426 WebKit::WebPage::didCompleteMediaDeviceEnumeration(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool) 5 0x105cacd9d void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool), std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>, 0ul, 1ul, 2ul, 3ul>(WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool), std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) 6 0x105cabb28 void IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool), std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>&&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool)) 7 0x105c9ea80 void IPC::handleMessage<Messages::WebPage::DidCompleteMediaDeviceEnumeration, WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool)>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool)) 8 0x105c93b10 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&) 9 0x105c2da7e WebKit::WebPage::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 10 0x105c2dac4 non-virtual thunk to WebKit::WebPage::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 11 0x1055b6dc8 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) 12 0x105e4eb4d WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 13 0x10548e463 IPC::Connection::dispatchMessage(IPC::Decoder&) 14 0x105483b98 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) 15 0x10548ea60 IPC::Connection::dispatchOneMessage() 16 0x1054a6d9d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() 17 0x1054a6cf9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() 18 0x114c72fae WTF::Function<void ()>::operator()() const 19 0x114c92b53 WTF::RunLoop::performWork() 20 0x114c93324 WTF::RunLoop::performWork(void*) 21 0x7fffa88703e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 22 0x7fffa885165c __CFRunLoopDoSources0 23 0x7fffa8850b46 __CFRunLoopRun 24 0x7fffa8850544 CFRunLoopRunSpecific 25 0x7fffa7db0ebc RunCurrentEventLoopInMode 26 0x7fffa7db0cf1 ReceiveNextEventCommon 27 0x7fffa7db0b26 _BlockUntilNextEventMatchingListInModeWithFilter 28 0x7fffa6349a54 _DPSNextEvent 29 0x7fffa6ac57ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] 30 0x7fffa633e3db -[NSApplication run] 31 0x7fffa6308e0e NSApplicationMain LEAK: 14 WebPageProxy
Feel free to roll out. Likely we just need to add an if statement to fix it.
Reverted r218530 for reason: This revision caused multiple media stream test crashes on Debug builds. Committed r218601: <http://trac.webkit.org/changeset/218601>
Maybe I would have seen this on EWS if I had posted the final version of the patch.