[webkit-reviews] review granted: [Bug 121935] [MediaStream] Cleanup platform interface : [Attachment 212712] Updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 26 09:21:17 PDT 2013
Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 121935: [MediaStream] Cleanup platform interface
https://bugs.webkit.org/show_bug.cgi?id=121935
Attachment 212712: Updated patch
https://bugs.webkit.org/attachment.cgi?id=212712&action=review
------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212712&action=review
r=me, with nits.
> Source/WebCore/Modules/mediastream/MediaStream.cpp:61
> +static MediaStreamCenter*& mediaStreamCenterOverride()
> +{
> + static MediaStreamCenter* override;
> + return override;
> +}
> +
> +MediaStreamCenter& MediaStream::sharedStreamCenter()
> +{
> + MediaStreamCenter* override = mediaStreamCenterOverride();
> + if (override)
> + return *override;
> +
> + return MediaStreamCenter::shared();
> +}
> +
> +void MediaStream::setSharedStreamCenter(MediaStreamCenter* center)
> +{
> + mediaStreamCenterOverride() = center;
> +}
> +
Seems like this should be in MediaStreamCenter and should override the return
of MediaStreamCenter::instance().
> Source/WebCore/Modules/mediastream/MediaStream.cpp:91
> - MediaStreamCenter::instance().didCreateMediaStream(descriptor.get());
> +
MediaStream::sharedStreamCenter().didCreateMediaStream(descriptor.get());
Then, this wouldn't be necessary.
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:182
> - if
(!MediaStreamCenter::instance().getMediaStreamTrackSources(request.release()))
> + if
(!MediaStream::sharedStreamCenter().getMediaStreamTrackSources(request.release(
)))
Ditto.
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:214
> -void MediaStreamTrack::didEndTrack()
> +void MediaStreamTrack::trackEnded()
Why not MediaStreamTrack::trackDidEnd()? Especially since the client method was
renamed from trackEnded() to trackDidEnd().
> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:217
> + // FIXME: this is wrong, the track shouldn't have to call the
descriptor's client!
> + MediaStreamDescriptorClient* client = m_source ?
m_source->stream()->client() : 0;
Bug #? :-)
> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:57
> -
MediaStreamCenter::instance().didCreateMediaStream(m_stream->descriptor());
> +
MediaStream::sharedStreamCenter().didCreateMediaStream(m_stream->descriptor());
One more.
More information about the webkit-reviews
mailing list