[webkit-reviews] review denied: [Bug 68460] Add WebCore platform interfaces needed by updated MediaStream and PeerConnection design : [Attachment 109780] Updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 5 09:22:20 PDT 2011

Adam Barth <abarth at webkit.org> has denied Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request for review:
Bug 68460: Add WebCore platform interfaces needed by updated MediaStream and
PeerConnection design

Attachment 109780: Updated patch

------- Additional Comments from Adam Barth <abarth at webkit.org>
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

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

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

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

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.

More information about the webkit-reviews mailing list