[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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109780|review?                     |review-
               Flag|                            |




--- Comment #28 from Adam Barth <abarth at webkit.org>  2011-10-05 09:22:21 PST ---
(From update of attachment 109780)
View in context: https://bugs.webkit.org/attachment.cgi?id=109780&action=review

Thanks for updating the patch.  I feel like we're making progress here.  My two main concerns with this patch are MediaStreamCenter, which I don't really understand, and the fact that your objects have a bunch of public member variables.

One way to proceed is to not add MediaStreamCenter yet and add it when needed by later patches.  That will help me understand it's role.

As for the structs with public data members, can we start out with these private and make them public as needed in future patches?  That will help us see whether they actually need to be public or whether we've be better served by having them private and exposing methods that manipulate them.

> Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:57
> +void MediaStreamCenter::streamEnded(MediaStreamDescriptor* streamDescriptor)
> +{
> +    MediaStream* stream = streamDescriptor->owner;
> +    if (stream)
> +        stream->streamEnded();
> +    else
> +        streamDescriptor->ended = true;
> +}

This looks like it should be a method on MediaStreamDescriptor.

> Source/WebCore/platform/mediastream/MediaStreamCenter.h:44
> +MediaStreamCenter& mediaStreamCenter();

I'm not sure I understand the role of this static.  It's very unusual to have statics in WebKit because separate PageGroups should be almost entirely separate.  That said, we do have some statics, so it's not a hand-and-fast rule.

> Source/WebCore/platform/mediastream/MediaStreamCenter.h:53
> +    void mediaStreamTrackEnabledSet(const MediaStreamDescriptor*, unsigned componentIndex);

I'm not sure what this function does.  It's a noun phrase, so I'd expect it to return a value, but it is a void function, so maybe it's an action?

> Source/WebCore/platform/mediastream/MediaStreamCenter.h:54
> +    void stopLocalMediaStream(const MediaStreamDescriptor*);

Why isn't this a method on MediaStream ?

> Source/WebCore/platform/mediastream/MediaStreamCenter.h:56
> +    void streamEnded(MediaStreamDescriptor*);

This sounds like a notification that the stream ended.  Does that notification flow up from the network or down from the API?

I think my main confusion is that I don't understand what this object is all about.  MediaStreamCenter isn't really a name that means anything.  I wonder if we can remove this object entirely.

> Source/WebCore/platform/mediastream/MediaStreamComponent.h:54
> +    { }

Nit: These should be on separate lines.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:63
> +    { }

Nit: These should be on separate lines.

> Source/WebCore/platform/mediastream/MediaStreamSource.h:65
> +    { }

Nit: These should be on separate lines.

> Source/WebCore/platform/mediastream/PeerHandler.cpp:41
> +// FIXME: remove when real implementations are available
> +// Empty implementations for ports that build with MEDIA_STREAM enabled by default.
> +#if PLATFORM(CHROMIUM) || PLATFORM(GTK)

I would just remove this ifdef.  We'd not going to need it.

> Source/WebCore/platform/mediastream/PeerHandler.h:54
> +class PeerHandler {

We'll probably need to split this into PeerHandlerBase and PeerHandler, like we do for ResourceHandle, but we can do that in the future when needed.

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