Bug 58550

Created attachment 104670 [details] Patch

Comment on attachment 104670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104670&action=review We'll need fishd to review this patch b/c of the Chromium API change. > Source/WebKit/chromium/src/MediaStreamClientProxy.cpp:52 > + if (m_client) > + // Ownership of the WebMediaStreamController is transferred to the client. > + m_client->setController(new WebMediaStreamController(controller)); nit: Multi-line if statements need { } even if they only contain one statement.

Comment on attachment 104670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104670&action=review > Source/WebKit/chromium/public/WebMediaStreamClient.h:36 > +enum WebGenerateStreamOptionFlag { nit: one new header file per type. nit: the enum name is WebFooOptionFlag, but the values are WebFooRequestX. we have a convention of naming enums like so: enum WebFoo { WebFooX, WebFooY, WebFooZ }; > Source/WebKit/chromium/public/WebMediaStreamClient.h:42 > +typedef unsigned WebGenerateStreamOptionFlags; we normally just accomplish this with a comment. i'd give generateStream an int parameter with a name like optionFlags. then, i'd comment that optionFlags can be a bit-wise combination of WebGenerateStreamOptionFlag values. note: If you only intend this enum to be used with this interface, then you could just define WebMediaStreamClient::OptionFlag. > Source/WebKit/chromium/public/WebMediaStreamClient.h:48 > + // The controller is valid until mediaStreamDestroyed() is invoked. these comments seem to contradict one another: "The controller is valid until mediaStreamDestroyed() is invoked." says that the WebMediaStreamClient does not have ownership of the given pointer since something else is responsible for telling you when it gets invalidated. "Ownership of the WebMediaStreamController is transferred to the client." says exactly the opposite. > Source/WebKit/chromium/public/WebMediaStreamClient.h:51 > + virtual void mediaStreamDestroyed() = 0; maybe this method should be named "mediaStreamControllerDestroyed()" to better indicate that the WebMediaStreamController has been destroyed? why do we need this method? why not just have WebKit call setController(0) to tell the client when the controller is no longer valid? > Source/WebKit/chromium/public/WebMediaStreamClient.h:53 > + virtual void generateStream(int requestId, WebGenerateStreamOptionFlags, const WebSecurityOrigin&) = 0; what is this "requestId"? > Source/WebKit/chromium/public/WebMediaStreamClient.h:56 > + virtual void processSignalingMessage(int peerConnectionId, const WebString& message) = 0; what is this "peerConnectionId"? why do we have identifiers here and not objects? do these identifiers exist to support chrome's multi-process architecture? it seems like perhaps there is a peer connection interface that should be used here? won't we have something like these methods for direct peer connection usage? > Source/WebKit/chromium/public/WebMediaStreamClient.h:59 > + virtual void newPeerConnection(int peerConnectionId, const WebString& configuration) = 0; what is the format of the "configuration" string, or how is it defined? documentation seems to be missing. > Source/WebKit/chromium/public/WebMediaStreamController.h:45 > + // Same as WebCore::NavigatorUserMediaError::PERMISSION_DENIED. we normally avoid comments that mention WebCore implementation details in WebKit API headers. no one will think to ever fix these comments when they become untrue. WebCore is hacked on by a lot of non-Chromium folks!! > Source/WebKit/chromium/public/WebMediaStreamController.h:50 > + WEBKIT_EXPORT void streamGenerated(int requestId, const WebString& streamLabel, WebMediaStreamTrackList&); it seems like there is a whole processing model for WebMediaStreamController and WebMediaStreamClient that could use some documentation. it is not obvious, if i am implementing WebMediaStreamClient, how I am to use WebMediaStreamController.

Comment on attachment 104670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104670&action=review >> Source/WebKit/chromium/public/WebMediaStreamClient.h:36 >> +enum WebGenerateStreamOptionFlag { > > nit: one new header file per type. > > nit: the enum name is WebFooOptionFlag, but the values are WebFooRequestX. we have a convention of naming enums like so: > > enum WebFoo { > WebFooX, > WebFooY, > WebFooZ > }; Fixed. >> Source/WebKit/chromium/public/WebMediaStreamClient.h:42 >> +typedef unsigned WebGenerateStreamOptionFlags; > > we normally just accomplish this with a comment. i'd give generateStream an int > parameter with a name like optionFlags. then, i'd comment that optionFlags can > be a bit-wise combination of WebGenerateStreamOptionFlag values. > > note: If you only intend this enum to be used with this interface, then you > could just define WebMediaStreamClient::OptionFlag. Fixed. >> Source/WebKit/chromium/public/WebMediaStreamClient.h:48 >> + // The controller is valid until mediaStreamDestroyed() is invoked. > > these comments seem to contradict one another: > > "The controller is valid until mediaStreamDestroyed() is invoked." says that > the WebMediaStreamClient does not have ownership of the given pointer since > something else is responsible for telling you when it gets invalidated. > > "Ownership of the WebMediaStreamController is transferred to the client." says > exactly the opposite. Both comments are correct, the controller must be invalidated when the frame/page it is associated with disappears or moves. >> Source/WebKit/chromium/public/WebMediaStreamClient.h:51 >> + virtual void mediaStreamDestroyed() = 0; > > maybe this method should be named "mediaStreamControllerDestroyed()" to better > indicate that the WebMediaStreamController has been destroyed? > > why do we need this method? why not just have WebKit call setController(0) to > tell the client when the controller is no longer valid? One could do so, but this is related to more things than the controller. If/when the page/frame goes away all streams have to be stopped, the webcam/audio device shut down etc. Maybe shutdown() would be better. >> Source/WebKit/chromium/public/WebMediaStreamClient.h:53 >> + virtual void generateStream(int requestId, WebGenerateStreamOptionFlags, const WebSecurityOrigin&) = 0; > > what is this "requestId"? This requestId is for finding the original request when this asynchronous call finished. See void WebMediaStreamController::streamGenerated(int requestId, const WebString& streamLabel, WebMediaStreamTrackList&); void WebMediaStreamController::streamGenerationFailed(int requestId, Error code); Opening a camera device etc can take a relatively long time. >> Source/WebKit/chromium/public/WebMediaStreamClient.h:56 >> + virtual void processSignalingMessage(int peerConnectionId, const WebString& message) = 0; > > what is this "peerConnectionId"? why do we have identifiers here and not objects? > > do these identifiers exist to support chrome's multi-process architecture? > > it seems like perhaps there is a peer connection interface that should be used > here? won't we have something like these methods for direct peer connection > usage? Each WebCore peer connection object has its unique ID. Using objects would be of no extra value in our current implementation, since all information is conveyed through the functions calls. Also I don't understand the last paragraph. >> Source/WebKit/chromium/public/WebMediaStreamClient.h:59 >> + virtual void newPeerConnection(int peerConnectionId, const WebString& configuration) = 0; > > what is the format of the "configuration" string, or how is it defined? documentation seems to be missing. The configuration string is defined in the WebRtc standard here: http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html#peerconnection >> Source/WebKit/chromium/public/WebMediaStreamController.h:45 >> + // Same as WebCore::NavigatorUserMediaError::PERMISSION_DENIED. > > we normally avoid comments that mention WebCore implementation details in > WebKit API headers. no one will think to ever fix these comments when they > become untrue. WebCore is hacked on by a lot of non-Chromium folks!! Fixed. >> Source/WebKit/chromium/public/WebMediaStreamController.h:50 >> + WEBKIT_EXPORT void streamGenerated(int requestId, const WebString& streamLabel, WebMediaStreamTrackList&); > > it seems like there is a whole processing model for WebMediaStreamController > and WebMediaStreamClient that could use some documentation. it is not obvious, > if i am implementing WebMediaStreamClient, how I am to use WebMediaStreamController. Messages from WebCore to Chromium goes through the Client, and messages from Chromium to WebKit goes through the Controller. Where would documentation like this ideally be placed?

Created attachment 105149 [details] Patch

(In reply to comment #4) > >> Source/WebKit/chromium/public/WebMediaStreamClient.h:48 > >> + // The controller is valid until mediaStreamDestroyed() is invoked. > > > > these comments seem to contradict one another: > > > > "The controller is valid until mediaStreamDestroyed() is invoked." says that > > the WebMediaStreamClient does not have ownership of the given pointer since > > something else is responsible for telling you when it gets invalidated. > > > > "Ownership of the WebMediaStreamController is transferred to the client." says > > exactly the opposite. > > Both comments are correct, the controller must be invalidated when the frame/page it is associated with disappears or moves. Then you should say that in the comments. The comments seem to suggest that the pointer becomes invalid after mediaStreamDestroyed, but actually it requires work from the embedder to make that happen. I think mediaStreamDestroyed sounds like a notification that a pointer has become invalid. So, I think you probably need to rename this method. > >> Source/WebKit/chromium/public/WebMediaStreamClient.h:51 > >> + virtual void mediaStreamDestroyed() = 0; > > > > maybe this method should be named "mediaStreamControllerDestroyed()" to better > > indicate that the WebMediaStreamController has been destroyed? > > > > why do we need this method? why not just have WebKit call setController(0) to > > tell the client when the controller is no longer valid? > > One could do so, but this is related to more things than the controller. If/when the page/frame goes away all streams have to be stopped, the webcam/audio device shut down etc. > Maybe shutdown() would be better. Maybe, yeah. I'm not sure why it is advantageous to make the embedder take ownership of the controller. > >> Source/WebKit/chromium/public/WebMediaStreamClient.h:53 > >> + virtual void generateStream(int requestId, WebGenerateStreamOptionFlags, const WebSecurityOrigin&) = 0; > > > > what is this "requestId"? > > This requestId is for finding the original request when this asynchronous call finished. See Right, got that part. > >> Source/WebKit/chromium/public/WebMediaStreamClient.h:56 > >> + virtual void processSignalingMessage(int peerConnectionId, const WebString& message) = 0; > > > > what is this "peerConnectionId"? why do we have identifiers here and not objects? > > > > do these identifiers exist to support chrome's multi-process architecture? > > > > it seems like perhaps there is a peer connection interface that should be used > > here? won't we have something like these methods for direct peer connection > > usage? > > Each WebCore peer connection object has its unique ID. > Using objects would be of no extra value in our current implementation, since all information is conveyed through the functions calls. I don't understand your argument. Don't these IDs correspond to objects that WebCore knows about? Is there not some kind of hash map from ID to object? > Also I don't understand the last paragraph. "peer connection" sounds to me like the proposed PeerConnection API. It is my understanding that PeerConnection is a proper JS API that can be used independently of WebRTC. It seems like you are creating APIs here that would also need to exist to support using PeerConnection independently. I think I'm missing some context about what the overall architecture is for these things, which makes it nearly impossible to give you a good code review at this level. It would be helpful to find a time to chat about this in the larger context. > >> Source/WebKit/chromium/public/WebMediaStreamClient.h:59 > >> + virtual void newPeerConnection(int peerConnectionId, const WebString& configuration) = 0; > > > > what is the format of the "configuration" string, or how is it defined? documentation seems to be missing. > > The configuration string is defined in the WebRtc standard here: http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html#peerconnection OK, I recommend indicating this in comments. > >> Source/WebKit/chromium/public/WebMediaStreamController.h:50 > >> + WEBKIT_EXPORT void streamGenerated(int requestId, const WebString& streamLabel, WebMediaStreamTrackList&); > > > > it seems like there is a whole processing model for WebMediaStreamController > > and WebMediaStreamClient that could use some documentation. it is not obvious, > > if i am implementing WebMediaStreamClient, how I am to use WebMediaStreamController. > > Messages from WebCore to Chromium goes through the Client, and messages from Chromium to WebKit goes through the Controller. > > Where would documentation like this ideally be placed? Well, you could document some of this flow on the Controller interface.

Created attachment 112181 [details] Patch New embedders based on the current WebRTC implementation

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 112181 [details] Patch Attachment 112181 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10200823

Comment on attachment 112181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112181&action=review > Source/WebCore/ChangeLog:10 > + No new tests. (OOPS!) We'll need to remove this line from the ChangeLog in order to land this patch. Generally it's a good idea to fill out the ChangeLog with an explanation of why you're making this change. > Source/WebCore/mediastream/PeerConnection.cpp:58 > - , m_peerHandler(PeerHandler::create(this, serverConfiguration, context->securityOrigin()->toString())) > + , m_peerHandler(adoptPtr(PeerHandler::create(this, serverConfiguration, context->securityOrigin()->toString()))) This is wrong. PeerHandler::create should call adoptPtr. > Source/WebCore/mediastream/PeerConnection.cpp:224 > -void PeerConnection::remoteStreamRemoved(MediaStreamDescriptor* streamDescriptor) > +void PeerConnection::remoteStreamRemoved(PassRefPtr<MediaStreamDescriptor> prpStreamDescriptor) > { > + RefPtr<MediaStreamDescriptor> streamDescriptor = prpStreamDescriptor; Why did you make this change? This function does not appear to take ownership of streamDescriptor. > Source/WebCore/platform/mediastream/PeerHandler.h:45 > class PeerHandler { This shouldn't be a virtual interface. Any given compilation of WebKit should only have one implementation of PeerHandler, which means this doesn't need to be virtual. We just need to link in different CPP files for different configurations. > Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:43 > + WEBKIT_EXPORT void initialize(); // FIXME Add params. You need a : after "FIXME". > Source/WebKit/chromium/src/PeerHandlerChromium.cpp:46 > +class PeerHandlerChromium : public WebCore::PeerHandler { This class should be in WebCore/platform and should use PlatformSupport to reach WebKit/chromium/src. > Source/WebKit/chromium/src/PeerHandlerChromium.cpp:122 > +namespace WebCore { > + > +PeerHandler* PeerHandler::create(PeerHandlerClient* client, const String& serverConfiguration, const String& securityOrigin) > +{ > + return new WebKit::PeerHandlerChromium(client, serverConfiguration, securityOrigin); > +} > + > +} // namespace WebCore WebKit/chromium/src cannot contain code in the WebCore namespace.

My guess is you'll want to move the existing PeerHandler.h to PeerHandlerBase.h and then make a new PeerHandler.h in Source/WebCore/platform/mediastream/chromium that is a subclass of PeerHandlerBase.

If I were writing this patch, I would also try to break it down into smaller pieces. The patch you posed clocks in at 34.48 KB, which is pretty large. One approach to making the patch smaller is to do the WebCore and the WebKit pieces of the patch separately. One reason smaller patches are better is because they're easier to review and iterate on, which means we'll land the code sooner.

Yes, I know that this patch is on the large size but at least for the first review I needed to convey the overall design since it is non-standard. Would you be comfortable in continuing on iterating on this patch or should I split it up as you suggested? (In reply to comment #12) > If I were writing this patch, I would also try to break it down into smaller pieces. The patch you posed clocks in at 34.48 KB, which is pretty large. One approach to making the patch smaller is to do the WebCore and the WebKit pieces of the patch separately. > > One reason smaller patches are better is because they're easier to review and iterate on, which means we'll land the code sooner.

And keep PeerHandlerBase virtual? (In reply to comment #11) > My guess is you'll want to move the existing PeerHandler.h to PeerHandlerBase.h and then make a new PeerHandler.h in Source/WebCore/platform/mediastream/chromium that is a subclass of PeerHandlerBase.

> Yes, I know that this patch is on the large size but at least for the first review I needed to convey the overall design since it is non-standard. Would you be comfortable in continuing on iterating on this patch or should I split it up as you suggested? Yeah, if that's how you'd like to proceed, that's fine. > And keep PeerHandlerBase virtual? Tommy and I chatted a bit, and we agreed to keep these non-virtual.

Comment on attachment 112181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112181&action=review >> Source/WebCore/ChangeLog:10 >> + No new tests. (OOPS!) > > We'll need to remove this line from the ChangeLog in order to land this patch. Generally it's a good idea to fill out the ChangeLog with an explanation of why you're making this change. Done. >> Source/WebCore/mediastream/PeerConnection.cpp:58 >> + , m_peerHandler(adoptPtr(PeerHandler::create(this, serverConfiguration, context->securityOrigin()->toString()))) > > This is wrong. PeerHandler::create should call adoptPtr. Fixed. >> Source/WebCore/mediastream/PeerConnection.cpp:224 >> + RefPtr<MediaStreamDescriptor> streamDescriptor = prpStreamDescriptor; > > Why did you make this change? This function does not appear to take ownership of streamDescriptor. Undone change that shouldn't have been in the patch.

Created attachment 112315 [details] Patch

Comment on attachment 112181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112181&action=review >> Source/WebCore/platform/mediastream/PeerHandler.h:45 >> class PeerHandler { > > This shouldn't be a virtual interface. Any given compilation of WebKit should only have one implementation of PeerHandler, which means this doesn't need to be virtual. We just need to link in different CPP files for different configurations. By making this class virtual allows for embedders to easily inherit from this class and add their own members. >> Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:43 >> + WEBKIT_EXPORT void initialize(); // FIXME Add params. > > You need a : after "FIXME". Done. >> Source/WebKit/chromium/src/PeerHandlerChromium.cpp:46 >> +class PeerHandlerChromium : public WebCore::PeerHandler { > > This class should be in WebCore/platform and should use PlatformSupport to reach WebKit/chromium/src. After discussion over chat we have decided that for now this class stays in WebKit but is moved to a new directory named "bridge". >> Source/WebKit/chromium/src/PeerHandlerChromium.cpp:122 >> +} // namespace WebCore > > WebKit/chromium/src cannot contain code in the WebCore namespace. This code needs to do the same as ResourceHandle, at least for now.

Comment on attachment 112315 [details] Patch Attachment 112315 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10211286

Comment on attachment 112315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112315&action=review Looks like we're getting close. (Of course, we'll need to go through API review also since you've included changes to WebKit/chromium/public in here too.) Also, this needs to build on GTK before we can land it. > Source/WebCore/mediastream/PeerConnection.cpp:58 > - , m_peerHandler(PeerHandler::create(this, serverConfiguration, context->securityOrigin()->toString())) > + , m_peerHandler(PeerHandlerBase::create(this, serverConfiguration, context->securityOrigin()->toString())) This should call PeerHandler::create. That's the point of having a separate "base". > Source/WebCore/mediastream/PeerConnection.h:131 > - OwnPtr<PeerHandler> m_peerHandler; > + OwnPtr<PeerHandlerBase> m_peerHandler; This should hold a PeerHandle. If you look at the folks outside of the platform layer that interact with ResourceHandle, you'll see that they all use ResourceHandle as the type, not ResourceHandleBase. > Source/WebCore/platform/mediastream/PeerHandlerBase.h:57 > + virtual void produceInitialOffer(const MediaStreamDescriptorVector& pendingAddStreams) = 0; > + virtual void handleInitialOffer(const String& sdp) = 0; > + virtual void processSDP(const String& sdp) = 0; > + virtual void processPendingStreams(const MediaStreamDescriptorVector& pendingAddStreams, const MediaStreamDescriptorVector& pendingRemoveStreams) = 0; > + virtual void sendDataStreamMessage(const char* data, unsigned length) = 0; > + > + virtual void stop() = 0; I thought we agreed that these would not be virtual. > Source/WebKit/chromium/bridge/PeerHandlerChromium.cpp:46 > +class PeerHandlerChromium : public WebCore::PeerHandlerBase { PeerHandlerChromium should just be called PeerHandler. The declaration should live in WebCore/platform/mediastream/chromium/PeerHandle.h. > Source/WebKit/chromium/bridge/PeerHandlerChromium.cpp:62 > +PeerHandlerChromium::PeerHandlerChromium(WebCore::PeerHandlerClient* client, const String& serverConfiguration, const String& securityOrigin) All this code should be in the WebCore namespace.

Comment on attachment 112315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112315&action=review > Source/WebKit/chromium/public/WebKitPlatformSupport.h:328 > + virtual WebPeerHandler* createPeerHandler(const WebPeerHandlerClient&) { return 0; } nit: we usually pass clients as non-const pointers. > Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:53 > + // FIXME: Add functions. nit: we normally add functions above the WEBKIT_IMPLEMENTATION block. that should be the last part of the public section. > Source/WebKit/chromium/public/WebPeerHandler.h:37 > +class WebPeerHandler { i'm a little bothered by this class name. what is a "peer handler"? ... maybe if this were called WebPeerConnectionHandler or just WebPeerConnection, the name would make sense. > Source/WebKit/chromium/public/WebPeerHandler.h:43 > + virtual void produceInitialOffer(const WebVector<WebMediaStreamDescriptor>& pendingAddStreams) = 0; it seems like there is a processing model here that should be explained. you could either refer to a document or explain it in comments. nit: pendingAddStreams -> streamsToAdd? > Source/WebKit/chromium/public/WebPeerHandler.h:45 > + virtual void processSDP(const WebString& sdp) = 0; what does SDP stand for? maybe this should be spelled out? > Source/WebKit/chromium/public/WebPeerHandler.h:46 > + virtual void processPendingStreams(const WebVector<WebMediaStreamDescriptor>& pendingAddStreams, const WebVector<WebMediaStreamDescriptor>& pendingRemoveStreams) = 0; nit: perhaps the parameters would be better named "streamsToAdd" and "streamsToRemove" > Source/WebKit/chromium/public/WebPeerHandler.h:47 > + virtual void sendDataStreamMessage(const char* data, unsigned length) = 0; nit: use size_t instead of unsigned for byte counts > Source/WebKit/chromium/public/WebPeerHandlerClient.h:41 > +class WebPeerHandlerClient : public WebNonCopyable { it is more common for this to be an interface with pure virtual functions. see WebURLLoaderClient for example. it seems like the current design suggests that WebPeerHandlerClient could be copied around, meaning that we may have a difficult time cleaning up back pointers to WebCore::PeerHandlerClient if we needed to. i'm not sure if that is a problem. i just observe that this is a different approach to wrapping a WebCore interface. > Source/WebKit/chromium/public/WebPeerHandlerClient.h:43 > + WEBKIT_EXPORT void iceProcessingCompleted(); nit: notifications usually take the form of {did,will}{Action}{Subject}... e.g.: didCompleteICEProcessing didGenerateSDP didReceiveDataStreamMessage didAddRemoteStream didRemoveRemoteStream perhaps the last two should be did{Add,Remove}MediaStream since the parameter type is Web_MediaStream_Descriptor? it helps to use similar words when naming a method and object parameters.

It's nice to see a Chromium implementation of the PeerHandler interface. What's the motivation behind having a PeerHandlerBase (without a cpp-file)? I interpret the base pattern as a way to share common code between the implementations and I don't expect PeerHandler implementations to be able to share any code. You can just add your own cpp-file for PeerHandler and opt-out from the empty implementation in PeerHandler.cpp.

Created attachment 112503 [details] Patch

Comment on attachment 112315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112315&action=review >> Source/WebCore/mediastream/PeerConnection.cpp:58 >> + , m_peerHandler(PeerHandlerBase::create(this, serverConfiguration, context->securityOrigin()->toString())) > > This should call PeerHandler::create. That's the point of having a separate "base". Fixed. >> Source/WebCore/mediastream/PeerConnection.h:131 >> + OwnPtr<PeerHandlerBase> m_peerHandler; > > This should hold a PeerHandle. > > If you look at the folks outside of the platform layer that interact with ResourceHandle, you'll see that they all use ResourceHandle as the type, not ResourceHandleBase. Fixed. >> Source/WebCore/platform/mediastream/PeerHandlerBase.h:57 >> + virtual void stop() = 0; > > I thought we agreed that these would not be virtual. Fixed >> Source/WebKit/chromium/bridge/PeerHandlerChromium.cpp:46 >> +class PeerHandlerChromium : public WebCore::PeerHandlerBase { > > PeerHandlerChromium should just be called PeerHandler. The declaration should live in WebCore/platform/mediastream/chromium/PeerHandle.h. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerChromium.cpp:62 >> +PeerHandlerChromium::PeerHandlerChromium(WebCore::PeerHandlerClient* client, const String& serverConfiguration, const String& securityOrigin) > > All this code should be in the WebCore namespace. Fixed. >> Source/WebKit/chromium/public/WebKitPlatformSupport.h:328 >> + virtual WebPeerHandler* createPeerHandler(const WebPeerHandlerClient&) { return 0; } > > nit: we usually pass clients as non-const pointers. Fixed. >> Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:53 >> + // FIXME: Add functions. > > nit: we normally add functions above the WEBKIT_IMPLEMENTATION block. that should be the > last part of the public section. Fixed. >> Source/WebKit/chromium/public/WebPeerHandler.h:37 >> +class WebPeerHandler { > > i'm a little bothered by this class name. what is a "peer handler"? ... maybe if this > were called WebPeerConnectionHandler or just WebPeerConnection, the name would make > sense. I have mimicked the naming of things from their WebCore equivalents. Personally I think that it would be more confusing if function calls changed names in/out of WebKit but maybe that's not how the embedder layer works? A PeerHandler is the handler of calls from a PeerConnection intended for the embedder, it's not a PeerConnection. >> Source/WebKit/chromium/public/WebPeerHandler.h:45 >> + virtual void processSDP(const WebString& sdp) = 0; > > what does SDP stand for? maybe this should be spelled out? SDP stands for Service Discovery Protocol. All implementers of the PeerConnection feature knows (or at least need to know) what this is and therefore in a sense it doesn't need to be spelled out. >> Source/WebKit/chromium/public/WebPeerHandler.h:47 >> + virtual void sendDataStreamMessage(const char* data, unsigned length) = 0; > > nit: use size_t instead of unsigned for byte counts Fixed. >> Source/WebKit/chromium/public/WebPeerHandlerClient.h:41 >> +class WebPeerHandlerClient : public WebNonCopyable { > > it is more common for this to be an interface with pure virtual functions. see WebURLLoaderClient for example. > > it seems like the current design suggests that WebPeerHandlerClient could be copied around, meaning that we > may have a difficult time cleaning up back pointers to WebCore::PeerHandlerClient if we needed to. i'm not > sure if that is a problem. i just observe that this is a different approach to wrapping a WebCore interface. Fixed.

> What's the motivation behind having a PeerHandlerBase (without a cpp-file)? Yeah, if there's no code or declarations to share, then PeerHandlerBase doesn't serve a purpose.

Comment on attachment 112503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112503&action=review > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:55 > + delete m_webHandler; We should use OwnPtr rather than calling delete manually. You can adoptPtr immediately after calling WebKit::webKitPlatformSupport()->createPeerHandler.

Thanks Tommy. The structure of this code looks good. Looking at the details now.

Comment on attachment 112503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112503&action=review Below are a bunch of small nits. Once you've addressed them, I'm happy with this patch. Thanks for iterating! > Source/WebCore/ChangeLog:17 > + * WebCore.gypi: > + * mediastream/PeerConnection.cpp: > + (WebCore::PeerConnection::PeerConnection): > + * mediastream/PeerConnection.h: > + * platform/mediastream/PeerHandlerBase.h: Renamed from Source/WebCore/platform/mediastream/PeerHandler.h. > + (WebCore::PeerHandlerBase::~PeerHandlerBase): > + * platform/mediastream/PeerHandlerClient.h: Renamed from Source/WebCore/platform/mediastream/PeerHandler.cpp. > + (WebCore::PeerHandlerClient::~PeerHandlerClient): This information isn't accurate anymore. You might need to run prepare-ChangeLog again. > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:46 > +PeerHandlerInternal::PeerHandlerInternal(PeerHandlerClient* client, const String& serverConfiguration, const String& securityOrigin) : m_client(client) ": m_client(client)" should be on its own line. > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:49 > + m_webHandler = WebKit::webKitPlatformSupport()->createPeerHandler(this); m_webHandler = adoptPtr(WebKit::webKitPlatformSupport()->createPeerHandler(this)); > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:60 > + size_t numStreamsToAdd = pendingAddStreams.size(); numStreamsToAdd => numberOfStreamsToAdd (please use complete words in variable names) > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:68 > +void PeerHandlerInternal::handleInitialOffer(const String& sdp) sdp = ? It would be nice to use a more descriptive variable name for us P2P noobs. > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:83 > + size_t numStreamsToAdd = pendingAddStreams.size(); > + WebKit::WebVector<WebKit::WebMediaStreamDescriptor> streamsToAdd(numStreamsToAdd); > + for (size_t i = 0 ; i < numStreamsToAdd ; ++i) > + streamsToAdd[i].assign(pendingAddStreams[i]); This pattern seems repeated a bunch of times. Maybe factor out into a static function in this file? > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:111 > +void PeerHandlerInternal::sdpGenerated(const WebKit::WebString& sdp) > +{ > + m_client->sdpGenerated(sdp); > +} Same comment here about sdp. > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:125 > + m_client->remoteStreamRemoved(((PassRefPtr<WebCore::MediaStreamDescriptor>)webMediaStreamDescriptor).get()); This line of code is very strange. We usually don't use C-style casts in WebKit. Why not have an operator that returns a raw pointer? Going through PassRefPtr seems very wrong. > Source/WebKit/chromium/bridge/PeerHandlerInternal.h:53 > + PeerHandlerInternal(PeerHandlerClient*, const String& serverConfiguration, const String& securityOrigin); Why is securityOrigin a string? It probably should be a SecurityOrigin. > Source/WebKit/chromium/bridge/PeerHandlerInternal.h:56 > + virtual void produceInitialOffer(const MediaStreamDescriptorVector& Pendingaddstreams); Pendingaddstreams => pendingAddStreams ? > Source/WebKit/chromium/bridge/PeerHandlerInternal.h:60 > + virtual void sendDataStreamMessage(const char* data, unsigned length); unsigned => size_t ? > Source/WebKit/chromium/bridge/PeerHandlerInternal.h:71 > + WebKit::WebPeerHandler* m_webHandler; OwnPtr > Source/WebKit/chromium/bridge/PeerHandlerInternal.h:72 > + PeerHandlerClient* m_client; What's the ownership model for m_client ? > Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:51 > + operator WTF::PassRefPtr<WebCore::MediaStreamDescriptor>() const; Is this common in the API ? It seems strange. This doesn't take the reference out of m_private, so there's no advantage to using a PassRefPtr. It should just use a raw pointer. > Source/WebKit/chromium/public/WebPeerHandler.h:6 > +-- * are met: This looks slightly corrupted. > Source/WebKit/chromium/public/WebPeerHandler.h:41 > + virtual void initialize(const WebString& serverConfiguration, const WebString& securityOrigin) = 0; Should this be a WebSecurityOrigin ? > Source/WebKit/chromium/src/WebMediaStreamDescriptor.cpp:56 > + Looks like you have an extra blank line here.

Created attachment 112657 [details] Patch

Comment on attachment 112503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112503&action=review >> Source/WebCore/ChangeLog:17 >> + (WebCore::PeerHandlerClient::~PeerHandlerClient): > > This information isn't accurate anymore. You might need to run prepare-ChangeLog again. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:46 >> +PeerHandlerInternal::PeerHandlerInternal(PeerHandlerClient* client, const String& serverConfiguration, const String& securityOrigin) : m_client(client) > > ": m_client(client)" should be on its own line. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:49 >> + m_webHandler = WebKit::webKitPlatformSupport()->createPeerHandler(this); > > m_webHandler = adoptPtr(WebKit::webKitPlatformSupport()->createPeerHandler(this)); Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:55 >> + delete m_webHandler; > > We should use OwnPtr rather than calling delete manually. You can adoptPtr immediately after calling WebKit::webKitPlatformSupport()->createPeerHandler. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:60 >> + size_t numStreamsToAdd = pendingAddStreams.size(); > > numStreamsToAdd => numberOfStreamsToAdd (please use complete words in variable names) Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:68 >> +void PeerHandlerInternal::handleInitialOffer(const String& sdp) > > sdp = ? It would be nice to use a more descriptive variable name for us P2P noobs. SDP is short for "Session Description Protocol" and can contain codec, ip and/or time candidates. The naming is a bit like HTML; everyone is expected to know that it stands for "HyperText Markup Language". See http://en.wikipedia.org/wiki/Session_Description_Protocol for more info I could rename all occurrences of sdp to [sS]essionDescriptionProtocol if you think it genuinely would help? >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:83 >> + streamsToAdd[i].assign(pendingAddStreams[i]); > > This pattern seems repeated a bunch of times. Maybe factor out into a static function in this file? Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:125 >> + m_client->remoteStreamRemoved(((PassRefPtr<WebCore::MediaStreamDescriptor>)webMediaStreamDescriptor).get()); > > This line of code is very strange. We usually don't use C-style casts in WebKit. Why not have an operator that returns a raw pointer? Going through PassRefPtr seems very wrong. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.h:53 >> + PeerHandlerInternal(PeerHandlerClient*, const String& serverConfiguration, const String& securityOrigin); > > Why is securityOrigin a string? It probably should be a SecurityOrigin. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.h:56 >> + virtual void produceInitialOffer(const MediaStreamDescriptorVector& Pendingaddstreams); > > Pendingaddstreams => pendingAddStreams ? Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.h:60 >> + virtual void sendDataStreamMessage(const char* data, unsigned length); > > unsigned => size_t ? Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.h:71 >> + WebKit::WebPeerHandler* m_webHandler; > > OwnPtr Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.h:72 >> + PeerHandlerClient* m_client; > > What's the ownership model for m_client ? We just hold on to the pointer, and don't own it. >> Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:51 >> + operator WTF::PassRefPtr<WebCore::MediaStreamDescriptor>() const; > > Is this common in the API ? It seems strange. This doesn't take the reference out of m_private, so there's no advantage to using a PassRefPtr. It should just use a raw pointer. It is a very common pattern in the API. >> Source/WebKit/chromium/public/WebPeerHandler.h:6 >> +-- * are met: > > This looks slightly corrupted. Fixed. >> Source/WebKit/chromium/public/WebPeerHandler.h:41 >> + virtual void initialize(const WebString& serverConfiguration, const WebString& securityOrigin) = 0; > > Should this be a WebSecurityOrigin ? Fixed. >> Source/WebKit/chromium/src/WebMediaStreamDescriptor.cpp:56 >> + > > Looks like you have an extra blank line here. Fixed.

Comment on attachment 112657 [details] Patch Attachment 112657 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10227378

Created attachment 112660 [details] Patch

I see two parallel PeerHandler interfaces in this patch (gstreamer + chromium). The "shared header file" pattern used by PeerHandler would just require a chromium cpp-file and a single ifdef'ed line in PeerHandler.h for the PeerHandlerInternal private member.

(In reply to comment #30) > (From update of attachment 112503 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112503&action=review > > >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:68 > >> +void PeerHandlerInternal::handleInitialOffer(const String& sdp) > > > > sdp = ? It would be nice to use a more descriptive variable name for us P2P noobs. > > SDP is short for "Session Description Protocol" and can contain codec, ip and/or time candidates. The naming is a bit like HTML; everyone is expected to know that it stands for "HyperText Markup Language". > See http://en.wikipedia.org/wiki/Session_Description_Protocol for more info > > I could rename all occurrences of sdp to [sS]essionDescriptionProtocol if you think it genuinely would help? Many add a link to the Wikipedia article for us noobs. :) > >> Source/WebKit/chromium/bridge/PeerHandlerInternal.h:72 > >> + PeerHandlerClient* m_client; > > > > What's the ownership model for m_client ? > > We just hold on to the pointer, and don't own it. Who owns it? How do we make sure we don't hold dangling pointers when it dies? > >> Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:51 > >> + operator WTF::PassRefPtr<WebCore::MediaStreamDescriptor>() const; > > > > Is this common in the API ? It seems strange. This doesn't take the reference out of m_private, so there's no advantage to using a PassRefPtr. It should just use a raw pointer. > > It is a very common pattern in the API. Ok. I'll leave it for fishd to review then.

(In reply to comment #33) > I see two parallel PeerHandler interfaces in this patch (gstreamer + chromium). The "shared header file" pattern used by PeerHandler would just require a chromium cpp-file and a single ifdef'ed line in PeerHandler.h for the PeerHandlerInternal private member. The question is what will happent to this code over time. ResourceHandle probably started out with just one or two ifdefs, but not it's ifdefed all over. I don't feel strongly about that though. It's a very revisable decision.

Comment on attachment 112660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112660&action=review This looks good to me. Thanks for iterating! (If you'd like to include a link to the Wikipedia article on sdp, I do think that would be helpful.) Over to fishd for API review. > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:60 > +void PeerHandlerInternal::copyItems(WebKit::WebVector<WebKit::WebMediaStreamDescriptor>& result, const MediaStreamDescriptorVector& streams) This can just be a free (static) function. It doesn't interact with |this| at all. > Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:64 > + for (size_t i = 0 ; i < numberOfStreams ; ++i) 0 ; => 0;

(In reply to comment #24) ... > >> Source/WebKit/chromium/public/WebPeerHandler.h:37 > >> +class WebPeerHandler { > > > > i'm a little bothered by this class name. what is a "peer handler"? ... maybe if this > > were called WebPeerConnectionHandler or just WebPeerConnection, the name would make > > sense. > > I have mimicked the naming of things from their WebCore equivalents. Personally I think that it would be more confusing if function calls changed names in/out of WebKit but maybe that's not how the embedder layer works? > > A PeerHandler is the handler of calls from a PeerConnection intended for the embedder, it's not a PeerConnection. It sounds like you are saying that it is a PeerConnection handler. So, how about {Web}PeerConnectionHandler? It isn't handling peers, it is handling connections to peers, right? > >> Source/WebKit/chromium/public/WebPeerHandler.h:45 > >> + virtual void processSDP(const WebString& sdp) = 0; > > > > what does SDP stand for? maybe this should be spelled out? > > SDP stands for Service Discovery Protocol. All implementers of the PeerConnection feature knows (or at least need to know) what this is and therefore in a sense it doesn't need to be spelled out. I dislike uncommon acronyms, but OK.

(In reply to comment #37) > (In reply to comment #24) > ... > > >> Source/WebKit/chromium/public/WebPeerHandler.h:37 > > >> +class WebPeerHandler { > > > > > > i'm a little bothered by this class name. what is a "peer handler"? ... maybe if this > > > were called WebPeerConnectionHandler or just WebPeerConnection, the name would make > > > sense. > > > > I have mimicked the naming of things from their WebCore equivalents. Personally I think that it would be more confusing if function calls changed names in/out of WebKit but maybe that's not how the embedder layer works? > > > > A PeerHandler is the handler of calls from a PeerConnection intended for the embedder, it's not a PeerConnection. > > It sounds like you are saying that it is a PeerConnection handler. So, how about {Web}PeerConnectionHandler? It isn't handling peers, it is handling connections to peers, right? > > Much better, thanks. Will change.

Created attachment 112835 [details] Patch

Comment on attachment 112660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112660&action=review >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:60 >> +void PeerHandlerInternal::copyItems(WebKit::WebVector<WebKit::WebMediaStreamDescriptor>& result, const MediaStreamDescriptorVector& streams) > > This can just be a free (static) function. It doesn't interact with |this| at all. Fixed. >> Source/WebKit/chromium/bridge/PeerHandlerInternal.cpp:64 >> + for (size_t i = 0 ; i < numberOfStreams ; ++i) > > 0 ; => 0; Fixed.

Comment on attachment 112835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112835&action=review > Source/WebCore/GNUmakefile.list.am:2745 > + Source/WebCore/platform/mediastream/gstreamer/PeerHandler.cpp \ > + Source/WebCore/platform/mediastream/gstreamer/PeerHandler.h \ I thought you were going to rename this to PeerConnectionHandler > Source/WebKit/chromium/public/WebPeerConnectionHandler.h:47 > +class WebPeerConnectionHandler { I see, you just renamed the API names. It's probably worth renaming the WebCore classes too. (I completely understand if you'd like to do that in a followup patch.)

Comment on attachment 112835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112835&action=review >> Source/WebKit/chromium/public/WebPeerConnectionHandler.h:47 >> +class WebPeerConnectionHandler { > > I see, you just renamed the API names. It's probably worth renaming the WebCore classes too. (I completely understand if you'd like to do that in a followup patch.) Yes, will do that asap this patch lands.

Created attachment 112864 [details] Patch

Comment on attachment 112864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112864&action=review Here's some comments on the API. This patch looks great overall! > Source/WebKit/chromium/bridge/PeerConnectionHandlerInternal.cpp:61 > +{ Did you try just doing: result = streams; There's an assignment operator on WebVector that takes a generic container. See WebVector<T>::operator(const C&) > Source/WebKit/chromium/public/WebPeerConnectionHandlerClient.h:37 > + virtual void iceProcessingCompleted() = 0; nit-pick: we normally name event methods with a "did" or "will" prefix. see WebURLLoaderClient for instance. didCompleteICEProcessing didGenerateSDP didReceiveDataStreamMessage didAddRemoteStream didRemoveRemoteStream For consistency with WebKit APIs, I'd recommend doing the same here, even if it might seem a bit more awkward at first :)

Comment on attachment 112864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112864&action=review >> Source/WebKit/chromium/bridge/PeerConnectionHandlerInternal.cpp:61 > > Did you try just doing: result = streams; > > There's an assignment operator on WebVector that takes a generic container. See WebVector<T>::operator(const C&) Ahh, fixed. >> Source/WebKit/chromium/public/WebPeerConnectionHandlerClient.h:37 >> + virtual void iceProcessingCompleted() = 0; > > nit-pick: we normally name event methods with a "did" or "will" prefix. see WebURLLoaderClient for instance. > > didCompleteICEProcessing > didGenerateSDP > didReceiveDataStreamMessage > didAddRemoteStream > didRemoveRemoteStream > > For consistency with WebKit APIs, I'd recommend doing the same here, even if it might seem a bit more awkward at first :) Fixed.

Created attachment 113079 [details] Patch

Comment on attachment 113079 [details] Patch Clearing flags on attachment: 113079 Committed r98926: <http://trac.webkit.org/changeset/98926>

All reviewed patches have been landed. Closing bug.