[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]

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
https://bugs.webkit.org/show_bug.cgi?id=68460

Attachment 109780: Updated patch
https://bugs.webkit.org/attachment.cgi?id=109780&action=review

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