[webkit-reviews] review granted: [Bug 70896] MediaStreamRegistry should hold references to MediaStreamDescriptor rather than MediaStream : [Attachment 113327] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 70896: MediaStreamRegistry should hold references to MediaStreamDescriptor
rather than MediaStream
https://bugs.webkit.org/show_bug.cgi?id=70896

Attachment 113327: Patch
https://bugs.webkit.org/attachment.cgi?id=113327&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113327&action=review


> Source/WebCore/mediastream/MediaStreamRegistry.cpp:50
> +    m_streamDescriptors.set(url.string(), streamDescriptor);

streamDescriptor.release(), presumably.

> Source/WebCore/mediastream/MediaStreamRegistry.cpp:59
> -MediaStream* MediaStreamRegistry::mediaStream(const KURL& url) const
> +PassRefPtr<MediaStreamDescriptor>
MediaStreamRegistry::lookupMediaStreamDescriptor(const String& key)

The key isn't a URL?

Also, this should just return a raw pointer.  The function doesn't transfer
ownership.  If you haven't read <http://www.webkit.org/coding/RefPtr.html>, it
might help you with this sort of thing.

> Source/WebCore/mediastream/MediaStreamRegistry.h:49
> +    PassRefPtr<MediaStreamDescriptor> lookupMediaStreamDescriptor(const
String& key);

I don't see any callers of this function in this patch.  I'm not sure whether
it should take a KURL or whether it should take a String named url, but the
keys do seem to be URLs.  Maybe we should wait to add this until its needed?