[webkit-reviews] review denied: [Bug 121954] [MediaStream API] allow a stream source to be shared : [Attachment 214107] First draft

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Thiago de Barros Lacerda
<thiago.lacerda at openbossa.org>'s request for review:
Bug 121954: [MediaStream API] allow a stream source to be shared
https://bugs.webkit.org/show_bug.cgi?id=121954

Attachment 214107: First draft
https://bugs.webkit.org/attachment.cgi?id=214107&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214107&action=review


This is a great start, but I think it definitely needs a test to make sure the
states are being set correctly. For example, what makes a source->readyState()
go to "Ended"?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:197
> +    m_descriptor->addSource(prpTrack->source());

You should use "track" rather than "prpTrack".

> Source/WebCore/Modules/mediastream/MediaStream.cpp:243
> +bool MediaStream::hasTrackWithSource(PassRefPtr<MediaStreamSource> source)

Nit: I think "haveTrackWithSource" would be a slightly better name.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:257
> +    if (source->type() == MediaStreamSource::Audio) {
> +	   for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) {
> +	       if (track->source() == source.get())
> +		   return true;
> +	   }
> +    } else {
> +	   for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) {
> +	       if (track->source() == source.get())
> +		   return true;
> +	   }
> +    }
> +
> +    return false;

The "else" isn't necessary:

if (source->type() == MediaStreamSource::Audio) {
    for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) {
	if (track->source() == source.get())
	    return true;
    }
    return false;
}

for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) {
    if (track->source() == source.get())
	return true;
}

return false;

> Source/WebCore/Modules/mediastream/MediaStream.cpp:346
>	       index = i;
> +	       removedTracks.append((*tracks)[i]);
> +	       tracks->remove(i);
>	       break;

Don't you want to look through all of the tracks instead of breaking on the
first one that uses the source?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:353
> +    if (index == notFound)
> +	   return;

if (!removedTracks.size()) return;