Issue 198890043: ICE candidates support domain names
|
Created:
9 years, 3 months ago by luczo.laszlo.86 Modified:
9 years, 1 month ago CC:
bemasc_google.com Base URL:
http://webrtc.googlecode.com/svn/branches/38/ Visibility:
Public. |
DescriptionAdding support for domain names in ICE candidates instead of IP addresses.
According to https://tools.ietf.org/html/rfc5245
BUG=4165
Patch Set 1 #
Total comments: 24
Patch Set 2 : fix according to comments #
Total comments: 25
Patch Set 3 : fix according to comments 2 #
Total comments: 24
Patch Set 4 : fix according to comments 3 #
MessagesTotal messages: 9
Needs unit tests. And the description should describe what it does, not the problem it is solving. And the format for referencing bugs in the description is "BUG=XXXX". https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc File talk/p2p/base/transport.cc (right): https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:41: #include "webrtc/base/nethelpers.h" Please put // for AsyncResolver https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:80: class CandidateAsyncResolver : public rtc::AsyncResolver { This might be cleaner embedding rather than inheriting. Then you wouldn't need the type casting below, and you could put the candidate right in the signal. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:96: struct ResolverParams: public rtc::MessageData { Is this used anywhere? https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:541: CandidateAsyncResolver* candidateResolver = static_cast<CandidateAsyncResolver*>(resolver); candidate_resolver https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:545: candidate.set_address(address); Why not just candidate.set_address(candidate_resolver->address())? https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:547: OnCandidateReady_w(candidate); We fire OnCadidateReady weven when GetError() != 0? Shouldn't we log an error and skip the candidate? https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:548: resolver->Destroy(false); I think it would read more easily if you moved this up to above OnCandidateReady_w. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:549: } Please put OnResolveResult_w after OnRemoteCandidates_w so it reads more chronologically. Also, OnCandidateAddressResolveResult_w would be a better name. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:554: if(candidate.address().IsUnresolvedIP()){ Please use an early return: if (!candidate.address().IsUnresolvedIP()) { OnCandidateReady_w(candidate); return; } // ... https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:557: resolver->SignalDone.connect(this, &Transport::OnResolveResult_w); Is there a guarantee of what thread the SignalDone will be fired on? Is it guaranteed to come back on the same thread we start it on? https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:564: void Transport::OnCandidateReady_w(const Candidate& candidate){ The name should indicate whether it's a remote or local candidate. But I think we should just be explicit about what this method does. How about we just call it PushRemoteCandidateDownToChannel_w?
Sign in to reply to this message.
This is headed in the right direction. Please also add a unit test where you can test this with some sort of FakeAsyncResolver object. You can add a virtual CreateAsyncResolver method that can be overridden in the test to return the fake. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc File talk/p2p/base/transport.cc (right): https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:80: class CandidateAsyncResolver : public rtc::AsyncResolver { On 2015/01/22 19:53:13, pthatcher wrote: > This might be cleaner embedding rather than inheriting. Then you wouldn't need > the type casting below, and you could put the candidate right in the signal. We should also find a way to allow this to use a FakeResolver for testing. Embedding a AsyncResolverInterface* is probably the best way to do that.
Sign in to reply to this message.
https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc File talk/p2p/base/transport.cc (right): https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:41: #include "webrtc/base/nethelpers.h" On 2015/01/22 19:53:14, pthatcher wrote: > Please put // for AsyncResolver Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:80: class CandidateAsyncResolver : public rtc::AsyncResolver { On 2015/01/22 19:53:13, pthatcher wrote: > This might be cleaner embedding rather than inheriting. Then you wouldn't need > the type casting below, and you could put the candidate right in the signal. Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:96: struct ResolverParams: public rtc::MessageData { On 2015/01/22 19:53:13, pthatcher wrote: > Is this used anywhere? No. I remove this. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:96: struct ResolverParams: public rtc::MessageData { On 2015/01/22 19:53:13, pthatcher wrote: > Is this used anywhere? No. I remove this. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:541: CandidateAsyncResolver* candidateResolver = static_cast<CandidateAsyncResolver*>(resolver); On 2015/01/22 19:53:13, pthatcher wrote: > candidate_resolver Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:545: candidate.set_address(address); On 2015/01/22 19:53:14, pthatcher wrote: > Why not just candidate.set_address(candidate_resolver->address())? Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:547: OnCandidateReady_w(candidate); On 2015/01/22 19:53:13, pthatcher wrote: > We fire OnCadidateReady weven when GetError() != 0? Shouldn't we log an error > and skip the candidate? Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:548: resolver->Destroy(false); On 2015/01/22 19:53:14, pthatcher wrote: > I think it would read more easily if you moved this up to above > OnCandidateReady_w. Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:549: } On 2015/01/22 19:53:14, pthatcher wrote: > Please put OnResolveResult_w after OnRemoteCandidates_w so it reads more > chronologically. > > Also, OnCandidateAddressResolveResult_w would be a better name. Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:554: if(candidate.address().IsUnresolvedIP()){ On 2015/01/22 19:53:14, pthatcher wrote: > Please use an early return: > > if (!candidate.address().IsUnresolvedIP()) { > OnCandidateReady_w(candidate); > return; > } > > // ... Acknowledged. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:557: resolver->SignalDone.connect(this, &Transport::OnResolveResult_w); On 2015/01/22 19:53:13, pthatcher wrote: > Is there a guarantee of what thread the SignalDone will be fired on? Is it > guaranteed to come back on the same thread we start it on? Yes. The SignalThread::OnWorkDone runs on main thread. // Context: Main Thread. Subclass should override to do post-work cleanup. https://codereview.appspot.com/198890043/diff/1/talk/p2p/base/transport.cc#ne... talk/p2p/base/transport.cc:564: void Transport::OnCandidateReady_w(const Candidate& candidate){ On 2015/01/22 19:53:14, pthatcher wrote: > The name should indicate whether it's a remote or local candidate. > > But I think we should just be explicit about what this method does. How about > we just call it PushRemoteCandidateDownToChannel_w? Acknowledged.
Sign in to reply to this message.
https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession.h File talk/p2p/base/fakesession.h (right): https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:338: public: Please put one space before "public: " and "private: ". https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:381: int error_code_; Can you try arranging to test CandidateAysncResolver without subclassing it? I think you can if you pass in async_resolver_. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:491: FakeCandidateAsyncResolver* resolver_; Can this be done without the complexity of ref-counting? I think if we didn't subclass CandidateAyncResolver, you could just keep one AsyncResolver around which might make things easier. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.cc File talk/p2p/base/transport.cc (right): https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:502: CandidateAsyncResolver* Transport::CreateCandidateAsyncResolver(const Candidate& candidate) { Long line: Please wrap after the "(". https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:528: resolver->SignalDone.connect(this, &Transport::OnCandidateAddressResolveResult_w); Same here with long line. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1003: , asyncResolver_(NULL) { Can you put the "," on the previous line right after ")"? https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1015: asyncResolver_->SignalDone.connect(this, &CandidateAsyncResolver::OnResolveResult); Same here with long line. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1023: void CandidateAsyncResolver::OnResolveResult(rtc::AsyncResolverInterface* async_resolver) { And here https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1031: Release(); Might be more clear as: if(async_resolver->GetError()) { LOG(LS_WARNING) << "Unable to resolve hostname: " << async_resolver->address().hostname() << " error: " << async_resolver->GetError(); } else { candidate_.set_address(async_resolver->address()); SignalDone(candidate_); } Release(); https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h File talk/p2p/base/transport.h (right): https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:192: class CandidateAsyncResolver : public sigslot::has_slots<>, public rtc::RefCountInterface { Long line: Please break after the ",". https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:204: rtc::AsyncResolverInterface* asyncResolver_; async_resolver_ Or better yet, just "resolver_". https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:204: rtc::AsyncResolverInterface* asyncResolver_; Member variables should be private. I suggest passing in a the AsyncResolverInterface* rather than creating it here. I think that will make the testing of it easier (it won't require subclassing it). https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:420: virtual CandidateAsyncResolver* CreateCandidateAsyncResolver(const Candidate& candidate); Long line: Please break after the "(".
Sign in to reply to this message.
https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession.h File talk/p2p/base/fakesession.h (right): https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:338: public: On 2015/02/03 20:06:50, pthatcher wrote: > Please put one space before "public: " and "private: ". Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:381: int error_code_; On 2015/02/03 20:06:50, pthatcher wrote: > Can you try arranging to test CandidateAysncResolver without subclassing it? I > think you can if you pass in async_resolver_. Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:491: FakeCandidateAsyncResolver* resolver_; On 2015/02/03 20:06:50, pthatcher wrote: > Can this be done without the complexity of ref-counting? I think if we didn't > subclass CandidateAyncResolver, you could just keep one AsyncResolver around > which might make things easier. Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.cc File talk/p2p/base/transport.cc (right): https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:502: CandidateAsyncResolver* Transport::CreateCandidateAsyncResolver(const Candidate& candidate) { On 2015/02/03 20:06:50, pthatcher wrote: > Long line: Please wrap after the "(". Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:528: resolver->SignalDone.connect(this, &Transport::OnCandidateAddressResolveResult_w); On 2015/02/03 20:06:50, pthatcher wrote: > Same here with long line. Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1003: , asyncResolver_(NULL) { On 2015/02/03 20:06:50, pthatcher wrote: > Can you put the "," on the previous line right after ")"? Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1015: asyncResolver_->SignalDone.connect(this, &CandidateAsyncResolver::OnResolveResult); On 2015/02/03 20:06:51, pthatcher wrote: > Same here with long line. Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1023: void CandidateAsyncResolver::OnResolveResult(rtc::AsyncResolverInterface* async_resolver) { On 2015/02/03 20:06:50, pthatcher wrote: > And here Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1031: Release(); On 2015/02/03 20:06:50, pthatcher wrote: > Might be more clear as: > > if(async_resolver->GetError()) { > LOG(LS_WARNING) << "Unable to resolve hostname: " > << async_resolver->address().hostname() > << " error: " << async_resolver->GetError(); > > } else { > candidate_.set_address(async_resolver->address()); > SignalDone(candidate_); > } > Release(); Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h File talk/p2p/base/transport.h (right): https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:192: class CandidateAsyncResolver : public sigslot::has_slots<>, public rtc::RefCountInterface { On 2015/02/03 20:06:50, pthatcher wrote: > Long line: Please break after the ",". Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:204: rtc::AsyncResolverInterface* asyncResolver_; On 2015/02/03 20:06:50, pthatcher wrote: > async_resolver_ > > Or better yet, just "resolver_". Acknowledged. https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:420: virtual CandidateAsyncResolver* CreateCandidateAsyncResolver(const Candidate& candidate); On 2015/02/03 20:06:50, pthatcher wrote: > Long line: Please break after the "(". Acknowledged.
Sign in to reply to this message.
On 2015/02/16 10:32:27, luczo.laszlo.86 wrote: > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession.h > File talk/p2p/base/fakesession.h (right): > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... > talk/p2p/base/fakesession.h:338: public: > On 2015/02/03 20:06:50, pthatcher wrote: > > Please put one space before "public: " and "private: ". > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... > talk/p2p/base/fakesession.h:381: int error_code_; > On 2015/02/03 20:06:50, pthatcher wrote: > > Can you try arranging to test CandidateAysncResolver without subclassing it? > I > > think you can if you pass in async_resolver_. > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/fakesession... > talk/p2p/base/fakesession.h:491: FakeCandidateAsyncResolver* resolver_; > On 2015/02/03 20:06:50, pthatcher wrote: > > Can this be done without the complexity of ref-counting? I think if we didn't > > subclass CandidateAyncResolver, you could just keep one AsyncResolver around > > which might make things easier. > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.cc > File talk/p2p/base/transport.cc (right): > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... > talk/p2p/base/transport.cc:502: CandidateAsyncResolver* > Transport::CreateCandidateAsyncResolver(const Candidate& candidate) { > On 2015/02/03 20:06:50, pthatcher wrote: > > Long line: Please wrap after the "(". > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... > talk/p2p/base/transport.cc:528: resolver->SignalDone.connect(this, > &Transport::OnCandidateAddressResolveResult_w); > On 2015/02/03 20:06:50, pthatcher wrote: > > Same here with long line. > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... > talk/p2p/base/transport.cc:1003: , asyncResolver_(NULL) { > On 2015/02/03 20:06:50, pthatcher wrote: > > Can you put the "," on the previous line right after ")"? > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... > talk/p2p/base/transport.cc:1015: asyncResolver_->SignalDone.connect(this, > &CandidateAsyncResolver::OnResolveResult); > On 2015/02/03 20:06:51, pthatcher wrote: > > Same here with long line. > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... > talk/p2p/base/transport.cc:1023: void > CandidateAsyncResolver::OnResolveResult(rtc::AsyncResolverInterface* > async_resolver) { > On 2015/02/03 20:06:50, pthatcher wrote: > > And here > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.c... > talk/p2p/base/transport.cc:1031: Release(); > On 2015/02/03 20:06:50, pthatcher wrote: > > Might be more clear as: > > > > if(async_resolver->GetError()) { > > LOG(LS_WARNING) << "Unable to resolve hostname: " > > << async_resolver->address().hostname() > > << " error: " << async_resolver->GetError(); > > > > } else { > > candidate_.set_address(async_resolver->address()); > > SignalDone(candidate_); > > } > > Release(); > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h > File talk/p2p/base/transport.h (right): > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... > talk/p2p/base/transport.h:192: class CandidateAsyncResolver : public > sigslot::has_slots<>, public rtc::RefCountInterface { > On 2015/02/03 20:06:50, pthatcher wrote: > > Long line: Please break after the ",". > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... > talk/p2p/base/transport.h:204: rtc::AsyncResolverInterface* asyncResolver_; > On 2015/02/03 20:06:50, pthatcher wrote: > > async_resolver_ > > > > Or better yet, just "resolver_". > > Acknowledged. > > https://codereview.appspot.com/198890043/diff/20001/talk/p2p/base/transport.h... > talk/p2p/base/transport.h:420: virtual CandidateAsyncResolver* > CreateCandidateAsyncResolver(const Candidate& candidate); > On 2015/02/03 20:06:50, pthatcher wrote: > > Long line: Please break after the "(". > > Acknowledged. done
Sign in to reply to this message.
Some common style issues:
space between if and (
{ at end of line, not on next line
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession.h
File talk/p2p/base/fakesession.h (right):
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:336: class FakeResolver : public
rtc::AsyncResolverInterface
this should go into its own base/fakeresolver.h file
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:337: {
style: { at end of line (here and elsewhere)
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:340: FakeResolver(int error_code) :
error_code_(error_code) {}
I think this is better handled with a set_error_code mutator, makes for more
readable code.
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:343: SignalDone(this);
Use AsyncInvoke to ensure that this executes asynchronously.
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:348: addr->SetIP(0x6F6F6F);
Suggest allowing the IP to return to be set through some set_ip function.
Needs to work properly for AF_INET and AF_INET6 values for |family|.
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:358: virtual void Destroy(bool) {
this should delete itself
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:365: class CandidateAsyncResolverObserver : public
sigslot::has_slots<> {
Not clear this is useful.
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession...
talk/p2p/base/fakesession.h:499: rtc::AsyncResolverInterface* async_resover_;
this should be an internal detail of resolver_
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.cc
File talk/p2p/base/transport.cc (right):
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.c...
talk/p2p/base/transport.cc:1033: SignalDone(candidate_);
I think we want to SignalDone in either case, but still expose an error code so
the upper layer code can know if something bad happened
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.h
File talk/p2p/base/transport.h (right):
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.h...
talk/p2p/base/transport.h:193: public rtc::RefCountInterface {
why does this need to be refcounted? We avoid refcounting unless there is a
clear situation where multiple ownership is necessary.
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.h...
talk/p2p/base/transport.h:198: virtual void Start();
I think it would be better if |candidate| was passed into Start()
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport_u...
File talk/p2p/base/transport_unittest.cc (right):
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport_u...
talk/p2p/base/transport_unittest.cc:461: TEST_F(TransportTest,
TestCandidateAsyncResolver)
I'm not sure this test delivers all that much value, since it's almost entirely
duplicated in the Transport tests below. I would remove them and/or fold any
worthwhile functionality into those tests.
Sign in to reply to this message.
+ben as FYI
Sign in to reply to this message.
https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession.h File talk/p2p/base/fakesession.h (right): https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:336: class FakeResolver : public rtc::AsyncResolverInterface On 2015/02/26 23:58:09, juberti wrote: > this should go into its own base/fakeresolver.h file Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:337: { On 2015/02/26 23:58:08, juberti wrote: > style: { at end of line (here and elsewhere) Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:340: FakeResolver(int error_code) : error_code_(error_code) {} On 2015/02/26 23:58:08, juberti wrote: > I think this is better handled with a set_error_code mutator, makes for more > readable code. Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:343: SignalDone(this); On 2015/02/26 23:58:07, juberti wrote: > Use AsyncInvoke to ensure that this executes asynchronously. Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:348: addr->SetIP(0x6F6F6F); On 2015/02/26 23:58:06, juberti wrote: > Suggest allowing the IP to return to be set through some set_ip function. > > Needs to work properly for AF_INET and AF_INET6 values for |family|. Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:358: virtual void Destroy(bool) { On 2015/02/26 23:58:07, juberti wrote: > this should delete itself Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:365: class CandidateAsyncResolverObserver : public sigslot::has_slots<> { On 2015/02/26 23:58:08, juberti wrote: > Not clear this is useful. Called by CandidateAsyncResolver::SignalDone. I will move it to transport_unittest.cc https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/fakesession... talk/p2p/base/fakesession.h:499: rtc::AsyncResolverInterface* async_resover_; On 2015/02/26 23:58:08, juberti wrote: > this should be an internal detail of resolver_ Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.cc File talk/p2p/base/transport.cc (right): https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.c... talk/p2p/base/transport.cc:1033: SignalDone(candidate_); On 2015/02/26 23:58:09, juberti wrote: > I think we want to SignalDone in either case, but still expose an error code so > the upper layer code can know if something bad happened "pthatcher 2015/01/22 19:53:13 We fire OnCadidateReady weven when GetError() != 0? Shouldn't we log an error and skip the candidate?" so I log only error message, but I can notify higher layer if is really needed https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.h File talk/p2p/base/transport.h (right): https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:193: public rtc::RefCountInterface { On 2015/02/26 23:58:05, juberti wrote: > why does this need to be refcounted? We avoid refcounting unless there is a > clear situation where multiple ownership is necessary. I call addRef in Start and Release after SignalDone, so this object can delete itself after SignalDone. I use it so Transport needn't store ref to every resolver. I could call delete after SignalDone but I thing is an uglier solution. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport.h... talk/p2p/base/transport.h:198: virtual void Start(); On 2015/02/26 23:58:05, juberti wrote: > I think it would be better if |candidate| was passed into Start() Acknowledged. https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport_u... File talk/p2p/base/transport_unittest.cc (right): https://codereview.appspot.com/198890043/diff/60001/talk/p2p/base/transport_u... talk/p2p/base/transport_unittest.cc:461: TEST_F(TransportTest, TestCandidateAsyncResolver) On 2015/02/26 23:58:05, juberti wrote: > I'm not sure this test delivers all that much value, since it's almost entirely > duplicated in the Transport tests below. I would remove them and/or fold any > worthwhile functionality into those tests. Which test case do you think of? I can't see which test covers this function.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
