825560 - Creating a PeerConnection object and immediately checking the ready state - value is undefined, but should be "new"

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

Steps:

1. Load the attached test case html file
2. Create Peer Connection 1
3. Output the peer connection attributes

Expected:

The readyState should be new.

Actual:

The readyState is undefined.

Comment 1

[george]

I also met this issue on 22 nightly. It has been solved?

Comment 2

[Suhas]

jib .. taking a stab with a patch for this. please let me know if i take this bug for now.

Comment 3

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

np.

Comment 4

[Suhas]

Attachment: Supports readyState reporting on PeerConnection

This patch allows appropriate reporting of readyState on the PeerConnection object. 

Also the WebRTC Spec has inconsistent definition of readyState as of today. But chrome seems to support it in the latest canary and this patch surfaces similar information to JS API for Firefox.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 730314 [details] [diff] [review]
Supports readyState reporting on PeerConnection

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

::: dom/media/PeerConnection.js
@@ +685,5 @@
> +        break;
> +      case Ci.IPeerConnection.kClosed:
> +        state = "closed"; 
> +        break;
> +    }   

delete-trailing-whitespace
(on a bunch of lines)

Comment 6

[Suhas]

Attachment: Supports readyState reporting on PeerConnection

Taking r+ from jesup. If jib thinks this looks fine, we can check this in.

Comment 7

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

Comment on attachment 730314 [details] [diff] [review]
Supports readyState reporting on PeerConnection

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

Yay, my first review! :-)

I was wondering, could the Test Case you attached be reworked to a mochitest? Something along the lines of dom/media/tests/mochitest/test_peerConnection_bug825703.html ? Just a suggestion.

r+ with whitespace nits and IDL enum reuse in PeerConnectionImpl.cpp.

::: dom/media/bridge/IPeerConnection.idl
@@ +88,5 @@
> +  const long kNew = 0;
> +  const long kNegotiating = 1;
> +  const long kActive = 2;
> +  const long kClosing = 3;
> +  const long kClosed = 4;

It took me a moment to realize we have these enums in two places. Now that they're here, can we please reuse them in PeerConnectionImpl.h to avoid bugs? XPIDL generates anonymous enums unfortunately, but the following should work without loosing type-safety:

  enum ReadyState {
    kNew = IPeerConnection::kNew,
    ...

Comment 8

[george]

Hi, if the fix is checked in, do I just update my firefox nightly on the version 22.0a1 to get fixed version?

Comment 9

[george]

And does it is related to the bug 825560 (https://bugzilla.mozilla.org/show_bug.cgi?id=825560)? Is there anyone to look at this issue?
Thanks

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/627021947bfd

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

george: You need to wait until it a) gets "uplifted" from mozilla-inbound to mozilla-central, then b)_ wait for it to get into a daily build.  If it uplifts before roughly midnight eastern US time, perhaps a couple of hours later, it will be in the nightly build the next day.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/627021947bfd

Comment 13

[Jason Smith [:jsmith]]

Followup bug is bug 859432. Also, see bug 857894 as well.

Comment 14

[george]

Hi, now I am on the latest nightly 23.0a1. I suppose my firefox should include this fix, right?

Comment 15

[Jason Smith [:jsmith]]

(In reply to george from comment #14)
> Hi, now I am on the latest nightly 23.0a1. I suppose my firefox should
> include this fix, right?

Yes.