[webkit-reviews] review granted: [Bug 123477] Adding addRemoteTrack and removeRemoteTrack functions to MediaStreamDescriptor and MediaStream : [Attachment 215681] Updated patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Thiago de Barros Lacerda
<thiago.lacerda at openbossa.org>'s request for review:
Bug 123477: Adding addRemoteTrack and removeRemoteTrack functions to
MediaStreamDescriptor and MediaStream
https://bugs.webkit.org/show_bug.cgi?id=123477

Attachment 215681: Updated patch
https://bugs.webkit.org/attachment.cgi?id=215681&action=review

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


> Source/WebCore/ChangeLog:19
> +	   (WebCore::MediaStream::removeTrack): Splitted in two parts that can
be used by old removeTrack and new
> +	   removeRemoteTrack.
> +
> +	   (WebCore::MediaStream::addRemoteSource): Reusing code in new
addTrack method.

Nit: the blank lines between methods in the same file are not necessary.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:196
> +bool MediaStream::removeTrack(PassRefPtr<MediaStreamTrack> track)
> +{
> +    // This is a common part used by removeTrack called by JavaScript
> +    // and removeRemoteTrack and only removeRemoteTrack must fire
removetrack event
> +    RefPtr<MediaStreamTrack> trackRef = track;

Nit: the convention is to prefix the PassRefPtr name with prp: "Unless the use
of the argument is very simple, the argument should be transferred to a RefPtr
at the start of the function; the argument can be named with a “prp” prefix in
such cases."

http://www.webkit.org/coding/RefPtr.html

> Source/WebCore/Modules/mediastream/MediaStream.cpp:197
> +    MediaStreamTrackVector* tracks =
getTrackVectorForType(trackRef->source()->type());

Nit: track->type() returns the same thing as track->source()->type()

> Source/WebCore/Modules/mediastream/MediaStream.cpp:302
> +    Vector<int> tracksToRemove;
> +    for (size_t i = 0; i < tracks->size(); ++i) {
> +	   if ((*tracks)[i]->source() == source)
> +	       tracksToRemove.append(i);
>      }

Nit: I know this isn't new code, but you could clean this up by using
Vector<RefPtr<MediaStreamTrack>> and "for (auto ..." here and you wouldn't have
to loop backwards below.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:322
> +    switch (privateTrack->source()->type()) {

Nit: privateTrack->type()