This helps getting getStats' dictionaries to/from c++, for backend in bug 902003. Patch coming.

- After working out how to represent constraints in webidl in bug 882145, do the same for PeerConnection's constraints. - Had to relax three of the mochitests surrounding mixing [] and {} as they were stricter than webidl. - The structural checking of mandatory constraints will improve from changes in the next patch in this bug (too hard to explain).

Rebased. Try (this patch only) - https://tbpl.mozilla.org/?tree=Try&rev=dc4678d70eb6

Working patch. - C++ PeerConnectionImpl is now a webidl object - JS PeerConnectionObserver is now a webidl object - Forward-declared everything in PeerConnectionImpl.h (Bug 785103) - No longer opens mozilla namespace in PeerConnectionImpl.h (Bug 798033) - Refactored MediaConstraints top-down to be specific all the way down (Bug 811360) - Fixed queueOrRun() calls in PeerConnection.js to work with concrete methods. - Replaced weakRef to PCObserver (which won't work on concretes) with a nullable raw pointer construct, which is fine because lifetime is controlled (can't use cycle-collection because we're multi-threaded) - No longer uses MediaStreamList (but see known bug below) - Minor PeerConnectionImpl refactor to keep some deep c-includes out of bindings - Moved relevant enums to (separate) webidl, and am using them in unittests - Added USE_FAKE_PCOBSERVER swap-in for unittests (an abstract c++ class) - Added NS_IMETHODIMP_TO_ERRORRESULT to minimize refactor and preserve blame - Added MediaConstraintsExternal as we can't use bindings in unittests (nsString!!) - Added PCObserverString typedef to abstract UTF-8/UTF-16 - Added WrappableJSErrorResult to handle ErrorResult with WrapRunnable - All unittests pass and no leaks on regular use I know of one bug in pc.localStream, which prevents a successful try. I hope to fix this soon, but I decided to upload to unblock, and there's plenty to review.

Removed a dangling #include

It's not clear to me how this bug helps with bug 785103.

I ended up having to forward-declare a lot of stuff in PeerConnectionImpl.h instead of including it, in order to get it to compile with bindings. I'm not sure if it helps in the larger picture.

Comment on attachment 813041 [details] [diff] [review] First, update PeerConnection's constraints to webidl (2) > for (let constraint in constraints.mandatory) { ... > constraints.mandatory.hasOwnProperty(constraint)) { How about: for (let constraint of Object.keys(constraints.mandatory)) { and then you'll only get own properties. r=me with that, I think.

Which parts of the big patch do you particularly want me to review? Reviewing the whole thing seems a bit pointless, since I'm not that familiar with the relevant code.

Sorry for not breaking it up better. If you could review all .webidl and .idl tiles, plus Bindings.config, PeerConnection.manifest and PeerConnection.js that would be great (the patch is functionally neutral, moving stuff from .idl to webidl. Some enums also moved from PeerConnectionImpl.h). You should probably also look at class WrappableJSErrorResult in PeerConnectionImpl.cpp. Thanks!

(In reply to comment #6) > I ended up having to forward-declare a lot of stuff in PeerConnectionImpl.h > instead of including it, in order to get it to compile with bindings. I'm not > sure if it helps in the larger picture. That is *much* appreciated! :-)

Fixed nit. Carrying forward r+ from bz and jesup. Thanks!

I put the wrong description in bugzilla. Re-uploading to avoid confusion.

Rebased, and fixed the getLocal/RemoteStream calls, which were JS typos.

getLocal/RemoteStream calls now actually work (apparently we don't have tests for these).

Check-in needed, first patch only.

Backed out for failing mochitest-2 on all platforms: https://tbpl.mozilla.org/php/getParsedLog.php?id=28877783&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=28878057&tree=Mozilla-Inbound https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9ff97dd09df4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/17411d6192e4 Were the tests run locally?

(In reply to Ed Morley [:edmorley UTC+1] from comment #17) > Were the tests run locally? Yes, on Oct 2nd (see Comment 2). Unfortunately, the test that failed is from Oct 3rd: http://hg.mozilla.org/mozilla-central/rev/b7c5cd333e9c - The test is correct, the patch no longer detects unknown mandatory constraints, because it changed to an approach that only works once everything here is webidl, which is the second patch (I had both applied locally when I tested again recently, so it worked for me, sorry). The solution seems to be to land the patches together once the second one is reviewed.

Ah! Thank you :-)

Compiles on Windows and B2G. - Avoids __stdcall .h/.cpp mixup for PeerConnectionImpl::Initialize on Windows - Avoids ?: operator type confusion with webidl enums on B2G. (see https://tbpl.mozilla.org/?tree=Try&rev=4c48bf39734e for errors) These improvements aside, I'm getting odd errors on try at the moment https://tbpl.mozilla.org/?tree=Try&rev=38cb363ce2c1 something about timecard.h not found. Perhaps unrelated or something broke my patch? - Will investigate.

Actually, it was just the exception text being different that broke inbound (the new test checks it). Reverting text from "unsupported" to "unknown" fixes it. Carrying forward r+ from bz and jesup.

Please feel free to adjust that test as needed if you want to change the text of the message.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20) > I'm getting odd errors on try at the moment ... timecard.h not found. Fixed by adding media/webrtc/signaling/src/common/time_profiling to dom/bindings/Makefile.in. Try - https://tbpl.mozilla.org/?tree=Try&rev=472f86e17bbc

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #22) > Please feel free to adjust that test as needed if you want to change the > text of the message. Sure thanks. I think "unknown" is the correct super-set here.

Comment on attachment 815215 [details] [diff] [review] Second, convert PeerConnectionImpl and PeerConnectionObserver to webidl (6) >+ this._queueOrRun({ func: this._close, args: [false], wait: false }); this._close takes no args, right? >- _signalingStateMap: [ >- 'invalid', Why the change from 'invalid' to "" for the invalid case? >+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp >+ for (int i = 0; i < int(array.Length()); i++) { Should probably use uint32_t instead. >+class JSErrorResult : public ErrorResult >+ WouldReportJSException(); No, that's bad. The contract for this is supposed to be in ErrorResult.h: // Code that would call ReportJSException* or // StealJSException as needed must first call WouldReportJSException even if // this ErrorResult has not failed. Maybe it should be more clear: calling WouldReportJSException() promises that you _will_ in fact call one of those other methods as needed, which JSErrorResult is definitely not doing. In this case that means a dangling root pointer in the JS engine pointing to the stack location of mJSException, which is bad. You really want to call StealJSException() as needed here or something. Or we should add a new ForgetJSException, perhaps... Either way you'll need to get a JSContext or at least JSRuntime in the destructor, right? Also, I suggest putting JSErrorResult in an anonymous namespace. >+class WrappableJSErrorResult { >+ WrappableJSErrorResult(WrappableJSErrorResult &other) : mRv() {} I don't understand this constructor. Why does it exist? It doesn't seem to be a useful copy constructor... It's worth documenting what the point of this class is, I think. As things stand, I can't tell what it's for. >+ nsIDOMDataChannel *retval; >+ rv = NS_NewDOMDataChannel(dataChannel.forget(), mWindow, &retval); You want to initialize retval to null before calling NS_NewDOMDataChannel, I think. >+ return CreateOffer(MediaConstraintsExternal(ConvertConstraints(aConstraints))); What ends up freeing the memory ConvertConstraints() allocates? >+ rv, static_cast<JSCompartment*>(nullptr)), Do you need the JSCompartment* bit even though that argument is optional? >+ NS_IMETHODIMP_TO_ERRORRESULT_RET(nsDOMDataChannel*, This leaks the nsDOMDataChannel, no? In particular, callee addref it, but the function that macro will declare does not return an already_AddRefed<nsDOMDataChannel>. >+#undef NS_IMETHODIMP_TO_ERRORRESULT What about NS_IMETHODIMP_TO_ERRORRESULT_RET? r- because of the JSErrorResult bits and the leak. I didn't go through most of the details of the PeerConnectionImpl.cpp/h changes or the test bits; I assume rjesup will do that.

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #25) > >+ this._queueOrRun({ func: this._close, args: [false], wait: false }); > > this._close takes no args, right? Yes, though this is unchanged code. I'll try this._queueOrRun({ func: this._close, args: [], wait: false }); > >- _signalingStateMap: [ > >- 'invalid', > > Why the change from 'invalid' to "" for the invalid case? http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCSignalingState doesn't have an "invalid" state, but we seem to use one internally (only), so leaving it leaves the indexes the same as before, and there's at least one test against it in signaling_unittest.cpp, probably used to mean unset: > void SetRemote(TestObserver::Action action, std::string remote, > bool ignoreError = false, > PCImplSignalingState endState = > PCImplSignalingState::SignalingInvalid) { > > if (endState == PCImplSignalingState::SignalingInvalid) { > endState = (action == TestObserver::OFFER ? > PCImplSignalingState::SignalingHaveRemoteOffer : > PCImplSignalingState::SignalingStable); > } > > pObserver->state = TestObserver::stateNoResponse; > ASSERT_EQ(pc->SetRemoteDescription(action, remote.c_str()), NS_OK); > ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse, > kDefaultTimeout); > ASSERT_EQ(signaling_state(), endState); From what I can tell, this value should never get out of the API, so I felt leaving it as "" seemed like a reasonable compromise short of refactoring it out. Thoughts? > >+class JSErrorResult : public ErrorResult > >+ WouldReportJSException(); > > No, that's bad. The contract for this is supposed to be in ErrorResult.h: > > // Code that would call ReportJSException* or > // StealJSException as needed must first call WouldReportJSException even > if > // this ErrorResult has not failed. > > Maybe it should be more clear: calling WouldReportJSException() promises > that you _will_ in fact call one of those other methods as needed, which > JSErrorResult is definitely not doing. In this case that means a dangling > root pointer in the JS engine pointing to the stack location of > mJSException, which is bad. You really want to call StealJSException() as > needed here or something. Or we should add a new ForgetJSException, > perhaps... Either way you'll need to get a JSContext or at least JSRuntime > in the destructor, right? Ok, things get dispatched and executed later, so not sure where to get a JSContext from. Is AutoJSContext ok here? I'm essentially trying to ignore errors thrown by the observer. > >+class WrappableJSErrorResult { > >+ WrappableJSErrorResult(WrappableJSErrorResult &other) : mRv() {} > > I don't understand this constructor. Why does it exist? It doesn't seem to > be a useful copy constructor... > > It's worth documenting what the point of this class is, I think. As things > stand, I can't tell what it's for. The WrapRunnable() macros copy passed-in args and passes them to the function later on the other thread. ErrorResult cannot be passed like this because it disallows copy-semantics. The WrappableJSErrorResult hack solves this by not actually copying the ErrorResult, but creating a new one instead, which works because we don't care about the result. I'll add this as a comment. > >+ nsIDOMDataChannel *retval; > >+ rv = NS_NewDOMDataChannel(dataChannel.forget(), mWindow, &retval); > > You want to initialize retval to null before calling NS_NewDOMDataChannel, I think. Can NS_NewDOMDataChannel succeed without filling it in? Does it rely on it being nulled? > >+ return CreateOffer(MediaConstraintsExternal(ConvertConstraints(aConstraints))); > > What ends up freeing the memory ConvertConstraints() allocates? fsmdef_free_constraints() in media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c frees this c structure. This is comparable to before, though this patch simplifies things to a single allocation. > >+ rv, static_cast<JSCompartment*>(nullptr)), > > Do you need the JSCompartment* bit even though that argument is optional? Yes, or I get http://mxr.mozilla.org/mozilla-central/source/media/mtransport/runnable_utils_generated.h#125 error: too few arguments to function call, expected 2, have 1 ((*o_).*m_)(a0_); Looks like VARARG macros need the full set of args. > >+ NS_IMETHODIMP_TO_ERRORRESULT_RET(nsDOMDataChannel*, > > This leaks the nsDOMDataChannel, no? In particular, callee addref it, but > the function that macro will declare does not return an > already_AddRefed<nsDOMDataChannel>. Good catch, thanks! (yes it did leak) > >+#undef NS_IMETHODIMP_TO_ERRORRESULT > > What about NS_IMETHODIMP_TO_ERRORRESULT_RET? Whoops, thanks! All other comments have been addressed.

Comment on attachment 815490 [details] [diff] [review] Second, convert PeerConnectionImpl and PeerConnectionObserver to webidl (7) >+ Optional<JS::Handle<JS::Value> > value(cx); >+ StealJSException(cx, &value.Value()); JS::Rooted<JS::Value> value(cx); StealJSException(cx, &value); I do think AutoJSContext is ok here, as long as this always runs on the main thread. I worry about that "as long as" given your comments on WrapRunnable... > Can NS_NewDOMDataChannel succeed without filling it in? Does it rely on it > being nulled? "No", and "maybe" in that order.

Comment on attachment 815490 [details] [diff] [review] Second, convert PeerConnectionImpl and PeerConnectionObserver to webidl (7) r=me with a on-mainthread assert added to WrappableJSErrorResult::~WrappableJSErrorResult if it was constructed with the copy ctor.

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #28) > r=me with a on-mainthread assert added to > WrappableJSErrorResult::~WrappableJSErrorResult if it was constructed with > the copy ctor. Added. Carrying forward r+ from bz.

Try looks green (all oranges appear unrelated): https://tbpl.mozilla.org/?tree=Try&rev=87e513efb537

Unbitrotted signaling_unittests.cpp (since bug 902003 depends on it). Carrying forward r+ from bz. Leaving open previous version for review.

Comment on attachment 816595 [details] [diff] [review] Second, convert PeerConnectionImpl and PeerConnectionObserver to webidl (9) r=bz Review of attachment 816595 [details] [diff] [review]: ----------------------------------------------------------------- I assume you've manually run all the unit tests as well ::: dom/media/PeerConnection.js @@ +690,5 @@ > if(this._closed) { > return "closed"; > } > + return { > + "SignalingInvalid": "", What's the reason for changing this from "invalid" to ""? ::: dom/webidl/PeerConnectionImpl.webidl @@ +68,5 @@ > + /* Data channels */ > + [Throws] > + DataChannel createDataChannel(DOMString label, DOMString protocol, > + unsigned short type, boolean outOfOrderAllowed, > + unsigned short maxTime, unsigned short maxNum, We should get the W3 to change these to uint32's, since the underlying protocol has been changed - but currently that's the spec, so it's fine for now. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +699,5 @@ > MOZ_ASSERT(pcctx); > STAMP_TIMECARD(mTimeCard, "Done Initializing PC Ctx"); > > + mInternal->mCall = pcctx->createCall(); > + if(!mInternal->mCall.get()) { space after if (check others in patch too) @@ +1301,5 @@ > + if (pcctx) { > + *aState = pcctx->sipcc_state(); > + } else { > + *aState = PCImplSipccState::Idle; > + } Couldn't you also cast one of them to the matching type? Not important, however

(In reply to Randell Jesup [:jesup] from comment #32) > I assume you've manually run all the unit tests as well I did earlier and they all passed, though thanks for reminding me to run them again, as the main-thread assert I added recently broke signaling_unittest. I've #ifdef'ed it out for unit-tests, and it passes again. The TURN tests all fail on me now, but they fail without these patches as well, so it appears unrelated (perhaps my TURN_SERVER_ADDRESS is outdated?) The STUN ones succeed, so I suspect we are good to go here. > What's the reason for changing this from "invalid" to ""? See comment 26. > @@ +1301,5 @@ > > + if (pcctx) { > > + *aState = pcctx->sipcc_state(); > > + } else { > > + *aState = PCImplSipccState::Idle; > > + } > > Couldn't you also cast one of them to the matching type? Not important, > however Our C++11 enum class shim looks quite elaborate to me, so I felt it safer to steer clear of any assumptions about it, for fear of generating the wrong code in some cases. All other comments addressed. Thanks! Carrying forward r+ from bz and rjesup.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6417162f58a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/e93223d403fe

https://hg.mozilla.org/mozilla-central/rev/6417162f58a3 https://hg.mozilla.org/mozilla-central/rev/e93223d403fe

Traceback (most recent call last): File "/home/markus/mozilla-central/config/pythonpath.py", line 56, in <module> main(sys.argv[1:]) File "/home/markus/mozilla-central/config/pythonpath.py", line 48, in main execfile(script, frozenglobals) File "/home/markus/mozilla-central/dom/bindings/GlobalGen.py", line 81, in <module> main() File "/home/markus/mozilla-central/dom/bindings/GlobalGen.py", line 56, in main parserResults = parser.finish() File "/home/markus/mozilla-central/dom/bindings/parser/WebIDL.py", line 4849, in finish production.finish(self.globalScope()) File "/home/markus/mozilla-central/dom/bindings/parser/WebIDL.py", line 642, in finish member.finish(scope) File "/home/markus/mozilla-central/dom/bindings/parser/WebIDL.py", line 3196, in finish type = returnType.complete(scope) File "/home/markus/mozilla-central/dom/bindings/parser/WebIDL.py", line 1448, in complete [self.location]) WebIDL.WebIDLError: error: Unresolved type '<unresolved scope>::DataChannel'., PeerConnectionImpl.webidl line 70:2 DataChannel createDataChannel(DOMString label, DOMString protocol, ^ make[5]: *** [ParserResults.pkl] Error 1 make[5]: Leaving directory `/var/tmp/moz-build-dir/dom/bindings'

(In reply to Octoploid from comment #36) > WebIDL.WebIDLError: error: Unresolved type '<unresolved > scope>::DataChannel'., PeerConnectionImpl.webidl line 70:2 > DataChannel createDataChannel(DOMString label, DOMString protocol, > ^ Are you building with --disable-webrtc? If so, see Bug 926799. Hopefully that patch will land today. Sorry about that.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #37) > (In reply to Octoploid from comment #36) > > WebIDL.WebIDLError: error: Unresolved type '<unresolved > > scope>::DataChannel'., PeerConnectionImpl.webidl line 70:2 > > DataChannel createDataChannel(DOMString label, DOMString protocol, > > ^ > > Are you building with --disable-webrtc? If so, see Bug 926799. Hopefully > that patch will land today. Sorry about that. Yes. Sorry for just dumping my log here.