Better determination of Symmetric NAT

Better determination of Symmetric NAT.

If we're using shared socket mode, Symmetric NAT can be correctly determined by having more than 1 srflx per Requester (i.e. socket).

Design Doc updated at https://docs.google.com/document/d/11zruojoNqZgZYDrSVk0O3JHqFz2cFldWYQbroZdZRBg/edit#heading=h.t7npdccia97t

BUG=4576
R=pthatcher@webrtc.org

Committed: https://crrev.com/72e9f044471078f40999c93823424e4f0e825c21
Cr-Commit-Position: refs/heads/master@{#9396}

[guoweis_webrtc, 2015-06-05 20:33:51]

guoweis@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[guoweis_webrtc, 2015-06-05 20:33:59]

PTAL.

[pthatcher2, 2015-06-05 22:39:40]

pthatcher@chromium.org changed reviewers:
+ pthatcher@chromium.org

[pthatcher2, 2015-06-05 22:39:41]

Mostly naming things.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
File webrtc/p2p/stunprober/main.cc (right):

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:125: case NATTYPE::NATTYPE_NO_NAT:
NATTYPE_NONE?

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:125: case NATTYPE::NATTYPE_NO_NAT:
Do you really need "NATTYPE::" Prefixed everywhere?  I don't think you do.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:127: case NATTYPE::NATTYPE_NAT_TYPE_UNKNOWN:
Why not NATTYPE_UNKNOWN?

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:129: case NATTYPE::NATTYPE_SYM_NAT:
NATTYPE_SYMMETRIC?

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:131: case NATTYPE::NATTYPE_NON_SYM_NAT:
NATTYPE_NON_SYMMETRIC?

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:132: return "Non-Symmetric NAT";
Please capitalize all of them or none of them.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:134: return "unknown";
Is this the same as NATTYPE_UNKNOWN?

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:158: HistogramName((stats.nat_type >
NATTYPE::NATTYPE_NO_NAT),
What if it's NATTYPE_UNKNOWN?

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:165: HistogramName((stats.nat_type >
NATTYPE::NATTYPE_NO_NAT),
You could avoid duplication with 

bool behind_nat = stats.nat_type > NATTYPE_UNKNOWN.

Or reuse the "is_behind_nat" function.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
File webrtc/p2p/stunprober/stunprober.cc (right):

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
webrtc/p2p/stunprober/stunprober.cc:36: }
It's probably not worth a separate function if you only call it once.


I'd call it just "behind_nat"

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
File webrtc/p2p/stunprober/stunprober.h (right):

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
webrtc/p2p/stunprober/stunprober.h:140: enum NATTYPE {
NatType, please.

[guoweis_webrtc, 2015-06-07 17:29:40]

PTAL.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
File webrtc/p2p/stunprober/main.cc (right):

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:125: case NATTYPE::NATTYPE_NO_NAT:
On 2015/06/05 22:39:41, pthatcher2 wrote:
> Do you really need "NATTYPE::" Prefixed everywhere?  I don't think you do.

Done.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:125: case NATTYPE::NATTYPE_NO_NAT:
On 2015/06/05 22:39:41, pthatcher2 wrote:
> NATTYPE_NONE?

Done.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:127: case NATTYPE::NATTYPE_NAT_TYPE_UNKNOWN:
On 2015/06/05 22:39:41, pthatcher2 wrote:
> Why not NATTYPE_UNKNOWN?

Done.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:129: case NATTYPE::NATTYPE_SYM_NAT:
On 2015/06/05 22:39:41, pthatcher2 wrote:
> NATTYPE_SYMMETRIC?

Done.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:131: case NATTYPE::NATTYPE_NON_SYM_NAT:
On 2015/06/05 22:39:41, pthatcher2 wrote:
> NATTYPE_NON_SYMMETRIC?

Done.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:132: return "Non-Symmetric NAT";
On 2015/06/05 22:39:41, pthatcher2 wrote:
> Please capitalize all of them or none of them.

Done.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:134: return "unknown";
On 2015/06/05 22:39:41, pthatcher2 wrote:
> Is this the same as NATTYPE_UNKNOWN?

It's invalid. Updated.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:158: HistogramName((stats.nat_type >
NATTYPE::NATTYPE_NO_NAT),
On 2015/06/05 22:39:41, pthatcher2 wrote:
> What if it's NATTYPE_UNKNOWN?

Remove the code. Histogram doesn't make sense here.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:165: HistogramName((stats.nat_type >
NATTYPE::NATTYPE_NO_NAT),
On 2015/06/05 22:39:41, pthatcher2 wrote:
> You could avoid duplication with 
> 
> bool behind_nat = stats.nat_type > NATTYPE_UNKNOWN.
> 
> Or reuse the "is_behind_nat" function.

Remove the code. Histogram doesn't make sense here.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
File webrtc/p2p/stunprober/stunprober.cc (right):

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
webrtc/p2p/stunprober/stunprober.cc:36: }
On 2015/06/05 22:39:41, pthatcher2 wrote:
> It's probably not worth a separate function if you only call it once.
> 
> 
> I'd call it just "behind_nat"

I changed the name but I like it being separated as it's more clear when
compared with request.behind_nat.

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
File webrtc/p2p/stunprober/stunprober.h (right):

https://codereview.webrtc.org/1166013002/diff/20001/webrtc/p2p/stunprober/stu...
webrtc/p2p/stunprober/stunprober.h:140: enum NATTYPE {
On 2015/06/05 22:39:41, pthatcher2 wrote:
> NatType, please.

Done.

[guoweis_webrtc, 2015-06-07 18:02:59]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org

[pthatcher1, 2015-06-08 20:47:17]

lgtm

nits

https://codereview.webrtc.org/1166013002/diff/40001/webrtc/p2p/stunprober/mai...
File webrtc/p2p/stunprober/main.cc (right):

https://codereview.webrtc.org/1166013002/diff/40001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:109: const char* PrintNATType(stunprober::NatType
type) {
PrintNatType

https://codereview.webrtc.org/1166013002/diff/40001/webrtc/p2p/stunprober/mai...
webrtc/p2p/stunprober/main.cc:112: return "Not behind NAT";
Not behind a NAT

(notice the "a")

[guoweis_webrtc, 2015-06-09 04:04:06]

Committed patchset #4 (id:60001) manually as
72e9f044471078f40999c93823424e4f0e825c21 (presubmit successful).

[commit-bot: I haz the power, 2015-06-09 04:04:12]

Patchset 4 (id:??) landed as
https://crrev.com/72e9f044471078f40999c93823424e4f0e825c21
Cr-Commit-Position: refs/heads/master@{#9396}