Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1498993002:
Add ufrag to the ICE candidate signaling. (Closed)
|
Created:
5 years ago by honghaiz3 Modified:
5 years ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, juberti1, jiayl2 Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd ufrag to the ICE candidate signaling.
On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received.
This could avoid a bunch of ICE generation issues.
BUG=webrtc:5138, webrt:5292
Committed: https://crrev.com/a54a0801121e05f797e514731cc5c9bad2f5e597
Cr-Commit-Position: refs/heads/master@{#11060}
Patch Set 1 : #
Total comments: 18
Patch Set 2 : #
Total comments: 22
Patch Set 3 : #Patch Set 4 : Fix a type-check warning #
Total comments: 16
Patch Set 5 : Merge and address comments #
Total comments: 4
Patch Set 6 : #
Messages
Total messages: 34 (22 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add ufrag to the ICE candidate signaling. BUG= ========== to ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. BUG= ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. BUG= ========== to ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. BUG=webrtc:5138,webrt:5292 ==========
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
Description was changed from ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. BUG=webrtc:5138,webrt:5292 ========== to ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. This could avoid a bunch of ICE generation issues. BUG=webrtc:5138,webrt:5292 ==========
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1088: } else if (fields[i] == kAttributeCandidateUsername) { I think we should change this to check for kAttributeCandidateUsernameFragment https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1762: os << " " << kAttributeCandidateUsername << " " << it->username(); This would make it "username", but it's not really a username. It's a "username fragment". So we should probably make the string "ufrag". Maybe call it kAttributeCandidateUsernameFragment https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:2672: it != candidates_orig.end(); ++it) { Since this is parsing candidates in the SDP (not trickled), shouldn't we assert that it's empty or that it matches the value in the transport description? https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:655: << ufrag << " is old."; Perhaps say "because its ufrag indicates it was for a previous ICE generation" https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:683: // Candidate generation is still used for determining the priority. You can remove the "is still" https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:701: } When is the pwd on the candidate going to be filled in? I see that it is in the unit test, but it's not clear when that happens. A comment explaining when that happens would be useful. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:797: return std::max(remote_candidate_generation_, candidate.generation()); I think this should go lookup the generation by looking in remote_generations_ for which ufrag matches. If none do, then use candidate.generation(). If that's not set, then assume it's the latest. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:263: std::set<std::string> old_remote_ufrags_; I think a good name for this would be remote_generations_. And then maybe store a struct of IceGenerations like this: struct IceGeneration { std::string ufrag; } Then later, we could possibly add other things, like pwd. Additionally, I think we should put the current one in there and have remote_ufrag() and remote_pwd() getters return remote_generations_[0]... https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1852: EXPECT_TRUE(GetConnectionTo(&ch, "2.2.2.2", 2) == nullptr); Do we also need a test to make sure that if generation isn't signalled, but ufrag is, that the right generation (and thus prioritization) is assigned?
Patchset #3 (id:170001) has been deleted
Patchset #3 (id:190001) has been deleted
Patchset #3 (id:210001) has been deleted
Patchset #2 (id:140002) has been deleted
Thanks! PTAL. https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1088: } else if (fields[i] == kAttributeCandidateUsername) { On 2015/12/04 20:43:49, pthatcher1 wrote: > I think we should change this to check for kAttributeCandidateUsernameFragment Done. https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1762: os << " " << kAttributeCandidateUsername << " " << it->username(); On 2015/12/04 20:43:49, pthatcher1 wrote: > This would make it "username", but it's not really a username. It's a "username > fragment". So we should probably make the string "ufrag". Maybe call it > kAttributeCandidateUsernameFragment I just used the one kAttributeUfrag which is "ice-ufrag", so that it matches the one in the offer/answer. https://codereview.webrtc.org/1498993002/diff/140001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:2672: it != candidates_orig.end(); ++it) { On 2015/12/04 20:43:49, pthatcher1 wrote: > Since this is parsing candidates in the SDP (not trickled), shouldn't we assert > that it's empty or that it matches the value in the transport description? Yes. Thanks. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:655: << ufrag << " is old."; On 2015/12/04 20:43:50, pthatcher1 wrote: > Perhaps say "because its ufrag indicates it was for a previous ICE generation" Done. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:683: // Candidate generation is still used for determining the priority. On 2015/12/04 20:43:50, pthatcher1 wrote: > You can remove the "is still" Moved set_generation to AddRemoteIceCandidate() https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:701: } On 2015/12/04 20:43:50, pthatcher1 wrote: > When is the pwd on the candidate going to be filled in? I see that it is in the > unit test, but it's not clear when that happens. A comment explaining when that > happens would be useful. Done. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:797: return std::max(remote_candidate_generation_, candidate.generation()); On 2015/12/04 20:43:50, pthatcher1 wrote: > I think this should go lookup the generation by looking in remote_generations_ > for which ufrag matches. If none do, then use candidate.generation(). If > that's not set, then assume it's the latest. Done. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:263: std::set<std::string> old_remote_ufrags_; On 2015/12/04 20:43:50, pthatcher1 wrote: > I think a good name for this would be remote_generations_. And then maybe store > a struct of IceGenerations like this: > > struct IceGeneration { > std::string ufrag; > } > > Then later, we could possibly add other things, like pwd. > > Additionally, I think we should put the current one in there and have > remote_ufrag() and remote_pwd() getters return remote_generations_[0]... Done. added the pwd there too. https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1498993002/diff/140001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1852: EXPECT_TRUE(GetConnectionTo(&ch, "2.2.2.2", 2) == nullptr); On 2015/12/04 20:43:50, pthatcher1 wrote: > Do we also need a test to make sure that if generation isn't signalled, but > ufrag is, that the right generation (and thus prioritization) is assigned? Done.
Mostly ideas for readability. https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1075: // the candidate. I think this comment would be more valuable as "Though non-standard, we support the ICE ufrag and pwd being signaled on the candidate to avoid issues with confusing which generation a candidate belongs to when trickling multiple generations at the same time." https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1286: BuildCandidate(candidates, false, message); Can you leave a comment like "We don't include the ufrag on candidates in the SDP because it's redundant with the values already in the m-line."? https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... File talk/app/webrtc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp_unittest.cc:401: " eth0 ice-ufrag user_rtp ice-pwd password_rtp generation 2\r\n"; I think this should be "ufrag" and "pwd", not "ice-ufrag" and "ice-pwd". "ice-" is just redundant. It's not redundant in the m-line because the m-line contains lots of non-ice things. But within a candidate line, it's redundant. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:354: } I think this might be a little better using an optional and operator==: Optional<IceParameters> gen1 = remote_ice(); IceParaemeters gen2 = IceParameters(ice_ufrag, ice_pwd); if (!gen1 || *gen1 != gen2) { remote_ice_generations_.push_back(gen2); } https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:533: } Same here, with optional: if (remote_password.empty() && remote_ice()) { remote_password = remote_ice().pwd; } https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:800: } I think this could be more readable by using std::find_if; const auto& params = remote_ice_parameters_; if (!candidate.username().empty()) { auto it = std::find_if(params.begin(), params.end(), [candidate](const IceParameters& ice) { return ice.ufrag = candidate.username(); }); if (it == params.end()) { // If not found, assume it's the next (future) generation; return remote_ice_generation() + 1; } return it - params.begin(); } https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:808: return generation; I think this would be readable as: // If candidate generation is set, use that. if (candidate.generation() > 0) { return candidate.generation(); } // If we have remote parameters, use that. if (!params.empty()) { return remote_ice_generation(); } // Otherwise, assume it's generation 0. return 0; https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:39: struct IceGeneration { Now that I see the code, I see that it gets confusing that "generation" sometimes means "generation index" and other times means "generation credentials". So perhaps we should just keep generation as "index" and instead call this "IceParameters" https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:42: IceGeneration(const std::string& ice_ufrag, const std::string& ice_pwd) { Could be: IceGenration(const std::string& ufrag, const std::string& pwd) : ufrag(ufrag), pwd(pwd) {} https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:250: } I think it would be better to have a remote_ice() getter that returns an Optional<IceParameters>. See how it could be used in the next file. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:275: std::vector<IceGeneration> remote_ice_generations_; And remote_ice_parameters_ might make sense here.
Patchset #3 (id:250001) has been deleted
Patchset #3 (id:270001) has been deleted
PTAL. https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1075: // the candidate. On 2015/12/10 22:08:06, pthatcher1 wrote: > I think this comment would be more valuable as "Though non-standard, we support > the ICE ufrag and pwd being signaled on the candidate to avoid issues with > confusing which generation a candidate belongs to when trickling multiple > generations at the same time." Done. https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp.cc:1286: BuildCandidate(candidates, false, message); On 2015/12/10 22:08:06, pthatcher1 wrote: > Can you leave a comment like "We don't include the ufrag on candidates in the > SDP because it's redundant with the values already in the m-line."? Done. https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... File talk/app/webrtc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1498993002/diff/230001/talk/app/webrtc/webrtcsd... talk/app/webrtc/webrtcsdp_unittest.cc:401: " eth0 ice-ufrag user_rtp ice-pwd password_rtp generation 2\r\n"; On 2015/12/10 22:08:06, pthatcher1 wrote: > I think this should be "ufrag" and "pwd", not "ice-ufrag" and "ice-pwd". "ice-" > is just redundant. It's not redundant in the m-line because the m-line contains > lots of non-ice things. But within a candidate line, it's redundant. Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:354: } On 2015/12/10 22:08:06, pthatcher1 wrote: > I think this might be a little better using an optional and operator==: > > Optional<IceParameters> gen1 = remote_ice(); > IceParaemeters gen2 = IceParameters(ice_ufrag, ice_pwd); > if (!gen1 || *gen1 != gen2) { > remote_ice_generations_.push_back(gen2); > } Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:533: } On 2015/12/10 22:08:06, pthatcher1 wrote: > Same here, with optional: > > if (remote_password.empty() && remote_ice()) { > remote_password = remote_ice().pwd; > } Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:800: } On 2015/12/10 22:08:06, pthatcher1 wrote: > I think this could be more readable by using std::find_if; > > const auto& params = remote_ice_parameters_; > if (!candidate.username().empty()) { > auto it = std::find_if(params.begin(), params.end(), [candidate](const > IceParameters& ice) { > return ice.ufrag = candidate.username(); > }); > if (it == params.end()) { > // If not found, assume it's the next (future) generation; > return remote_ice_generation() + 1; > } > return it - params.begin(); > } > > Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:808: return generation; On 2015/12/10 22:08:06, pthatcher1 wrote: > I think this would be readable as: > > // If candidate generation is set, use that. > if (candidate.generation() > 0) { > return candidate.generation(); > } > > // If we have remote parameters, use that. > if (!params.empty()) { > return remote_ice_generation(); > } > > // Otherwise, assume it's generation 0. > return 0; Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:39: struct IceGeneration { On 2015/12/10 22:08:06, pthatcher1 wrote: > Now that I see the code, I see that it gets confusing that "generation" > sometimes means "generation index" and other times means "generation > credentials". So perhaps we should just keep generation as "index" and instead > call this "IceParameters" Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:42: IceGeneration(const std::string& ice_ufrag, const std::string& ice_pwd) { On 2015/12/10 22:08:06, pthatcher1 wrote: > Could be: > > IceGenration(const std::string& ufrag, const std::string& pwd) : ufrag(ufrag), > pwd(pwd) {} Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:250: } On 2015/12/10 22:08:06, pthatcher1 wrote: > I think it would be better to have a remote_ice() getter that returns an > Optional<IceParameters>. See how it could be used in the next file. Done. https://codereview.webrtc.org/1498993002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:275: std::vector<IceGeneration> remote_ice_generations_; On 2015/12/10 22:08:06, pthatcher1 wrote: > And remote_ice_parameters_ might make sense here. Done.
https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:349: rtc::Optional<IceParameter>&& ice_current = remote_ice(); I think it would read a little better as current_ice and new_ice. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:533: if (ice_current && remote_username == ice_current->ufrag) { I think it would read a little better as current_ice here as well. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:796: [candidate](const IceParameter& param) -> bool { The "-> bool" can be inferred, so you can leave it off. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:40: struct IceParameter { IceParameters (with an "s") Yes, I know that makes ice_parameters_ a little weird, but I think that's OK. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:46: bool operator!=(const IceParameter& other) { Can you implement both == and != just to make sure we don't mess up some logic where we use one instead of the other? https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:244: rtc::Optional<IceParameter> remote_ice() { Can you leave a comment saying it's the latest remote ice parameters? https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:248: } If it's too much of a problem to avoid copying with an Optional, we could just return a IceParameter*: return remote_ice_parameters_.empty() ? nullptr : &remote_ice_parameters_.back(); https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:249: uint32_t remote_ice_generation() { Can you leave a comment saying it's the index of the latest remote ice parameters, or 0 if there are none?
Patchset #5 (id:330001) has been deleted
PTAL. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:349: rtc::Optional<IceParameter>&& ice_current = remote_ice(); On 2015/12/12 00:30:34, pthatcher1 wrote: > I think it would read a little better as current_ice and new_ice. Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:533: if (ice_current && remote_username == ice_current->ufrag) { On 2015/12/12 00:30:34, pthatcher1 wrote: > I think it would read a little better as current_ice here as well. Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:796: [candidate](const IceParameter& param) -> bool { On 2015/12/12 00:30:34, pthatcher1 wrote: > The "-> bool" can be inferred, so you can leave it off. Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:40: struct IceParameter { On 2015/12/12 00:30:34, pthatcher1 wrote: > IceParameters > > (with an "s") > > Yes, I know that makes ice_parameters_ a little weird, but I think that's OK. Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:46: bool operator!=(const IceParameter& other) { On 2015/12/12 00:30:34, pthatcher1 wrote: > Can you implement both == and != just to make sure we don't mess up some logic > where we use one instead of the other? Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:244: rtc::Optional<IceParameter> remote_ice() { On 2015/12/12 00:30:34, pthatcher1 wrote: > Can you leave a comment saying it's the latest remote ice parameters? Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:248: } On 2015/12/12 00:30:34, pthatcher1 wrote: > If it's too much of a problem to avoid copying with an Optional, we could just > return a IceParameter*: > > return remote_ice_parameters_.empty() ? nullptr : > &remote_ice_parameters_.back(); Done. https://codereview.webrtc.org/1498993002/diff/310001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:249: uint32_t remote_ice_generation() { On 2015/12/12 00:30:34, pthatcher1 wrote: > Can you leave a comment saying it's the index of the latest remote ice > parameters, or 0 if there are none? Done.
lgtm, with nits https://codereview.webrtc.org/1498993002/diff/350001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/350001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:44: bool operator!=(const IceParameters& other) { I think this can be return !(this == other); https://codereview.webrtc.org/1498993002/diff/350001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:245: // Returns the latest remote ICE parameters or nullptr if there are no remote ICE parameters yet.
https://codereview.webrtc.org/1498993002/diff/350001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1498993002/diff/350001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:44: bool operator!=(const IceParameters& other) { On 2015/12/16 19:47:26, pthatcher1 wrote: > I think this can be > > return !(this == other); Done. https://codereview.webrtc.org/1498993002/diff/350001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:245: // Returns the latest remote ICE parameters On 2015/12/16 19:47:26, pthatcher1 wrote: > or nullptr if there are no remote ICE parameters yet. Done.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1498993002/#ps370001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1498993002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1498993002/370001
Message was sent while issue was closed.
Description was changed from ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. This could avoid a bunch of ICE generation issues. BUG=webrtc:5138,webrt:5292 ========== to ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. This could avoid a bunch of ICE generation issues. BUG=webrtc:5138,webrt:5292 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:370001)
Message was sent while issue was closed.
Description was changed from ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. This could avoid a bunch of ICE generation issues. BUG=webrtc:5138,webrt:5292 ========== to ========== Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. This could avoid a bunch of ICE generation issues. BUG=webrtc:5138,webrt:5292 Committed: https://crrev.com/a54a0801121e05f797e514731cc5c9bad2f5e597 Cr-Commit-Position: refs/heads/master@{#11060} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a54a0801121e05f797e514731cc5c9bad2f5e597 Cr-Commit-Position: refs/heads/master@{#11060} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
