[webkit-reviews] review granted: [Bug 121940] [MediaStream API] update MediaStreamTrack object to match spec : [Attachment 213612] Updated patch

[bugzilla-daemon at webkit.org]

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.