[Webkit-unassigned] [Bug 123301] Adding platform implementation of MediaStreamTrack
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 24 17:42:20 PDT 2013
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.
More information about the webkit-unassigned
mailing list