Bug 61539 - Media Stream PeerConnection API: adding the StreamList and supporting classes.
Summary: Media Stream PeerConnection API: adding the StreamList and supporting classes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on: 56666
Blocks: 56459
  Show dependency treegraph
 
Reported: 2011-05-26 09:29 PDT by Tommy Widenflycht
Modified: 2011-06-14 06:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (25.18 KB, patch)
2011-05-26 09:36 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (25.22 KB, patch)
2011-05-30 05:44 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (25.26 KB, patch)
2011-06-08 05:45 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (26.41 KB, patch)
2011-06-14 04:24 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2011-05-26 09:29:16 PDT
This patch introduces the StreamList class, together with a supporting class, according to the lastest specification.
Comment 1 Tommy Widenflycht 2011-05-26 09:36:20 PDT
Created attachment 94989 [details]
Patch
Comment 2 Leandro Graciá Gil 2011-05-26 11:24:22 PDT
(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?
Comment 3 Tommy Widenflycht 2011-05-30 05:40:42 PDT
(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.
Comment 4 Tommy Widenflycht 2011-05-30 05:44:44 PDT
Created attachment 95338 [details]
Patch
Comment 5 Tommy Widenflycht 2011-06-08 05:45:45 PDT
Created attachment 96409 [details]
Patch
Comment 6 Tony Gentilcore 2011-06-14 02:47:50 PDT
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 7 Tommy Widenflycht 2011-06-14 02:58:46 PDT
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 8 Tony Gentilcore 2011-06-14 03:11:02 PDT
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 9 Tommy Widenflycht 2011-06-14 03:19:05 PDT
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.
Comment 10 Tommy Widenflycht 2011-06-14 04:24:41 PDT
Created attachment 97094 [details]
Patch
Comment 11 WebKit Review Bot 2011-06-14 06:58:24 PDT
Comment on attachment 97094 [details]
Patch

Clearing flags on attachment: 97094

Committed r88798: <http://trac.webkit.org/changeset/88798>
Comment 12 WebKit Review Bot 2011-06-14 06:58:28 PDT
All reviewed patches have been landed.  Closing bug.