Bug 209950

<rdar://problem/49677331>

Created attachment 395343 [details] Patch

Comment on attachment 395343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395343&action=review r=me, with nits. > Source/WebCore/dom/Document.cpp:6426 > void Document::addMediaCanStartListener(MediaCanStartListener& listener) > { > - ASSERT(!m_mediaCanStartListeners.contains(&listener)); > - m_mediaCanStartListeners.add(&listener); > + ASSERT(!m_mediaCanStartListeners.contains(listener)); > + m_mediaCanStartListeners.add(listener); The way I'd do this would be: void Document::addMediaCanStartListener(WeakPtr<MediaCanStartListener>&& listener) { ASSERT(!m_mediaCanStartListeners.contains(listener)); m_mediaCanStartListeners.add(WTFMove(listener)); } This way, MediaCanStartListener doesn't need to directly inherit from CanMakeWeakPtr. > Source/WebCore/dom/Document.cpp:6437 > + if (!m_mediaCanStartListeners.computeSize()) I think this could be: `if (m_mediaCanStartListeners.computeEmpty())`. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:37 > -class UserMediaPermissionRequestManager : public CanMakeWeakPtr<UserMediaPermissionRequestManager>, private WebCore::MediaCanStartListener { > +class UserMediaPermissionRequestManager : private WebCore::MediaCanStartListener { Then this could get reverted. It would mean all the call sites to addMediaCanStartListener() would have to change, though.

Comment on attachment 395343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395343&action=review >> Source/WebCore/dom/Document.cpp:6426 >> + m_mediaCanStartListeners.add(listener); > > The way I'd do this would be: > > void Document::addMediaCanStartListener(WeakPtr<MediaCanStartListener>&& listener) > { > ASSERT(!m_mediaCanStartListeners.contains(listener)); > m_mediaCanStartListeners.add(WTFMove(listener)); > } > > This way, MediaCanStartListener doesn't need to directly inherit from CanMakeWeakPtr. That's a good idea! Unfortunately it requires us to add WeakHashSet::add(WeakPtr<U>&& value) to the current implementation. >> Source/WebCore/dom/Document.cpp:6437 >> + if (!m_mediaCanStartListeners.computeSize()) > > I think this could be: `if (m_mediaCanStartListeners.computeEmpty())`. Yes, that will work. I chose to use computeSize() because it will clean up nullptrs. >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:37 >> +class UserMediaPermissionRequestManager : private WebCore::MediaCanStartListener { > > Then this could get reverted. > > It would mean all the call sites to addMediaCanStartListener() would have to change, though. Maybe we can do it later after extending WeakHashSet.

Created attachment 395349 [details] Patch for landing (replace computeSize() with computesEmpty())

Committed r259447: <https://trac.webkit.org/changeset/259447> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395349 [details].