An idea from Martin, this would streamline things nicely in PeerConnection.js' dealings with the c++ code and remove the internal PeerConnectionObserver interface. However, with the existing limitatons in the c++ code, like having to compile to compile outside of #define MOZ_INTERNAL_API for the webrtc c++ unit-tests we run locally, it's less clear that this would be a win in the c++ code. We'd likely need to make a FakePromise or something (to replace FakePCObserver), since so little is available outside of MOZ_INTERNAL_API. Furthermore, this would likely affect c++ users of PeerConnection (wasn't there work on PeerConnection as a library?). Also, the c++ Promise class does not appear to be a template and seems aimed primarily at use from JS, e.g. assumes JSContext and operates in JS::Value's.

I don't much like PCObserver, but you can't replace it entirely with promises, since it is also used for notification of events which happen repeatedly, like state changes.

Note also that one of the problems here is the ownership relationship between the PCImpl and the JS. The PCImpl is effectively holding a weak pointer to the JS objects that it interacts with (thus avoiding the creation of a circular reference) but that means that you need to check for liveness of those JS objects prior to referencing them. The observer is what mediates that interaction

The remaining notifications would be onStateChange, onIceCandidate, onAddStreamTrack/(Stream), onRemoveTrack/(Stream) and notifyDataChannel. We could leave them alone, or we could move them to the main PeerConnection JS object as individual ChromeOnly methods. It should work the same, with one less indirection. RTCPeerConnection has strong references to PCImpl and the observer, so weak-references to RTCPeerConnection should work the same as to the observer. The observer doesn't really add anything that I can see other than some organization, which is less necessary once most of its guts are gone.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3) > The remaining notifications would be onStateChange, onIceCandidate, > onAddStreamTrack/(Stream), onRemoveTrack/(Stream) and notifyDataChannel. We > could leave them alone, or we could move them to the main PeerConnection JS > object as individual ChromeOnly methods. It should work the same, with one > less indirection. That seems worse, not better. > RTCPeerConnection has strong references to PCImpl and the observer, so > weak-references to RTCPeerConnection should work the same as to the observer. Yes, but you'll also need to have weak pointers to the promises. The easy thing would probably be to simply have a notion of a weak pointer to a callback object. > The observer doesn't really add anything that I can see other than some > organization, which is less necessary once most of its guts are gone. I would certainly agree if you were to remove the entire notion of an object with a bunch of event methods. However, if you're going to have some event methods, I don't see any value in moving them to be on the main object.

(In reply to Eric Rescorla (:ekr) from comment #4) > (In reply to Jan-Ivar Bruaroey [:jib] from comment #3) > > We could leave them alone, or we could move them to the main PeerConnection > > JS object as individual ChromeOnly methods. It should work the same, with one > > less indirection. > > That seems worse, not better. It doesn't strike me as a big difference, but I think less code is better and when the observer has to go through RTCPeerConnection anyway for each method to dispatch events, it's one less wrapper. Historically, the observer inherited from a separate XPCom interface (IPeerConnectionObserver) that c++ accessed, but now that it's in WebIDL it's difference from RTCPeerConnection itself seems marginal. > > RTCPeerConnection has strong references to PCImpl and the observer, so > > weak-references to RTCPeerConnection should work the same as to the observer. > > Yes, but you'll also need to have weak pointers to the promises. I think we actually want strong pointers there. In the case where the PeerConnection is *not* closed and the JS holds no explicit reference to the main PeerConnection object, we currently risk the PeerConnection getting garbage-collected before its promise has been resolved, causing it to never resolve, which seems wrong.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5) > (In reply to Eric Rescorla (:ekr) from comment #4) > > (In reply to Jan-Ivar Bruaroey [:jib] from comment #3) > > > We could leave them alone, or we could move them to the main PeerConnection > > > JS object as individual ChromeOnly methods. It should work the same, with one > > > less indirection. > > > > That seems worse, not better. > > It doesn't strike me as a big difference, but I think less code is better > and when the observer has to go through RTCPeerConnection anyway for each > method to dispatch events, it's one less wrapper. Historically, the observer > inherited from a separate XPCom interface (IPeerConnectionObserver) that c++ > accessed, but now that it's in WebIDL it's difference from RTCPeerConnection > itself seems marginal. > > > > RTCPeerConnection has strong references to PCImpl and the observer, so > > > weak-references to RTCPeerConnection should work the same as to the observer. > > > > Yes, but you'll also need to have weak pointers to the promises. > > I think we actually want strong pointers there. This has the potential to create circular references which means that you need cycle collection, which it appears we do not currently have. http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h?from=PeerConnectionImpl&case=true#680 > In the case where the > PeerConnection is *not* closed and the JS holds no explicit reference to the > main PeerConnection object, we currently risk the PeerConnection getting > garbage-collected before its promise has been resolved, causing it to never > resolve, which seems wrong. Why would that be wrong? If you create a series of structures which don't point to anything but themselves, they should generally be garbage collected.

tl;dr: Unless and until we get Promise classes in C++ that are sensible in a C++-only context, we need to stick with the current callback model. So, the one thing I'd be careful about here is any move that makes the C++ interface to PeerConnection difficult to use. If the current Promise class is geared primarily towards mirroring JS promises (and does things like assume a JS context), I would strongly suggest we avoid rolling it into the PeerConnectionImpl class (or anything else on the C++ side of the interface). I'll point out that the prospect of using the C++ PeerConnection outside of Gecko remains feasible at this point (and looks even more attractive with the landing of SDParta), and that there are currently active projects that do exactly this. Aside from the unnecessary churn that this change would make for them, I'm really worried that forcing these projects to use the Promise objects as they currently exist would be unrealistic (I would hate to force these projects to muck with JS::Value, for example). Once we establish that the C++ API shouldn't change, I'm somewhat ambivalent about introducing Promises here. I mean, they're nice and all, but I question whether the churn is worth the value (which I'll admit I don't completely understand in this context). If this work goes forward, I think we want to isolate it to PeerConnection.js, where you can adapt the one-shot event handlers into Promises by wrapping them, if you so desire. As I said, I'm not completely sure what you think the value proposition is here for converting to Promises, but I suspect this dilutes it somewhat - which probably argues for keeping the status quo.

Excellent point, Adam. Our objective should be an internal PeerConnection API that is insulated as much as possible from JS, not to intertwine JS with PCImpl

(In reply to Eric Rescorla (:ekr) from comment #6) > This has the potential to create circular references which means that > you need cycle collection, which it appears we do not currently have. > > http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/ > peerconnection/PeerConnectionImpl.h?from=PeerConnectionImpl&case=true#680 I had to think about this some, but I think temporal cycles are ok, and that we only need to participate in cycle-collection when there's a risk of a cycle keeping something alive permanently or semi-permanently. I don't think there's that risk here because the promise's lifespan is tied to the c++ method finishing, failing or getting closed before either of those things happen, at which point mPromise would be released, ending any cycle. > Why would that be wrong? If you create a series of structures which > don't point to anything but themselves, they should generally be > garbage collected. I think in general people rely on asynchronous browser-chrome methods to finish even with zero other references. E.g. http://jsfiddle.net/jib1/5uLhhdy9

I agree with Adam that we need to consider the c++ interface, and that JS::Value is probably undesirable here as I touch on in comment 0. I know bholley has added a MediaPromise in Bug 1097823, but I know very little about it, or whether it has any overlap with the Promise class.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #9) > I think in general people rely on asynchronous browser-chrome methods to > finish even with zero other references. E.g. http://jsfiddle.net/jib1/5uLhhdy9 Although, as I write this, I guess the real question should be if the window had zero references, would setTimeout still finish?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #10) > I agree with Adam that we need to consider the c++ interface, and that > JS::Value is probably undesirable here as I touch on in comment 0. I know > bholley has added a MediaPromise in Bug 1097823, but I know very little > about it, or whether it has any overlap with the Promise class. MediaPromises have nothing to do with DOM promises - they're a C++ asynchronous message passing tool. DOM promises are inherently JS-y. If you just need to use them in code with external linkage, you might want to look at bug 1011642.

(In reply to Bobby Holley (:bholley) from comment #12) > DOM promises are inherently JS-y. Are they? I guess I find this surprising given that WebIDL is type-specific about promise values. e.g.: Promise<mozRTCSessionDescription> createAnswer (); and wherever else types are available like this, we seem to generate bindings that map to type-safe c++ calls, but not here. Couldn't the Promise class have been a c++ template that took ResultType as its template argument? Would it then not be usable from c++ code (inside libxul) as well as JS code? > If you just need to use them in code with external linkage, you might > want to look at bug 1011642. Thanks for the link, though from that patch, it looks like it is still type-agnostic: * This class wraps a mozilla::dom::Promise so that it is usable from non-libxul * code. To avoid the use of JSAPI, this implementation uses nsIVariant instead * of JS::Value to represent the resolution values. In addition, all rejection * values are converted to nsresult error codes.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #13) > and wherever else types are available like this, we seem to generate > bindings that map to type-safe c++ calls, but not here. Couldn't the Promise > class have been a c++ template that took ResultType as its template > argument? Promises themselves are inherently weakly typed per spec. But we could have a wrapper or subclassing setup to create TypedPromises. It would just be a bit of work on the codegen side. NI Boris in case he has any thoughts on the matter.

Promises in the web platform are defined by the JS spec. So they're as JS-y as things get, in theory. In our impl they're actually not JS-y enough to follow the spec completely; at some point they will move into SpiderMonkey, and then they will be _really_ JS-y. At that point, of course, we'll try to preserve the convenience APIs we have for them right now (e.g. you don't need to have a JSContext or worry about JS anything to resolve or reject a promise today, or to create one!). In any case, given that you _can_ create, resolve and reject promises without interacting with JS today, as well as add callbacks to a promise without interacting with JS, what exactly is the usability from C++ code issue? The only requirement is that the type you resolve the promise with must be convertible to a JS representation, but your code doesn't have to do that conversion; the promise code handles it for you. As for IDL, the type inside Promise<> is just annotation for spec readers. Nothing in the IDL spec enforces it. We could try to do something more complicated internally with templates, but it's not that simple. For example, a reasonable expectation is that a Promise<Node> be usable as an argument to a function that expects Promise<any>, but C++ templates can't do the whole covariant type thing, so wouldn't be able to support this use case. As far as comment 7 goes, past experiments in having parts of Gecko be "reusable" (necko, imagelib) largely led to maintenance nightmares where Gecko couldn't get the behavior it wanted from those components plus performance sucked. It's only since we gave up on that pipe dream that we've been able to get any progress at all on Gecko/necko and Gecko/imagelib interactions. There's similar friction with SpiderMonkey, by the way; there is performance and simplicity fruit we're leaving on the table to enable non-Gecko SpiderMonkey embeddings... There are similar problems with moz2d, too. So I view any claim that we want something to be reusable outside Gecko and therefore this should affect how Gecko interacts with it somewhat suspiciously. That said, I haven't looked into the details in this case, so this is just my gut feeling, not data.

(In reply to :bz (Vacation Dec 15-31) from comment #15) > As far as comment 7 goes, past experiments in having parts of Gecko be > "reusable" (necko, imagelib) largely led to maintenance nightmares where > Gecko couldn't get the behavior it wanted from those components plus > performance sucked. It's only since we gave up on that pipe dream that > we've been able to get any progress at all on Gecko/necko and Gecko/imagelib > interactions. There's similar friction with SpiderMonkey, by the way; there > is performance and simplicity fruit we're leaving on the table to enable > non-Gecko SpiderMonkey embeddings... There are similar problems with moz2d, > too. So I view any claim that we want something to be reusable outside > Gecko and therefore this should affect how Gecko interacts with it somewhat > suspiciously. That said, I haven't looked into the details in this case, so > this is just my gut feeling, not data. I view them suspiciously as well, however: rbarker has been working to create a standalone version of our webrtc code to enable creating Loop/Hello standalone clients that don't need the entire browser and can be implemented as apps on iOS (and perhaps Android or other standalone non-browser devices). So there *may* be a valid reason for this to be separable here - or to minimize the pain in separation, of which rbarker has run into plenty with xpcom/etc.

(In reply to Randell Jesup [:jesup] from comment #16) > (In reply to :bz (Vacation Dec 15-31) from comment #15) view them suspiciously as well, however: > > rbarker has been working to create a standalone version of our webrtc code > to enable creating Loop/Hello standalone clients that don't need the entire > browser and can be implemented as apps on iOS (and perhaps Android or other > standalone non-browser devices). I think the key point from bz is to not let the fact that we want a reusable module change how Gecko and that module interact. Some changes are required such as having interfaces that these modules interact with to wrap gecko internals so you don't wind up with spaghetti dependencies, but the core interaction model shouldn't change to satisfy the desire to have a standalone/reusable module.

It's worth noting that we already have a worked example that it's possible to build a standalone library here, as that's how Google's WebRTC library does. I've also heard quite substantial demand for a standalone version of our stack, and I think we can do it with an interaction model relatively similar to the current one, but that has minimal gecko dependencies.

(In reply to Randell Jesup [:jesup] from comment #16) > (In reply to :bz (Vacation Dec 15-31) from comment #15) > > > As far as comment 7 goes, past experiments in having parts of Gecko be > > "reusable" (necko, imagelib) largely led to maintenance nightmares where > > Gecko couldn't get the behavior it wanted from those components plus > > performance sucked. It's only since we gave up on that pipe dream that > > we've been able to get any progress at all on Gecko/necko and Gecko/imagelib > > interactions. There's similar friction with SpiderMonkey, by the way; there > > is performance and simplicity fruit we're leaving on the table to enable > > non-Gecko SpiderMonkey embeddings... There are similar problems with moz2d, > > too. So I view any claim that we want something to be reusable outside > > Gecko and therefore this should affect how Gecko interacts with it somewhat > > suspiciously. That said, I haven't looked into the details in this case, so > > this is just my gut feeling, not data. > > I view them suspiciously as well, however: > > rbarker has been working to create a standalone version of our webrtc code > to enable creating Loop/Hello standalone clients that don't need the entire > browser and can be implemented as apps on iOS (and perhaps Android or other > standalone non-browser devices). > > So there *may* be a valid reason for this to be separable here - or to > minimize the pain in separation, of which rbarker has run into plenty with > xpcom/etc. I currently have the majority of the WebRTC unit test running using the standalone XPCOM and some Necko bits. Having a standalone WebRTC library will definitely require a C++ interface since standalone is not planned to be run with a JS engine.

Mass change P4->P5 to align with new Mozilla triage process.