Last Comment Bug 846110 - PeerConnection ondatachannel callback receives channel in Firefox, but in Chrome an event is passed in
: PeerConnection ondatachannel callback receives channel in Firefox, but in Chr...
Status: VERIFIED FIXED
[WebRTC][blocking-webrtc+][parity-chr...
: compat
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: 22 Branch
: x86_64 Linux
: -- minor (vote)
: mozilla22
Assigned To: Randell Jesup [:jesup]
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-27 17:33 PST by Alan K [:ack]
Modified: 2013-04-07 13:51 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP) (48.29 KB, patch)
2013-03-26 09:52 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP) (55.45 KB, patch)
2013-03-27 23:35 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
ondatachannel() should take an event not a channel (951 bytes, patch)
2013-03-28 21:30 PDT, Randell Jesup [:jesup]
bugs: review+
Details | Diff | Splinter Review

Description User image Alan K [:ack] 2013-02-27 17:33:11 PST
In Firefox:

peerconnection.ondatachannel = function(channel) {...}

In Chrome:

peerconnection.ondatachannel = function(event) { var channel = event.channel; ...}

This should be standardized.
Comment 1 User image Jason Smith [:jsmith] 2013-02-27 18:07:32 PST
Eric - Are we doing this right or is Chrome is doing right?
Comment 2 User image Jason Smith [:jsmith] 2013-03-03 08:54:17 PST
Potential compat issue. Might block, but let's see what Randell and Ekr say.
Comment 3 User image Eric Rescorla (:ekr) 2013-03-03 08:59:35 PST
I'm having trouble finding a detailed answer in the specification, but based on the example in
the spec, it appears that the argument is supposed to be an event:

      // setup chat on incoming data channel
        pc.ondatachannel = function (evt) {
            channel = evt.channel;
            setupChat();
        };
Comment 5 User image Jason Smith [:jsmith] 2013-03-04 07:40:35 PST
Sounds like this is a bug on our end then.

I'm calling blocking on this one because this sounds like an obvious place for a cross-browser compatibility issue that developers will experience. And we should really try not to encourage ugly JS practices or worse, user agent sniffing to work around this problem.
Comment 6 User image Maire Reavy [:mreavy] Please needinfo me 2013-03-04 08:04:30 PST
(In reply to Jason Smith [:jsmith] from comment #5)
> Sounds like this is a bug on our end then.
> 
> I'm calling blocking on this one because this sounds like an obvious place
> for a cross-browser compatibility issue that developers will experience. And
> we should really try not to encourage ugly JS practices or worse, user agent
> sniffing to work around this problem.

Yes, I think you're right.   I believe this is an easy fix to make BUT it will break backwards compatibility.  So we'll want to let developers know via Hacks and Developer Engagement a few days before we fix this bug and uplift the patch.
Comment 7 User image Randell Jesup [:jesup] 2013-03-26 09:52:49 PDT
Created attachment 729604 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP)
Comment 8 User image Randell Jesup [:jesup] 2013-03-27 23:35:31 PDT
Created attachment 730557 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP)
Comment 9 User image Randell Jesup [:jesup] 2013-03-27 23:43:21 PDT
Comment on attachment 730557 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP)

Trying a review request to Michael Tuexen, since he knows the protocol and has written patches.
Comment 10 User image Randell Jesup [:jesup] 2013-03-27 23:49:00 PDT
Comment on attachment 730557 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP)

Oops.  Uploaded patch to the wrong bug...  Real bug is bug 855623
Comment 11 User image Randell Jesup [:jesup] 2013-03-28 21:30:35 PDT
Created attachment 731032 [details] [diff] [review]
ondatachannel() should take an event not a channel
Comment 12 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2013-03-29 05:06:00 PDT
Comment on attachment 731032 [details] [diff] [review]
ondatachannel() should take an event not a channel

über-hack, but I guess ok until we have proper EventTarget.
Comment 14 User image Ryan VanderMeulen [:RyanVM] 2013-04-01 10:57:52 PDT
https://hg.mozilla.org/mozilla-central/rev/ea296ec9e471
Comment 15 User image Jason Smith [:jsmith] 2013-04-07 13:51:13 PDT
Verified through a sanity test with ondatachannel - I'm getting an event object that I can grab a channel off of it.

Note You need to log in before you can comment on or make changes to this bug.