[webkit-reviews] review granted: [Bug 123301] Adding platform implementation of MediaStreamTrack : [Attachment 215121] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 24 17:42:19 PDT 2013
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.
More information about the webkit-reviews
mailing list