[webkit-reviews] review granted: [Bug 123301] Adding platform implementation of MediaStreamTrack : [Attachment 215121] 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 123301: Adding platform implementation of MediaStreamTrack
https://bugs.webkit.org/show_bug.cgi?id=123301

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

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


> Source/WebCore/ChangeLog:63
> +	   * Modules/mediastream/AudioStreamTrack.cpp:
> +	   (WebCore::AudioStreamTrack::create):
> +	   (WebCore::AudioStreamTrack::AudioStreamTrack):
> +	   * Modules/mediastream/AudioStreamTrack.h:
> +	   * Modules/mediastream/MediaStream.cpp:
> +	   (WebCore::MediaStream::MediaStream):
> +	   (WebCore::MediaStream::addRemoteSource):
> +	   * Modules/mediastream/MediaStreamTrack.cpp:
> +	   (WebCore::MediaStreamTrack::MediaStreamTrack):
> +	   (WebCore::MediaStreamTrack::~MediaStreamTrack):
> +	   (WebCore::MediaStreamTrack::kind):
> +	   (WebCore::MediaStreamTrack::setSource):
> +	   (WebCore::MediaStreamTrack::id):
> +	   (WebCore::MediaStreamTrack::label):
> +	   (WebCore::MediaStreamTrack::enabled):
> +	   (WebCore::MediaStreamTrack::setEnabled):
> +	   (WebCore::MediaStreamTrack::muted):
> +	   (WebCore::MediaStreamTrack::readonly):
> +	   (WebCore::MediaStreamTrack::remote):
> +	   (WebCore::MediaStreamTrack::readyState):
> +	   (WebCore::MediaStreamTrack::states):
> +	   (WebCore::MediaStreamTrack::capabilities):
> +	   (WebCore::MediaStreamTrack::clone):
> +	   (WebCore::MediaStreamTrack::stopProducingData):
> +	   (WebCore::MediaStreamTrack::ended):
> +	   (WebCore::MediaStreamTrack::sourceStateChanged):
> +	   (WebCore::MediaStreamTrack::sourceMutedChanged):
> +	   (WebCore::MediaStreamTrack::sourceEnabledChanged):
> +	   (WebCore::MediaStreamTrack::configureTrackRendering):
> +	   (WebCore::MediaStreamTrack::stopped):
> +	   (WebCore::MediaStreamTrack::trackDidEnd):
> +	   (WebCore::MediaStreamTrack::trackReadyStateChanged):
> +	   (WebCore::MediaStreamTrack::trackMutedChanged):
> +	   * Modules/mediastream/MediaStreamTrack.h:
> +	   (WebCore::MediaStreamTrack::source):
> +	   (WebCore::MediaStreamTrack::descriptor):
> +	   * Modules/mediastream/VideoStreamTrack.cpp:
> +	   (WebCore::VideoStreamTrack::create):
> +	   (WebCore::VideoStreamTrack::VideoStreamTrack):
> +	   * Modules/mediastream/VideoStreamTrack.h:
> +	   * platform/mediastream/MediaStreamDescriptor.cpp:
> +	   (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> +	   (WebCore::MediaStreamDescriptor::addTrack):
> +	   (WebCore::MediaStreamDescriptor::removeTrack):
> +	   * platform/mediastream/MediaStreamDescriptor.h:
> +	   (WebCore::MediaStreamDescriptor::numberOfAudioSources):
> +	   (WebCore::MediaStreamDescriptor::audioSources):
> +	   (WebCore::MediaStreamDescriptor::numberOfVideoSources):
> +	   (WebCore::MediaStreamDescriptor::videoSources):
> +	   (WebCore::MediaStreamDescriptor::numberOfAudioTracks):
> +	   (WebCore::MediaStreamDescriptor::audioTracks):
> +	   (WebCore::MediaStreamDescriptor::numberOfVideoTracks):
> +	   (WebCore::MediaStreamDescriptor::videoTracks):

I would prefer to see brief comments about what changed in each method.

> Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:41
> +    RefPtr<MediaStreamTrackPrivate> descriptor =
MediaStreamTrackPrivate::create(0);
> +    return adoptRef(new AudioStreamTrack(context, descriptor.get(),
&audioConstraints));

Nit: the local variable is unnecessary.

> Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:54
> +AudioStreamTrack::AudioStreamTrack(ScriptExecutionContext* context,
MediaStreamTrackPrivate* descriptor, const Dictionary* audioConstraints)

The private track is stored in the base class as a
RefPtr<MediaStreamTrackPrivate>, why pass it as a MediaStreamTrackPrivate*
(other than that is what the code did with the source :-) )?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:56
> +    , m_descriptor(descriptor)

Nit: "m_descriptor" isn't a great name now that you have changed the name of
the class (thanks for that!). I suggest something like "m_privateTrack".

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:272
>      for (Vector<Observer*>::iterator i = m_observers.begin(); i !=
m_observers.end(); ++i)
>	   (*i)->trackDidEnd();
> +
> +    m_descriptor->setReadyState(MediaStreamSource::Ended);

Should the private track readyState be set before calling the observers?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:98
> +    // MediaStreamTrackDescriptorClient

MediaStreamTrackDescriptorClient -> MediaStreamTrackPrivateClient

> Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp:40
> +    RefPtr<MediaStreamTrackPrivate> descriptor =
MediaStreamTrackPrivate::create(0);

Nit: the local variable is unnecessary.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109
>	   m_audioStreamSources.append(audioSources[i]);
> +	  
m_audioTrackDescriptors.append(MediaStreamTrackPrivate::create(audioSources[i])
);

Is m_audioStreamSources necessary, can't we use trackPrivate->source()?

m_audioTrackDescriptors isn't a great name not that the class name has changed.


> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:174
> +    if (source())
> +	   source()->setEnabled(enabled);

This is not correct, we don't necessarily want to disable a shared source. We
don't have enough infrastructure in place yet to fix this correctly, so why
don't you just remove it an add a FIXME and we can follow up with a fix in
another patch.