WebRTC Code Reviews
Help | Chromium Project | Sign in
(11452)

Issue 57489004: Support multiple URLs in PeerConnectionInterface::IceServer (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by joachim
Modified:
4 years, 2 months ago
Reviewers:
tommi, pthatcher, jiayang
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie.mao, ajm, tterriberry, qiang.lu, Niklas
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Support multiple URLs in PeerConnectionInterface::IceServer This adds support for multiple URLs in a IceServer configuration as defined in http://w3c.github.io/webrtc-pc/#idl-def-RTCIceServer. BUG=2096 R=pthatcher@webrtc.org Committed: https://crrev.com/7c4e7458b5ce99c13a75d5be7d718ef94e2f8f9f Cr-Commit-Position: refs/heads/master@{#9320}

Patch Set 1 #

Total comments: 12

Patch Set 2 : Changes based on feedback. #

Total comments: 5

Messages

Total messages: 12 (2 generated)
joachim
Ptal, this CL moves the actual parsing to a new function and calls that for ...
4 years, 2 months ago (2015-05-27 22:41:35 UTC) #2
pthatcher
https://webrtc-codereview.appspot.com/57489004/diff/1/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://webrtc-codereview.appspot.com/57489004/diff/1/talk/app/webrtc/peerconnection.cc#newcode169 talk/app/webrtc/peerconnection.cc:169: bool ParseIceServer(const PeerConnectionInterface::IceServer& server, Should probably call this ParseIceServerUrl, ...
4 years, 2 months ago (2015-05-28 19:42:30 UTC) #3
joachim
All changed, thanks for your feedback. https://webrtc-codereview.appspot.com/57489004/diff/1/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://webrtc-codereview.appspot.com/57489004/diff/1/talk/app/webrtc/peerconnection.cc#newcode169 talk/app/webrtc/peerconnection.cc:169: bool ParseIceServer(const PeerConnectionInterface::IceServer& ...
4 years, 2 months ago (2015-05-28 20:02:11 UTC) #4
pthatcher
lgtm
4 years, 2 months ago (2015-05-28 20:19:25 UTC) #5
joachim
Committed patchset #2 (id:20001) manually as 7c4e7458b5ce99c13a75d5be7d718ef94e2f8f9f (presubmit successful).
4 years, 2 months ago (2015-05-28 21:06:49 UTC) #6
commit-bot
Patchset 2 (id:??) landed as https://crrev.com/7c4e7458b5ce99c13a75d5be7d718ef94e2f8f9f Cr-Commit-Position: refs/heads/master@{#9320}
4 years, 2 months ago (2015-05-28 21:06:52 UTC) #7
tommi
drive-by https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/peerconnection.cc#newcode190 talk/app/webrtc/peerconnection.cc:190: std::string uri_without_transport = tokens[0]; This could potentially be ...
4 years, 2 months ago (2015-05-29 15:41:04 UTC) #9
joachim
On 2015/05/29 15:41:04, tommi wrote: > drive-by > > https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/peerconnection.cc > File talk/app/webrtc/peerconnection.cc (right): > ...
4 years, 2 months ago (2015-05-29 15:44:43 UTC) #10
tommi
On 2015/05/29 15:44:43, joachim wrote: > On 2015/05/29 15:41:04, tommi wrote: > > drive-by > ...
4 years, 2 months ago (2015-05-29 15:47:42 UTC) #11
joachim
4 years, 2 months ago (2015-05-29 15:53:58 UTC) #12
Message was sent while issue was closed.
On 2015/05/29 15:47:42, tommi wrote:
> On 2015/05/29 15:44:43, joachim wrote:
> > On 2015/05/29 15:41:04, tommi wrote:
> > > drive-by
> > > 
> > >
> >
>
https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/pee...
> > > File talk/app/webrtc/peerconnection.cc (right):
> > > 
> > >
> >
>
https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/pee...
> > > talk/app/webrtc/peerconnection.cc:190: std::string uri_without_transport =
> > > tokens[0];
> > > This could potentially be a security is if tokens is ever empty.  Can you
> fix?
> > > (e.g. check if url is empty and return failure or DCHECK that it's never
> > empty,
> > > also DCHECK that tokens contains the expected number of tokens if that
makes
> > > sense)
> > > 
> > >
> >
>
https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/pee...
> > > talk/app/webrtc/peerconnection.cc:195: if (tokens[0] == kTransport) {
> > > no check for tokens.size() here either.
> > > 
> > >
> >
>
https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/pee...
> > > talk/app/webrtc/peerconnection.cc:198: if (tokens[1] != kUdpTransportType
&&
> > > tokens[1] != kTcpTransportType) {
> > > or here
> > > 
> > >
> >
>
https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/pee...
> > > talk/app/webrtc/peerconnection.cc:202: turn_transport_type = tokens[1];
> > > here too please :)
> > > 
> > >
> >
>
https://webrtc-codereview.appspot.com/57489004/diff/20001/talk/app/webrtc/pee...
> > > talk/app/webrtc/peerconnection.cc:221: ASSERT(!tokens.empty());
> > > ah, here is some code that checks this. Looks like this is moved code that
> has
> > > had similar issues that I mention above, fixed.  (this is moved code,
> right?)
> > 
> > Yes, I moved the whole block to a separate function, but didn't
reorder/change
> > any code inside the block and thus didn't review the existing code.
> > 
> > I can check the issues you mentioned and will create a new CL with any
fixes.
> 
> excellent. I can review it if you want.

Great, thanks - I'll let you know when I have something ready.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted