Bug 68460 – Add WebCore platform interfaces needed by updated PeerConnection design

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Bergkvist]

Add the files in WebCore/platform/mediastream nedded by MediaStream and PeerConnection.

[Adam Bergkvist]

Created attachment 108042 [details]
Proposed patch

[Adam Barth]

Comment on attachment 108042 [details]
Proposed patch

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?

[Tommy Widenflycht]

Comment on attachment 108042 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108042&action=review

> Source/WebCore/platform/mediastream/PeerHandler.h:46
> +struct PeerHandlerConfiguration : public RefCounted<PeerHandlerConfiguration> {

Shouldn't this class be called PeerConnectionConfiguration?

>> Source/WebCore/platform/mediastream/PeerHandler.h:51
>> +        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?

I agree with you here, but "unfortunately" the spec specifies exactly how the configuration string should look like. The alternative, which I prefer, is to just pass the string to the platform specific code.

[Alexey Proskuryakov]

I'm certainly not an expert on MediaStream, but the code added here looks very specific to your high level task, not like building blocks that could be used for something else.

As such, I think that putting this code in platform/ is wrong.

[Adam Barth]

It seems clear that there should be some sort of platform abstraction in WebCore/platform.  The question somewhat revolves around what that API should look like.

[Adam Bergkvist]

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

[Adam Bergkvist]

(In reply to comment #3)
> (From update of attachment 108042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108042&action=review
> 
> > Source/WebCore/platform/mediastream/PeerHandler.h:46
> > +struct PeerHandlerConfiguration : public RefCounted<PeerHandlerConfiguration> {
> 
> Shouldn't this class be called PeerConnectionConfiguration?
>

Perhaps PeerConnectionConfiguration would better explain that it's created from the PeerConnection serverConfiguration argument, but it's the object that configures the PeerHandler and it's never used by PeerConnection. It's also stored as a member on PeerHandler.

> >> Source/WebCore/platform/mediastream/PeerHandler.h:51
> >> +        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?
> 
> I agree with you here, but "unfortunately" the spec specifies exactly how the configuration string should look like. The alternative, which I prefer, is to just pass the string to the platform specific code.

The serverConfiguration argument string must be specified in order to achieve interoperability between implementations. I think the parsing should be done in the shared WebCore code to make the behavior consistent between ports (and to avoid duplicate code). The PeerHandlerConfiguration struct could easily be serialized back to a string if it's easier to pass it to chromium that way.

[Adam Bergkvist]

(In reply to comment #4)
> I'm certainly not an expert on MediaStream, but the code added here looks very specific to your high level task, not like building blocks that could be used for something else.

You're right, it's not reusable low-level building blocks, but rather platform abstractions for a media stream and peer-to-peer backends. Unless we want to perform ICE-processing logic in shared WebCore code I can't see how we could create more low-level abstractions.

[Alexey Proskuryakov]

What is ICE-processing?

[Adam Barth]

> What is ICE-processing?

ICE is the network protocol used to establish peer-to-peer UDP connections.  It's very complex, and we certainly don't want to re-implement it in WebCore.

[Tommy Widenflycht]

(In reply to comment #10)
> > What is ICE-processing?
> 
> ICE is the network protocol used to establish peer-to-peer UDP connections.  It's very complex, and we certainly don't want to re-implement it in WebCore.

http://en.wikipedia.org/wiki/Interactive_Connectivity_Establishment

[Adam Barth]

> This pattern is used by MediaPlayer to provide media backend abstractions for HTMLMediaElement.

Unfortunately, MediaPlayer isn't the best model to work off of.  I feel like you're not being receptive to my comments, which makes me less interested in reviewing these patches.

Other reviewers should feel free to take over reviewing this patch.

[Adam Bergkvist]

(In reply to comment #12)
> I feel like you're not being receptive to my comments, which makes me less interested in reviewing these patches.

I apologize, it wasn't at all my intention to be unreceptive to you comments. I just wanted to provide motivations to some of the design decisions we've made, as a complement to the code. I didn't feel I had all that I needed to iterate the patch, and wanted to get another round of feedback from you.

> Other reviewers should feel free to take over reviewing this patch.

I certainly hope that you can reconsider your decision to stop reviewing our patches since I value your feedback and respect you as a reviewer with good knowledge about details as well as the structure and organization of the project as a whole.

> Unfortunately, MediaPlayer isn't the best model to work off of.

Could you point me to some feature/files in WebKit that uses a platform abstraction model you think would be more suitable in our case?

[Adam Barth]

I think there's some miscommunication here, so let me start from the beginning.  My sense is that we've got two somewhat competing approaches to implementing this feature.  On the one hand, there's this patch and, on the other hand, there's https://bugs.webkit.org/attachment.cgi?id=105149&action=review.

Neither of these approaches seems like exactly what we want, but they both have good aspects to them that hopefully we can combine.  To start with, I like your approach of having a Platform layer and a WebCore/platform/mediastream directory.  The main question, as Alexey points out, is what API the Platform layer should expose to the rest of WebCore.  Ideally, we'd expose a simple API that could be implemented by multiple low-level libraries (e.g., that handle ICE and the various other RTC-specific bits).

Tommy's patch has a relatively simple API that I'd be interested in whether you think would work here.  (In his patch, he's put the API at the Client layer, but putting it at the Platform layer seems more appropriate.)  Specifically, consider the API implied by this object:

http://trac.webkit.org/browser/trunk/Source/WebCore/page/MediaStreamClient.h

One thing we should change is to replace the various IDs with actual objects, but aside from that, these seem like the basic operations (and an appropriate level of detail) for the Platform to expose.

How would feel about moving MediaStreamClient from being a Client interface to being a Platform interface?

[Adam Bergkvist]

(In reply to comment #14)
> I think there's some miscommunication here, so let me start from the beginning.  My sense is that we've got two somewhat competing approaches to implementing this feature.  On the one hand, there's this patch and, on the other hand, there's https://bugs.webkit.org/attachment.cgi?id=105149&action=review.
> 
> Neither of these approaches seems like exactly what we want, but they both have good aspects to them that hopefully we can combine.  To start with, I like your approach of having a Platform layer and a WebCore/platform/mediastream directory.  The main question, as Alexey points out, is what API the Platform layer should expose to the rest of WebCore.  Ideally, we'd expose a simple API that could be implemented by multiple low-level libraries (e.g., that handle ICE and the various other RTC-specific bits).
>

I was recently contacted by Tommy who thinks our code looks OK, and we intend to collaborate around this.
 
> Tommy's patch has a relatively simple API that I'd be interested in whether you think would work here.  (In his patch, he's put the API at the Client layer, but putting it at the Platform layer seems more appropriate.)  Specifically, consider the API implied by this object:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/page/MediaStreamClient.h
> 
> One thing we should change is to replace the various IDs with actual objects, but aside from that, these seem like the basic operations (and an appropriate level of detail) for the Platform to expose.
> 
> How would feel about moving MediaStreamClient from being a Client interface to being a Platform interface?

I think MediaStreamClient is too high-level. E.g., the PeerConnection-related methods in MediaStreamClient maps almost directly to the PeerConnection JavaScript API. This makes PeerConnection a thin layer that just forwards calls to the platform. The PeerHandler API is the result of us following the steps in the specification as closely as possible and call the platform when necessary.

[Adam Barth]

> > How would feel about moving MediaStreamClient from being a Client interface to being a Platform interface?
> 
> I think MediaStreamClient is too high-level. E.g., the PeerConnection-related methods in MediaStreamClient maps almost directly to the PeerConnection JavaScript API. This makes PeerConnection a thin layer that just forwards calls to the platform. The PeerHandler API is the result of us following the steps in the specification as closely as possible and call the platform when necessary.

This raises the question of how much of the implementation should live in WebCore versus how much of the implementation should live in an external library.  My understanding in talking with Tommy is that he'd like to use a relatively high-level library to perform those operations because they don't require any specific coupling with WebCore.  You seem interested in using a lower-level library and including more code in WebCore.

Both of these approaches seem somewhat reasonable.  It's often a tough call what to do with code that's not coupled with the rest of WebCore.  We can either put it in Platform, where it can be easily reused by multiple ports or we can factor it out into a separate library, which keeps WebCore lean.  My preference is for the later.  I actually wish we did more features that way, e.g. as depicted in this diagram:

https://docs.google.com/drawings/d/10WlCj2J3arxf4cDGRKteNinaP755iFnmYtYtnNSCQOY/edit?authkey=CP6plYAI&hl=en_US#

I don't want to twist your arm, but it seems like we could set this up in the following layers:

JavaScript Bindings
PeerConnection DOM objects / events / etc
MediaStreamClient-like API
 -> Option to implement the API with a high-level library
 -> Option to implement the API with PeerHandler and a lower-level library

My my ideal world, we'd have the PeerHandler implementation in svn.webkit.org in a new top-level module (as a peer to JavaScriptCore and WebCore), but that's more of a dream for the future than something we need to do in this iteration.

[Adam Bergkvist]

(In reply to comment #16)
> This raises the question of how much of the implementation should live in WebCore versus how much of the implementation should live in an external library.  My understanding in talking with Tommy is that he'd like to use a relatively high-level library to perform those operations because they don't require any specific coupling with WebCore.  You seem interested in using a lower-level library and including more code in WebCore.

Yes, there will be some more code in WebCore with PeerHandler compared to MediaStreamClient. But PeerHandler is still very high-level. The extra code in WebCore is basically the algorithms from the HTML specification (a couple of hundred lines) that defines the behaviour of PeerConnection as a JavaScript API. Isn't it preferable to keep that kind of code in WebCore for consistent behavior between ports and to avoid having to change every implementation of the platform interface when the PeerConnection APi is updated?

Please have a look at how PeerConnection uses PeerHandler in https://bugs.webkit.org/attachment.cgi?id=108421&action=review , as compared to how it uses MediaStreamClient at the moment.

[Adam Barth]

> Isn't it preferable to keep that kind of code in WebCore for consistent behavior between ports and to avoid having to change every implementation of the platform interface when the PeerConnection APi is updated?

There's definitely a trade-off between putting things in WebCore versus in libraries.  It's not always clear where to draw the line.  For example, if HTTP were invented today, it's very likely Hixie would have written the XMLHttpRequest as imperatively manipulating HTTP protocol elements.  We could, of course, suck the implementation of the whole HTTP protocol into WebCore, as we have done with WebSockets, but there's a trade-off.

In many ways, we're still feeling our way though these kinds of decisions.  We can certainly discuss this topic more on webkit-dev if you'd like to get input from the broader community (who might not all be CCed on this bug).

My perspective is that we'll need to modularize WebCore more as time goes on, otherwise WebCore will grow to be unbounded in complexity and won't be lean.  Core functionality, like layout and rendering should stay, of course, but over time we should move more into separate libraries with clear dependencies.  Whether those libraries are in svn.webkit.org or elsewhere is a separate issue.

> Please have a look at how PeerConnection uses PeerHandler in https://bugs.webkit.org/attachment.cgi?id=108421&action=review , as compared to how it uses MediaStreamClient at the moment.

I really appreciate that you've posted the whole patch so we can use that as a reference for discussion.  There are a lot of details in how PeerConnection uses PeerHandler that I think are less than ideal.  For example, I'd rather WebCore be ignorant of concepts like STUN.  I suspect if you asked the vast majority of WebKit developers what STUN was, they'd have no idea.  That means code that talks about STUN is mysterious and won't be well maintained.

I guess I don't understand what you find objectionable about the layering diagram in Comment #16.  It gives folks the option of using a high-level library or a lower-level library.  The issue of whether to spin the PeerHandler-based implementation out into a separate library is something we can worry about in the future.

[Tommy Widenflycht]

Since I have gotten some comments and questions regarding this patch (I submitted most of the initial code) I just wanted to clarify that I am supportive of this effort. "My" implementation had a less-than-ideal design we discovered recently and this approach is better.

I think that it is a good idea to move the options parsing to an optional module in WebCore/platform as I understand Adam suggested, but not further away.

[Adam Bergkvist]

(In reply to comment #18)
> I really appreciate that you've posted the whole patch so we can use that as a reference for discussion.  There are a lot of details in how PeerConnection uses PeerHandler that I think are less than ideal.  For example, I'd rather WebCore be ignorant of concepts like STUN.  I suspect if you asked the vast majority of WebKit developers what STUN was, they'd have no idea.  That means code that talks about STUN is mysterious and won't be well maintained.

Yes, it's easier to talk about PeerHandler together with PeerConnection rather than out of context (since PeerConnection isn't included in the patch for this bug). The word "STUN" is used in the configuration string argument to the PeerConnection constructor and therefore a part of the configuration object. For example

var pc = new PeerConnection("STUN stun.example.org:3478", callback);

We can remove the parsing and simply pass the configuration string to the platform. Would that be acceptable?

The logic we want to add to PeerConnection is only related to the JavaScript API. For example

pc.addStream(stream1);
pc.addStream(stream2);

in the same event-loop iteration should, according to the specification, be treated as if you're adding a list of streams at once. The streams should be handled together and only result in one signaling message. That's why PeerHandler has addPendingStreams() rather than a direct counterpart to PeerConnection's addStream().

> I guess I don't understand what you find objectionable about the layering diagram in Comment #16.  It gives folks the option of using a high-level library or a lower-level library.  The issue of whether to spin the PeerHandler-based implementation out into a separate library is something we can worry about in the future.

The layering diagram looks fine to me. I see the logic that we're adding to PeerConnection as part of the "Bindings" box and PeerHandler is the API towards the "P2P" box. The "P2P" box can then be implemented with either a high-level or a lower-level library.

[Adam Bergkvist]

(In reply to comment #18)
> There are a lot of details in how PeerConnection uses PeerHandler that I think are less than ideal.

Other than the parsing of the configuration string, what more do you want me to change before I iterate the patch?

[Adam Barth]

Are you planning to revise your patch along the lines described in Comment #16?  In particular, it seems important to understand the interface between the DOM-layer code and the Platform-layer code.

[Adam Bergkvist]

(In reply to comment #22)
> Are you planning to revise your patch along the lines described in Comment #16? In particular, it seems important to understand the interface between the DOM-layer code and the Platform-layer code.

To do that, I think it's important to define the border between the DOM-layer code and the Platform-layer code. The PeerConnection spec has a natural separation between the concerns of the API and the needs of the platform in the form of an "ICE Agent" (which hides ICE processing, RTP and other low-level stuff from PeerConnection). Could you please give your view on this? We think that this separation makes sense.

The function names in PeerHandler, which are of course up for discussion, are derived from the spec text. Please have a look below at the current PeerHandler API, annotated with relevant sentences from the PeerConnection spec. We're confident that this API can be implemented with a high-level library like the one that Tommy is using.

// ========== Calls from the DOM-layer to the Platform-layer ==========

// "If connection's ICE started flag is still false, start the PeerConnection ICE Agent and send the initial offer."
// "The initial offer must include a media description for [...] all the streams in localStreams"
void produceInitialOffer(const MediaStreamDescriptorVector& pendingAddStreams);

// "Start the PeerConnection ICE Agent and pass it sdp as the initial offer from the other peer;"
// "the ICE Agent will then (asynchronously) construct the initial answer and transmit it as described above."
void handleInitialOffer(const String& sdp);

// "If connection's ICE started flag is true, then pass sdp to the PeerConnection ICE Agent as a subsequent offer
// or answer, to be interpreted as appropriate given the current state of the ICE Agent"
void processSDP(const String& sdp);

// "Have the PeerConnection's PeerConnection ICE Agent add [or remove] a media stream for stream the next time the user
// agent provides a stable state. Any other pending stream additions and removals must be processed at the same time."
void processPendingStreams(const MediaStreamDescriptorVector& pendingAddStreams, const MediaStreamDescriptorVector& pendingRemoveStreams);

// "Let data be message encoded as UTF-8."
// "Transmit a data packet to a peer using the PeerConnection's PeerConnection data UDP media stream with data as the message."
void sendDataStreamMessage(const char* data, unsigned length);

// "Destroy the PeerConnection ICE Agent, abruptly ending any active ICE processing and any active streaming,
// and releasing any relevant resources (e.g. TURN permissions)."
void stop();

// ========== Calls from the Platform-layer to the DOM-layer ==========

// "When a PeerConnection ICE Agent completes ICE processing (even if there are no active streams), the user
// agent must queue a task that sets the PeerConnection object's [...]"
void iceProcessingCompleted();

// "When a PeerConnection ICE Agent is required to send SDP offers or answers, the user agent must follow these steps:"
// "Let sdp be the SDP offer or answer to be sent."
void sdpGenerated(const String& sdp);

// "When a packet that is part of a data UDP media stream is received, the user agent must run the following steps:"
void dataStreamMessageReceived(const char* data, unsigned length);

// "When a user agent starts receiving media for a component and a candidate was provided for that component
// by a PeerConnection ICE Agent, the user agent must follow these steps:"
void remoteStreamAdded(PassRefPtr<MediaStreamDescriptor>);

// "When a PeerConnection ICE Agent finds that a stream from the remote peer has been removed (its port has been
// set to zero in a media description sent on the signaling channel), the user agent must follow these steps:"
void remoteStreamRemoved(MediaStreamDescriptor*);

[Adam Barth]

Sorry for the delay in replying Adam.  I've talked the issue over with Tommy, and he agrees that the API you propose in Comment #23 should work for Chromium as well.

My sense we'll want to be able to choose different implementations of that API at compile time.  For example, we'll likely have an implementation using Tommy's high-level library and an (larger) implementation using Gstreamer.

Maybe a good first step is to check in the header files for the Platform API?  Then we can proceed to add the callers and the implementations of the API in parallel.

[Adam Bergkvist]

(In reply to comment #24)
> Sorry for the delay in replying Adam.  I've talked the issue over with Tommy, and he agrees that the API you propose in Comment #23 should work for Chromium as well.
>
> My sense we'll want to be able to choose different implementations of that API at compile time.  For example, we'll likely have an implementation using Tommy's high-level library and an (larger) implementation using Gstreamer.
>

Our idea has been to have separate implementations of the PeerHandlerPrivate interface and select one at compile time as you describe.

#if PLATFORM(CHROMIUM)
#include "PeerHandlerPrivateChromium.h"
#define PeerHandlerPrivateClassName PeerHandlerPrivateChromium
#elif USE(GSTREAMER)
...

The code above would be at line 40 in PeerHandler.cpp, but is omitted in this patch since there are no implementations available yet.

> Maybe a good first step is to check in the header files for the Platform API?  Then we can proceed to add the callers and the implementations of the API in parallel.

Yes, that seems like a good starting point. I've created bugs 68462 and 68464 to add the caller code in PeerConnection and MediaStream respectivly.

[Adam Barth]

> Our idea has been to have separate implementations of the PeerHandlerPrivate interface and select one at compile time as you describe.
> 
> #if PLATFORM(CHROMIUM)
> #include "PeerHandlerPrivateChromium.h"
> #define PeerHandlerPrivateClassName PeerHandlerPrivateChromium
> #elif USE(GSTREAMER)
> ...

That's not the usual way we do that sort of thing in WebKit.  It's probably better follow the model used, for example, by ResourceHandle.  In that approach, there is a shared h file and separate cpp files that different ports can link in.

[Adam Bergkvist]

Created attachment 109780 [details]
Updated patch

Updated patch based on review feedback

[Adam Barth]

Comment on attachment 109780 [details]
Updated patch

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.

[Adam Bergkvist]

Created attachment 110163 [details]
Patch 3

(In reply to comment #28)
> (From update of attachment 109780 [details])
> 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.

Thank you for a quick review.

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

Let's do that. I've updated the name of this bug and I'll create a new one for the MediaStream platform stuff. I'll address the review comments about MediaStreamCenter in the new bug.

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

You're right, there are a few mebers that can be read-only. I've changed the structs to classes (with private data members) and added the necessary getters and setters.

> > Source/WebCore/platform/mediastream/MediaStreamComponent.h:54
> > +    { }
> 
> Nit: These should be on separate lines.

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

Fixed.

> Nit: These should be on separate lines.
> 
> > Source/WebCore/platform/mediastream/MediaStreamSource.h:65
> > +    { }
> 
> Nit: These should be on separate lines.

Fixed.

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

That works. When a port has its own implementations it can opt out from using these empty ones.

[Adam Barth]

Comment on attachment 110163 [details]
Patch 3

This looks great.  Thanks for iterating on the patch and discussing variations on your original design.

[WebKit Review Bot]

Comment on attachment 110163 [details]
Patch 3

Clearing flags on attachment: 110163

Committed r96959: <http://trac.webkit.org/changeset/96959>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Bergkvist]

(In reply to comment #30)
> (From update of attachment 110163 [details])
> This looks great.  Thanks for iterating on the patch and discussing variations on your original design.

Thank you for reviewing this. I've updated the patch in http://webkit.org/b/68462 to make PeerConnection use the platform interface. It would be great if you could take a look at it.