Bug 61539

Created attachment 94989 [details] Patch

(In reply to comment #1) > Created an attachment (id=94989) [details] > Patch Just a few comments. View in context: https://bugs.webkit.org/attachment.cgi?id=94989&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This will make the commit queue bots to abort your patch. Since the test framework for the MediaStream API is not ready yet, you should point that tests for this functionality will be provided by bug 56587 and make sure to include later some tests checking this. > Source/WebCore/dom/StreamList.cpp:41 > + m_streams = streams; Since this is a constructor you should use initialization lists. > Source/WebCore/dom/StreamList.cpp:57 > + return PassRefPtr<Stream>(); I have a limited knowledge about Javascript arrays but, is there or there should be any exception throwing to be managed here?

(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=94989) [details] [details] > > Patch > > Just a few comments. > > View in context: https://bugs.webkit.org/attachment.cgi?id=94989&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > This will make the commit queue bots to abort your patch. Since the test framework for the MediaStream API is not ready yet, you should point that tests for this functionality will be provided by bug 56587 and make sure to include later some tests checking this. Done. > > Source/WebCore/dom/StreamList.cpp:41 > > + m_streams = streams; > > Since this is a constructor you should use initialization lists. Done. > > Source/WebCore/dom/StreamList.cpp:57 > > + return PassRefPtr<Stream>(); > > I have a limited knowledge about Javascript arrays but, is there or there should be any exception throwing to be managed here? No, according to the ecmascript standard one should just return undefined aka null.

Created attachment 95338 [details] Patch

Created attachment 96409 [details] Patch

Comment on attachment 96409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96409&action=review > Source/WebCore/dom/StreamContainer.h:50 > + if (j == index) Does this assume that HashMap guarantees iteration in the same order the items were added? If so, maybe it should be a Vector instead? What other properties does this container need to exhibit (e.g. can a stream be added multiple times)? It is hard to tell exactly how this will be used, but I'm wondering if this container class is really necessary at all or if the StreamList should just typedef the appropriate wtf container.

Comment on attachment 96409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96409&action=review >> Source/WebCore/dom/StreamContainer.h:50 >> + if (j == index) > > Does this assume that HashMap guarantees iteration in the same order the items were added? If so, maybe it should be a Vector instead? What other properties does this container need to exhibit (e.g. can a stream be added multiple times)? > > It is hard to tell exactly how this will be used, but I'm wondering if this container class is really necessary at all or if the StreamList should just typedef the appropriate wtf container. As far as I understand we just need to be able to iterate over the data, and I choose a hashmap since that makes the other operations much more efficient. A stream can't be added more than once, I probably should add an assert for that here instead of just checking in PeerConnection. The StreamList and PeerConnection needs to share the container, the spec explicitly states that the StreamList is "live". I choose this approach since it makes clear that they share the list, but it can be refactored by merging StreamList and StreamContainer into the same class.

Comment on attachment 96409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96409&action=review >>> Source/WebCore/dom/StreamContainer.h:50 >>> + if (j == index) >> >> Does this assume that HashMap guarantees iteration in the same order the items were added? If so, maybe it should be a Vector instead? What other properties does this container need to exhibit (e.g. can a stream be added multiple times)? >> >> It is hard to tell exactly how this will be used, but I'm wondering if this container class is really necessary at all or if the StreamList should just typedef the appropriate wtf container. > > As far as I understand we just need to be able to iterate over the data, and I choose a hashmap since that makes the other operations much more efficient. A stream can't be added more than once, I probably should add an assert for that here instead of just checking in PeerConnection. > > The StreamList and PeerConnection needs to share the container, the spec explicitly states that the StreamList is "live". I choose this approach since it makes clear that they share the list, but it can be refactored by merging StreamList and StreamContainer into the same class. Where is StreamList specced? I don't see it in the whatwg spec.

Comment on attachment 96409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96409&action=review >>>> Source/WebCore/dom/StreamContainer.h:50 >>>> + if (j == index) >>> >>> Does this assume that HashMap guarantees iteration in the same order the items were added? If so, maybe it should be a Vector instead? What other properties does this container need to exhibit (e.g. can a stream be added multiple times)? >>> >>> It is hard to tell exactly how this will be used, but I'm wondering if this container class is really necessary at all or if the StreamList should just typedef the appropriate wtf container. >> >> As far as I understand we just need to be able to iterate over the data, and I choose a hashmap since that makes the other operations much more efficient. A stream can't be added more than once, I probably should add an assert for that here instead of just checking in PeerConnection. >> >> The StreamList and PeerConnection needs to share the container, the spec explicitly states that the StreamList is "live". I choose this approach since it makes clear that they share the list, but it can be refactored by merging StreamList and StreamContainer into the same class. > > Where is StreamList specced? I don't see it in the whatwg spec. It isn't speced as such, if you look at the PeerConnection idl you see this: readonly attribute Stream[] localStreams; readonly attribute Stream[] remoteStreams; To implement these "simple" arrays one needs to create a new class I learned by looking at the existing code. See NodeList etc.

Created attachment 97094 [details] Patch

Comment on attachment 97094 [details] Patch Clearing flags on attachment: 97094 Committed r88798: <http://trac.webkit.org/changeset/88798>

All reviewed patches have been landed. Closing bug.