1075133 - onsignalingstatechange is not fired as an event

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Byron Campen [:bwc]]

onsignalingstatechange is passed the signaling state, and not an Event as required by the specification. Additionally, the mochitests expect this non-standard behavior.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: fire onsignalingstatechange as an event

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee33501ae46e

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8523372 [details] [diff] [review]
fire onsignalingstatechange as an event

Review of attachment 8523372 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +104,2 @@
>  
> +    finished.then(networkTestFinished);

So this is where |finish| is set? I would expect the signalingstatechange event to fire before we get down here. Wouldn't that mean |finish| isn't set yet when we call it?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Byron Campen [:bwc] from comment #2)
> > +    finished.then(networkTestFinished);
> 
> So this is where |finish| is set? I would expect the signalingstatechange
> event to fire before we get down here. Wouldn't that mean |finish| isn't set
> yet when we call it?

|finish| is the resolve-function of the promise |finished|. [1]

The promise is "fulfilled" when finish() is called, either by the onsignalingstate function or the timer function, whichever happen first (subsequent calls are ignored).

Promises can be subscribed to (with .then()) before or after they’ve been fulfilled [2] so there's no timing issue. If the promise has already been fulfilled, then the then-function is called immediately, otherwise it gets queued and called later when the promise is eventually fulfilled.

[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-constructor
[1] http://www.w3.org/2001/tag/doc/promises-guide#one-time-events

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8523372 [details] [diff] [review]
fire onsignalingstatechange as an event

Review of attachment 8523372 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so the function you pass the Promise is executed immediately. Got it.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Right. I think they did that to encourage scoping of the execution to be inside the constructor, which of course I violate here (to be fair: for the sake of hg blame).

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8523372 [details] [diff] [review]
fire onsignalingstatechange as an event

Review of attachment 8523372 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM assuming we add the event verification back in.

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +33,1 @@
>        clearTimeout(eTimeout);

You deleted the event verification itself here. I think it would be good to keep that verification.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: fire onsignalingstatechange as an event (2) r=bwc, r=drno

Updated commit msg.

(In reply to Nils Ohlmeier [:drno] from comment #6)
> LGTM assuming we add the event verification back in.

Agreed on irc that no verification is needed since event is just Event.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce06b517bd3f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce06b517bd3f

Comment 10

[Nils Ohlmeier [:drno]]

Not sure which are the right key words here, but this change should get documented somewhere.

Comment 11

[Jean-Yves Perrier [:teoli]]

I've added a note here: 
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection.onsignalingstatechange
and in 
https://developer.mozilla.org/en-US/Firefox/Releases/36#Interfaces.2FAPIs.2FDOM