Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16025003:
Adds DataChannelInit param to createDataChannel (Closed)
|
Created:
7 years, 7 months ago by jiayl Modified:
7 years, 6 months ago CC:
blink-reviews, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdds a createDataChannel method that takes DataChannelInit as input to WebKit::WebRTCPeerConnectionHandler
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151675
Patch Set 1 #Patch Set 2 : dataChannelInit #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 34 (0 generated)
This is a starting point for passing the source id to createDataChannel for SCTP data channels. If you think the idl change looks good, I'll go ahead adding the other necessary changes to pass the value to glue code.
On 2013/05/24 22:38:36, jiayl wrote:
> This is a starting point for passing the source id to createDataChannel for
SCTP
> data channels. If you think the idl change looks good, I'll go ahead adding
the
> other necessary changes to pass the value to glue code.
I think we want the sid param to end up in the dictionary object. Here's what
Peter's been suggesting:
dictionary RTCDataChannelInit {
boolean outOfOrderAllowed;
unsigned short maxRetransmitTime;
unsigned short maxRetransmitNum;
DOMString protocol;
boolean sendOpenMessage; // Added
unsigned short id; // Added
};
OK, then we don't need to change RTCPeerConnection.idl at all. Do we need the change in RTCDataChannel.idl for getting the id attribute? On Fri, May 24, 2013 at 3:42 PM, <juberti@google.com> wrote: > On 2013/05/24 22:38:36, jiayl wrote: > >> This is a starting point for passing the source id to createDataChannel >> for >> > SCTP > >> data channels. If you think the idl change looks good, I'll go ahead >> adding >> > the > >> other necessary changes to pass the value to glue code. >> > > I think we want the sid param to end up in the dictionary object. Here's > what > Peter's been suggesting: > > dictionary RTCDataChannelInit { > boolean outOfOrderAllowed; > unsigned short maxRetransmitTime; > unsigned short maxRetransmitNum; > DOMString protocol; > boolean sendOpenMessage; // Added > unsigned short id; // Added > }; > > https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1... >
Either there or in the glue code. On May 24, 2013 4:03 PM, "Jiayang Liu" <jiayl@chromium.org> wrote: > OK, then we don't need to change RTCPeerConnection.idl at all. Do we need > the change in RTCDataChannel.idl for getting the id attribute? > > > On Fri, May 24, 2013 at 3:42 PM, <juberti@google.com> wrote: > >> On 2013/05/24 22:38:36, jiayl wrote: >> >>> This is a starting point for passing the source id to createDataChannel >>> for >>> >> SCTP >> >>> data channels. If you think the idl change looks good, I'll go ahead >>> adding >>> >> the >> >>> other necessary changes to pass the value to glue code. >>> >> >> I think we want the sid param to end up in the dictionary object. Here's >> what >> Peter's been suggesting: >> >> dictionary RTCDataChannelInit { >> boolean outOfOrderAllowed; >> unsigned short maxRetransmitTime; >> unsigned short maxRetransmitNum; >> DOMString protocol; >> boolean sendOpenMessage; // Added >> unsigned short id; // Added >> }; >> >> https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1... >> > >
FYI, I think it's supposed to be "ordered", not outOfOrderAllowed, and "sendOpenMessage" was never finalized or agreed upon, so it might have to change later. On May 24, 2013 3:42 PM, <juberti@google.com> wrote: > On 2013/05/24 22:38:36, jiayl wrote: > >> This is a starting point for passing the source id to createDataChannel >> for >> > SCTP > >> data channels. If you think the idl change looks good, I'll go ahead >> adding >> > the > >> other necessary changes to pass the value to glue code. >> > > I think we want the sid param to end up in the dictionary object. Here's > what > Peter's been suggesting: > > dictionary RTCDataChannelInit { > boolean outOfOrderAllowed; > unsigned short maxRetransmitTime; > unsigned short maxRetransmitNum; > DOMString protocol; > boolean sendOpenMessage; // Added > unsigned short id; // Added > }; > > https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1... >
On 2013/05/24 22:38:36, jiayl wrote: > This is a starting point for passing the source id to createDataChannel for SCTP > data channels. If you think the idl change looks good, I'll go ahead adding the > other necessary changes to pass the value to glue code. FYI we can't just change the API like this; as a bare minimum we need a thread discussing these changes on the W3C mailing list where there is agreement.
It should only be visible to JS if it is needed for the app developer, otherwise
it should be kept internal.
On 2013/05/24 23:20:15, juberti wrote:
> Either there or in the glue code.
> On May 24, 2013 4:03 PM, "Jiayang Liu" <mailto:jiayl@chromium.org> wrote:
>
> > OK, then we don't need to change RTCPeerConnection.idl at all. Do we need
> > the change in RTCDataChannel.idl for getting the id attribute?
> >
> >
> > On Fri, May 24, 2013 at 3:42 PM, <mailto:juberti@google.com> wrote:
> >
> >> On 2013/05/24 22:38:36, jiayl wrote:
> >>
> >>> This is a starting point for passing the source id to createDataChannel
> >>> for
> >>>
> >> SCTP
> >>
> >>> data channels. If you think the idl change looks good, I'll go ahead
> >>> adding
> >>>
> >> the
> >>
> >>> other necessary changes to pass the value to glue code.
> >>>
> >>
> >> I think we want the sid param to end up in the dictionary object. Here's
> >> what
> >> Peter's been suggesting:
> >>
> >> dictionary RTCDataChannelInit {
> >> boolean outOfOrderAllowed;
> >> unsigned short maxRetransmitTime;
> >> unsigned short maxRetransmitNum;
> >> DOMString protocol;
> >> boolean sendOpenMessage; // Added
> >> unsigned short id; // Added
> >> };
> >>
> >>
>
https://codereview.chromium.**org/16025003/%3Chttps://codereview.chromium.org...>
> >>
> >
> >
This was discussed extensively on the list - this addition is the result of that discussion. http://lists.w3.org/Archives/Public/public-webrtc/2013Mar/0059.html was the original discussion. On Sun, May 26, 2013 at 11:49 PM, <tommyw@chromium.org> wrote: > On 2013/05/24 22:38:36, jiayl wrote: > >> This is a starting point for passing the source id to createDataChannel >> for >> > SCTP > >> data channels. If you think the idl change looks good, I'll go ahead >> adding >> > the > >> other necessary changes to pass the value to glue code. >> > > FYI we can't just change the API like this; as a bare minimum we need a > thread > discussing these changes on the W3C mailing list where there is agreement. > > https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1... >
Sorry, don't know how I managed to miss that discussion... On 2013/05/27 17:08:25, juberti wrote: > This was discussed extensively on the list - this addition is the result of > that discussion. > > http://lists.w3.org/Archives/Public/public-webrtc/2013Mar/0059.html was the > original discussion. > >
Since the on-list dicussion wasn't completely conclusive, there was more
discussion off the list with Randell, and I think they're moving ahead with:
dictionary RTCDataChannelInit {
boolean ordered;
unsigned short maxRetransmitTime;
unsigned short maxRetransmitNum;
DOMString protocol;
boolean sendOpenMessage; // Added
unsigned short id; // Added
};
The only one of those that I'm not sure about is "sendOpenMessage". They
might be going with "preset". It wasn't really clear what they would end
up making it.
On Tue, May 28, 2013 at 8:23 AM, <tommyw@chromium.org> wrote:
> Sorry, don't know how I managed to miss that discussion...
>
>
> On 2013/05/27 17:08:25, juberti wrote:
>
>> This was discussed extensively on the list - this addition is the result
>> of
>> that discussion.
>>
>
>
http://lists.w3.org/Archives/**Public/public-webrtc/2013Mar/**0059.html<http:...
the
>> original discussion.
>>
>
>
>
>
>
https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1...
>
I suggest you make it whatever you think it should be and then we can suggest they follow our precedent :). On Tue, May 28, 2013 at 11:35 AM, Peter Thatcher <pthatcher@google.com>wrote: > Since the on-list dicussion wasn't completely conclusive, there was more > discussion off the list with Randell, and I think they're moving ahead with: > > dictionary RTCDataChannelInit { > boolean ordered; > > unsigned short maxRetransmitTime; > unsigned short maxRetransmitNum; > DOMString protocol; > boolean sendOpenMessage; // Added > unsigned short id; // Added > }; > > The only one of those that I'm not sure about is "sendOpenMessage". They > might be going with "preset". It wasn't really clear what they would end > up making it. > > > > On Tue, May 28, 2013 at 8:23 AM, <tommyw@chromium.org> wrote: > >> Sorry, don't know how I managed to miss that discussion... >> >> >> On 2013/05/27 17:08:25, juberti wrote: >> >>> This was discussed extensively on the list - this addition is the result >>> of >>> that discussion. >>> >> >> http://lists.w3.org/Archives/**Public/public-webrtc/2013Mar/**0059.html<http:... the >>> original discussion. >>> >> >> >> >> >> https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1... >> > >
I think we should make it what I just sent sent:
dictionary RTCDataChannelInit {
boolean ordered; // Default: true
unsigned short maxRetransmitTime;
unsigned short maxRetransmitNum;
DOMString protocol;
boolean sendOpenMessage; // Default: true
unsigned short id; // Default: Choose by browser
};
On Tue, May 28, 2013 at 11:47 AM, Justin Uberti <juberti@google.com> wrote:
> I suggest you make it whatever you think it should be and then we can
> suggest they follow our precedent :).
>
>
> On Tue, May 28, 2013 at 11:35 AM, Peter Thatcher <pthatcher@google.com>wrote:
>
>> Since the on-list dicussion wasn't completely conclusive, there was more
>> discussion off the list with Randell, and I think they're moving ahead with:
>>
>> dictionary RTCDataChannelInit {
>> boolean ordered;
>>
>> unsigned short maxRetransmitTime;
>> unsigned short maxRetransmitNum;
>> DOMString protocol;
>> boolean sendOpenMessage; // Added
>> unsigned short id; // Added
>> };
>>
>> The only one of those that I'm not sure about is "sendOpenMessage". They
>> might be going with "preset". It wasn't really clear what they would end
>> up making it.
>>
>>
>>
>> On Tue, May 28, 2013 at 8:23 AM, <tommyw@chromium.org> wrote:
>>
>>> Sorry, don't know how I managed to miss that discussion...
>>>
>>>
>>> On 2013/05/27 17:08:25, juberti wrote:
>>>
>>>> This was discussed extensively on the list - this addition is the
>>>> result of
>>>> that discussion.
>>>>
>>>
>>>
http://lists.w3.org/Archives/**Public/public-webrtc/2013Mar/**0059.html<http:...
the
>>>> original discussion.
>>>>
>>>
>>>
>>>
>>>
>>>
https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1...
>>>
>>
>>
>
PTAL. I'll remove the default impl once Chrome is updated with the real impl.
On 2013/05/28 21:27:47, jiayl wrote: > PTAL. I'll remove the default impl once Chrome is updated with the real impl. lgtm
Tommy, Could you take a look?
tkent, could you take a look?
dglazkov, could you take a look?
On 2013/05/30 17:08:22, jiayl wrote: rslgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/21001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_layout. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/32003
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/32003
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/32003
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
FYI, according to this: https://github.com/fluffy/webrtc-w3c/blob/master/webrtc.html They are going with dictionary RTCDataChannelInit { boolean ordered; // Default: true unsigned short maxRetransmitTime; unsigned short maxRetransmits; DOMString protocol; boolean negotiated; // Default: false unsigned short id; // Default: Choose by browser }; So, we need to change sendOpenMessage to negotiated and maxRetransmitNum to maxRetransmits. On Tue, May 28, 2013 at 11:51 AM, Peter Thatcher <pthatcher@google.com>wrote: > I think we should make it what I just sent sent: > > dictionary RTCDataChannelInit { > boolean ordered; // Default: true > unsigned short maxRetransmitTime; > unsigned short maxRetransmitNum; > DOMString protocol; > boolean sendOpenMessage; // Default: true > unsigned short id; // Default: Choose by browser > }; > > > > > > > > > On Tue, May 28, 2013 at 11:47 AM, Justin Uberti <juberti@google.com>wrote: > >> I suggest you make it whatever you think it should be and then we can >> suggest they follow our precedent :). >> >> >> On Tue, May 28, 2013 at 11:35 AM, Peter Thatcher <pthatcher@google.com>wrote: >> >>> Since the on-list dicussion wasn't completely conclusive, there was more >>> discussion off the list with Randell, and I think they're moving ahead with: >>> >>> dictionary RTCDataChannelInit { >>> boolean ordered; >>> >>> unsigned short maxRetransmitTime; >>> unsigned short maxRetransmitNum; >>> DOMString protocol; >>> boolean sendOpenMessage; // Added >>> unsigned short id; // Added >>> }; >>> >>> The only one of those that I'm not sure about is "sendOpenMessage". >>> They might be going with "preset". It wasn't really clear what they would >>> end up making it. >>> >>> >>> >>> On Tue, May 28, 2013 at 8:23 AM, <tommyw@chromium.org> wrote: >>> >>>> Sorry, don't know how I managed to miss that discussion... >>>> >>>> >>>> On 2013/05/27 17:08:25, juberti wrote: >>>> >>>>> This was discussed extensively on the list - this addition is the >>>>> result of >>>>> that discussion. >>>>> >>>> >>>> http://lists.w3.org/Archives/**Public/public-webrtc/2013Mar/** >>>>> 0059.html<http://lists.w3.org/Archives/Public/public-webrtc/2013Mar/0059.html>was the >>>>> original discussion. >>>>> >>>> >>>> >>>> >>>> >>>> https://codereview.chromium.**org/16025003/<https://codereview.chromium.org/1... >>>> >>> >>> >> >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/49001
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/49001
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/16025003/49001
Message was sent while issue was closed.
Change committed as 151675 |
