Update the MediaStreamRegistry to use MediaStreamDescriptor instead of MediaStream.
Created attachment 113327 [details] Patch
Adam, hope you don't mind me taking over this bug...
Comment on attachment 113327 [details] Patch 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?
Comment on attachment 113327 [details] Patch 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. Fixed. >> Source/WebCore/mediastream/MediaStreamRegistry.cpp:59 >> +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. Sorry, i do this misstake now and then :/ This class will be extended to index MediaStream IDs as well as URLs in the next step, but I have changed the parameter name to url for now. >> 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? This will be called from the WebKit API that I will (re)add as soon as this patch goes in.
Created attachment 113330 [details] Patch
Comment on attachment 113330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113330&action=review > Source/WebCore/ChangeLog:14 > + Tests for the Media Stream API will be provided by the bug 56587, pending enough landed code. Usually we put this information above the list of files/function names. > Source/WebCore/mediastream/MediaStreamRegistry.cpp:46 > - m_streams.add(url.string(), stream); > + m_streamDescriptors.set(url.string(), stream->descriptor()); You wrote "fixed", but this doesn't appear to have changed. :)
Comment on attachment 113330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113330&action=review >> Source/WebCore/ChangeLog:14 >> + Tests for the Media Stream API will be provided by the bug 56587, pending enough landed code. > > Usually we put this information above the list of files/function names. OK >> Source/WebCore/mediastream/MediaStreamRegistry.cpp:46 >> + m_streamDescriptors.set(url.string(), stream->descriptor()); > > You wrote "fixed", but this doesn't appear to have changed. :) But I don't store the descriptor in a RefPtr any more... Just passing a raw pointer. Realized that a stream always have a descriptor.
Created attachment 113334 [details] Patch
Comment on attachment 113334 [details] Patch Ah, you're right.
Comment on attachment 113334 [details] Patch Clearing flags on attachment: 113334 Committed r99087: <http://trac.webkit.org/changeset/99087>
All reviewed patches have been landed. Closing bug.