Bug 80589

Created attachment 130808 [details] Patch

I know the patch is quite big but all files that has the name Deprecated in them are just renames of existing files.

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

(In reply to comment #0) > This patch deprecates the current Peerconnection by renaming it to DeprecatedPeerConnection and adds a new PeerConnection implementation based on: I would have preferred to call the new files JSEPPeerConnection or ExperimentalPeerConnection since the API discussion is still ongoing in W3C. The one you call DeprecatedPeerConnection is the one in the current specification (http://dev.w3.org/2011/webrtc/editor/webrtc.html).

The RTCWEB / WEBRTC consensus seems to go with JSEP: http://www.w3.org/2012/02/01-webrtc-minutes.html#item03 (Mountain View meeting) ended up with going with JSEP unless alternatives were found. No alternatives have been proposed to date, and the main ROAP author is now a co-author of JSEP. See http://www.ietf.org/proceedings/interim/2012/01/31/rtcweb/minutes/rtcweb.txt for the RTCWEB discussion. It's taken a while to get the editors to do the editing.

(In reply to comment #5) > The RTCWEB / WEBRTC consensus seems to go with JSEP: > > http://www.w3.org/2012/02/01-webrtc-minutes.html#item03 (Mountain View meeting) ended up with going with JSEP unless alternatives were found. No alternatives have been proposed to date, and the main ROAP author is now a co-author of JSEP. It's true that there's consensus around the JSEP model. However the discussion around the API, to interface with JSEP, is still ongoing - that's why I suggested to use the "Experimental" prefix at this point.

My take on this is that the API will change to a new model and therefore the old API will be deprecated. Therefore the renaming of current PeerConnection to DeprecatedPeerConnection is correct imho.

> It's true that there's consensus around the JSEP model. However the discussion around the API, to interface with JSEP, is still ongoing - that's why I suggested to use the "Experimental" prefix at this point. The risk of using a name like JSEPPeerConnection is that we'll be stuck with that as the final name for the API. If we us PeerConnection for JSEP and change the existing API to DeprecatedPeerConnection, there's no risk of getting stuck with the wrong name for the final API. Also, using this naming arrangement encourages folks to move over to the new API, making it easier for us to remove DeprecatedPeerConnection when that's appropriate.

Comment on attachment 130808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130808&action=review This patch is much to big to review. I noted a few minor issues. Mostly we need to divide this patch into many smaller pieces. For example, I'd start with one patch that just renames the existing APIs. Then I'd introduce the new interfaces in a series of patches. > Source/WebCore/Modules/mediastream/IceCallback.h:43 > +class IceCallback : public RefCounted<IceCallback> { I would have expected this to inherit from one of our callback base classes, but maybe that's not necessary anymore. > Source/WebCore/Modules/mediastream/IceCandidate.h:61 > + RefPtr<IceCandidateBase> m_base; Why isn't this a base class? > Source/WebCore/Modules/mediastream/IceCandidate.idl:41 > + // the m= line this candidate is associated with > + readonly attribute DOMString label; > + > + // creates a SDP-ized form of this candidate > + DOMString toSdp(); Generally we write comments like these in IDL files. That's what the specs are for. :) > Source/WebCore/Modules/mediastream/MediaHints.h:60 > + RefPtr<MediaHintsBase> m_base; I don't really understand this pattern. Why are these "base" objects being held as members? > Source/WebCore/Modules/mediastream/PeerConnection.cpp:375 > +bool PeerConnection::canSuspend() const > +{ > + return true; > +} Suspending is really complex. Are you sure you're implementing this correctly? I'd recommend just returning false for now and worrying about suspension later.

It looks like your patch is 196.18 KB. The tool that creates the patch file is supposed to warning you whenever you create a patch that's larger than 20KB. It looks like you're over that guideline by an order of magnitude. (Of course, not all patches can be small. For example, I'd expect the renaming patch to be larger---but that's easy to rubber-stamp as long as you're just moving code around and not introducing new code.)

Comment on attachment 130808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130808&action=review >> Source/WebCore/Modules/mediastream/IceCallback.h:43 >> +class IceCallback : public RefCounted<IceCallback> { > > I would have expected this to inherit from one of our callback base classes, but maybe that's not necessary anymore. No, the person that worked on the IDL parser simplified things a lot. Specifying "Callback" in the IDL fixes everything else. >> Source/WebCore/Modules/mediastream/IceCandidate.h:61 >> + RefPtr<IceCandidateBase> m_base; > > Why isn't this a base class? It is the split into WebCore proper and WebCore platform that complicates things. Since IceCandidates and SessionDescriptions can be created both from the embedder and JavaScript this kind of pattern is necessary. We do the same with MediaStream and MediaStreamDescriptor but since SessionDescriptionDescriptor is kind of clunky I choose to use *Base instead. In hindsight that was probably more confusing than helpful. >> Source/WebCore/Modules/mediastream/IceCandidate.idl:41 >> + DOMString toSdp(); > > Generally we write comments like these in IDL files. That's what the specs are for. :) Yeah! Comments in WebKit. Will fix. >> Source/WebCore/Modules/mediastream/MediaHints.h:60 >> + RefPtr<MediaHintsBase> m_base; > > I don't really understand this pattern. Why are these "base" objects being held as members? See above. >> Source/WebCore/Modules/mediastream/PeerConnection.cpp:375 >> +} > > Suspending is really complex. Are you sure you're implementing this correctly? I'd recommend just returning false for now and worrying about suspension later. Fixed.

The renaming patch is out now (bug 80962) and is ~145KB. And btw, the script doesn't complain in the least. (In reply to comment #10) > It looks like your patch is 196.18 KB. The tool that creates the patch file is supposed to warning you whenever you create a patch that's larger than 20KB. It looks like you're over that guideline by an order of magnitude. > > (Of course, not all patches can be small. For example, I'd expect the renaming patch to be larger---but that's easy to rubber-stamp as long as you're just moving code around and not introducing new code.)

(In reply to comment #8) > The risk of using a name like JSEPPeerConnection is that we'll be stuck with that as the final name for the API. If we us PeerConnection for JSEP and change the existing API to DeprecatedPeerConnection, there's no risk of getting stuck with the wrong name for the final API. Isn't the final name pending until the prefixes are removed anyhow? But I agree that JSEPPeerConnection is bad. ExperimentalPeerConnection would be better IMO. > Also, using this naming arrangement encourages folks to move over to the new API, making it easier for us to remove DeprecatedPeerConnection when that's appropriate. The discussion is still ongoing in W3C on which changes JSEP will have on the API. It shouldn't prevent anyone from prototyping though.

Adam: I'm going to reply to your comment on Bug 80692 because that's where the rename patch is now.

Closing this bug since RTCPeerConnection has now landed, and the old ROAD based implementation has been deleted.