Closed Bug 1075133 Opened 10 years ago Closed 10 years ago

onsignalingstatechange is not fired as an event

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bwc, Assigned: jib)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

onsignalingstatechange is passed the signaling state, and not an Event as required by the specification. Additionally, the mochitests expect this non-standard behavior.
Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee33501ae46e
Assignee: docfaraday → jib
Status: NEW → ASSIGNED
Attachment #8523372 - Flags: review?(drno)
Attachment #8523372 - Flags: review?(docfaraday)
Blocks: 1091898
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?
(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 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.
Attachment #8523372 - Flags: review?(docfaraday) → review+
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 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.
Attachment #8523372 - Flags: review?(drno) → review+
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.
Attachment #8523372 - Attachment is obsolete: true
Attachment #8526374 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ce06b517bd3f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1104326
Not sure which are the right key words here, but this change should get documented somewhere.
You need to log in before you can comment on or make changes to this bug.