[webkit-reviews] review granted: [Bug 121940] [MediaStream API] update MediaStreamTrack object to match spec : [Attachment 213612] Updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 7 12:53:04 PDT 2013
Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 121940: [MediaStream API] update MediaStreamTrack object to match spec
https://bugs.webkit.org/show_bug.cgi?id=121940
Attachment 213612: Updated patch
https://bugs.webkit.org/attachment.cgi?id=213612&action=review
------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213603&action=review
r=me, with nits.
> Source/WebCore/Modules/mediastream/AllAudioCapabilities.h:42
> + static PassRefPtr<AllAudioCapabilities>
create(PassRefPtr<MediaStreamSourceCapabilities> capabilities)
Nit: PassRefPtrs are no longer needed, ever since Anders added move-semantics
to the RefPtr class. You can return RefPtr here.
> Source/WebCore/Modules/mediastream/AllVideoCapabilities.h:40
> + static PassRefPtr<AllVideoCapabilities>
create(PassRefPtr<MediaStreamSourceCapabilities> capabilities)
Ditto.
> Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:50
> PassRefPtr<AudioStreamTrack>
AudioStreamTrack::create(ScriptExecutionContext* context, const Dictionary&
audioConstraints)
> {
> - RefPtr<AudioStreamTrack> track = adoptRef(new AudioStreamTrack(context,
0, &audioConstraints));
> - return track.release();
> + return adoptRef(new AudioStreamTrack(context, 0, &audioConstraints));
> }
>
> PassRefPtr<AudioStreamTrack>
AudioStreamTrack::create(ScriptExecutionContext* context, MediaStreamSource*
source)
> {
> - RefPtr<AudioStreamTrack> track = adoptRef(new AudioStreamTrack(context,
source, 0));
> - return track.release();
> + return adoptRef(new AudioStreamTrack(context, source, 0));
> +}
> +
> +PassRefPtr<AudioStreamTrack> AudioStreamTrack::create(MediaStreamTrack*
track)
> +{
> + return adoptRef(new AudioStreamTrack(track));
And super-ditto on these three.
> Source/WebCore/Modules/mediastream/CapabilityRange.cpp:43
> +PassRefPtr<CapabilityRange> CapabilityRange::create(const
MediaStreamSourceCapabilityRange& rangeInfo)
Yet another ditto.
> Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:36
> +PassRefPtr<MediaSourceStates> MediaSourceStates::create(const
MediaStreamSourceStates& states)
Ditto.
> Source/WebCore/Modules/mediastream/MediaStreamCapabilities.cpp:40
> +PassRefPtr<MediaStreamCapabilities>
MediaStreamCapabilities::create(PassRefPtr<MediaStreamSourceCapabilities>
capabilities)
Ditto.
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:120
> void MediaStreamTrack::setSource(MediaStreamSource* source)
> {
> ASSERT(source == m_source || !m_source);
(Existing code)
Wha wha? What exactly is this ASSERT protecting us from?
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:254
> +PassRefPtr<MediaTrackConstraints> MediaStreamTrack::constraints() const
> +{
> + // FIXME: https://bugs.webkit.org/show_bug.cgi?id=122428
> + notImplemented();
> + return 0;
> +}
> +
> +PassRefPtr<MediaSourceStates> MediaStreamTrack::states() const
> +{
> + if (!m_source)
> + return 0;
> +
> + return MediaSourceStates::create(m_source->states());
> +}
> +
> +PassRefPtr<MediaStreamCapabilities> MediaStreamTrack::capabilities() const
> +{
> + if (!m_source)
> + return 0;
> +
> + return MediaStreamCapabilities::create(m_source->capabilities());
> +}
Another mega PassRefPtr -> RefPtr Ditto.
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:338
> + // 4.3.1
> + // ... media from the source only flows when a MediaStreamTrack object
is both unmuted and enabled
> +}
This looks suspicious. Was there supposed to be a FIXME here?
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:386
> + Vector<RefPtr<Event>> events;
> + {
> + MutexLocker locker(m_mutex);
> +
> + m_eventDispatchScheduled = false;
> + if (!scriptExecutionContext()) {
> + m_scheduledEvents.clear();
> + return;
> + }
> +
> + events.swap(m_scheduledEvents);
> + }
I feel like this could be cleaned up slightly to minimize the time inside the
mutex:
Vector<RefPtr<Event>> events;
{
MutexLocker locker(m_mutex);
m_eventDispatchScheduled = false;
events.swap(m_scheduledEvents);
}
if (!scriptExecutionContext())
return;
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:390
> + Vector<RefPtr<Event> >::iterator it = events.begin();
> + for (; it != events.end(); ++it)
> + dispatchEvent((*it).release());
The newly supported "auto" keyword would be awesome here:
for (auto it = events.begin(); it != events.end(); ++it)
dispatchEvent((*it).release());
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:392
> + events.clear();
This seems unnecessary.
> Source/WebCore/Modules/mediastream/MediaTrackConstraint.cpp:37
> +PassRefPtr<MediaTrackConstraint> MediaTrackConstraint::create(const
Dictionary& constraint)
PassRefPtr -> RefPtr ditto.
> Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.cpp:37
> +PassRefPtr<MediaTrackConstraintSet> MediaTrackConstraintSet::create(const
Dictionary& constraints)
Man, I'm only 1/2 way through? Phew.
Oh, ditto.
> Source/WebCore/Modules/mediastream/MediaTrackConstraints.cpp:41
> +PassRefPtr<MediaTrackConstraints>
MediaTrackConstraints::create(PassRefPtr<MediaConstraintsImpl> constraints)
Ditto.
> Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp:50
> +PassRefPtr<VideoStreamTrack> VideoStreamTrack::create(MediaStreamTrack*
track)
Ditto.
> Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp:38
> +PassRefPtr<MediaStreamAudioSource> MediaStreamAudioSource::create()
Ditto.
More information about the webkit-reviews
mailing list