[webkit-reviews] review granted: [Bug 209950] WebCore::HTMLMediaElement::mediaCanStart crashes : [Attachment 395343] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 209950: WebCore::HTMLMediaElement::mediaCanStart crashes
https://bugs.webkit.org/show_bug.cgi?id=209950

Attachment 395343: Patch

https://bugs.webkit.org/attachment.cgi?id=395343&action=review




--- Comment #3 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 395343
  --> https://bugs.webkit.org/attachment.cgi?id=395343
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.