805533 - Signaling - SDP Parser should pass errors up to PeerConnection

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Ethan Hugg [:ehugg]]

The SDP Parser currently just logs errors to NSPR.  It needs to pass them up to PeerConnection so they can be accessible from JS.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - send parse errors through single function

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - send parse errors through single function

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - send parse errors through single function

Comment 4

[Ethan Hugg [:ehugg]]

Comment on attachment 675668 [details] [diff] [review]
Patch 1 - send parse errors through single function


-Changed all SDP parsing errors to go through one function, other SDP errors still go to log.
-Moved SDP structure defines into sdp.h to get rid of some void* folderol.
-Moved PC_HANDLE_SIZE def to a more central location since I added pc handle to sdp_t.
-Got rid of SDP_ERROR, SDP_WARN and sdp_log_errmsg
-removed unused sipsdp_create_from_buf

Next patch will be hooking up the output through vcmcSIPCCBinding and up to PC.

Comment 5

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

Comment on attachment 675668 [details] [diff] [review]
Patch 1 - send parse errors through single function

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

r+, though MEGO on all the mechanical debug changes

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/26bc10a97fcd

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26bc10a97fcd

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: Pass SDP parsing errors up to PeerConnection

Comment 9

[Ethan Hugg [:ehugg]]

Comment on attachment 682086 [details] [diff] [review]
Pass SDP parsing errors up to PeerConnection


Looking for feedback for how we should hook this up with the IDL and JS.

Comment 10

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

If we can only have one SDP parse (per PeerConnection) in flight at a time (i.e. SetLocal/RemoteDescription), then we probably *can* just use a single onsdpparseerror attribute.

The alternative is to add it as an optional parameter to SetLocal/RemoteDescription, which might make a little more sense, and if it's optional, it won't break existing uses.

Obviously all this should be in the purview of the W3C; if it hasn't been discussed we can pick one for now and then suggest it (or suggest both) on public-webrtc.  If they have discussed an API for this, let's start with that.

Once we decide on which, implementation can just copy existing onblah or callback code.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Pass SDP parsing errors up to PeerConnection

Comment 12

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Pass SDP parsing errors up to PeerConnection

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 721930 [details] [diff] [review]
Patch 2 - Pass SDP parsing errors up to PeerConnection


EKR suggested that I pipe these error messages up one more layer and store them in a list in PeerConnection.  Then we could close this bug and open a new one when the standard tells us how to report them.

Comment 14

[Ethan Hugg [:ehugg]]

Try - https://tbpl.mozilla.org/?tree=Try&rev=2fe55b8d86b3

Comment 15

[Ethan Hugg [:ehugg]]

Comment on attachment 721930 [details] [diff] [review]
Patch 2 - Pass SDP parsing errors up to PeerConnection


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

Comment 16

[Ed Morley [:emorley]]

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