[Webkit-unassigned] [Bug 123301] Adding platform implementation of MediaStreamTrack

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123301


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #215121|review?                     |review+
               Flag|                            |




--- Comment #2 from Eric Carlson <eric.carlson at apple.com>  2013-10-24 17:41:06 PST ---
(From update of attachment 215121)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.