[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 #2 from Adam Barth <abarth at webkit.org>  2011-09-20 13:59:23 PST ---
(From update of attachment 108042)
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.

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

enableMediaStreamComponent ?

> 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.

> 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.

> Source/WebCore/platform/mediastream/PeerHandler.h:51
> +        TypeNONE,
> +        TypeSTUN,
> +        TypeTURN

I'm not quite clear which concepts are going to be in WebCore and which concepts are only in the underlying library.  I was hoping that STUN would be a library-internal concept.  Is there some reason WebCore needs to be aware of STUN?

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