[Webkit-unassigned] [Bug 68460] Add WebCore platform interfaces needed by updated MediaStream and PeerConnection design

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-09-21 06:51:42 PST ---
(In reply to comment #2)
> (From update of attachment 108042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108042&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamManager.cpp:47
> > +MediaStreamManager& mediaStreamManager()
> 
> Historically WebKit has shied away from "manager" in class names.  It's a very general name.  If we had a more specific name, that might help focus what this class is about.
>

MediaStream is an abstract representation of a media stream in the platform backend. MediaStreamManager is a single entry point for propagating changes between LocalMediaStream/MediaStream/MediaStreamTrack and the platform backend. It also manages the lifetime of a LocalMediaStream, from querying the backend for media stream components, to revoking further access to the underlying media streams. If you have a suggestion for a better name we are more than happy to change it.

> > Source/WebCore/platform/mediastream/MediaStreamManager.cpp:73
> > +void MediaStreamManager::setMediaStreamComponentEnabled(const MediaStreamDescriptor* streamDescriptor, unsigned componentIndex, bool enabled)
> 
> enableMediaStreamComponent ?
>

The reason for the somewhat awkward name is that it takes a boolean argument which maps well to the 'enabled' property setter on MediaStreamTrack. enableMediaStreamComponent sort of implies that there is a disableMediaStreamComponent as well.

> > Source/WebCore/platform/mediastream/MediaStreamManager.h:56
> > +    void setMediaStreamComponentEnabled(const MediaStreamDescriptor*, unsigned componentIndex, bool enabled);
> > +    void stopLocalMediaStream(const MediaStreamDescriptor*);
> > +
> > +    void streamEnded(MediaStreamDescriptor*);
> 
> These methods looks suspiciously like they might be better as member functions of MediaStreamDescriptor.
>

The MediaStreamDescriptor struct is meant to be a lightweight platform-independent container for MediaStream to store its data in, to be used by the platform backend. Moving the methods above to MediaStreamDescriptor would mean that we also have to add a way for it to talk to the platform. 

> > Source/WebCore/platform/mediastream/MediaStreamManagerPrivate.h:48
> > +class MediaStreamManagerPrivateInterface {
> > +public:
> > +    virtual ~MediaStreamManagerPrivateInterface() { }
> > +
> > +    virtual void setMediaStreamComponentEnabled(const MediaStreamDescriptor*, unsigned componentIndex, bool enabled) = 0;
> > +    virtual void stopLocalMediaStream(const MediaStreamDescriptor*) = 0;
> > +};
> 
> I'm slightly unclear about what this class is for.  Is this virtual interface something like PlatformSupport?  MediaStreamManagerPrivateInterface is quite a mouthful and pretty opaque.
> 
> > Source/WebCore/platform/mediastream/PeerHandler.h:44
> > +class PeerHandlerPrivateInterface;
> 
> I'm not super excited about this "private interface" approach.  It seems like we should follow one of the existing design patterns.
>

This pattern is used by MediaPlayer to provide media backend abstractions for HTMLMediaElement. We prefer this pattern over other common patterns such as the one with a shared header file, which usually leads to the header file being cluttered with PLATFORM and OS ifdefs.

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