[webkit-reviews] review denied: [Bug 123477] Adding addRemoteTrack and removeRemoteTrack functions to MediaStreamDescriptor and MediaStream : [Attachment 215441] Patch

[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 123477: Adding addRemoteTrack and removeRemoteTrack functions to
MediaStreamDescriptor and MediaStream
https://bugs.webkit.org/show_bug.cgi?id=123477

Attachment 215441: Patch
https://bugs.webkit.org/attachment.cgi?id=215441&action=review

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


As we talked about on irc, it would be good to get rid of
MediaStreamDescriptor::addSource and MediaStreamDescriptor::removeSource.

> Source/WebCore/ChangeLog:27
> +	   * Modules/mediastream/MediaStream.cpp:
> +	   (WebCore::MediaStream::addTrack):
> +	   (WebCore::MediaStream::removeTrack):
> +	   (WebCore::MediaStream::addRemoteSource):
> +	   (WebCore::MediaStream::removeRemoteSource):
> +	   (WebCore::MediaStream::addRemoteTrack):
> +	   (WebCore::MediaStream::removeRemoteTrack):
> +	   (WebCore::MediaStream::getTrackVector):
> +	   * Modules/mediastream/MediaStream.h:
> +	   * Modules/mediastream/MediaStreamTrack.h:
> +	   (WebCore::MediaStreamTrack::privateTrack):
> +	   * platform/mediastream/MediaStreamDescriptor.cpp:
> +	   (WebCore::MediaStreamDescriptor::addRemoteTrack):
> +	   (WebCore::MediaStreamDescriptor::removeRemoteTrack):
> +	   * platform/mediastream/MediaStreamDescriptor.h:

Comments please.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:163
> +    MediaStreamTrackVector* tracks =
getTrackVector(track->source()->type());

This won't work because "track" is NULL after begin used above. You need to add
something like "RefPtr<MediaStreamTrack> track = prpTrack" before the
PassRefPtr is dereferenced.

Nit: maybe change the name to "getTrackVectorForType"?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:196
> +    MediaStreamTrackVector* tracks =
getTrackVector(track->source()->type());
> +
> +    if (!tracks)
> +	   return false;
>  
> +    size_t pos = tracks->find(track);

Ditto about using a PassRefPtr.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:279
> +    RefPtr<MediaStreamTrackPrivate> track =
MediaStreamTrackPrivate::create(source);
> +    addRemoteTrack(track.get());

Nit: this can be done on a single line.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:342
>	  
scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEve
nt, false, false, track.release()));

Why don't we dispatch the event in removeTrack?

> Source/WebCore/Modules/mediastream/MediaStream.h:101
>      virtual void addRemoteSource(MediaStreamSource*) OVERRIDE FINAL;
>      virtual void removeRemoteSource(MediaStreamSource*) OVERRIDE FINAL;
> +    virtual void addRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;
> +    virtual void removeRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;


Why do we need both versions, can we just have add/removeRemoteTrack?

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:53
>      virtual void addRemoteSource(MediaStreamSource*) = 0;
>      virtual void removeRemoteSource(MediaStreamSource*) = 0;

Can these be removed - is there ever a reason to pass the source directly to
MediaStream instead of creating the private track and passing that?